fix(datagrid): keep a tab's grid selection across a tab or result-mode switch - #2679
Merged
Merged
Conversation
Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.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.
Fixes #2667.
Root cause
handleTabChangeis a save-outgoing / restore-incoming pair, and for the grid selection only the restore half was ever written.MainContentCoordinator+TabSwitch.swift:93has always doneselectionState.indices = newTab.selectedRowIndices, but nothing ever wrote that field: an exhaustive grep finds one non-clearing writer,RowEditingCoordinator.pasteRows. Everything else sets it to empty. So the field was permanently empty and every switch back replayed an empty set over whatever the reader had selected.It shipped untested because the existing test seeds the field by hand (
MainContentCoordinatorTabSwitchTests.restoresIncomingSelectedRows), which exercises the half that worked.The first attempt at a fix captured the selection in the grid's own
dismantleNSView. Tracing the running app showed why that is not enough on its own:dismantle: enterfires, the closure is wired, the table view is alive, andselectedRowIndexesis already empty. On a tab switch the sharedGridSelectionStateis repointed at the incoming tab while the outgoing grid is still mounted and bound to it, sosyncSelectionclears the outgoing table view before its teardown runs. The capture therefore happens inhandleTabChange, before the repoint, and the teardown capture is kept only for the case it is authoritative for: a result-mode switch, which destroys the grid without changing tabs.What this changes
QueryTabgainscellSelectionbesideselectedRowIndices, andselectedDisplayRowsderives the row span from whichever is authoritative. Storing rows alone would be wrong, not merely incomplete:publishRowSelectionprojects a cell rectangle down toaffectedRows, which is lossy and not invertible, so restoring from it would widen a three-column block into whole rows and make Copy copy whole rows.handleTabChange), on a result-mode switch (dismantleNSView, gated on the tab still being selected), and when a tab is handed to another window (enrichedForPersistence, which snapshots before the grid is torn down and whose own call site already claimed to carry the selection).updateNSViewafterapplyStructuralUpdate, never before: that pass ends inselectionController.clear()andreloadData().GridSelectionRestoreis the pure, tested clamp: rows against the row count, the rectangle against the presented data column count.GridCoord,GridRectandGridSelectionmove fromViews/Results/Selection/toModels/UI/, since a model type now stores one. They are Foundation-only value types.TableViewCoordinatormoves intoDataGridView+MountState.swift; the class was one line under the 1100-line type-body limit.Invalidation was already correct and is extended rather than replaced:
resetSelectionForNewResultnow clears both halves, a retarget clears both, a paste supersedes a stored rectangle, and hiding a column clears a stored rectangle whose display positions it renumbers.Measured AppKit behaviour this rests on
A
swiftcprobe against the Xcode-beta toolchain, run twice:selectRowIndexes(_:byExtendingSelection:)is all-or-nothing on an out-of-range member. On 20 rows,{5, 20}and{5, 999}both give[]. It does not clamp and does not keep the valid members, so an unclamped restore selects nothing at all rather than the rows that do still exist.reloadData()clearsselectedRowIndexesunconditionally, including when the row count is unchanged, and fires notableViewSelectionDidChange. This contradicted the doc comment onTableViewCoordinator.clearRowSelection, which is corrected here: the deselect is what publishes the change, not what makes it.removeFromSuperviewand re-adding preserve the selection, so the defect is object destruction, not detachment.autosaveTableColumnscovers width, order, sort and hidden state, never selection, andencodeRestorableStateis relaunch-scoped. There is no AppKit mechanism to adopt here.Review
Two
codex reviewpasses on the branch produced eleven findings; all were acted on. The substantive ones: the restore read the shared channel rather than the tab's own rows, so a Data → Structure → Data round trip resolved schema-grid positions; it selected every row a rectangle covered, which AppKit fills edge to edge and whichDataGridRowViewthen skips its partial fill on, painting the block as whole rows; and it wrote aGridCoord.displayColumnstraight intoKeyHandlingTableView.focusedColumn, which indexestableColumnsand so pointed the cell cursor at the row-number column or a spacer, against theCLAUDE.mdinvariant that no fixed position intableColumnsnames a data column.One finding was not acted on: the teardown capture publishes observable state during SwiftUI reconciliation.
flushPendingColumnLayoutPersistencealready does exactly this from the samedismantleNSView, reaching the sametabManager.mutate, so this follows the established path rather than opening a new one. The write is also skipped entirely when nothing changed.Not done
Selection is memory-only;
PersistedTabgains no field. Display positions are only meaningful against an identical sort, filter and page, which is the hazard behind the "Selection indices are display positions" invariant (#1837). It is also not converted to stableRowIDidentity:resetSelectionForNewResultalready drops the selection whenever rows are replaced, and a background tab's display order cannot move under it.Verification
All on this branch, in a worktree at the merge base:
buildPASStestPASS across the suites owning the changed types: 92 cases overGridSelectionRestoreTests,MainContentCoordinatorGridSelectionTests,MainContentCoordinatorSelectionResetTests,MainContentCoordinatorTabSwitchTests,DataGridSelectionTests,PublishedRowSelectionTests,CoordinatorColumnVisibilityTests,GridSelectionOwnerTests; and 175 cases over those plus the tab-lifecycle neighbours (RecentlyClosedTabStoreTests,TabCloseProtectionTests,MainContentCoordinatorLazyLoadTests,MainContentCoordinatorDisplayStateTests,FKNavigationTests,CancelledExecutionOwnershipTests,CommandActionsDispatchTests,DisplayedResultReaderTests).uitest GridSelectionAcrossTabsUITestsPASS, run four times. It failed against the unfixed code withExpected 1 of 1.000 rows selected, got 1-1.000 of 3.503 rows, which is the reported symptom, so it is a real regression guard rather than a test that happens to pass.swiftlint --strictclean for this change. Twolegacy_swiftui_aspect_ratioerrors remain inImportFromAppSourcePicker.swiftandSupportView.swift; both are pre-existing onmainand untouched here.No screenshots: the change is a state-lifetime fix with no new or altered chrome. What a reviewer would look at is a selection highlight before and after a tab switch, which is what the UI test asserts through the status readout.
https://claude.ai/code/session_01STT2h6Y53XJah8xPXAH42L