Repository navigation
macos: filter the window list on .titled, so a hidden window still counts - #2401
Merged
Merged
Conversation
…unts The windowObscured gate shipped in #2394 never became true. @nichtlegacy found why and measured it: canBecomeMain describes a window's CURRENT STATE, not its kind. A hidden or miniaturised window answers false, so the app window left the filtered list at exactly the moment the gate was meant to shut, the list went empty, and the deliberate empty-list rule then read that as "not obscured". visible all=3 cbm=1 titled=2 (one on screen) obscured=N hidden all=4 cbm=0 titled=2 (none on screen) obscured=N, should be Y minimised all=4 cbm=0 titled=2 (none on screen) obscured=N, should be Y Two decisions that are each defensible on their own, cancelling out in the one case this exists for. Hidden still cost more than visible on their build: 23.7% of a core visible against 28.5% hidden, with LiquidRender frames still in a sample taken while AppKit reported the process not visible. styleMask.contains(.titled) does both jobs. NSStatusBarWindow reported titled=N in every state they logged, so the MenuBarExtra is still excluded, and the app window kept titled=Y while visible, hidden and miniaturised, which is the property canBecomeMain never had. The pure decision now takes the window LIST rather than two summary Bools. That is not tidying: given only hasWindows and anyWindowOnScreen there was no way to express "the app window is hidden while the status-item window is on screen", which is the exact state that was misread, so no test over that signature could have failed on this bug. The source census pinned the literal canBecomeMain and passed the whole time the gate did nothing. A census cannot see a predicate that is right for a visible window and wrong for a hidden one. It is kept for what a census IS good for, noticing a known-bad predicate coming back, and now asserts the absence of canBecomeMain as well as the presence of the replacement, with comment lines excluded so the source can explain itself. The behaviour is pinned over window states instead, in tests that reproduce the three the reporter logged and fail against the old predicate. Cost unchanged: same notifications, same recompute, no timer, no radio work. Verified by parse only here (no macOS SDK); the reporter has offered to re-run their measurements against this.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@nichtlegacy is right, and their diagnosis is exact. The gate I merged in #2394 never became true, so nothing it was meant to stop ever stopped.
What I got wrong
canBecomeMaindescribes a window's CURRENT STATE, not its kind. A hidden or miniaturised window answers false, so the app window left the filtered list at exactly the moment the gate was meant to shut, the list went empty, and the empty-list rule I had deliberately written then read that as "not obscured":Both decisions were individually defensible, which is how they got past me and past review, and they cancel out in the single case the change existed for.
The predicate
styleMask.contains(.titled), as measured in the report.NSStatusBarWindowreportedtitled=Nin every logged state, so theMenuBarExtrais still excluded; the app window kepttitled=Ywhile visible, hidden and miniaturised, which is the propertycanBecomeMainnever had.Why the shape of the decision changed too
The pure half now takes the window LIST rather than two summary Bools. That is not tidying. Given only
hasWindowsandanyWindowOnScreen, there is no way to express "the app window is hidden while the status-item window is on screen", which is the exact state that was misread, so no test over that signature could have failed on this bug. The summary was where the information had already been lost.The test that passed while the gate did nothing
The report calls this out and it is the part worth sitting with:
QuietMotionCoverageTestspinned the literal stringcanBecomeMainto stop the filter being dropped, and it passed for the entire life of the defect. A source census cannot see a predicate that is correct for a visible window and wrong for a hidden one.It is kept for what a census is actually good for, noticing a known-bad predicate coming back, and now asserts the ABSENCE of
canBecomeMainalongside the presence of the replacement, with comment lines excluded so the source is free to explain itself. The behaviour moved to where it belongs:WindowObscuredGateTestsnow reproduces the three window states the reporter logged, plus the two-titled-window case, and the hidden and miniaturised cases fail against the old predicate.Unchanged
Same five notifications, same recompute, no timer, no radio work. The empty-list rule stays, and is now safe for the reason it was written: with a state-independent filter, an empty list once again means "no windows yet", not "the window is hidden".
Verification
Parse-checked here; there is no macOS SDK in this environment, so CI compiles it and the package suite runs the new tests. @nichtlegacy offered to re-run their measurements against a revised push, which is the only way to confirm the CPU actually drops, and I would take them up on it.
One caveat carried over from the report rather than resolved:
SheetPresentationWindowentries reporttitled=Y. In everything observed they track the main window's visibility, so they do not change the outcome, and the two-titled-window test pins both directions of that assumption, but nobody has tested it with a sheet genuinely open.