Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Get macOS window title through python bindings instead of running AppleScript #40

Closed
wants to merge 8 commits into from

Conversation

xylix
Copy link
Contributor

@xylix xylix commented Mar 22, 2020

I merged master into https://github.com/ActivityWatch/aw-watcher-window/pull/29/files and started working on updating to the newer API's and implementing the feedback from the PR.

There's still a problem getting the window title with Quartz methods, even though this seems to be the recommended way.

This should help with fixing ActivityWatch/activitywatch#380 and ActivityWatch/activitywatch#376

ianobermiller and others added 4 commits February 14, 2018 23:07
- Switch from the opaque scpt file to a normal JS file using JavaScript for Automation.
- Grab the first window (which is the active one) and get its name as the title. This worked across all apps I tried, including Sublime, Atom, Chrome, Safari, and iTerm
- Clean up the api to the macos.py file (have it return the dict directly)
- Fix a typo in README
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 22, 2020

This pull request fixes 1 alert when merging 4e88cec into 25ae699 - view on LGTM.com

fixed alerts:

  • 1 for Module is imported with 'import' and 'import from'

@ErikBjare
Copy link
Member

There's still a problem getting the window title with Quartz methods, even though this seems to be the recommended way.

Any progress on this? What's the issue?

@xylix
Copy link
Contributor Author

xylix commented Mar 24, 2020

@ErikBjare I've almost gotten this cleaned up and working but there's a bug where the "current app" get's stuck to the app that was open when aw-watcher-window was first started which is really weird.

https://stackoverflow.com/questions/52000530/appkit-not-dynamically-sensing-the-frontmost-app this seems related to my problem, but using the "activeApplication" instead of "frontmostApplication" like the suggested solution is considered deprecated by apple.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 24, 2020

This pull request introduces 1 alert and fixes 1 when merging 38591c5 into 25ae699 - view on LGTM.com

new alerts:

  • 1 for Unused import

fixed alerts:

  • 1 for Module is imported with 'import' and 'import from'

@xylix
Copy link
Contributor Author

xylix commented Mar 24, 2020

Oh I guess it's related to this https://developer.apple.com/documentation/appkit/nsrunningapplication?language=objc

Some properties of an app are fixed, such as the bundle identifier. Other properties may vary over time, such as whether the app is hidden. Properties that vary can be observed with key-value observing, in which case the description comment for the method notes this capability.

Properties that vary over time are inherently race-prone. For example, a hidden app may unhide itself at any time. To ameliorate this, properties persist until the next turn of the main run loop in a common mode. For example, if you repeatedly poll an unhidden app for its hidden property without allowing the run loop to run, it will continue to return NO, even if the app hides, until the next turn of the run loop.

Since we don't have a main macOS Application loop the properties retrieved with appkit methods are static. It is possible to program an event based solution, but it would be different from the other platform solution. It would also probably be more battery efficient.

@ErikBjare
Copy link
Member

It's totally fine to go with a event-based option over polling, if you can and want to figure that out :)

@johan-bjareholt
Copy link
Member

event-based is more efficient so it should be recommended wherever it is possible. It's just a shame that none of the previous solutions did that.

aw-watcher-window-wayland also does it in an event-based manner, much more simple to implement actually IMO.

@xylix
Copy link
Contributor Author

xylix commented Mar 25, 2020

Okay yeah. I did some testing and pushed a work in progress version.

Looks to me like it would require an extra if platform=="macos" in the main.py to initialize the macOS event loop on the first time we call ´get_current_window()` on macOS or an initialization for the macOS itself. It does require at least a bit more state than the current macos window watcher code, that's for sure.

I'll try and look into the aw-watcher-wayland code for some ideas about the event based approach.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 25, 2020

This pull request introduces 2 alerts and fixes 1 when merging 7ffb3fc into 25ae699 - view on LGTM.com

new alerts:

  • 2 for Unused import

fixed alerts:

  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 28, 2020

This pull request introduces 2 alerts and fixes 1 when merging 52852ae into 25ae699 - view on LGTM.com

new alerts:

  • 2 for Unused import

fixed alerts:

  • 1 for Module is imported with 'import' and 'import from'

@xylix
Copy link
Contributor Author

xylix commented Mar 28, 2020

@johan-bjareholt @ErikBjare I've mostly got a hang of the current workings of aw-watcher-window and to me it looks like I'll need a separate, event loop based "main thread" for the event-based version, if I want to actually conserve user's cpu cycles / battery life.

If I just add a new event based window switch checker on a different thread it's going to be hacky and just run all the time on top of the existing heartbeat loop. I tried an approach in this latest commit where I initialize the macos event handlers on aw_watcher_window, but it doesn't still solve the event loop init problem.

The upside of creating a "proper" macos main event loop would be that we could tie into system sleep / suspend / lock events on top of the app switch event, and not even need a heartbeat (except maybe with a long delay, to save during long single app user-sessions).

I'm not really sure on how much macos activitywatch battery life has been a problem outside of bugs, and for that this seems like an overkill solution. It'd be nice to get the window-watching permissions fixed, but I'm not even sure if this will fix that.

Any input where to go with this? I'd personally be fine with committing more time into implementing an event based solution but it's probably not going to affect the user experience in the end very much.

@@ -38,12 +38,12 @@ def get_current_window_windows() -> Optional[dict]:
return {"appname": app, "title": title}


def get_current_window() -> Optional[dict]:
def get_current_window() -> Optional[Callable[[], Dict[str, str]]]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to get_current_window_fn?

@ErikBjare
Copy link
Member

ErikBjare commented Mar 28, 2020

If you think it'd be interesting to build a fully event-based macOS loop then feel free to. You might want to skip trying to consolidate it with the other platforms.

It'd be nice to get the window-watching permissions fixed, but I'm not even sure if this will fix that.

If the problem is triggering a dialog that asks for them: You could check if you have them (as I wrote how in ActivityWatch/activitywatch#376 (comment)) and if you don't open a dialog asking the user to give them if they don't exist (no qt needed, could use straight AppKit, examples here: https://programtalk.com/python-examples/AppKit.NSAlert.alertWithMessageText_defaultButton_alternateButton_otherButton_informativeTextWithFormat_/).

You could try to do that without switching to the event-based stuff, if you think that would fix the permission issue.

@johan-bjareholt
Copy link
Member

and to me it looks like I'll need a separate, event loop based "main thread" for the event-based version, if I want to actually conserve user's cpu cycles / battery life.

Correct, it would need a seperate code path.

Any input where to go with this? I'd personally be fine with committing more time into implementing an event based solution but it's probably not going to affect the user experience in the end very much.

Agreed, I'd consider it more of a nice to have rather than something needed maybe I was not clear on that.

Another thing which is better with an even based solution is better accuracy, but since the polling version has a 1s poll interval that's not much of an user experience improvement either.

@xylix
Copy link
Contributor Author

xylix commented Mar 28, 2020

@ErikBjare Oh right, I had actually forgotten about that simple fix. I created a work in progress example at #41 , it was very quick to work through with that approach.

@xylix xylix changed the title WIP: Macos window title WIP: Get macOS window title through python bindings instead of running AppleScript Mar 28, 2020
@ErikBjare
Copy link
Member

ErikBjare commented Apr 21, 2020

So from the new discussion in ActivityWatch/activitywatch#380 it appears the notification isn't a complete fix for pre-Catalina.

Are you still interested in working on this PR @xylix? Or would you rather work on something else? (understandable)

@xylix
Copy link
Contributor Author

xylix commented Apr 21, 2020

@ErikBjare At least for now I am interested. I've been very busy with school this month, it should resolve itself within 1.5 weeks though.

I can fix the comment on https://github.com/ActivityWatch/aw-watcher-window/pull/41/files tomorrow though, so that could help the situation a bit. Will see what I can do.

I think I will try to work on this for a couple more hours in beginning of May and then decide how to go forward.

@ErikBjare
Copy link
Member

Take your time, I'm also super busy with school stuff atm so I know your situation :)

@ErikBjare
Copy link
Member

Closing and resuming work in #49

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants