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

Use JXA for window status on macOS, include url in event data #52

Merged
merged 18 commits into from May 26, 2021

Conversation

iloveitaly
Copy link
Contributor

This will allow us to create rules based on domains instead of just the window titles for more accurate tracking.

@iloveitaly
Copy link
Contributor Author

@ErikBjare you look like the best person to review. Take a look and let me know what you think!

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 1, 2021

This pull request fixes 1 alert when merging 0e5c115 into c9bf820 - view on LGTM.com

fixed alerts:

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

@ErikBjare
Copy link
Member

ErikBjare commented May 1, 2021

Thanks for taking the time to work on this!

You're not the first to have made the change to JS, and there has been quite a bit of discussion around this, and why all AppleScript/JS approaches are undesirable from a performance standpoint. See #29 and #49.

Clever that you managed to get the URL from the Accessibility API! However, the planned/preferred platform- and browser-independent way is to use aw-watcher-web to collect URLs, we just need to get them merged into the window events (there's an open PR in aw-server-rust that aims to do just that).

@iloveitaly
Copy link
Contributor Author

Thanks for the quick reply! #49 looks interesting—the event-based approach is definitely an improvement over polling!

However, the planned/preferred platform- and browser-independent way is to use aw-watcher-web to collect URLs

Curious why this is the preferred approach. For FireFox, it's the only approach, but for the all Chrome-based browsers and Safari the jxa approach is nearly the same (you could execute javascript on the tab to extract additional properties if needed).

From a UX perspective, extensions are a worse experience. Users would need to install a separate browser extension, on each browser they use. If they use Brave, Chrome, and Google Chrome Canary they'd need to install a separate browser on each. Then, each of those extensions would send their individual heartbeats to aw-server, which adds more load to the system (more heartbeats). Additionally, updates to the extension wouldn't be deployed at the same time as updates to the core AW software.

I think for FF, an extension is definitely needed, but I wonder if we could keep things more simple and use JXA for all of the other browsers.

and why all AppleScript/JS approaches are undesirable from a performance standpoint

A JXA approach definitely won't be as performant as a pyobjc bridge-based solution, but (a) if we are using eventing vs polling and (b) if we executed JXA directly vs executing via osascript my guess is the performance his would be negligible, especially when compared to heartbeats running across 2+ browsers on a user's machine.

What do you think? Thanks for being open to a conversation here!

@ErikBjare
Copy link
Member

ErikBjare commented May 1, 2021

Curious why this is the preferred approach.

Because its the only approach on other platforms than macOS.

From a dev perspective it's important that things work roughly the same way across platforms, to minimize special/platform-specific cases.

I'm still open to merging something like this and having the URL watching enabled either by default or through a flag/config option, but care needs to be taken so that it doesn't conflict with the web extension approach.

Additionally, updates to the extension wouldn't be deployed at the same time as updates to the core AW software.

I don't think this is an issue. WebExtensions generally update automatically, and breaking changes in the heartbeat API should be extremely rare.

(performance comments)

If you can make the AppleScript/JXA more performant by calling it directly instead of spawning a new process, that'd be great!

But in the end the AppleScript approach (and I assume the same holds for JXA) has been a frequent cause of issues/bugs (often relating to permissions). It makes sense that such is the case, as it seems the AppleScript approach gives us permissions we otherwise shouldn't have (as seen in #49 where I need more permissions than required by the AppleScript approach).

...especially when compared to heartbeats running across 2+ browsers on a user's machine.

The web watcher doesn't use polling, so the number of browsers used shouldn't impact performance.

@ErikBjare
Copy link
Member

ErikBjare commented May 1, 2021

And as I mentioned in #49, I'm open to having multiple approaches merged and having configuration options for which specific one to use.

This would help evaluate the different ways to go about this, without getting bogged down by having to choose a particular approach (avoids risk of PRs getting stale/abandoned).

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 1, 2021

This pull request fixes 1 alert when merging e93d6be into c9bf820 - view on LGTM.com

fixed alerts:

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

@iloveitaly
Copy link
Contributor Author

@ErikBjare rewrote this to avoid shelling out to osascript for execution. Going to let this bake locally for a bit to see if any issues come up.

This feels like a improvement over the existing implementation and will only run on macOS, so I'm not sure we need a flag to enable this right now, although that may make sense when work on the other PR is done.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 5, 2021

This pull request fixes 1 alert when merging 9d5580b into c9bf820 - view on LGTM.com

fixed alerts:

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

@iloveitaly
Copy link
Contributor Author

Another benefit to this implementation which I just noticed is Electron-based applications report their real name, instead of simply "Electron"

Copy link
Member

@ErikBjare ErikBjare left a comment

Choose a reason for hiding this comment

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

Looks good!

I want to merge this, but in case there are any unexpected side effects (on old/new macOS versions or such) I'd like to keep the old approach in the codebase.

Would you mind moving the JXA approach to a new file and restore the old one? Then add a config option to let users set which method to use.

And again: Great work! Looking forward to our call tomorrow :)



def get_current_window_macos() -> Optional[dict]:
# TODO should we use unknown when the title is blank like the other platforms?
Copy link
Member

Choose a reason for hiding this comment

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

I think so yes, I'll elaborate in #54.

@ErikBjare
Copy link
Member

@iloveitaly Just a friendly ping that I'd really like to get this merged.

If you could make the requested change I'll merge it asap :)

@iloveitaly
Copy link
Contributor Author

@ErikBjare updated! Let me know what you think

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 18, 2021

This pull request introduces 1 alert and fixes 1 when merging 9c88ae9 into c9bf820 - view on LGTM.com

new alerts:

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

fixed alerts:

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

Copy link
Member

@ErikBjare ErikBjare left a comment

Choose a reason for hiding this comment

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

Looks good!

A few nits, mostly regarding variable naming. Also:

  • Need to restore the printAppTitle.scpt file

aw_watcher_window/lib.py Outdated Show resolved Hide resolved
aw_watcher_window/lib.py Outdated Show resolved Hide resolved
aw_watcher_window/main.py Outdated Show resolved Hide resolved
aw_watcher_window/main.py Outdated Show resolved Hide resolved
aw_watcher_window/main.py Outdated Show resolved Hide resolved
aw_watcher_window/lib.py Outdated Show resolved Hide resolved
"exclude_title": False,
"poll_time": "1.0"
"poll_time": "1.0",
"macos_strategy": "jxa"
Copy link
Member

@ErikBjare ErikBjare May 23, 2021

Choose a reason for hiding this comment

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

Suggested change
"macos_strategy": "jxa"
"strategy_macos": "jxa"

Copy link
Member

Choose a reason for hiding this comment

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

Did this change work without you having to manually edit the config file? (we've had some issues where new defaults won't be applied for old installs...)

Copy link
Member

Choose a reason for hiding this comment

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

I now realize why you chose to prepend macos_ to the strategy...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm confused here: do you want the var name to change before merging?

Copy link
Member

Choose a reason for hiding this comment

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

No sorry, I already applied the suggested change :)

aw_watcher_window/config.py Outdated Show resolved Hide resolved
@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 23, 2021

This pull request introduces 1 alert and fixes 1 when merging cd767df into c9bf820 - view on LGTM.com

new alerts:

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

fixed alerts:

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

@iloveitaly
Copy link
Contributor Author

  • Need to restore the printAppTitle.scpt file

This was restored, although it it modified from the original file. The source code is exactly the same, so I'm guessing the difference in the file was caused by the more recent macos version I'm on.

I've restored the original version in the latest push. Take a look and let me know if there are any other tweaks!

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 25, 2021

This pull request introduces 1 alert and fixes 1 when merging 31043e3 into c9bf820 - view on LGTM.com

new alerts:

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

fixed alerts:

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

@ErikBjare
Copy link
Member

Looks good! Waiting for the CI to pass, and then I'll merge :)

@ErikBjare
Copy link
Member

ErikBjare commented May 26, 2021

Looks like type checking fails in a few places. I'll clean it up.

For future PRs:

  • Run black (it's like prettier for Python) on your code, so formatting stays consistent and easy to read.
  • Run mypy (with make typecheck) to ensure types are consistent.

@ErikBjare ErikBjare changed the title Use JXA for window status on macOS, include url in data event payload Use JXA for window status on macOS, include url in event data May 26, 2021
@ErikBjare ErikBjare merged commit aecf47f into ActivityWatch:master May 26, 2021
@ErikBjare
Copy link
Member

ErikBjare commented May 26, 2021

Merged! 🎉

Thanks for all your work on this! Hopefully it'll lead to a lot more reliable macOS experience in the next release :)

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 26, 2021

This pull request introduces 1 alert and fixes 1 when merging 2ef5bb5 into 44e1904 - view on LGTM.com

new alerts:

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

fixed alerts:

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

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.

None yet

2 participants