-
-
Notifications
You must be signed in to change notification settings - Fork 289
fix(datagrid): Delete key and right-click respect multi-row/cell-range selection#1648
fix(datagrid): Delete key and right-click respect multi-row/cell-range selection #1648J2TeamNNL wants to merge 1 commit into
Conversation
...e selection D12A: delete(_:) checks gridSelection.affectedRows first, mirroring copy(). D12B: rightMouseDown override skips super when clicking inside existing selection, calls NSMenu.popUpContextMenu directly to preserve the selection set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f37596613
i️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2 Badge Clear the cell-range selection after deleting inserted rows
When the selected range includes newly inserted rows, dataGridDeleteRows(Set(rows)) physically removes those rows, but the grid selection is left intact: applyRemovedRows only updates/removes table rows and syncSelection performs the next row selection programmatically, so tableViewSelectionDidChange does not clear selectionController. After deleting a cell range over inserted rows, the Delete menu remains enabled from the stale grid selection and pressing Delete again deletes whatever rows shifted into the old indexes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2 Badge Preserve right-clicks inside cell-range selections
This fast path only recognizes rows in selectedRowIndexes, but a grid cell-range selection can span many rows while the NSTableView row selection contains only the focused row. Right-clicking a selected cell on another row falls through to super.rightMouseDown, which selects the clicked row; tableViewSelectionDidChange then clears selectionController when the change is not programmatic, so the context menu still appears after collapsing the cell-range selection. The condition needs to also treat clicks inside gridSelection.selection as selected.
Useful? React with 👍 / 👎.
Root cause
D12A —
delete(_:)only readselectedRowIndexes. When cells are selected via drag (grid-range selection),selectedRowIndexescan be empty, so Delete did nothing.D12B — NSTableView's default
rightMouseDownreplacesselectedRowIndexeswith just the clicked row beforemenu(for:)is called. By the time the context menu appeared, the multi-row selection was gone.Fix
D12A —
delete(_:)now checksgridSelection.affectedRowsfirst, then falls back toselectedRowIndexes. Mirrors the existingcopy()pattern exactly.validateUserInterfaceItemanddeleteSelectedRowsIfPossibleupdated to match.D12B —
rightMouseDownoverride: if the clicked row is already inside the current selection, skipsuper(which would collapse it) and callNSMenu.popUpContextMenu(_:with:for:)directly.File:
TablePro/Views/Results/KeyHandlingTableView.swiftTest plan