Windows: capture cursor #272

toxicrecker · 2024-01-25T10:11:31Z

Changes proposed in this PR

Capture cursor implemented for windows

It is very important to keep up to date tests and documentation.

Tests added/updated
Documentation updated

Is your code right?

PEP8 compliant
flake8 passed

sourcery-ai

PR Type: Enhancement

PR Summary: The pull request introduces functionality to capture the cursor on Windows systems. It adds new structures to hold cursor and icon information, updates function mappings for cursor-related API calls, and implements methods to capture cursor data and handle monochrome cursors.

Decision: Comment

📝 Type: 'Enhancement' - not supported yet.

Sourcery currently only approves 'Typo fix' PRs.

✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.

No details provided.

✅ Small diff: the diff is small enough to approve with confidence.

No details provided.

General suggestions:

Ensure that the new cursor capture functionality is robustly integrated with the existing codebase, including proper error handling and validation of API call results.
Clarify the necessity of casting 'memdc' to 'HDC' in the 'DrawIcon' call and document the reasoning within the code to aid future maintainability.
Consider the implications of the change in the return type of '_grab_impl' to 'Optional[ScreenShot]' and verify that all calling code is updated to handle potential 'None' values.

Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨

Share Sourcery

Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

sourcery-ai · 2024-01-25T10:12:51Z

src/mss/windows.py

+        self._handles.ci = ci
+
+        iconinfo = ICONINFO() # 'ii' felt uncomfortable
+        self._handles.iconinfo = iconinfo


issue (llm): The initialization of 'iconinfo' without checking the return value of 'GetIconInfo' could lead to issues if 'GetIconInfo' fails. It would be safer to check the return value before assuming that 'iconinfo' has been properly initialized.

sourcery-ai · 2024-01-25T10:12:51Z

src/mss/windows.py

+            self._handles.bmp = gdi.CreateCompatibleBitmap(srcdc, width, height)
+            gdi.SelectObject(memdc, self._handles.bmp)
+
+        user32.DrawIcon(HDC(memdc), 0, 0, hcursor) # Why HDC? [1]


question (llm): The comment '[1]' questions the need for casting 'memdc' to 'HDC'. It's important to clarify whether this cast is indeed necessary and to document the reason in the code to avoid confusion for future maintainers.

BoboTiG

Great PR, thank you !

Would you mind adding tests like I did in https://github.com/BoboTiG/python-mss/blob/e66199adcdad737ef4d66183083bd26c827090da/src/tests/test_gnu_linux.py 🙏🏻 ?

BoboTiG · 2024-01-25T10:52:07Z

src/mss/windows.py

@@ -121,6 +155,13 @@ def __init__(self, /, **kwargs: Any) -> None:
        bmi.bmiHeader.biClrImportant = 0  # See grab.__doc__ [3]
        self._handles.bmi = bmi

+        ci = CURSORINFO()


Could you use more explicit name? cusor_info seems a good one.

sure, will do

BoboTiG · 2024-01-25T10:53:34Z

src/mss/windows.py

+        ci.cbSize = ctypes.sizeof(CURSORINFO)
+        self._handles.ci = ci
+
+        iconinfo = ICONINFO()  # 'ii' felt uncomfortable


Suggested change

iconinfo = ICONINFO() # 'ii' felt uncomfortable

icon_info = ICONINFO()

yea i will fix that too

BoboTiG · 2024-01-25T10:56:59Z

src/mss/windows.py

@@ -200,7 +241,7 @@ def _callback(monitor: int, data: HDC, rect: LPRECT, dc_: LPARAM) -> int:
        callback = MONITORNUMPROC(_callback)
        user32.EnumDisplayMonitors(0, 0, callback, 0)

-    def _grab_impl(self, monitor: Monitor, /) -> ScreenShot:
+    def _grab_impl(self, monitor: Monitor, /) -> Optional[ScreenShot]:


I would like to not change that part. If the process fails, then an exception will be raised.

it was changed for the linux implementation so i did it but i can see why you would like to avoid that

We are all good on Linux side: https://github.com/BoboTiG/python-mss/blob/v9.0.1/src/mss/linux.py#L424

BoboTiG · 2024-01-25T10:59:44Z

src/mss/windows.py

+        cursors the alpha value of every pixel is 0 for some reason. Therefore, the alpha
+        value of every non black pixel has to be manually set to 255.
+        """
+        srcdc, memdc = self._handles.srcdc, self._handles.memdc


I love the documentation 💪🏻

For the implementation, do we need to copy all from the grab() method? Can it be simplified to only the cursor itself?

i did not care to look into it but now that you said it i will see if something can be done about simplifying it to just the cursor
i will update you once im done

UPDATE: im afraid it would not be possible since the cursor is a part of the screen's device context, it should not be a huge issue as im making a small 32x32 bitmap and capturing only the cursor's image
im sorry if im wrong, im not an expert at windows api and i would love to learn something new

toxicrecker · 2024-01-25T12:11:16Z

Great PR, thank you !

Would you mind adding tests like I did in https://github.com/BoboTiG/python-mss/blob/e66199adcdad737ef4d66183083bd26c827090da/src/tests/test_gnu_linux.py 🙏🏻 ?

I have absolutely no experience in writing tests, sorry
Best I could do is copy paste what you did with linux tests and remove the hasattr(sct, "xfixes") assertation, I will do that if you want

toxicrecker · 2024-01-25T12:36:44Z

I was trying to make sense of how your linux tests worked, and I assume that you try taking screenshots of a black screen one with cursor and one without cursor and then check if you find an RGB value other than 0, am I right? And if I'm right how do you run this test? I can't make sense of how you are actually going to run this test in a way it does it's job because the screen is mostly never pitch black, do you have a second completely blank monitor that you use for this test? Sorry if I'm just being dumb I don't have any experience with testing.

BoboTiG · 2024-01-25T12:52:31Z

Windows tests will be more tricky, for sure.
I will be able to help on that side 👍🏻

toxicrecker · 2024-01-25T13:12:07Z

Windows tests will be more tricky, for sure. I will be able to help on that side 👍🏻

Thank you, is there anything else I could help with?

BoboTiG · 2024-01-25T13:21:46Z

Thank you, is there anything else I could help with?

It's all good for now. I'll check the code, test it on a VM, and get back to the PR when I'll have some time.

Pawloland · 2024-05-04T10:42:54Z

src/mss/windows.py

+            "left": round(pos_screen.x * ratio - self._handles.icon_info.xHotspot),
+            "top": round(pos_screen.y * ratio - self._handles.icon_info.yHotspot),


Hi, I used this PR in my project and from my testing multiplying by ratio actually messes things up. When i removed it everything worked fine even on strange DPI scalings in multiple different positions of multi monitor setup. For testing I used https://github.com/pavlobu/deskreen with https://www.amyuni.com/forum/viewtopic.php?t=3030

toxicrecker added 3 commits January 24, 2024 12:34

captures non monochrome cursors on windows

de9c4c1

captures cursors on windows

cace5d5

using ctypes insetad of pywin32 extensions

1c77da6

sourcery-ai bot reviewed Jan 25, 2024

View reviewed changes

fixed most flake8 issues

d1306a7

BoboTiG reviewed Jan 25, 2024

View reviewed changes

minor changes

19cb97f

BoboTiG mentioned this pull request Jan 25, 2024

Mouse support #55

Open

Pawloland reviewed May 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows: capture cursor #272

Windows: capture cursor #272

toxicrecker commented Jan 25, 2024 •

edited

sourcery-ai bot left a comment

sourcery-ai bot Jan 25, 2024

sourcery-ai bot Jan 25, 2024

BoboTiG left a comment

BoboTiG Jan 25, 2024

toxicrecker Jan 25, 2024

BoboTiG Jan 25, 2024

toxicrecker Jan 25, 2024

BoboTiG Jan 25, 2024

toxicrecker Jan 25, 2024

BoboTiG Jan 25, 2024

BoboTiG Jan 25, 2024

toxicrecker Jan 25, 2024

toxicrecker Jan 25, 2024

toxicrecker commented Jan 25, 2024

toxicrecker commented Jan 25, 2024

BoboTiG commented Jan 25, 2024

toxicrecker commented Jan 25, 2024

BoboTiG commented Jan 25, 2024 •

edited

Pawloland May 4, 2024

	iconinfo = ICONINFO() # 'ii' felt uncomfortable
	icon_info = ICONINFO()

		"left": round(pos_screen.x * ratio - self._handles.icon_info.xHotspot),
		"top": round(pos_screen.y * ratio - self._handles.icon_info.yHotspot),

Windows: capture cursor #272

Are you sure you want to change the base?

Windows: capture cursor #272

Conversation

toxicrecker commented Jan 25, 2024 • edited

Changes proposed in this PR

sourcery-ai bot left a comment

Choose a reason for hiding this comment

sourcery-ai bot Jan 25, 2024

Choose a reason for hiding this comment

sourcery-ai bot Jan 25, 2024

Choose a reason for hiding this comment

BoboTiG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toxicrecker commented Jan 25, 2024

toxicrecker commented Jan 25, 2024

BoboTiG commented Jan 25, 2024

toxicrecker commented Jan 25, 2024

BoboTiG commented Jan 25, 2024 • edited

Choose a reason for hiding this comment

toxicrecker commented Jan 25, 2024 •

edited

BoboTiG commented Jan 25, 2024 •

edited