Kalmat / PyWinCtl

Cross-Platform module to get info on and control windows on screen
Other
179 stars 19 forks source link

Add a public method to get a window's `_rect` #37

Closed MestreLion closed 1 year ago

MestreLion commented 1 year ago

So, when trying to integrate PyWinCtl and Pillow's PIL.ImageGrab.grab() to take screenshots of a single window, I stumbled at an annoying API issue:

So a client must either call PIL.ImageGrab.grab(window._rect), directly accessing a private member, or PIL.ImageGrab.grab((*window.topleft, *window.bottomright)), which works fine but performs a lot of redundant conversions and property calls.

Any particular reason why Window has no public method to retrieve its _rect? It would be very convenient as it shares the same semantics as Pillow's bbox!

MestreLion commented 1 year ago

Speaking of recs and boxes, PyWInCtl's naming for its NamedTuple classes is a bit unfortunate:

So, in terms of interoperability with other popular libraries, the semantics are reversed! Your Rect should be Box and vice-versa! Rect is the most problematic, since you're actually using Pyrect for rect manipulation in Window, requiring you to do all sorts of conversions.

This is just a minor rant, I'm fully aware that right now it's probably too late to change the API.

Kalmat commented 1 year ago

HI!

There is no stone-written reason by which there is no rect property in the form you propose. It's a, let's say, "historical" issue.

As yourself realized, the module is based on PyRect (both, the original PyGetWindow, from which I forked PyWinCtl, and PyRect belong to the same author!!!). So I kept it when I started to work on it because I then knew even less than now! HAHAHAHA!

In addition to that, in MS-Windows the windows rects are in the form (x, y, right, bottom)... Since PyGetWindow was initially an MS-Windows only module, perhaps its box property is based on that (in return, in Linux and macOS, windows' geometry() is in the form (x , y, width, height))...

For sure we can add a rect property... But I don't see now any way to solve the problem of the "confusing" naming without breaking retro-compatibility... what if we call it "bbox"?

MestreLion commented 1 year ago

Also, why are you casting Window._rect coords to float in properties such as Window.topleft()? In both your Rect class and in Pygame's, "The coordinates for Rect objects are all integers"

MestreLion commented 1 year ago

I just noticed pyrect.Rect has a box property that returns a Box NamedTuple (left, top, width, height). This property is not in the original Pygame, it's a Pyrect-only extension, and this might be the origin of the misnaming. It also seems there is no method or property to directly get a (left, top, right, bottom) tuple.

For sure we can add a rect property... But I don't see now any way to solve the problem of the "confusing" naming without breaking retro-compatibility... what if we call it "bbox"?

Originally I considered suggesting to name it get_rect() (or even a rect() property with getter and setter) and call it a day, but...

Box semantics are less important than Rect's. First because there's no official "source" for the semantics of a box. PIL/Pillow might be popular but it's not authoritative, and they named the argument bbox, not box. There is no box in Pygame, and Pyrect decided for its box to use the same semantics as the rect itself (what a waste of a good name!). So anyone can use whatever semantics they want for box.

As for rect... well... that is a Pygame thing, I've never seen this in any other context, so to change its semantics by exposing a public rect/get_rect returning (left, top, right, bottom) would surprise many users and thus a bad idea. So I think it's a good thing that your _rect remain a private, implementation detail.

So we can't use box as it would break retro-compability, and we shouldn't use rect as it would have different semantics than the established one... hum. Is geometry ruled out too? (in X world this also means (x, y, width, height), right?) If it is, then go with bbox, or even bounding_box. Maybe bbox is better.

Kalmat commented 1 year ago

HAHAHAHA! Welcome to my world! This is the kind of discussions I use to have with myself whenever I have to add or modify anything... So having other ideas and points of view really help a lot! Ok then, let's go for the "bbox" property!

Taking a closer look, I am also confused why the original author used pywinctl.Rect as (x, y, right, bottom) while his own module pyrect.Rect is (x, y, width, height)... Perhaps the reason is that I was pointing before: win32gui.GetWindowRect returns a tuple in the form (x, y, right, bottom)

Also, why are you casting Window._rect coords to float in properties such as Window.topleft()? In both your Rect class and in Pygame's, "The coordinates for Rect objects are all integers"

This comes from a modification that another user so kindly did to adapt the module so it's now "typed" (to be honest, black magic to me!). pyrect.Rect accepts both, float and int:

class Rect(object):
    def __init__(
        self,
        left=0,
        top=0,
        width=0,
        height=0,
        enableFloat=False,
        readOnly=False,
        onChange=None,
        onRead=None,
    ):

But you're right it should not be the case in pywinctl. I will change it. Thank you again for pointing it out!!!

Now that we have defined what we expect from activate(), things get a little weird. A window can be:

So we have to determine what to do in every case...

One last thing: do you want me to set your user as "contributor"? To be honest, I don't know what that means in practical terms. I mean, I don't know if that facilitates any future contribution you want to do (I guess it does, but don't know how it works, sorry).

MestreLion commented 1 year ago

Now that we have defined what we expect from activate(), things get a little weird. A window can be:

  • (...) a lot of things (...)

So we have to determine what to do in every case...

I think the burden of that should not be on you, let the WM handle all those cases. PyWinCtl is issuing the proper _NET_ACTIVE_WINDOW request as per the EWMH Spec, so how this will be done is the WM's job. Remember Unity already did the right thing with a minimized window: restored it, brought to front and gave it focus. Let's just assume it'll know how to deal the other cases as well.

One last thing: do you want me to set your user as "contributor"? To be honest, I don't know what that means in practical terms. I mean, I don't know if that facilitates any future contribution you want to do (I guess it does, but don't know how it works, sorry).

Nah, don't bother. Those PRs are all I need to move on with my little bot project, and I think Github will automatically list me as a contributor (no write permission of course), and the commit credit is more than enough for me.

Kalmat commented 1 year ago

I think the burden of that should not be on you, let the WM handle all those cases. PyWinCtl is issuing the proper _NET_ACTIVE_WINDOW request as per the EWMH Spec, so how this will be done is the WM's job. Remember Unity already did the right thing with a minimized window: restored it, brought to front and gave it focus. Let's just assume it'll know how to deal the other cases as well.

I was thinking on other OSs as well, but you're right. It's better to let the WM make its own decissions.

Nah, don't bother. Those PRs are all I need to move on with my little bot project, and I think Github will automatically list me as a contributor (no write permission of course), and the commit credit is more than enough for me.

Sorry, no documentation at the moment. What is this "bot" for? Any kind of game?

MestreLion commented 1 year ago

It's a bot for a mini-game inside the game Exapunks. There's no README yet as I'm still in early stages of prototyping.