Bound the overlap-offset scan and offset maximized windows too - #1810
Conversation
The offset's window scan runs over the accessibility API on the main thread, so a hung or heavily loaded app could stall each window move for the full default AX timeout. Cap messaging for the scan and restore the default straight after; an app that cannot answer in time is skipped, which just means no offset. The cap is per request, so this bounds what any one unresponsive app costs rather than the total. Window frames are also read once per scan now instead of once per cascade step. Maximize is included in the offset. It does not cycle, but two maximized windows land exactly on top of each other, which is the case where the covered window is least visible - so eligibility moves to its own property rather than reusing positionCycles. The cascade math moves into OverlapOffsetGeometry, which is pure and directly testable, and it now checks each axis for room separately. A window with nowhere to shift is left alone instead of being pushed flush against the screen edge, so the offset is a no-op at the default of no gaps and works normally once there is room for it. Every bug this feature has had lived in that math and none of it was reachable by a test before.
7c65464 to
6eb7d5d
Compare
| /// on a position other windows already occupy. Kept free of accessibility and | ||
| /// screen lookups so the cascade math can be tested directly - every bug this | ||
| /// feature has had lived here. | ||
| enum OverlapOffsetGeometry { |
There was a problem hiding this comment.
WindowManager is the easiest file to add a lot of code to, and a difficult one to pull things out of after the fact. For things that can live in a separate helper file, like this enum, let's go ahead and move it out.
It has no dependency on WindowManager's state, so it does not need to live there.
|
Good call - moved to Your reasoning applies to more than this enum, so worth saying what I did with the rest: |
|
I tried out moving There's one more bug that I'm looking at, which is drag to screen edge snapping causing an offset - we need to ignore the footprint window. Should be a quick fix that I can roll in. |
|
Ah, I didn't run the tests, and probably broke them with my change. I have some broken tests to fix anyway so I'll take a look. |
|
Not at all - I like it better in the utility too, and having the accessibility scan sit next to the math it feeds reads fine. On the tests: I checked out main and ran them, and it is a build failure rather than assertion failures, which is worth knowing because it fails differently than you would expect - the test target does not compile, so zero tests run rather than some going red. Moving Dropping Happy to push that as a small PR if it saves you a step, or leave it to you if you are already in there - I do not want to collide with the footprint-window fix you are working on. Just say which. On that one: ignoring the footprint window makes sense to me. |
|
Thanks for the quick reply! I went ahead and pulled out the private declaration from all of them, and fixed the other unrelated failures. |
|
No problem, I was just sitting here anyway. Pulled main and ran it - 233 passing, 1 failing ( |
Follow-on to the overlap offset shipped in v0.96. Three things, all in the same code path.
The scan can no longer stall the UI on an unresponsive app.
applyOverlapOffsetIfNeededenumerates windows over the accessibility API on the main thread, so a hung app — or any app under heavy system load — could block each window move for the full default AX timeout. The scan now caps messaging and restores the default immediately after. An app that can't answer in time is skipped, which just means it doesn't get an offset. To be precise about the guarantee: the cap is per request, so this bounds what any single unresponsive app costs rather than the total. Window frames are also read once per scan now, instead of once per cascade step.Maximize gets the offset. It doesn't cycle, so
positionCyclesexcluded it — but two maximized windows land exactly on top of each other, which is the case where the covered window is least visible. Eligibility is now its own property rather than a second meaning layered ontopositionCycles.The cascade only shifts where there's room. Previously the edge clamp could undo the shift entirely, which made the offset a silent no-op for a maximized window at the default of no gaps, and could eat the gap on the other side when gaps were smaller than the offset. Each axis is now checked for room separately: a window with nowhere to go is left alone, and a left-half window with room only horizontally still offsets horizontally.
The math moved to
OverlapOffsetGeometry, a pure enum with no accessibility or screen lookups, because every bug this feature has had lived in that math and none of it was reachable by a test. The new tests assert resulting rects rather than the flags that gate them — an earlier version of this change was certified green by a test that only checked an eligibility boolean while the offset it enabled did nothing.No behavior change for anyone who hasn't enabled the offset. This also supersedes #1813, which covered the maximized-window half; that half is here and the badge half is in #1808.