Add immutable Monitor model - #566
Conversation
Replace monitor dictionaries with a frozen, slotted Monitor dataclass while preserving temporary string-key access. Update platform enumeration, legacy grab inputs, filename formatting, typing, tests, examples, and migration documentation.
Some samples would not work correctly. Also prepared for eventually phasing out string like access.
halldorfannar
changed the title
jholveck
left a comment
jholveck
left a comment
There was a problem hiding this comment.
I'm not really comfortable with this idea of making synthetic Monitor objects to send down through _impl.grab, rather than sending capture regions as dicts or tuples. Can you elaborate on why you made that choice?
|
|
||
| #### Immutable monitor objects | ||
|
|
||
| {py:attr}`mss.MSS.monitors` now returns frozen, slotted {py:class}`mss.models.Monitor` objects instead of dictionaries. |
There was a problem hiding this comment.
Why frozen? (I was going to ask about that in your latest proposal.)
There was a problem hiding this comment.
I have added documentation to explain why. It is important that customers cannot modify the monitors because they are owned by MSS and should be immutable.
There was a problem hiding this comment.
To clarify: I hid interpreted "frozen" in the sense of C++'s final, or possibly making some sort of future compatibility guarantee, but now that I see your latest commit, I see you meant it in the sense of "immutable".
Might that be a better term?
(I'm also not sure on why it matters to users that they're slotted, but that's not a problem.)
|
|
||
| if isinstance(monitor, Monitor): | ||
| left, top = monitor.left, monitor.top | ||
| else: # remove this once we deprecate the string key access |
There was a problem hiding this comment.
Deprecate it where? Why does this allow a dict to be passed to the constructor?
This relates to https://github.com/BoboTiG/python-mss/pull/566/changes#r3709864024
There was a problem hiding this comment.
My understanding was that during transition we wanted to support the key string access but eventually we will remove it. Just to soften the blow for existing customers and their code. We will mark this for deprecation and eventually remove it. My language is slightly off in the comment. We are marking it for deprecation and will later remove it. The process is normally:
- Deprecate
- At a later date remove
BUT now that we have moved to the CaptureRegion this code has all been thrown away and we will remain backwards compatible (no deprecation planned anymore).
There was a problem hiding this comment.
This ties into my earlier question about why we pass around these synthetic Monitor objects wherever we're talking about capture regions. Unfortunately, the link I got for that didn't work as expected.
At this point, when I wrote the review, the things that construct a ScreenShot - the _impl.grab methods - would always pass it a Monitor object, never a dict.
Also, this code would be in the context of allowing the caller to pass in a Monitor or a dict, but the comment about "string key access" ties into how a user can access the properties of a Monitor, which is a completely different matter.
But now that we're using CaptureRegion, then it's moot.
- MSS.grab() still accepts Monitor, dictionaries, and tuples, but normalizes them to CaptureRegion. - Platform backends and custom screenshot classes now receive regions, never synthetic monitors. - Added Monitor.as_capture_region(). - Applied the Linux **output_ids and comment cleanups. - Fixed the demo import grouping. - Documented why monitors are immutable. - Added test_wheel guidance to AGENTS.md.
|
One overall thing. I'm wondering if the name |
Yeah, I think that (Besides, Food for thought: should a |
2 participants
Changes proposed in this PR
Fixes #470
Replace heterogeneous monitor dictionaries with a frozen, slotted
Monitordataclass that exposes typed geometry andplatform metadata attributes.
Monitorobjects.grab().regions.
test_sdistwhen files are added, removed, or renamed.This gives callers a stable, read-only monitor representation without preventing existing applications from passing
dictionary-based capture regions.
./check.shpassedValidation:
uv run ./check.sh. The full pytest suite was not run during the final review.AI assistance disclosure
Codex assisted with reviewing the implementation, identifying and implementing follow-up fixes in the demos and
ScreenShotinitialization, updating the immutability test for static typing, running validation, and drafting this PRdescription. The contributor reviewed and directed the API and capture-region decisions.