Conversation
See also BoboTiG/python-mss issue BoboTiG#486. The user will always work with a single class: mss.MSS. Differences in implementation, such as platform or capture strategy, are hidden in an internal implementation object held by the mss.MSS object. This allows us to change the implementation, with arbitrary class hierarchies, without worrying about preserving compatibility with internal class names. This deprecates the existing `mss` factory function, although it can easily be kept for as long as needed to give users time to adapt. It also deprecates the existing `mss.{platform}.MSS` types. These are exposed to the user, so somebody calling `mss.{platform}.MSS()` in 10.x can still reasonably expect to get a `mss.{platform}.MSS` object back. However, in 11.0, we can remove the type entirely, and either remove those symbols, or make them deprecated aliases for `mss.MSS`. Where possible, deprecated functionality emits a `DeprecationWarning`. However, note that these are ignored by default, unless triggered by code in `__main__`. Many of the API docs are removed, since this change removes much of the API surface. However, they are still in available for backwards-compatibility. This change adds tests for everything that was in the 10.1 docs, examples, etc, at least at a basic level: for instance, it tests that `mss.linux.MSS` still works as both a constructor and a type (for `isinstance`), and that `mss.linux.ZPIXMAP` still exists (it was listed in the 10.1 docs). The existing code, tests, and docs are changed to use `mss.MSS`.
The mss_impl fixture would add an implicit display= argument, regardless of platform. The code at that time would ignore it, but we should be (and in the previous commit, were) more strict. Change mss_impl to only use display= if appropriate, so we can be more strict in the future. In 10.1, these were allowed at all times, and ignored if the platform didn't use them. Emulate this behavior in mss.MSS (and mss.mss), with DeprecationWarnings, and test.
I'm pretty sure it's unnecessary there. Not sure why it was being done.
This lets IDEs and code checkers know what is acceptable. This lets them autocomplete, and identify incorrect options in user code.
Co-authored-by: Mickaël Schoentgen <contact@tiger-222.fr>
📝 WalkthroughWalkthroughThis pull request introduces a new stable public API Changes
Sequence Diagram(s)sequenceDiagram
participant User as User Code
participant MSS as mss.MSS (Public API)
participant Dispatcher as _choose_impl()
participant Impl as MSSImplementation<br/>(Platform-Specific)
participant System as Platform Layer
User->>MSS: MSS(backend=?, display=?, ...)
activate MSS
MSS->>MSS: __init__(validate platform-specific kwargs)
MSS->>Dispatcher: _choose_impl(platform, kwargs)
activate Dispatcher
Dispatcher->>Dispatcher: detect platform via platform.system()
Dispatcher->>Impl: instantiate MSSImplDarwin/Linux/Windows
Dispatcher-->>MSS: return MSSImplementation instance
deactivate Dispatcher
MSS->>MSS: store as self._impl
deactivate MSS
User->>MSS: with MSS() as sct:
User->>MSS: sct.grab(monitor)
activate MSS
MSS->>Impl: self._impl.grab(monitor)
activate Impl
Impl->>System: platform-specific capture (CTCore/X11/Win32)
System-->>Impl: raw pixel bytearray + Size
Impl-->>MSS: return (bytearray, Size)
deactivate Impl
MSS->>MSS: construct ScreenShot(data, region)
MSS-->>User: ScreenShot object
deactivate MSS
User->>MSS: sct.monitors
activate MSS
MSS->>Impl: self._impl.monitors()
activate Impl
Impl->>System: query available displays
System-->>Impl: monitor list
Impl->>Impl: build Monitors dict
Impl-->>MSS: return Monitors
deactivate Impl
MSS-->>User: Monitors (cached)
deactivate MSS
sequenceDiagram
participant User as Legacy Code<br/>(v10.1 API)
participant Factory as mss.mss()<br/>(Deprecated Factory)
participant PublicMSS as mss.MSS<br/>(New Public)
participant Compat as Deprecated Wrappers<br/>(platform.MSS)
User->>Factory: mss.mss(display=?, backend=?)
activate Factory
Note over Factory: Emit DeprecationWarning<br/>(stacklevel=2)
Factory->>PublicMSS: MSS(**kwargs)
activate PublicMSS
PublicMSS-->>Factory: return MSS instance
deactivate PublicMSS
Factory-->>User: return MSS (as MSSBase type)
deactivate Factory
User->>Compat: mss.linux.MSS() or<br/>mss.darwin.MSS()
activate Compat
Note over Compat: Emit DeprecationWarning<br/>(platform-specific wrapper)
Compat->>PublicMSS: call super().__init__()
PublicMSS-->>Compat: initialized
Compat-->>User: return MSS instance
deactivate Compat
Note over User,Factory: Both paths return compatible<br/>MSS objects with same interface<br/>MSSBase is now alias for MSS
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mss/linux/base.py (1)
427-435:⚠️ Potential issue | 🟡 MinorUpdate
_grab_xgetimage()'s return contract.The helper now returns raw pixel bytes, but the docstring still says it returns a screenshot object. That mismatch will mislead the next implementation that reuses this fallback.
📝 Proposed doc fix
- :returns: A screenshot object containing the captured region. + :returns: Raw pixel bytes for the captured region.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mss/linux/base.py` around lines 427 - 435, The docstring for _grab_xgetimage(Monitor) is outdated: the function now returns raw pixel bytes (bytearray) instead of a screenshot object; update the docstring to state it returns a bytearray of raw pixel data (include pixel format/stride/order if known) and describe the shape/ordering (e.g., RGB/RGBA per pixel, row-major, bytes per pixel) so callers using _grab_xgetimage or the XShmGetImage fallback know how to interpret the returned bytearray.
🧹 Nitpick comments (10)
docs/source/examples/fps.py (1)
43-43: Use context manager for resource cleanup and ensure documentation consistency.The
fps.pyexample is the only one in the documentation that doesn't use the recommendedwith mss.MSS() as sct:pattern. All other examples follow this pattern for proper resource management. The web search confirms that context managers are the recommended approach to ensureclose()is called via__exit__, even if an exception occurs.Apply the refactor to move the while loop inside the context manager:
Proposed refactor
- sct = mss.MSS() - last_time = time.time() - - while time.time() - last_time < 1: - img = np.asarray(sct.grab(mon)) - fps += 1 - - cv2.imshow(title, img) - if cv2.waitKey(25) & 0xFF == ord("q"): - cv2.destroyAllWindows() - break + last_time = time.time() + with mss.MSS() as sct: + while time.time() - last_time < 1: + img = np.asarray(sct.grab(mon)) + fps += 1 + + cv2.imshow(title, img) + if cv2.waitKey(25) & 0xFF == ord("q"): + cv2.destroyAllWindows() + break🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/source/examples/fps.py` at line 43, The fps.py example creates the MSS instance without a context manager (sct = mss.MSS()), so refactor to use the recommended context-manager pattern: replace the standalone sct = mss.MSS() with a with mss.MSS() as sct: block and move the existing while loop and any code that uses sct inside that block so the MSS instance is automatically closed via __exit__; update any related variable scope accordingly to match other examples.CHANGES.md (1)
6-6: Spell out the deprecated entry points here.This entry introduces
mss.MSS, but it currently reads like an additive API note. The migration action users need is thatmss.mss()andmss.{platform}.MSSare now deprecated compatibility entry points.📝 Suggested wording
-- (This affected almost every file in some respect, but base.py was the most affected.) Introduced a new API, mss.MSS. This class can be used instead of the previous various MSSBase subclasses, so that the user can work with a consistent class regardless of implementation details. The methods implemented by MSSBase subclasses were renamed, and moved to MSSImplementation subclasses. The mss.MSS class now holds an MSSImplementation instance as an instance variable. (`#486`, `#494`) +- (This affected almost every file in some respect, but base.py was the most affected.) Introduced `mss.MSS` as the stable constructor. `mss.mss()` and `mss.{platform}.MSS` remain available as deprecated compatibility entry points during the 10.x transition. Internally, `mss.MSS` now delegates to an `MSSImplementation` instance. (`#486`, `#494`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGES.md` at line 6, Update the changelog text to explicitly list the deprecated compatibility entry points and the recommended replacement: state that the old factory function mss.mss() and any platform-specific classes mss.<platform>.MSS are deprecated and that users should migrate to the new mss.MSS class which now holds an MSSImplementation instance (the renaming from MSSBase methods to MSSImplementation methods). Mention that base.py was most affected and include a short migration note: replace calls to mss.mss() or mss.<platform>.MSS with creating an instance of mss.MSS instead.src/tests/test_windows.py (1)
21-23: Don’t bake the default backend into these cache tests.These cases are really testing the GDI implementation via
_impl._dib, but they start frommss.MSS()and then assert that the default implementation isMSSImplWindows. That makes the tests fail as soon as Windows gets another backend, which is exactly the flexibility this strategy refactor is trying to unlock. Pin the concrete implementation under test instead of treating the default_implchoice as part of the contract.Also applies to: 44-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tests/test_windows.py` around lines 21 - 23, The tests currently instantiate the default mss.MSS() and assert that sct._impl is mss.windows.MSSImplWindows, which couples the tests to the default backend; instead, import and instantiate the concrete implementation mss.windows.MSSImplWindows (or otherwise construct the concrete Windows backend object used in the test) and run the same assertions against that concrete instance (e.g., check its _dib behavior) so the tests no longer depend on MSS() choosing a particular default; apply the same change to the other similar block mentioned (the assertions around lines 44-50) so both tests pin the concrete Windows implementation rather than asserting MSS()'s default.src/tests/bench_grab_windows.py (1)
112-123: Pin this benchmark to the GDI backend explicitly.
benchmark_raw_bitblt()is measuring GDI internals (_srcdc/_memdc), but it still starts frommss.MSS()and assumes the default Windows backend isMSSImplWindows. Once backend selection changes, this either breaks or benchmarks the wrong implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tests/bench_grab_windows.py` around lines 112 - 123, The benchmark_raw_bitblt test currently does mss.MSS() and assumes the default backend; instead instantiate the MSS with the Windows/GDI backend explicitly so sct._impl is guaranteed to be mss.windows.MSSImplWindows and the measured fields (_srcdc, _memdc) are valid; update the code that creates sct (replace the mss.MSS() call) to construct or pass the Windows backend implementation (MSSImplWindows) so the test always pins to the GDI backend before calling sct.grab(...) and accessing sct._impl._srcdc/_memdc.src/mss/linux/xgetimage.py (1)
21-24: Minor: Update docstring cross-reference.The docstring references
mss.linux.base.MSSXCBBasebut the class now inherits fromMSSImplXCBBase. Consider updating the seealso reference for consistency.📝 Suggested docstring update
"""XCB backend using XGetImage requests on GNU/Linux. .. seealso:: - :py:class:`mss.linux.base.MSSXCBBase` + :py:class:`mss.linux.base.MSSImplXCBBase` Lists constructor parameters. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mss/linux/xgetimage.py` around lines 21 - 24, The docstring's seealso reference points to mss.linux.base.MSSXCBBase but the class now inherits from MSSImplXCBBase; update the cross-reference in the docstring to :py:class:`mss.linux.base.MSSImplXCBBase` (or the correct import path for MSSImplXCBBase) so the seealso matches the actual base class used by the class in src/mss/linux/xgetimage.py.src/tests/test_gnu_linux.py (1)
80-102: Consider narrowing the accepted exception types after investigation.The TODO comment indicates this needs review before the final PR. Accepting
(ImportError, ScreenShotError)for Windows (line 100) and(ScreenShotError, ValueError)for macOS (line 94) is broad and could mask unexpected failure modes.Once the cross-platform behavior is confirmed, consider documenting the specific exception each platform raises and narrowing these back to single types if possible.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tests/test_gnu_linux.py` around lines 80 - 102, Update the platform-specific tests to assert a single, specific exception per OS rather than broad tuples: for the macOS branch replace pytest.raises((ScreenShotError, ValueError)) with the single confirmed exception type raised by mss.MSS on Darwin (e.g., ScreenShotError or ValueError) and for the Windows branch replace pytest.raises((ImportError, ScreenShotError)) with the single confirmed exception (e.g., ImportError or ScreenShotError); locate these in the test where monkeypatch.setattr(platform, "system", ...) and mss.MSS(backend=backend) are used, run the cross-platform behavior check to determine the exact exception type, update the assertions accordingly, and remove or update the TODO comment once the expected exceptions are narrowed and documented.src/mss/darwin.py (1)
267-269: Explicitreturn Nonewould be clearer.Same as the Windows implementation - using
return Noneexplicitly would match the declared return type.Suggested change
def cursor(self) -> None: """Retrieve all cursor data. Pixels have to be RGB.""" - return + return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mss/darwin.py` around lines 267 - 269, The cursor() method currently ends with an implicit return; change it to an explicit return None to match its declared return type and mirror the Windows implementation—update the cursor function in class/method named cursor() to end with "return None" so the intent and type annotation (-> None) are clear.src/mss/windows.py (1)
413-415: Explicitreturn Nonewould be clearer.While
returnimplicitly returnsNone, usingreturn Noneexplicitly would match the declared return type and improve readability.Suggested change
def cursor(self) -> None: """Retrieve all cursor data. Pixels have to be RGB.""" - return + return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mss/windows.py` around lines 413 - 415, The cursor method currently uses a bare "return" which implicitly returns None; update the cursor() function to use an explicit "return None" to match its annotated return type and improve readability (replace the existing "return" statement in def cursor(self) -> None: with "return None").docs/source/usage.rst (1)
153-155: Minor inconsistency in CLI help text.The documented
-b, --backend BACKENDline shows only-bbut the actual argparse definition at line 63 of__main__.pyshows-b, --backend. The multi-line help text format here differs slightly from argparse's default output format. Consider verifying this matches actual--helpoutput.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/source/usage.rst` around lines 153 - 155, The CLI help text in usage.rst is inconsistent with the argparse definition: update the documented option line to exactly match the argparse flag names and formatting (i.e., include both "-b, --backend BACKEND") and ensure the following multi-line description matches argparse's wrapped help (platform-specific backend to use (Linux: default/xlib/xgetimage/xshmgetimage; macOS/Windows: default)); locate the argparse option definition for backend in __main__.py (the parser.add_argument call for "-b/--backend") and mirror its flag names and help text/line-wrapping in the usage.rst entry so the docs match the actual --help output.src/mss/linux/xlib.py (1)
451-454: Redundant condition check.Since
effective_with_cursoris alreadyrequested_with_cursor and bool(_XFIXES), theif _XFIXES is not Nonecheck inside the conditional load is redundant. The outerif effective_with_cursoralready guarantees_XFIXESis truthy.Simplify the conditional
if effective_with_cursor: - # MyPy doesn't quite realize that won't be called if _XFIXES is None. #: :meta private: - self.xfixes = cdll.LoadLibrary(_XFIXES) if _XFIXES is not None else None + self.xfixes = cdll.LoadLibrary(_XFIXES) # type: ignore[arg-type] + else: + self.xfixes = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mss/linux/xlib.py` around lines 451 - 454, The code redundantly checks _XFIXES inside the effective_with_cursor branch; update the block in xlib so when effective_with_cursor is true you directly call cdll.LoadLibrary(_XFIXES) to set self.xfixes (remove the inner "if _XFIXES is not None" check), keeping the outer condition that uses effective_with_cursor (which is defined as requested_with_cursor and bool(_XFIXES)); ensure you still satisfy type-checking (MyPy) by preserving the same branch so the loader only executes when _XFIXES is guaranteed truthy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mss/base.py`:
- Around line 525-533: The max_displays property currently accesses
self._impl.max_displays unconditionally which will raise AttributeError on
non-macOS implementations; modify the property (max_displays) to perform a
runtime guard (e.g. check platform.system() == "Darwin" or
isinstance(self._impl, MSSImplDarwin)) and only return self._impl.max_displays
for macOS, otherwise raise a clear NotImplementedError/PlatformError with a
message like "max_displays is only available on macOS (MSSImplDarwin)"; keep the
docstring but enforce the platform check to avoid AttributeError when self._impl
is MSSImplWindows or MSSImplXlib.
In `@src/mss/linux/xshmgetimage.py`:
- Line 74: The code currently appends healthy-state messages to
self.performance_status (e.g., the self.performance_status.append(full_msg)
call), making the list ambiguous; change the logic so performance_status only
records issues: remove or conditionalize any append of non-error/healthy
messages (including the other occurrences around the block at lines ~201-203)
and only append when a real degraded/error condition is detected (preserve error
message content and use the same identifier full_msg when adding). If you prefer
an audit trail instead, rename performance_status to something like
performance_events and update callers accordingly, but do not leave both healthy
and issue entries in performance_status.
In `@src/tests/test_compat_10_1.py`:
- Around line 160-167: The nested pytest.warns should be made platform-aware:
import platform and compute expected_count = {"Linux":1,"Darwin":2,"Windows":3}.
Replace the two nested pytest.warns blocks around mss.mss(**kwargs) with a
single pytest.warns(DeprecationWarning) as w (or use warnings.catch_warnings)
when calling context = mss.mss(**kwargs), then assert that one
DeprecationWarning matches r"^mss\.mss is deprecated" and that the number of
warnings whose message contains "is only available on" equals
expected_count[platform.system()]; keep the subsequent with context as sct and
isinstance(sct, MSSBase) check unchanged.
---
Outside diff comments:
In `@src/mss/linux/base.py`:
- Around line 427-435: The docstring for _grab_xgetimage(Monitor) is outdated:
the function now returns raw pixel bytes (bytearray) instead of a screenshot
object; update the docstring to state it returns a bytearray of raw pixel data
(include pixel format/stride/order if known) and describe the shape/ordering
(e.g., RGB/RGBA per pixel, row-major, bytes per pixel) so callers using
_grab_xgetimage or the XShmGetImage fallback know how to interpret the returned
bytearray.
---
Nitpick comments:
In `@CHANGES.md`:
- Line 6: Update the changelog text to explicitly list the deprecated
compatibility entry points and the recommended replacement: state that the old
factory function mss.mss() and any platform-specific classes mss.<platform>.MSS
are deprecated and that users should migrate to the new mss.MSS class which now
holds an MSSImplementation instance (the renaming from MSSBase methods to
MSSImplementation methods). Mention that base.py was most affected and include a
short migration note: replace calls to mss.mss() or mss.<platform>.MSS with
creating an instance of mss.MSS instead.
In `@docs/source/examples/fps.py`:
- Line 43: The fps.py example creates the MSS instance without a context manager
(sct = mss.MSS()), so refactor to use the recommended context-manager pattern:
replace the standalone sct = mss.MSS() with a with mss.MSS() as sct: block and
move the existing while loop and any code that uses sct inside that block so the
MSS instance is automatically closed via __exit__; update any related variable
scope accordingly to match other examples.
In `@docs/source/usage.rst`:
- Around line 153-155: The CLI help text in usage.rst is inconsistent with the
argparse definition: update the documented option line to exactly match the
argparse flag names and formatting (i.e., include both "-b, --backend BACKEND")
and ensure the following multi-line description matches argparse's wrapped help
(platform-specific backend to use (Linux: default/xlib/xgetimage/xshmgetimage;
macOS/Windows: default)); locate the argparse option definition for backend in
__main__.py (the parser.add_argument call for "-b/--backend") and mirror its
flag names and help text/line-wrapping in the usage.rst entry so the docs match
the actual --help output.
In `@src/mss/darwin.py`:
- Around line 267-269: The cursor() method currently ends with an implicit
return; change it to an explicit return None to match its declared return type
and mirror the Windows implementation—update the cursor function in class/method
named cursor() to end with "return None" so the intent and type annotation (->
None) are clear.
In `@src/mss/linux/xgetimage.py`:
- Around line 21-24: The docstring's seealso reference points to
mss.linux.base.MSSXCBBase but the class now inherits from MSSImplXCBBase; update
the cross-reference in the docstring to
:py:class:`mss.linux.base.MSSImplXCBBase` (or the correct import path for
MSSImplXCBBase) so the seealso matches the actual base class used by the class
in src/mss/linux/xgetimage.py.
In `@src/mss/linux/xlib.py`:
- Around line 451-454: The code redundantly checks _XFIXES inside the
effective_with_cursor branch; update the block in xlib so when
effective_with_cursor is true you directly call cdll.LoadLibrary(_XFIXES) to set
self.xfixes (remove the inner "if _XFIXES is not None" check), keeping the outer
condition that uses effective_with_cursor (which is defined as
requested_with_cursor and bool(_XFIXES)); ensure you still satisfy type-checking
(MyPy) by preserving the same branch so the loader only executes when _XFIXES is
guaranteed truthy.
In `@src/mss/windows.py`:
- Around line 413-415: The cursor method currently uses a bare "return" which
implicitly returns None; update the cursor() function to use an explicit "return
None" to match its annotated return type and improve readability (replace the
existing "return" statement in def cursor(self) -> None: with "return None").
In `@src/tests/bench_grab_windows.py`:
- Around line 112-123: The benchmark_raw_bitblt test currently does mss.MSS()
and assumes the default backend; instead instantiate the MSS with the
Windows/GDI backend explicitly so sct._impl is guaranteed to be
mss.windows.MSSImplWindows and the measured fields (_srcdc, _memdc) are valid;
update the code that creates sct (replace the mss.MSS() call) to construct or
pass the Windows backend implementation (MSSImplWindows) so the test always pins
to the GDI backend before calling sct.grab(...) and accessing
sct._impl._srcdc/_memdc.
In `@src/tests/test_gnu_linux.py`:
- Around line 80-102: Update the platform-specific tests to assert a single,
specific exception per OS rather than broad tuples: for the macOS branch replace
pytest.raises((ScreenShotError, ValueError)) with the single confirmed exception
type raised by mss.MSS on Darwin (e.g., ScreenShotError or ValueError) and for
the Windows branch replace pytest.raises((ImportError, ScreenShotError)) with
the single confirmed exception (e.g., ImportError or ScreenShotError); locate
these in the test where monkeypatch.setattr(platform, "system", ...) and
mss.MSS(backend=backend) are used, run the cross-platform behavior check to
determine the exact exception type, update the assertions accordingly, and
remove or update the TODO comment once the expected exceptions are narrowed and
documented.
In `@src/tests/test_windows.py`:
- Around line 21-23: The tests currently instantiate the default mss.MSS() and
assert that sct._impl is mss.windows.MSSImplWindows, which couples the tests to
the default backend; instead, import and instantiate the concrete implementation
mss.windows.MSSImplWindows (or otherwise construct the concrete Windows backend
object used in the test) and run the same assertions against that concrete
instance (e.g., check its _dib behavior) so the tests no longer depend on MSS()
choosing a particular default; apply the same change to the other similar block
mentioned (the assertions around lines 44-50) so both tests pin the concrete
Windows implementation rather than asserting MSS()'s default.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8349cc9-afd4-4c56-a88f-008b5a968c69
📒 Files selected for processing (59)
CHANGELOG.mdCHANGES.mdREADME.mddemos/cat-detector.pydemos/tinytv-stream-simple.pydemos/tinytv-stream.pydemos/video-capture-simple.pydemos/video-capture.pydocs/source/api.rstdocs/source/conf.pydocs/source/examples.rstdocs/source/examples/callback.pydocs/source/examples/custom_cls_image.pydocs/source/examples/fps.pydocs/source/examples/fps_multiprocessing.pydocs/source/examples/from_pil_tuple.pydocs/source/examples/linux_display_keyword.pydocs/source/examples/linux_xshm_backend.pydocs/source/examples/opencv_numpy.pydocs/source/examples/part_of_screen.pydocs/source/examples/part_of_screen_monitor_2.pydocs/source/examples/pil.pydocs/source/examples/pil_pixels.pydocs/source/index.rstdocs/source/usage.rstsrc/mss/__init__.pysrc/mss/__main__.pysrc/mss/base.pysrc/mss/darwin.pysrc/mss/factory.pysrc/mss/linux/__init__.pysrc/mss/linux/base.pysrc/mss/linux/xgetimage.pysrc/mss/linux/xlib.pysrc/mss/linux/xshmgetimage.pysrc/mss/windows.pysrc/tests/bench_bgra2rgb.pysrc/tests/bench_general.pysrc/tests/bench_grab_windows.pysrc/tests/conftest.pysrc/tests/test_cls_image.pysrc/tests/test_compat_10_1.pysrc/tests/test_compat_exports.pysrc/tests/test_compat_linux_api.pysrc/tests/test_find_monitors.pysrc/tests/test_get_pixels.pysrc/tests/test_gnu_linux.pysrc/tests/test_implementation.pysrc/tests/test_issue_220.pysrc/tests/test_leaks.pysrc/tests/test_macos.pysrc/tests/test_primary_monitor.pysrc/tests/test_save.pysrc/tests/test_setup.pysrc/tests/test_tools.pysrc/tests/test_windows.pysrc/tests/test_xcb.pysrc/tests/third_party/test_numpy.pysrc/tests/third_party/test_pil.py
💤 Files with no reviewable changes (1)
- docs/source/api.rst
Comment on lines
+525
to
+533
| @property | ||
| def max_displays(self) -> int: | ||
| """Maximum number of displays to handle. | ||
|
|
||
| Availability: macOS | ||
|
|
||
| .. versionadded:: 8.0.0 | ||
| """ | ||
| return self._impl.max_displays # type: ignore[attr-defined] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if max_displays is defined on non-Darwin implementations
ast-grep --pattern 'class MSSImplWindows($$$) {
$$$
max_displays
$$$
}'
rg -n 'max_displays' src/mss/linux/ src/mss/windows.pyRepository: jholveck/python-mss
Length of output: 45
🏁 Script executed:
# First, understand the repository structure
cd src/mss && ls -laRepository: jholveck/python-mss
Length of output: 963
🏁 Script executed:
# Find all implementation files
find src/mss -name "*.py" -type f | head -20Repository: jholveck/python-mss
Length of output: 452
🏁 Script executed:
# Search for max_displays in the entire codebase
rg -n 'max_displays' src/mss/Repository: jholveck/python-mss
Length of output: 1450
🏁 Script executed:
# Find all implementation class definitions
rg -n 'class MSSImpl' src/mss/Repository: jholveck/python-mss
Length of output: 513
🏁 Script executed:
# Check the base.py code around line 220 to see how max_displays is validated
sed -n '210,230p' src/mss/base.pyRepository: jholveck/python-mss
Length of output: 1080
🏁 Script executed:
# Check if MSSImplWindows has max_displays attribute
sed -n '185,250p' src/mss/windows.pyRepository: jholveck/python-mss
Length of output: 2436
🏁 Script executed:
# Check MSSImplXlib implementation
sed -n '390,450p' src/mss/linux/xlib.pyRepository: jholveck/python-mss
Length of output: 2260
🏁 Script executed:
# Look at the max_displays property and surrounding context in base.py
sed -n '520,540p' src/mss/base.pyRepository: jholveck/python-mss
Length of output: 636
🏁 Script executed:
# Check how _impl is initialized in the MSS class
rg -n '_impl' src/mss/base.py | head -20Repository: jholveck/python-mss
Length of output: 713
🏁 Script executed:
# Look at the __init__ method of MSS class to see how _impl is set
sed -n '190,250p' src/mss/base.pyRepository: jholveck/python-mss
Length of output: 2731
🏁 Script executed:
# Let's verify what happens at runtime by checking if there's any guard in the property or elsewhere
grep -n 'def max_displays' src/mss/base.py -A 10Repository: jholveck/python-mss
Length of output: 383
🏁 Script executed:
# Double-check that MSSImplWindows and MSSImplXlib truly don't have max_displays
grep -n 'max_displays' src/mss/windows.py src/mss/linux/xlib.pyRepository: jholveck/python-mss
Length of output: 45
🏁 Script executed:
# Check if there's any platform check in the property getter
sed -n '525,540p' src/mss/base.pyRepository: jholveck/python-mss
Length of output: 544
max_displays property will raise AttributeError on non-Darwin platforms.
The max_displays property accesses self._impl.max_displays unconditionally with a type: ignore[attr-defined] comment, but this attribute only exists in MSSImplDarwin. On Windows and Linux, the implementation classes (MSSImplWindows and MSSImplXlib) do not define this attribute, so accessing the property will raise AttributeError at runtime.
The docstring documents "Availability: macOS", but this limitation is not enforced. Consider adding a runtime guard that checks the platform before accessing the attribute, or raising a more explicit error message.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mss/base.py` around lines 525 - 533, The max_displays property currently
accesses self._impl.max_displays unconditionally which will raise AttributeError
on non-macOS implementations; modify the property (max_displays) to perform a
runtime guard (e.g. check platform.system() == "Darwin" or
isinstance(self._impl, MSSImplDarwin)) and only return self._impl.max_displays
for macOS, otherwise raise a clear NotImplementedError/PlatformError with a
message like "max_displays is only available on macOS (MSSImplDarwin)"; keep the
docstring but enforce the platform check to avoid AttributeError when self._impl
is MSSImplWindows or MSSImplXlib.
| if args: | ||
| full_msg += " | " + ", ".join(str(arg) for arg in args) | ||
| self.shm_fallback_reason = full_msg | ||
| self.performance_status.append(full_msg) |
There was a problem hiding this comment.
Keep performance_status semantically one-sided.
This list is used for fallback diagnostics here, but the success path also appends a healthy-state message. That makes a non-empty performance_status ambiguous for callers trying to detect degraded capture behavior. Either keep it issue-only, or document/rename it as a general event log.
Also applies to: 201-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mss/linux/xshmgetimage.py` at line 74, The code currently appends
healthy-state messages to self.performance_status (e.g., the
self.performance_status.append(full_msg) call), making the list ambiguous;
change the logic so performance_status only records issues: remove or
conditionalize any append of non-error/healthy messages (including the other
occurrences around the block at lines ~201-203) and only append when a real
degraded/error condition is detected (preserve error message content and use the
same identifier full_msg when adding). If you prefer an audit trail instead,
rename performance_status to something like performance_events and update
callers accordingly, but do not leave both healthy and issue entries in
performance_status.
Comment on lines
+160
to
+167
| with ( | ||
| pytest.warns(DeprecationWarning, match=r"^mss\.mss is deprecated"), | ||
| pytest.warns(DeprecationWarning, match=r"is only available on"), | ||
| ): | ||
| context = mss.mss(**kwargs) | ||
|
|
||
| with context as sct: | ||
| assert isinstance(sct, MSSBase) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if this test is skipped on Linux or has platform-specific handling
rg -n -A5 'test_deprecated_factory_accepts_documented_kwargs' src/tests/Repository: jholveck/python-mss
Length of output: 551
🏁 Script executed:
cat -n src/tests/test_compat_10_1.py | head -180 | tail -50Repository: jholveck/python-mss
Length of output: 2178
🏁 Script executed:
# Get the complete test function and check for platform-specific decorators
rg -n -B5 'def test_deprecated_factory_accepts_documented_kwargs' src/tests/test_compat_10_1.py | head -20Repository: jholveck/python-mss
Length of output: 304
🏁 Script executed:
# Check if there are any skipif decorators on this test
rg -n -B10 'def test_deprecated_factory_accepts_documented_kwargs' src/tests/test_compat_10_1.py | grep -E '(skipif|pytest.mark|sys.platform)'Repository: jholveck/python-mss
Length of output: 45
🏁 Script executed:
# Find the mss.mss factory function implementation
fd -t f '\.py$' -path '*/mss/*' | grep -v test | head -20Repository: jholveck/python-mss
Length of output: 231
🏁 Script executed:
# Search for the mss factory function and where warnings are emitted
rg -n "def mss" --type py | grep -v testRepository: jholveck/python-mss
Length of output: 115
🏁 Script executed:
# Search for where the "is only available on" warning is emitted
rg -n "is only available on" --type pyRepository: jholveck/python-mss
Length of output: 378
🏁 Script executed:
# Check the warning context in base.py
sed -n '215,240p' src/mss/base.pyRepository: jholveck/python-mss
Length of output: 1072
🏁 Script executed:
# Check the factory implementation
cat -n src/mss/factory.pyRepository: jholveck/python-mss
Length of output: 844
🏁 Script executed:
# Check the full test function including any decorators
rg -n -B15 'def test_deprecated_factory_accepts_documented_kwargs' src/tests/test_compat_10_1.py | head -25Repository: jholveck/python-mss
Length of output: 763
🏁 Script executed:
# Look for conftest or any pytest markers that might skip this test
find src/tests -name "conftest.py" -o -name "*.py" | xargs grep -l "test_deprecated_factory_accepts_documented_kwargs"Repository: jholveck/python-mss
Length of output: 92
🏁 Script executed:
# Check the entire test file for any module-level pytest.mark decorators or skipif conditions
head -50 src/tests/test_compat_10_1.pyRepository: jholveck/python-mss
Length of output: 1248
🏁 Script executed:
# Verify the platform module behavior to confirm our understanding
rg -n "platform.system()" src/mss/base.py -A2 -B2Repository: jholveck/python-mss
Length of output: 373
Fix the nested pytest.warns to account for platform-specific warning counts.
The test expects exactly two "is only available on" warnings via the nested pytest.warns context managers. However, the actual number of warnings depends on the platform:
- Linux: Only 1 warning (max_displays is invalid; with_cursor and display are valid)
- Darwin: 2 warnings (with_cursor and display are invalid; max_displays is valid)
- Windows: 3 warnings (all three are invalid)
The test needs to be refactored to either use conditional logic based on platform.system() or consolidate the warnings check to match platform-specific behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tests/test_compat_10_1.py` around lines 160 - 167, The nested
pytest.warns should be made platform-aware: import platform and compute
expected_count = {"Linux":1,"Darwin":2,"Windows":3}. Replace the two nested
pytest.warns blocks around mss.mss(**kwargs) with a single
pytest.warns(DeprecationWarning) as w (or use warnings.catch_warnings) when
calling context = mss.mss(**kwargs), then assert that one DeprecationWarning
matches r"^mss\.mss is deprecated" and that the number of warnings whose message
contains "is only available on" equals expected_count[platform.system()]; keep
the subsequent with context as sct and isinstance(sct, MSSBase) check unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See also BoboTiG/python-mss issue BoboTiG#486.
The user will always work with a single class: mss.MSS. Differences in implementation, such as platform or capture strategy, are hidden in an internal implementation object held by the mss.MSS object.
This allows us to change the implementation, with arbitrary class hierarchies, without worrying about preserving compatibility with internal class names.
This deprecates the existing mss factory function, although it can easily be kept for as long as needed to give users time to adapt.
It also deprecates the existing mss.{platform}.MSS types. These are exposed to the user, so somebody calling mss.{platform}.MSS() in 10.x can still reasonably expect to get a mss.{platform}.MSS object back. However, in 11.0, we can remove the type entirely, and either remove those symbols, or make them deprecated aliases for mss.MSS.
Where possible, deprecated functionality emits a DeprecationWarning. However, note that these are ignored by default, unless triggered by code in main.
Many of the API docs are removed, since this change removes much of the API surface. However, they are still in available for backwards-compatibility.
This change adds tests for everything that was in the 10.1 docs, examples, etc, at least at a basic level: for instance, it tests that mss.linux.MSS still works as both a constructor and a type (for isinstance), and that mss.linux.ZPIXMAP still exists (it was listed in the 10.1 docs).
The existing code, tests, and docs are changed to use mss.MSS.
--
As a contextual note, 10.1 (the most recent release) had only a single Linux backend, one based on Xlib.
One major change in 10.2 is the idea of us managing different backends, such as some that might be based on XComposite or capture to CUDA or do other things that require very different implementations. Another use case for multiple backends would be the different screen capture APIs that Windows exposes, which may or may not be applicable depending on the Windows version and use case.
Originally, I added the new Linux XCB-based backends (one using XShmGetImage and one using XGetImage), and made mss.linux.MSS - previously the class that was the only Linux implementation - be a factory function. But then I realized that this would break type declarations.
As I considered this further, I realized that a larger shift may be necessary, to allow us to have a consistent, stable API, while we change the internals. Rather than having the implementations be classes that inherit from the base class, it would make more sense to use a strategy pattern.
One important thing about this history lesson: the new Linux backends have not yet been part of a public release. That also includes the mss.linux.mss factory function that existed in main prior to this change; we can safely remove that, since it was never in a public release.
Summary by CodeRabbit
New Features
mss.MSS()as the recommended screen capture interfaceperformance_statusproperty for backend health monitoringDocumentation
mss.MSS()API usageDeprecations
mss.mss()factory function deprecated in favor ofmss.MSS(); still available in v10.2 for compatibility