Repository navigation
Make the default display ordering deterministic - #1960
Open
AkiyamaKunka wants to merge 2 commits into
Open
AkiyamaKunka wants to merge 2 commits into
AkiyamaKunka wants to merge 2 commits into
Conversation
ScreenDetection.order(screens:) in the default .yThenMinX mode sorted displays with a comparator that compares some pairs by Y and others by X. That predicate is not a strict weak ordering: for a staircase layout (A above B, B above C, with A and C disjoint) it yields A < B, B < C, and C < A, so Array.sorted(by:) returned an order that depended on how NSScreen.screens happened to enumerate the displays. The same layout could order three different ways, making next/previous-display cycling and the move-to-display-N mapping inconsistent. Group displays into horizontal bands of vertically-overlapping screens (top band first) and sort each band left-to-right. This is deterministic and matches the previous result on clean rows, columns and grids; it only changes the previously-undefined staircase case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
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.
What
In the default display-ordering mode (
.yThenMinX, set inDefaults.swift),ScreenDetection.order(screens:)sorts displays with a comparator that compares some pairs by their Y position and others by their X position:That predicate isn't a strict weak ordering. For a "staircase" layout where A sits above B and B above C but A and C don't overlap vertically, it reports
A < B,B < C, andC < A— a cycle.Array.sorted(by:)given an invalid comparator returns an unspecified order, so the result depends on the orderNSScreen.screenshappens to list the displays in. Since that enumeration order isn't guaranteed to be spatial, the same physical layout can come out three different ways:screensOrdereddrives next/previous-display cycling (adjacent) and the "move to display N" mapping (SpecificDisplayCalculation), so on affected layouts both become inconsistent from one call to the next.The fix
Group the displays into horizontal bands of vertically-overlapping screens (top band first) and sort each band left-to-right. Banding is a deterministic function of the layout, so the ordering no longer depends on enumeration order. It produces the same result as before on clean rows, columns and grids — and on bottom-aligned mixed-height rows — and only changes the previously-undefined staircase case, where the connected band now simply reads left-to-right.
The other two modes (
.midX,.minX) are single-key sorts and were already valid; they're untouched.Tests
Added
ScreenOrderingTests:main— the ordering must be independent of enumeration order, and the diagonal layout must have one deterministic order;The full test suite passes (729 tests).
This may also be behind the inconsistent next/previous-display behavior reported in #1383, though that report mentions multiple Spaces as well, so I haven't assumed it's the whole story.