Fix/accessibility observer #211
Conversation
* This make sure we fully initialized the accessibility tree
* Introducing the AppCoordinator which init/setup our singletons * Reset everything when needed
There was a problem hiding this comment.
PR Summary
This PR implements a major refactoring of the accessibility system in Onit, centralizing initialization and improving observer management. The changes focus on better organization and reliability of window management features.
- Added
AppCoordinatorto centralize accessibility initialization and configuration, replacing scattered initialization code across the app - Moved window finding logic from
AXUIElementextensions to process-based methods usingpid.getWindows()for more reliable window management - Added proper error tracking for AX server initialization failures through
AccessibilityAnalytics.logAXServerInitializationError - Several instances of
.framebeing used instead of.visibleFrameon NSScreen objects should be reviewed, particularly inPanelStatePinnedManager - Multiple AXUIElement frame operations are missing global coordinate space conversion (
convertedToGlobalCoordinateSpace: true), which could cause positioning issues
19 file(s) reviewed, 15 comment(s)
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
logic: This file is being completely removed but the functionality appears to be used by AccessibilityObserversManager and AccessibilityNotificationsManager. Need to ensure the target window detection logic exists elsewhere or this will break accessibility features.
There was a problem hiding this comment.
Windows accessors are moved into ProcessIdentifier+Accessibility.
There was a problem hiding this comment.
logic: isTargetWindow() checks for standard window attributes but doesn't convert coordinates to global space when checking button positions. This could cause positioning issues.
| panelStateCoordinator = PanelStateCoordinator.shared | ||
| featureFlagManager = FeatureFlagManager.shared | ||
|
|
||
| observerManager.delegate = notificationsManager |
There was a problem hiding this comment.
style: Setting delegate before configuring managers could lead to race conditions. Consider moving this line after the configure() calls
| default: | ||
| break | ||
| } |
There was a problem hiding this comment.
style: Default case should handle .notDetermined explicitly rather than silently breaking
There was a problem hiding this comment.
.notDetermined is filtered by the listener
| func accessibilityObserversManager(didReceiveNotification notification: String, | ||
| element: AXUIElement, | ||
| elementPid: pid_t, | ||
| info: [String: Any]) |
There was a problem hiding this comment.
style: inconsistent indentation of parameters - 'element', 'elementPid', and 'info' are indented with 7 spaces while 'notification' has no extra indentation
| let appElement = getAXUIElement() | ||
|
|
||
| if let value = appElement.attribute(forAttribute: kAXFocusedWindowAttribute as CFString) { | ||
| return (value as! AXUIElement) |
There was a problem hiding this comment.
logic: Unsafe force cast to AXUIElement. Use conditional cast (as?) and handle failure case
| return (value as! AXUIElement) | |
| if let window = value as? AXUIElement { | |
| return window | |
| } | |
| return nil |
|
|
||
| guard result == .success, | ||
| let windows = windowList as? [AXUIElement] else { | ||
| return [appElement] |
There was a problem hiding this comment.
style: Returning [appElement] when window list fails could be misleading - consider returning empty array instead
There was a problem hiding this comment.
We have to rework the window accessors
There was a problem hiding this comment.
@Niduank I will send a different PR for this, but I don't think we should be using getWindows(). Some applications don't implement the kAXWindowsAttribution. I can't remember which ones, but we had bugs before that I fixed by using 'getRootChildren()' instead and then filtering for windows from that list!
| func getAppName() -> String? { | ||
| NSRunningApplication(processIdentifier: self)?.localizedName | ||
| } |
There was a problem hiding this comment.
style: getAppName() should be a computed property like bundleIdentifier for consistency and better Swift style
* minimize & deminimize noitifications
|
|
||
| guard result == .success, | ||
| let windows = windowList as? [AXUIElement] else { | ||
| return [appElement] |
There was a problem hiding this comment.
@Niduank I will send a different PR for this, but I don't think we should be using getWindows(). Some applications don't implement the kAXWindowsAttribution. I can't remember which ones, but we had bugs before that I fixed by using 'getRootChildren()' instead and then filtering for windows from that list!
| @@ -44,7 +44,7 @@ class AccessibilityWindowsManager { | |||
| activeTrackedWindow = trackedWindow | |||
|
|
|||
| return trackedWindow | |||
| } else if let window = element.findFirstTargetWindow() { | |||
| } else if let window = pid.findFirstTargetWindow() { | |||
There was a problem hiding this comment.
Is this just a complete hallucination? Lol, I don't see getFrame() anywhere in this file?
| private func isAXServerInitialized(pid: pid_t) -> Bool { | ||
| func isAppleApplication(for pid: pid_t) -> Bool { | ||
| guard let bundleIdentifier = pid.bundleIdentifier else { | ||
| return false | ||
| } | ||
|
|
||
| return bundleIdentifier.starts(with: "com.apple.") | ||
| } | ||
|
|
||
| func canReadFocusedUIElement(for pid: pid_t) -> Bool { | ||
| let appElement = pid.getAXUIElement() | ||
|
|
||
| var focusedElement: CFTypeRef? | ||
| let result = AXUIElementCopyAttributeValue(appElement, kAXFocusedUIElementAttribute as CFString, &focusedElement) | ||
|
|
||
| if result == .success { | ||
| return true | ||
| } else { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
@Niduank - this is very cool (and a great idea!) but it's very hard to review in this format. The entire file registers as 'new' because of the refactor, even though only this function is new. In the future, I think it'd be better to do refactor as one PR, and then any new code as a different PR. I know I'm guilty of this too, but what do you think?
There was a problem hiding this comment.
@timlenardo
I'm afraid that a large PR that doesn't bring any value to the product or fix any bugs might not get merged for a long time.
I don't mind working that way, but you should know that the branch containing the bug fixes will be based on the "refacto" branch.
By the way, if you want, we can proceed like that for the next refactor, which will allow us to fix the pinned mode.
There was a problem hiding this comment.
@Niduank - yes that works. So I ended up reviewing this PR by looking at each commit individually, which solved a lot of the problem. But yeah, let's try working this way for the next refactor!
| // }) | ||
| // } | ||
| Task.detached { | ||
| await self.highlightedTextCoordinator.startPolling(pid: processID, selectionChangedHandler: { [weak self] text, frame in |
There was a problem hiding this comment.
Low-pro, but I'd suggest renaming this function to "maybeStartPolling" or something else that indicates that it doesn't do the polling unless it's one of the matching apps!
There was a problem hiding this comment.
@timlenardo Let's do a more conventional startPollingIfNeeded 👍
| @@ -25,18 +25,36 @@ class AccessibilityObserversManager { | |||
| private var observers: [pid_t: AXObserver] = [:] | |||
| private var persistentObservers: [pid_t: AXObserver] = [:] | |||
|
|
|||
| private var skipNextDeactivation = false | |||
| // Protection contre les appels rapides multiples | |||
| // Check if Onit is activated so we don't deactivate the active app | ||
| if let activeApp = NSWorkspace.shared.frontmostApplication, | ||
| authorizationState(for: activeApp.processIdentifier) == .current { | ||
| log.debug("Onit is active, ignoring deactivation of `\(app.localizedName ?? "Unknown")` (\(app.processIdentifier))") |
There was a problem hiding this comment.
This is an interesting fix... should we have had a lot of issues before? Like anytime you launch Onit in Tethered mode, it should deactivate the other app and stop the observers? How come this wasn't breaking more stuff before?
There was a problem hiding this comment.
@timlenardo
This mostly happened when switching to Onit using CMD+TAB.
I didn't notice any specific bug, but I saw in the logs that the active window was no longer being observed which could lead to issues.
| @@ -25,9 +25,9 @@ class AccessibilityObserversManager { | |||
| private var observers: [pid_t: AXObserver] = [:] | |||
| private var persistentObservers: [pid_t: AXObserver] = [:] | |||
|
|
|||
| // Protection contre les appels rapides multiples | |||
| @@ -65,6 +65,16 @@ class AccessibilityObserversManager { | |||
| } | |||
| } | |||
|
|
|||
| func appLaunched(with pid: pid_t) { | |||
There was a problem hiding this comment.
I find the naming of this confusing. The naming implies it only gets called at launch, but it actually happens anytime the accessibility permission is changed. I guess this is because the state always starts as .notDetermined, and then you have to wait for an update to .granted or .denied? I'd suggest giving it a name closer to what it actually does (like "accessibilityGranted") and then add a comment about how that gets called when the app launches and is needed to start the listeners for the foregrounded app, because we don't get the appActivatedNotification.
| private var stateChangesCancellable: AnyCancellable? | ||
| private var isFirstPermissionReceived = true |
There was a problem hiding this comment.
Okay, this answered some of my questions from the last comment about 'appLaunched'. I would rename this to, 'hasRecievedFirstPermission' and default it to false, then it should be set to true after the first permission callback. Then, I understand the logic much more clearly!
| @@ -35,9 +35,38 @@ extension pid_t { | |||
| return targetWindows | |||
| } | |||
|
|
|||
| // We are currently using this but should instead use findTargetWindows with some logic to select to correct one. | |||
| func findFirstTargetWindow() -> AXUIElement? { | |||
There was a problem hiding this comment.
I would also rename this from findFirstTargetWindow to firstMainWindow, since we're no longer just taking the first one!
Architecture
AppCoordinator
AccessibilityObservers.AccessibilityNotificationsdata when permission is revoked.AccessibilityObserversManager
AXObserversand delegates events to theAccessibilityNotificationsManager.Improvements
Performance
ObserversManagertimeOutPids).Major Accessibility Issue
To avoid reproducing this bug in the future, the call to
AXUIElementCreateApplicationhas been centralized in a single location.Each time we try to access a window, we now go through the PID.
AccessibilityObserversManager: Log + send an analytic event when accessibility is broken.AXUIElement+Onitand replaced it withProcessIdentifier+Accessibility.Tim's code to find the main window