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: Ensure Accessibility permissions on macOS #41

Merged
merged 10 commits into from Jul 19, 2020

Conversation

xylix
Copy link
Contributor

@xylix xylix commented Mar 28, 2020

Screenshot 2020-03-28 at 17 43 21

Use AppKit to figure if we have accessibility perms and pyobjc to ask for them.

Still needs to:

  • ask with an actual buttoned dialog, for example something with an accept button that links to the correct settings page, a decline button and maybe a learn more link
  • If user declines not spam them with prompts, and instead just ask once per app lifetime

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.

Is it possible to add a button on the alert that opens the accessibility settings? I think I've seen that somewhere.

pyproject.toml Outdated Show resolved Hide resolved
@xylix
Copy link
Contributor Author

xylix commented Mar 28, 2020

Is it possible to add a button on the alert that opens the accessibility settings? I think I've seen that somewhere.

I found these https://macosxautomation.com/system-prefs-links.html which work as links, but I'm still looking for a way to make the button press "open" the link to the accessibility settings. A standard <a href> link didn't work, but there should be a sane way to link in NSAlert buttons.

Found it. Need to call NSWorkspace.sharedWorkspace().openURL_(NSURL.URLWithString_("x-apple.systempreferences:com.apple.preference.security?Privacy_Accessibility")) on button press.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 28, 2020

This pull request introduces 4 alerts when merging 890b6d6 into 25ae699 - view on LGTM.com

new alerts:

  • 3 for Unused import
  • 1 for Unused local variable

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 29, 2020

This pull request introduces 1 alert when merging f032ed1 into 25ae699 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@xylix
Copy link
Contributor Author

xylix commented Mar 29, 2020

Using a native python thread causes a

objc.error: NSInternalInconsistencyException - NSWindow drag regions should only be invalidated on the Main Thread!

Assertion failed: (NSViewIsCurrentlyBuildingLayerTreeForDisplay() != currentlyBuildingLayerTree), function NSViewSetCurrentlyBuildingLayerTreeForDisplay, file /BuildRoot/Library/Caches/com.apple.xbs/Sources/AppKit/AppKit-1894.30.142/AppKit.subproj/NSView.m, line 13568.
fish: 'python aw_watcher_window/__main…' terminated by signal SIGILL (Illegal instruction)

Error. Will need to look into using macOS's own work queue (https://developer.apple.com/documentation/dispatch/1452927-dispatch_get_global_queue?language=objc )

@ErikBjare
Copy link
Member

ErikBjare commented Mar 29, 2020 via email

@xylix
Copy link
Contributor Author

xylix commented Mar 30, 2020

Seems like it should work. I'll look into it later this week.

@xylix xylix added this to In progress in Road to 1.0 Apr 5, 2020
Road to 1.0 automation moved this from In progress to Reviewer approved Apr 21, 2020
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.

Forgot to re-review.

LGTM, feel free to fix up the conflict and merge :)

Road to 1.0 automation moved this from Reviewer approved to Review in progress Apr 21, 2020
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.

Woops, just skimmed the diff and missed there were things left to do in the comments. Sorry.

@xylix xylix force-pushed the dev/macos-ask-for-permission branch from caa43d0 to 0943007 Compare April 22, 2020 16:26
@xylix
Copy link
Contributor Author

xylix commented Apr 22, 2020

Multiprocessing got rid of the objc.error: NSInternalInconsistencyException - NSWindow drag regions should only be invalidated on the Main Thread! problem. The alert is still a bit wonky (it doesn't "pop up" in the desktop like an alert should, instead it stays in the background and shakes in the macOS dock.

It does now block execution though... which is weird. The window watcher continues execution after I close the modal.

I think I resolved the conflict correctly, will need to see what CI says.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Apr 22, 2020

This pull request introduces 1 alert when merging 3ad2af6 into 620b52c - view on LGTM.com

new alerts:

  • 1 for Unused local variable

…in main.py after logging has bene initialized
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Apr 25, 2020

This pull request introduces 1 alert when merging b829e04 into 620b52c - view on LGTM.com

new alerts:

  • 1 for Unused local variable

Road to 1.0 automation moved this from Review in progress to Reviewer approved Jul 16, 2020
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.

@xylix Looks like all issues have been resolved. Is this ready to merge?

@xylix
Copy link
Contributor Author

xylix commented Jul 16, 2020

There might have still been some problem, I'll need to manually test on macOS before I can be sure. I'll get that done tomorrow, bit busy today.

@xylix
Copy link
Contributor Author

xylix commented Jul 17, 2020

@ErikBjare Looks fine to merge, I tested it locally and solved the conflict of the lock file.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 17, 2020

This pull request introduces 1 alert when merging 724a3e2 into 8075f16 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@ErikBjare
Copy link
Member

@xylix Are the two things in the top comment resolved?

@xylix
Copy link
Contributor Author

xylix commented Jul 17, 2020

@ErikBjare Oh right. I guess the other one is not, because it is actually pretty hard in recent macOS to set a setting from dialog. Instead the dialog just gives a message and Open Accessibility Options button.

@ErikBjare
Copy link
Member

@xylix Should we just leave it as is then and merge?

@xylix
Copy link
Contributor Author

xylix commented Jul 19, 2020

Yeah I think we should. The other open PR requires more work before it is ready but this one will be an improvement even in it's current form.

@ErikBjare
Copy link
Member

Nice! Merging.

@ErikBjare ErikBjare merged commit 48491bc into ActivityWatch:master Jul 19, 2020
Road to 1.0 automation moved this from Reviewer approved to Done Jul 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Road to 1.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants