Fix multi-window Unity/similar app focus failure (#4192), Zoom/similar race condition, improve notification dots#5165
Fix multi-window Unity/similar app focus failure (#4192), Zoom/similar race condition, improve notification dots#5165chuttenh wants to merge 5 commits intolwouis:masterfrom
Conversation
Merges previous change with v7.39.0 Note that this solves the Photoshop / lazy app problem in a slightly different way
|
Thank you very much for this PR! I'll review it soon 👍 |
| } | ||
| DispatchQueue.main.async { | ||
| guard let app = Applications.find(pid) else { return } | ||
| app.isHidden = false |
There was a problem hiding this comment.
Why do we set the app as not hidden, if it just created a window? Can't an app be hidden yet create a (hidden) window?
| } | ||
| Windows.updateLastFocusDebounced(element, wid, pid: pid, debounce: 60, requireFrontmost: false) | ||
|
|
||
| // Also schedule a fallback to create the window if it doesn't exist yet after a short delay |
There was a problem hiding this comment.
What does it mean "the window doesn't exist yet"? I thought that when this method is called windowCreated(_ element: AXUIElement, _ pid: pid_t), we have a pid, and we have the AXUIElement of the created window.
Could you please clarify what we are waiting 120ms for? Which object or attribute is not ready initially?
| canDrawSubviewsIntoLayer = true | ||
| } | ||
|
|
||
| private func ensureOverlayLabel() { |
| case circledNumber0 = "" | ||
| case circledNumber10 = "" | ||
| case circledStar = "" | ||
| case filledCircledDot = "" |
There was a problem hiding this comment.
What you've done with the badges is really cool!
Could you please separate this in a different PR? I think I can quickly integrate the badge changes, and ship it in the next release. For the Focus changes, I'd like to dive deeper, and spend more time.
| var initialAttributedString: NSMutableAttributedString! | ||
| private var overlayLabel: NSTextField? | ||
| private var overlayCenterX: NSLayoutConstraint? | ||
| private var overlayCenterY: NSLayoutConstraint? |
There was a problem hiding this comment.
I've noticed during my tests that the number can be a bit off, on the Y-axis. It's off when the badge is very small (e.g. Titles Appearance) or very big (e.g. AppIcons Appearance + Large Size).
|
|
||
| func setStar() { | ||
| replaceCharIfNeeded(Symbols.circledStar.rawValue) | ||
| setSymbol(Symbols.circledStar.rawValue) |
There was a problem hiding this comment.
I'm curious about why you picked a white dot in the red circle.
I looked at Apple guidelines, and I can't find clear guidelines as to what apps should do when they can show a straight label. I created a test app that sets a Dock badge. I tried to set NSApplication.shared.dockTile.badgeLabel to these values:
nil"""1""10""100""1000""10000""test"
It either shows the value litterally, or it shows no badge, for the "" and nil values. Not an empty red disk, or a red disk with a white dot. No disk at all.
What do you think?
| } | ||
| @inline(__always) private static func currentFocusCommitToken() -> UInt { focusCommitSeq } | ||
|
|
||
| // Final assert: if another app steals focus right after we commit, reclaim it politely. |
There was a problem hiding this comment.
If another app steals focus, then that's that app decision, no? I feel we should force something at that level. If the user has apps that steal focus, that's their choice. Like if i setup an app to remind me after 5min, and I focus a window with AltTab after 4min59, and right after AltTab focuses that window, the alarm rings and shows me a snooze button window, then I don't think AltTab should decide "no actually the previous window should keep focus.
Events on the system, and apps wanting focus is the realm of the user, and the experience they chose to have, with the apps they install. Unless I'm missing something, I don't think we should force focus, no?
| } | ||
| } | ||
|
|
||
| // Poke the WindowServer: reading the on-screen window list can help settle focus/stacking state. |
There was a problem hiding this comment.
Is there any evidence that it is the case? I've never heard anything about this, in the window management community.
|
Something that I need, in order to evaluate this work, beyond the comments I've left on the code, would be to describe in this PR the scenarios that you've tested. You seem to be able to reproduce these focus issues deterministically. If I can reproduce on my side, I can help with this fix. It I can't reproduce on my side, all I can do is to theorize about the code, and how it may help. Thank you! |
2 participants
fix: Unity app focus (#4192 ), Zoom race condition, notification count cap
Bug Fixes
Features
Apologies that I have a very intense full time job and haven't had time to test under every circumstance
or match convention perfectly. I want to get this into circulation, though, since #4192 is breaking
for me, and the Zoom/Photoshop race condition has been a bear to track down. I've done extensive
testing on both of those prior to v7.39.0, which unfortunately stomped on several of the fixes.
I've done my best to re-test after a lot of merge conflicts, but I haven't had nearly as much time,
and I want to get this in over the holiday break before work takes over again. Apologies that I've
done my best, thanks for your work on this!