refactor(tabs): reorder editor tabs by direct manipulation instead of a drag session - #2472
Merged
Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
Reordering editor tabs was built as a SwiftUI drag-and-drop session:
.onDraghanding out anNSItemProvider, a per-tabDropDelegatecallingQueryTabManager.moveTabfromdropEntered, and a second delegate over the track to reset the state. That shape cannot express a cancel, because.onDragreports none, and it produced four separate defects. This replaces it with direct manipulation, which is what every system tab bar does.Found while investigating #2438. It does not close that issue: see "What this does not fix" below.
What was wrong
dropEnteredcommittedmoveTabas the pointer crossed each neighbour, and nothing recorded the pre-drag order, so there was no way back. Every one of those writes bumpedtabStructureVersion, which routes tosaveAggregated()and aTabDiskActorwrite, so a drag the user abandoned was already saved to disk and survived a relaunch. No undo.dropExited, which fires on a pointer exit rather than on a drag ending, cleared the drag state mid-gesture and the rest of that drag silently did nothing.EditorTabStripDropResethad novalidateDrop, so the protocol default accepted everypublic.textdrop,dropUpdatedreturned.move, andperformDropreturned true after doing nothing. Dragging SQL onto the strip reported success to the source and discarded it.The change
EditorTabReorderholds the order the strip draws and the order it came from, so the manager is written once, on release. A cancel is then just dropping that value, and nothing reaches the persisted tab list until the user has finished asking for it.EditorTabReorderResolverdecides where the dragged tab belongs. It swaps on a neighbour's midpoint rather than on contact, which is what stops a pair flipping against each other while the pointer sits on a boundary, and it answers with the furthest midpoint actually crossed, because macOS coalesces a fast drag and one event can arrive several tabs along.The gesture is a
DragGesturein a coordinate space named on the scroll view's content, so a position is measured against the whole run of tabs rather than the part on screen; named on the viewport it would place the drag one tab further along for every tab the track had scrolled past. It is attached withsimultaneousGestureso the tab's own button keeps the click that selects it.Cancelling is latched until the gesture ends.
reorder == nilcould not carry it, because that means both "not started" and "cancelled": the nextonChangedof an abandoned gesture rebuilt a reorder from the manager and the release committed it, so Escape read as working and reordered the strip anyway.EditorTabReorderCancelMonitorowns the Escape monitor, because aViewis a value with nodeinitand every other local monitor in this app is owned by a reference type.The drag also ends when the tab being dragged is closed under it, when a tab is opened beside it, and when the pane is unparented by a connection switch.
EditorTabDropDelegate,EditorTabStripDropReset,.onDragand theUniformTypeIdentifiersimport are gone.What this does not fix
Pressing a tab inside the leading region of the titlebar still drags the window rather than the tab. That is the other half of #2438 and it is not addressed here.
It is positional, not a property of the tab: measured in the app, a tab whose centre sat 135pt from the window's leading edge dragged the window, and in a standalone harness the same undecorated tab reordered normally once the strip was inset 200pt. Two candidate fixes were built and then refuted by measurement, so neither is in this branch: giving every tab an always-present fill changed nothing, and
mouseDownCanMoveWindow = falseover the track changed nothing in either placement, because AppKit consults the hit-tested view and its ancestors and a sibling background or overlay is neither.What is missing is the extent of the reserved region. The standalone harness contradicts itself run to run, so the number has to come from the app, and the UI harness on this machine began failing every case at launch with "The sample database never finished opening", including cases that had passed minutes earlier. That is an environment failure rather than a change failure, so the constant was not guessed at.
The UI tests here therefore assert the reorder and deliberately do not assert the window's origin.
Verified
buildPASS.testPASS, 63 cases: the newEditorTabReorderTestsandEditorTabReorderResolverTests, plusQueryTabReorderTestsand all five existingEditorTabStrip*suites.uitest EditorTabReorderUITestspassed its reorder cases on the last healthy run of the harness; the harness later degraded as described above, so the final run of this suite is INCONCLUSIVE rather than green.included:scope.docs/scripts/check-writing-style.shanddocs/scripts/check-docs-against-source.pyboth clean.ScrollView, so that gap is pre-existing rather than a regression.No screenshots: the change is behavioural and the strip looks identical at rest, so a still cannot show the difference.