Kalmat / PyWinCtl

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

Implemented getTopWindowAt #22

Closed Avasam closed 2 years ago

Avasam commented 2 years ago

Closes #20. Incidentally Fixes #18

Some notes:

Iterator + break loop early, at 5 windows

timeit.timeit("pywinctl._pywinctl_linux.getTopWindowAt(200, 200)", setup="import pywinctl", number=1000) 3.6824550530000124 timeit.timeit("pywinctl._pywinctl_linux.getTopWindowAt(200, 200)", setup="import pywinctl", number=1000) 3.649093485000776 timeit.timeit("pywinctl._pywinctl_linux.getTopWindowAt(200, 200)", setup="import pywinctl", number=1000) 3.676199136003561

# Reusing getWindowsAt, at 2 windows
>>> timeit.timeit("pywinctl._pywinctl_linux.getTopWindowAt(200, 200)", setup="import pywinctl", number=1000)
7.498686598002678
>>> timeit.timeit("pywinctl._pywinctl_linux.getTopWindowAt(200, 200)", setup="import pywinctl", number=1000)
7.513227786002972
>>> timeit.timeit("pywinctl._pywinctl_linux.getTopWindowAt(200, 200)", setup="import pywinctl", number=1000)
7.930830400997365

# Iterator + break loop early, at 2 windows
>>> timeit.timeit("pywinctl._pywinctl_linux.getTopWindowAt(200, 200)", setup="import pywinctl", number=1000)
3.472665073997632
>>> timeit.timeit("pywinctl._pywinctl_linux.getTopWindowAt(200, 200)", setup="import pywinctl", number=1000)
3.4019074109965004
>>> timeit.timeit("pywinctl._pywinctl_linux.getTopWindowAt(200, 200)", setup="import pywinctl", number=1000)
3.604884767002659

Note this means there's a performance improvement to be had everywhere we get all windows. I don't think getAllWindows itself should return a generator (as it's public API I think it's ok to make it a list and be consistent with the rest of the exposed methods). But they could all use a __getAllWindowsGenerator under the hood.
I didn't want to scope creep this PR, so I'll leave this for another time.

Kalmat commented 2 years ago

Hey! Sorry for the delay in my reply. These days, I have very limited time and access to all my "things".

Thank you so much for your time and help. I can not test, extend to all other methods and merge this all at the moment, I hope it's not a problem for you (if it is, just let me know and I will do my best!). Besides, I have to carefully read your advices and your code to understand everything. Bear in mind I have never studied nor worked in Python (I learned Python, very recently, and mainly on stackoverflow, HAHAHAHA!, Let's say it's sad but true!), so this all is really helpful to me!!!

Regarding your comments:

  1. I had to search what a "generator" is in Python HAHAHAHA! Thank you!
  2. I'm not using map, filter nor reduce, right? (Just to be sure. If so, I will eliminate them all)
  3. Not sure what means "using getAllWindowsGenerator". I think "remove_bad_windows" is in fact a generator, isn't it? So I can extend it (or a similar solution) to all other methods in which it can be necessary.
  4. Take into account macOS version (again macOS, yes...) uses AppleScript, which is slow and resource-consuming. It's strongly recommended to minimize how many scripts you need to run (this is why I pass the app and the geometry rect to MacOSWindow class, whenever I have them, in order to avoid the need to look for them running another AppScript). Besides, to have access to all methods and properties, a mix of Quartz objects and AppScripts (window level) together with NSRunninApplication (AppKit methods) is required.... but they have no "link" between them all apart from the pID or the title. Finally, that module is full of "hacks" which allow to have same features as the others, but it is complex and risky to modify. I'm referring all this just to explain why I need to put all together and test very carefully before publishing a new release.

Apart from this all, I think your code is fantastic and will work perfectly! Thank you for your help and "instruction".

Avasam commented 2 years ago
  1. Yep :)
  2. Not that've seen. I was just explaining my specific implementation.
  3. __remove_bad_windows is indeed a generator. But in this case it's more used as a workaround to filter a list using try-except. If it's used inside a list comprehension, it still has to yield all values. Since I knew I'd only have to check for 1-2 items in most cases, it was important I don't get the whole list. Just something to keep in mind for possible future improvements. And you are right it's totally reusable.
  4. I had noticed the speed issue. It makes sense and since I have basically no mac dev experience. Any solution is better than none for me. Although it's even more important here we don't run scripts we don't need to. You mentioned something about app ordering by last focused.

    Quartz returns the list of apps in "last to first focused" order, which in many cases may not match "upper".

I think it's worth implementing that way. Unfortunately I haven't had time, (and won't really have for the next 2 weeks) to look into it and read some apple documentation. So I threw together a very naïve implementation (maybe a NotImplementedError might be better at this point ? ). I've also hit other blockers with other module on Mac anyway for the project I'm working on. So feel free to edit that part/suggest code improvements.

No hurry on merging, I just install directly from my fork !

Kalmat commented 2 years ago

Understood and agree. I will complete that function on macOS as soon as I can, don't worry.

Have some nice days!