From 74b893c8a2997c01c0445f81b7b608dcb0db2291 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Thu, 30 Apr 2026 18:40:33 +0700 Subject: [PATCH 01/11] refactor(datagrid): full Phase 3 keyboard interaction cleanup --- .../ChangeTracking/DataChangeManager.swift | 16 -- .../Services/Query/RowOperationsManager.swift | 10 - TablePro/Resources/Localizable.xcstrings | 18 +- .../Extensions/DataGridView+Selection.swift | 5 + .../Views/Results/KeyHandlingTableView.swift | 262 ------------------ TablePro/Views/Results/TableSelection.swift | 17 -- .../DataChangeManagerExtendedTests.swift | 102 ++++--- .../DataChangeManagerTests.swift | 6 +- .../Views/Results/TableSelectionTests.swift | 38 +-- docs/development/datagrid-audit.md | 57 ++++ 10 files changed, 141 insertions(+), 390 deletions(-) create mode 100644 docs/development/datagrid-audit.md diff --git a/TablePro/Core/ChangeTracking/DataChangeManager.swift b/TablePro/Core/ChangeTracking/DataChangeManager.swift index 5587136a8..47e392a7d 100644 --- a/TablePro/Core/ChangeTracking/DataChangeManager.swift +++ b/TablePro/Core/ChangeTracking/DataChangeManager.swift @@ -369,22 +369,6 @@ final class DataChangeManager: ChangeManaging { } } - // MARK: - Undo/Redo Public API - - func undoLastChange() -> UndoResult? { - guard let um = undoManagerProvider?(), um.canUndo else { return nil } - lastUndoResult = nil - um.undo() - return lastUndoResult - } - - func redoLastChange() -> UndoResult? { - guard let um = undoManagerProvider?(), um.canRedo else { return nil } - lastUndoResult = nil - um.redo() - return lastUndoResult - } - // MARK: - SQL Generation func generateSQL() throws -> [ParameterizedStatement] { diff --git a/TablePro/Core/Services/Query/RowOperationsManager.swift b/TablePro/Core/Services/Query/RowOperationsManager.swift index dde885422..eb909fc2a 100644 --- a/TablePro/Core/Services/Query/RowOperationsManager.swift +++ b/TablePro/Core/Services/Query/RowOperationsManager.swift @@ -150,16 +150,6 @@ final class RowOperationsManager { ) } - func undoLastChange(tableRows: inout TableRows) -> UndoApplicationResult? { - guard let result = changeManager.undoLastChange() else { return nil } - return applyUndoResult(result, tableRows: &tableRows) - } - - func redoLastChange(tableRows: inout TableRows) -> UndoApplicationResult? { - guard let result = changeManager.redoLastChange() else { return nil } - return applyUndoResult(result, tableRows: &tableRows) - } - func applyUndoResult(_ result: UndoResult, tableRows: inout TableRows) -> UndoApplicationResult { switch result.action { case .cellEdit(let rowIndex, let columnIndex, _, let previousValue, _, _): diff --git a/TablePro/Resources/Localizable.xcstrings b/TablePro/Resources/Localizable.xcstrings index 07628c4c0..17d2330b3 100644 --- a/TablePro/Resources/Localizable.xcstrings +++ b/TablePro/Resources/Localizable.xcstrings @@ -7740,6 +7740,9 @@ }, "Choose a certificate or key file" : { + }, + "Choose a fetched model" : { + }, "Choose a folder to watch for .tablepro connection files" : { "localizations" : { @@ -24879,6 +24882,9 @@ } } } + }, + "Model name" : { + }, "Model not found: %@" : { "localizations" : { @@ -31137,6 +31143,9 @@ } } } + }, + "Priority %d" : { + }, "Privacy" : { "localizations" : { @@ -35645,9 +35654,6 @@ } } } - }, - "Select a model" : { - }, "Select a Plugin" : { "localizations" : { @@ -37490,6 +37496,12 @@ } } } + }, + "Sorted ascending" : { + + }, + "Sorted descending" : { + }, "Sorting will reload data and discard all unsaved changes." : { "localizations" : { diff --git a/TablePro/Views/Results/Extensions/DataGridView+Selection.swift b/TablePro/Views/Results/Extensions/DataGridView+Selection.swift index 9392f8601..c5872a261 100644 --- a/TablePro/Views/Results/Extensions/DataGridView+Selection.swift +++ b/TablePro/Views/Results/Extensions/DataGridView+Selection.swift @@ -42,6 +42,11 @@ extension TableViewCoordinator { } else if keyTableView.focusedRow < 0, let firstRow = newSelection.min() { keyTableView.focusedRow = firstRow keyTableView.focusedColumn = 1 + } else if tableView.numberOfSelectedRows == 1 { + let newRow = tableView.selectedRow + if keyTableView.focusedRow != newRow { + keyTableView.focusedRow = newRow + } } } } diff --git a/TablePro/Views/Results/KeyHandlingTableView.swift b/TablePro/Views/Results/KeyHandlingTableView.swift index 1eb8e59e7..5971f246e 100644 --- a/TablePro/Views/Results/KeyHandlingTableView.swift +++ b/TablePro/Views/Results/KeyHandlingTableView.swift @@ -1,24 +1,8 @@ -// -// KeyHandlingTableView.swift -// TablePro -// -// NSTableView subclass that handles keyboard shortcuts and TablePlus-style cell focus. -// Uses Apple's responder chain pattern with interpretKeyEvents for standard shortcuts. -// -// Architecture: -// - Keyboard events → interpretKeyEvents → Standard selectors (@objc moveUp, delete, etc.) -// - Uses KeyCode enum for readability (no magic numbers) -// - Responder chain validation via validateUserInterfaceItem -// - import AppKit -/// NSTableView subclass that handles keyboard shortcuts and TablePlus-style cell focus on click final class KeyHandlingTableView: NSTableView { weak var coordinator: TableViewCoordinator? - // MARK: - First Responder - override var acceptsFirstResponder: Bool { true } @@ -43,18 +27,6 @@ final class KeyHandlingTableView: NSTableView { set { selection.focusedColumn = newValue } } - var selectionAnchor: Int { - get { selection.anchor } - set { selection.anchor = newValue } - } - - var selectionPivot: Int { - get { selection.pivot } - set { selection.pivot = newValue } - } - - // MARK: - TablePlus-Style Cell Focus - override func mouseDown(with event: NSEvent) { window?.makeFirstResponder(self) @@ -67,11 +39,6 @@ final class KeyHandlingTableView: NSTableView { return } - if clickedRow >= 0 && !event.modifierFlags.contains(.shift) { - selectionAnchor = clickedRow - selectionPivot = clickedRow - } - let alreadyFocusedHere = clickedRow >= 0 && clickedColumn >= 0 && clickedRow == focusedRow @@ -103,8 +70,6 @@ final class KeyHandlingTableView: NSTableView { } } - // MARK: - Standard Edit Menu Actions - @objc func delete(_ sender: Any?) { guard coordinator?.isEditable == true else { return } guard !selectedRowIndexes.isEmpty else { return } @@ -143,17 +108,12 @@ final class KeyHandlingTableView: NSTableView { } } - // MARK: - Keyboard Handling - - /// Convert key events to standard selectors using interpretKeyEvents - /// This enables proper responder chain behavior and accessibility support override func keyDown(with event: NSEvent) { guard let key = KeyCode(rawValue: event.keyCode) else { super.keyDown(with: event) return } - // Handle Tab manually (NSTableView cell navigation requires custom logic) if key == .tab { if event.modifierFlags.contains(.shift) { handleShiftTabKey() @@ -164,37 +124,8 @@ final class KeyHandlingTableView: NSTableView { } let row = selectedRow - let modifiers = event.modifierFlags.intersection(.deviceIndependentFlagsMask) - let isShiftHeld = modifiers.contains(.shift) - - if modifiers.contains(.control) { - switch key { - case .h: - handleLeftArrow(currentRow: row) - return - case .j: - handleDownArrow(currentRow: row, isShiftHeld: isShiftHeld) - return - case .k: - handleUpArrow(currentRow: row, isShiftHeld: isShiftHeld) - return - case .l: - handleRightArrow(currentRow: row) - return - default: - break - } - } switch key { - case .upArrow: - handleUpArrow(currentRow: row, isShiftHeld: isShiftHeld) - return - - case .downArrow: - handleDownArrow(currentRow: row, isShiftHeld: isShiftHeld) - return - case .leftArrow: handleLeftArrow(currentRow: row) return @@ -203,27 +134,10 @@ final class KeyHandlingTableView: NSTableView { handleRightArrow(currentRow: row) return - case .home: - handleHome(isShiftHeld: isShiftHeld) - return - - case .end: - handleEnd(isShiftHeld: isShiftHeld) - return - - case .pageUp: - handlePageUp(isShiftHeld: isShiftHeld) - return - - case .pageDown: - handlePageDown(isShiftHeld: isShiftHeld) - return - default: break } - // FK preview: dispatch from user-configurable shortcut (default: Space) if let fkCombo = AppSettingsManager.shared.keyboard.shortcut(for: .previewFKReference), !fkCombo.isCleared, fkCombo.matches(event), @@ -234,14 +148,9 @@ final class KeyHandlingTableView: NSTableView { return } - // For all other keys, use interpretKeyEvents to map to standard selectors - // This handles Return → insertNewline(_:), Delete → deleteBackward(_:), ESC → cancelOperation(_:) interpretKeyEvents([event]) } - // MARK: - Standard Responder Selectors - - /// Handle Return/Enter key - start editing current cell @objc override func insertNewline(_ sender: Any?) { let row = selectedRow guard row >= 0, focusedColumn >= 1, coordinator?.isEditable == true else { @@ -258,7 +167,6 @@ final class KeyHandlingTableView: NSTableView { editColumn(focusedColumn, row: row, with: nil, select: true) } - /// Handle Delete/Backspace key - delete selected rows @objc override func deleteBackward(_ sender: Any?) { guard coordinator?.isEditable == true else { return } guard !selectedRowIndexes.isEmpty else { return } @@ -268,9 +176,6 @@ final class KeyHandlingTableView: NSTableView { @objc override func cancelOperation(_ sender: Any?) { } - // MARK: - Arrow Key and Tab Helpers - - /// Handle left arrow key - move focus to previous column private func handleLeftArrow(currentRow: Int) { if focusedColumn > 1 { focusedColumn -= 1 @@ -281,7 +186,6 @@ final class KeyHandlingTableView: NSTableView { } } - /// Handle right arrow key - move focus to next column private func handleRightArrow(currentRow: Int) { if focusedColumn >= 1 && focusedColumn < numberOfColumns - 1 { focusedColumn += 1 @@ -338,171 +242,6 @@ final class KeyHandlingTableView: NSTableView { scrollColumnToVisible(prevColumn) } - // MARK: - Arrow Key Selection Helpers - - private func handleUpArrow(currentRow: Int, isShiftHeld: Bool) { - guard numberOfRows > 0 else { return } - - if currentRow == -1 { - let targetRow = numberOfRows - 1 - selectionAnchor = targetRow - selectionPivot = targetRow - focusedRow = targetRow - selectRowIndexes(IndexSet(integer: targetRow), byExtendingSelection: false) - scrollRowToVisible(targetRow) - return - } - - if isShiftHeld { - if selectionAnchor == -1 { - selectionAnchor = currentRow - selectionPivot = currentRow - } - - let currentPivot = selectionPivot >= 0 ? selectionPivot : currentRow - let targetRow = max(0, currentPivot - 1) - selectionPivot = targetRow - - let startRow = min(selectionAnchor, selectionPivot) - let endRow = max(selectionAnchor, selectionPivot) - let range = IndexSet(integersIn: startRow...endRow) - selectRowIndexes(range, byExtendingSelection: false) - scrollRowToVisible(targetRow) - } else { - let targetRow = max(0, currentRow - 1) - selectionAnchor = targetRow - selectionPivot = targetRow - focusedRow = targetRow - selectRowIndexes(IndexSet(integer: targetRow), byExtendingSelection: false) - scrollRowToVisible(targetRow) - } - } - - private func handleDownArrow(currentRow: Int, isShiftHeld: Bool) { - guard numberOfRows > 0 else { return } - - if currentRow == -1 { - selectionAnchor = 0 - selectionPivot = 0 - focusedRow = 0 - selectRowIndexes(IndexSet(integer: 0), byExtendingSelection: false) - scrollRowToVisible(0) - return - } - - if isShiftHeld { - if selectionAnchor == -1 { - selectionAnchor = currentRow - selectionPivot = currentRow - } - - let currentPivot = selectionPivot >= 0 ? selectionPivot : currentRow - let targetRow = min(numberOfRows - 1, currentPivot + 1) - selectionPivot = targetRow - - let startRow = min(selectionAnchor, selectionPivot) - let endRow = max(selectionAnchor, selectionPivot) - let range = IndexSet(integersIn: startRow...endRow) - selectRowIndexes(range, byExtendingSelection: false) - scrollRowToVisible(targetRow) - } else { - let targetRow = min(numberOfRows - 1, currentRow + 1) - selectionAnchor = targetRow - selectionPivot = targetRow - focusedRow = targetRow - selectRowIndexes(IndexSet(integer: targetRow), byExtendingSelection: false) - scrollRowToVisible(targetRow) - } - } - - private func handleHome(isShiftHeld: Bool) { - guard numberOfRows > 0 else { return } - if isShiftHeld { - if selectionAnchor == -1 { - selectionAnchor = selectedRow >= 0 ? selectedRow : 0 - selectionPivot = selectionAnchor - } - selectionPivot = 0 - let range = IndexSet(integersIn: 0...selectionAnchor) - selectRowIndexes(range, byExtendingSelection: false) - } else { - selectionAnchor = 0 - selectionPivot = 0 - focusedRow = 0 - selectRowIndexes(IndexSet(integer: 0), byExtendingSelection: false) - } - scrollRowToVisible(0) - } - - private func handleEnd(isShiftHeld: Bool) { - guard numberOfRows > 0 else { return } - let lastRow = numberOfRows - 1 - if isShiftHeld { - if selectionAnchor == -1 { - selectionAnchor = selectedRow >= 0 ? selectedRow : lastRow - selectionPivot = selectionAnchor - } - selectionPivot = lastRow - let range = IndexSet(integersIn: selectionAnchor...lastRow) - selectRowIndexes(range, byExtendingSelection: false) - } else { - selectionAnchor = lastRow - selectionPivot = lastRow - focusedRow = lastRow - selectRowIndexes(IndexSet(integer: lastRow), byExtendingSelection: false) - } - scrollRowToVisible(lastRow) - } - - private func handlePageUp(isShiftHeld: Bool) { - guard numberOfRows > 0 else { return } - let visibleRows = max(1, Int(visibleRect.height / rowHeight) - 1) - let currentRow = selectedRow >= 0 ? selectedRow : 0 - let targetRow = max(0, currentRow - visibleRows) - - if isShiftHeld { - if selectionAnchor == -1 { - selectionAnchor = currentRow - selectionPivot = currentRow - } - selectionPivot = targetRow - let startRow = min(selectionAnchor, selectionPivot) - let endRow = max(selectionAnchor, selectionPivot) - selectRowIndexes(IndexSet(integersIn: startRow...endRow), byExtendingSelection: false) - } else { - selectionAnchor = targetRow - selectionPivot = targetRow - focusedRow = targetRow - selectRowIndexes(IndexSet(integer: targetRow), byExtendingSelection: false) - } - scrollRowToVisible(targetRow) - } - - private func handlePageDown(isShiftHeld: Bool) { - guard numberOfRows > 0 else { return } - let visibleRows = max(1, Int(visibleRect.height / rowHeight) - 1) - let currentRow = selectedRow >= 0 ? selectedRow : 0 - let lastRow = numberOfRows - 1 - let targetRow = min(lastRow, currentRow + visibleRows) - - if isShiftHeld { - if selectionAnchor == -1 { - selectionAnchor = currentRow - selectionPivot = currentRow - } - selectionPivot = targetRow - let startRow = min(selectionAnchor, selectionPivot) - let endRow = max(selectionAnchor, selectionPivot) - selectRowIndexes(IndexSet(integersIn: startRow...endRow), byExtendingSelection: false) - } else { - selectionAnchor = targetRow - selectionPivot = targetRow - focusedRow = targetRow - selectRowIndexes(IndexSet(integer: targetRow), byExtendingSelection: false) - } - scrollRowToVisible(targetRow) - } - override func menu(for event: NSEvent) -> NSMenu? { let point = convert(event.locationInWindow, from: nil) let clickedRow = row(at: point) @@ -515,7 +254,6 @@ final class KeyHandlingTableView: NSTableView { return rowView.menu(for: event) } - // Empty space: ask delegate for a fallback menu (e.g., Structure tab "Add" actions) if let menu = coordinator?.delegate?.dataGridEmptySpaceMenu() { return menu } diff --git a/TablePro/Views/Results/TableSelection.swift b/TablePro/Views/Results/TableSelection.swift index cca56fdae..0d35e6858 100644 --- a/TablePro/Views/Results/TableSelection.swift +++ b/TablePro/Views/Results/TableSelection.swift @@ -1,15 +1,8 @@ -// -// TableSelection.swift -// TablePro -// - import Foundation struct TableSelection: Equatable { var focusedRow: Int = -1 var focusedColumn: Int = -1 - var anchor: Int = -1 - var pivot: Int = -1 var hasFocus: Bool { focusedRow >= 0 && focusedColumn >= 0 } @@ -25,16 +18,6 @@ struct TableSelection: Equatable { focusedColumn = column } - mutating func resetAnchor(at row: Int) { - anchor = row - pivot = row - } - - mutating func clearAnchor() { - anchor = -1 - pivot = -1 - } - func reloadIndexes(from previous: TableSelection) -> (rows: IndexSet, columns: IndexSet)? { guard previous.focusedRow != focusedRow || previous.focusedColumn != focusedColumn else { return nil diff --git a/TableProTests/Core/ChangeTracking/DataChangeManagerExtendedTests.swift b/TableProTests/Core/ChangeTracking/DataChangeManagerExtendedTests.swift index 6230c9e75..d6137b9eb 100644 --- a/TableProTests/Core/ChangeTracking/DataChangeManagerExtendedTests.swift +++ b/TableProTests/Core/ChangeTracking/DataChangeManagerExtendedTests.swift @@ -85,7 +85,7 @@ struct DataChangeManagerExtendedTests { rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: "A", newValue: "B" ) - _ = manager.undoLastChange() + manager.undoManagerProvider?()?.undo() #expect(manager.canRedo) manager.recordRowInsertion(rowIndex: 5, values: ["a", "b", "c"]) #expect(!manager.canRedo) @@ -335,7 +335,7 @@ struct DataChangeManagerExtendedTests { rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: "Alice", newValue: "Bob" ) - _ = manager1.undoLastChange() + manager1.undoManagerProvider?()?.undo() #expect(manager1.canRedo) manager1.discardChanges() #expect(manager1.canRedo) @@ -346,7 +346,7 @@ struct DataChangeManagerExtendedTests { rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: "Alice", newValue: "Bob" ) - _ = manager2.undoLastChange() + manager2.undoManagerProvider?()?.undo() #expect(manager2.canRedo) manager2.clearChanges() #expect(!manager2.canUndo) @@ -395,8 +395,8 @@ struct DataChangeManagerExtendedTests { rowIndex: 1, columnIndex: 1, columnName: "name", oldValue: "Charlie", newValue: "Dave" ) - _ = manager.undoLastChange() - _ = manager.undoLastChange() + manager.undoManagerProvider?()?.undo() + manager.undoManagerProvider?()?.undo() #expect(manager.changes.isEmpty) #expect(!manager.hasChanges) } @@ -408,9 +408,9 @@ struct DataChangeManagerExtendedTests { rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: "A", newValue: "B" ) - _ = manager.undoLastChange() + manager.undoManagerProvider?()?.undo() #expect(manager.changes.isEmpty) - _ = manager.redoLastChange() + manager.undoManagerProvider?()?.redo() #expect(manager.changes.count == 1) #expect(manager.changes[0].cellChanges[0].newValue == "B") } @@ -419,7 +419,7 @@ struct DataChangeManagerExtendedTests { func undoRowInsertionRemovesFromIndices() { let manager = makeManager() manager.recordRowInsertion(rowIndex: 5, values: ["a", "b", "c"]) - _ = manager.undoLastChange() + manager.undoManagerProvider?()?.undo() #expect(!manager.isRowInserted(5)) } @@ -427,7 +427,7 @@ struct DataChangeManagerExtendedTests { func undoRowDeletionRemovesFromIndices() { let manager = makeManager() manager.recordRowDeletion(rowIndex: 2, originalRow: ["3", "Charlie", "c@test.com"]) - _ = manager.undoLastChange() + manager.undoManagerProvider?()?.undo() #expect(!manager.isRowDeleted(2)) } @@ -435,9 +435,9 @@ struct DataChangeManagerExtendedTests { func undoRowInsertionThenRedoReInserts() { let manager = makeManager() manager.recordRowInsertion(rowIndex: 5, values: ["a", "b", "c"]) - _ = manager.undoLastChange() + manager.undoManagerProvider?()?.undo() #expect(!manager.isRowInserted(5)) - _ = manager.redoLastChange() + manager.undoManagerProvider?()?.redo() #expect(manager.isRowInserted(5)) } @@ -445,9 +445,9 @@ struct DataChangeManagerExtendedTests { func undoRowDeletionThenRedoReDeletes() { let manager = makeManager() manager.recordRowDeletion(rowIndex: 2, originalRow: ["3", "Charlie", "c@test.com"]) - _ = manager.undoLastChange() + manager.undoManagerProvider?()?.undo() #expect(!manager.isRowDeleted(2)) - _ = manager.redoLastChange() + manager.undoManagerProvider?()?.redo() #expect(manager.isRowDeleted(2)) } @@ -464,63 +464,77 @@ struct DataChangeManagerExtendedTests { ) #expect(manager.changes.count == 2) - _ = manager.undoLastChange() + manager.undoManagerProvider?()?.undo() #expect(manager.changes.count == 1) - _ = manager.undoLastChange() + manager.undoManagerProvider?()?.undo() #expect(manager.changes.count == 0) - _ = manager.redoLastChange() + manager.undoManagerProvider?()?.redo() #expect(manager.changes.count == 1) - _ = manager.redoLastChange() + manager.undoManagerProvider?()?.redo() #expect(manager.changes.count == 2) } @Test("Undo returns cell edit action details with correct flags") func undoReturnsCellEditActionDetails() { let manager = makeManager() + var captured: UndoResult? + manager.onUndoApplied = { captured = $0 } manager.recordCellChange( rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: "Alice", newValue: "Bob" ) - let result = manager.undoLastChange() - #expect(result != nil) - #expect(result?.needsRowRemoval == false) - #expect(result?.needsRowRestore == false) + manager.undoManagerProvider?()?.undo() + #expect(captured != nil) + #expect(captured?.needsRowRemoval == false) + #expect(captured?.needsRowRestore == false) } @Test("Undo returns row insertion action details with needsRowRemoval") func undoReturnsRowInsertionActionDetails() { let manager = makeManager() + var captured: UndoResult? + manager.onUndoApplied = { captured = $0 } manager.recordRowInsertion(rowIndex: 5, values: ["a", "b", "c"]) - let result = manager.undoLastChange() - #expect(result != nil) - #expect(result?.needsRowRemoval == true) + manager.undoManagerProvider?()?.undo() + #expect(captured != nil) + #expect(captured?.needsRowRemoval == true) } @Test("Undo returns row deletion action details with needsRowRestore and restoreRow") func undoReturnsRowDeletionActionDetails() { let manager = makeManager() + var captured: UndoResult? + manager.onUndoApplied = { captured = $0 } manager.recordRowDeletion(rowIndex: 0, originalRow: ["1", "Alice"]) - let result = manager.undoLastChange() - #expect(result != nil) - #expect(result?.needsRowRestore == true) - #expect(result?.restoreRow == ["1", "Alice"]) + manager.undoManagerProvider?()?.undo() + #expect(captured != nil) + #expect(captured?.needsRowRestore == true) + #expect(captured?.restoreRow == ["1", "Alice"]) } - @Test("Undo returns nil when undo stack is empty") - func undoReturnsNilWhenStackEmpty() { + @Test("Undo does nothing when undo stack is empty") + func undoNoopWhenStackEmpty() { let manager = makeManager() - let result = manager.undoLastChange() - #expect(result == nil) + var captured: UndoResult? + manager.onUndoApplied = { captured = $0 } + let undoManager = manager.undoManagerProvider?() + #expect(undoManager?.canUndo == false) + undoManager?.undo() + #expect(captured == nil) } - @Test("Redo returns nil when redo stack is empty") - func redoReturnsNilWhenStackEmpty() { + @Test("Redo does nothing when redo stack is empty") + func redoNoopWhenStackEmpty() { let manager = makeManager() - let result = manager.redoLastChange() - #expect(result == nil) + var captured: UndoResult? + manager.onUndoApplied = { captured = $0 } + let undoManager = manager.undoManagerProvider?() + #expect(undoManager?.canRedo == false) + undoManager?.redo() + #expect(captured == nil) } // MARK: - Interaction Between Operations @@ -560,7 +574,7 @@ struct DataChangeManagerExtendedTests { rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: nil, newValue: "hello" ) - _ = manager.undoLastChange() + manager.undoManagerProvider?()?.undo() let state = manager.saveState() #expect(state.insertedRowData[0]?[1] == nil) } @@ -633,7 +647,7 @@ struct DataChangeManagerExtendedTests { (rowIndex: 1, originalRow: ["2", "Bob", "b@test.com"]), (rowIndex: 2, originalRow: ["3", "Charlie", "c@test.com"]) ]) - _ = manager.undoLastChange() + manager.undoManagerProvider?()?.undo() #expect(!manager.isRowDeleted(0)) #expect(!manager.isRowDeleted(1)) #expect(!manager.isRowDeleted(2)) @@ -755,7 +769,7 @@ struct DataChangeManagerExtendedTests { rowIndex: 0, columnIndex: 2, columnName: "email", oldValue: "a@test.com", newValue: "b@test.com" ) - _ = manager.undoLastChange() + manager.undoManagerProvider?()?.undo() #expect(!manager.isCellModified(rowIndex: 0, columnIndex: 2)) #expect(manager.isCellModified(rowIndex: 0, columnIndex: 1)) } @@ -767,8 +781,8 @@ struct DataChangeManagerExtendedTests { rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: "Alice", newValue: "Bob" ) - _ = manager.undoLastChange() - _ = manager.redoLastChange() + manager.undoManagerProvider?()?.undo() + manager.undoManagerProvider?()?.redo() #expect(manager.isCellModified(rowIndex: 0, columnIndex: 1)) #expect(!manager.changes.isEmpty) } @@ -780,11 +794,11 @@ struct DataChangeManagerExtendedTests { rowIndex: 0, columnIndex: 1, columnName: "name", oldValue: "Alice", newValue: "Bob" ) - _ = manager.undoLastChange() - _ = manager.redoLastChange() + manager.undoManagerProvider?()?.undo() + manager.undoManagerProvider?()?.redo() #expect(manager.isCellModified(rowIndex: 0, columnIndex: 1)) - _ = manager.undoLastChange() + manager.undoManagerProvider?()?.undo() #expect(!manager.isCellModified(rowIndex: 0, columnIndex: 1)) #expect(manager.changes.isEmpty) #expect(!manager.hasChanges) diff --git a/TableProTests/Core/ChangeTracking/DataChangeManagerTests.swift b/TableProTests/Core/ChangeTracking/DataChangeManagerTests.swift index aa70a39f4..b8974d0aa 100644 --- a/TableProTests/Core/ChangeTracking/DataChangeManagerTests.swift +++ b/TableProTests/Core/ChangeTracking/DataChangeManagerTests.swift @@ -408,7 +408,7 @@ struct DataChangeManagerTests { ) #expect(manager.changes.count == 1) - _ = manager.undoLastChange() + manager.undoManagerProvider?()?.undo() #expect(manager.changes.isEmpty) #expect(!manager.hasChanges) @@ -431,7 +431,7 @@ struct DataChangeManagerTests { newValue: "Bob" ) - _ = manager.undoLastChange() + manager.undoManagerProvider?()?.undo() #expect(manager.canRedo) } @@ -453,7 +453,7 @@ struct DataChangeManagerTests { newValue: "Bob" ) - _ = manager.undoLastChange() + manager.undoManagerProvider?()?.undo() #expect(manager.canRedo) manager.recordCellChange( diff --git a/TableProTests/Views/Results/TableSelectionTests.swift b/TableProTests/Views/Results/TableSelectionTests.swift index e5cc30363..d82aaa493 100644 --- a/TableProTests/Views/Results/TableSelectionTests.swift +++ b/TableProTests/Views/Results/TableSelectionTests.swift @@ -14,8 +14,6 @@ struct TableSelectionTests { let selection = TableSelection() #expect(selection.focusedRow == -1) #expect(selection.focusedColumn == -1) - #expect(selection.anchor == -1) - #expect(selection.pivot == -1) #expect(selection.hasFocus == false) } @@ -30,16 +28,13 @@ struct TableSelectionTests { #expect(selection.hasFocus == false) } - @Test("clearFocus resets focus only") - func clearFocusKeepsAnchor() { + @Test("clearFocus resets focus") + func clearFocus() { var selection = TableSelection() selection.setFocus(row: 5, column: 2) - selection.resetAnchor(at: 5) selection.clearFocus() #expect(selection.focusedRow == -1) #expect(selection.focusedColumn == -1) - #expect(selection.anchor == 5) - #expect(selection.pivot == 5) } @Test("setFocus assigns row and column") @@ -50,28 +45,10 @@ struct TableSelectionTests { #expect(selection.focusedColumn == 3) } - @Test("resetAnchor sets both anchor and pivot") - func resetAnchor() { - var selection = TableSelection() - selection.resetAnchor(at: 4) - #expect(selection.anchor == 4) - #expect(selection.pivot == 4) - } - - @Test("clearAnchor resets anchor and pivot") - func clearAnchor() { - var selection = TableSelection() - selection.resetAnchor(at: 4) - selection.clearAnchor() - #expect(selection.anchor == -1) - #expect(selection.pivot == -1) - } - - @Test("Equatable compares all four fields") + @Test("Equatable compares focus fields") func equatable() { var a = TableSelection() a.setFocus(row: 1, column: 2) - a.resetAnchor(at: 1) var b = a #expect(a == b) b.focusedRow = 2 @@ -89,15 +66,6 @@ struct TableSelectionReloadIndexesTests { #expect(selection.reloadIndexes(from: same) == nil) } - @Test("Anchor change without focus change returns nil") - func anchorOnlyChange() { - var previous = TableSelection() - previous.setFocus(row: 5, column: 2) - var current = previous - current.resetAnchor(at: 8) - #expect(current.reloadIndexes(from: previous) == nil) - } - @Test("Initial focus from empty includes new cell only") func initialFocus() { let previous = TableSelection() diff --git a/docs/development/datagrid-audit.md b/docs/development/datagrid-audit.md new file mode 100644 index 000000000..61a132f3c --- /dev/null +++ b/docs/development/datagrid-audit.md @@ -0,0 +1,57 @@ +# DataGrid Audit + +Tracks issues found during the DataGrid refactor (54 total) and their resolution status across phases. + +## Phase 3 Status (2026-04-30) + +Refactor complete. Phase 3 audit re-evaluated, most issues found to be already correct in current code (Phase 1/2 era fixes). Real changes in Phase 3: + +- Removed Ctrl+HJKL Vim navigation (#12) +- Removed custom Shift+Arrow / Home / End / PageUp / PageDown, replaced with NSTableView native via `interpretKeyEvents` (#34, #49) +- NSUndoManager dual-invocation cleanup (#24) + +Issues marked "Already correct" had explorer findings that disagreed with audit. See per-issue annotations. + +## Issue Index + +### #6 Single-click to edit +Not applicable. Single-click edit conflicts with cell selection in data grid context. Double-click is correct native pattern (matches Numbers, TablePlus, DBeaver, DataGrip). + +### #10 Escape key behavior +Already correct. Escape only cancels active edit, does not deselect rows. + +### #11 Right-click responder chain +Already correct. `CellTextField.rightMouseDown` walks responder chain to `TableRowViewWithMenu`. + +### #12 Ctrl+HJKL Vim navigation +Fixed in Phase 3. Ctrl+HJKL block removed from `KeyHandlingTableView.keyDown`. NSTableView native handles arrow keys via `interpretKeyEvents` and the standard `moveLeft:`/`moveRight:`/`moveUp:`/`moveDown:` selectors. + +### #13 Context menu key equivalents +Already correct. Context menu items use `keyEquivalent: ""` so they do not shadow system shortcuts. + +### #14 Overlay editor dismiss +Already correct. `CellOverlayEditor` uses NSPanel + `resignKey`, not a global event monitor. + +### #16 Action dispatch +Already correct. No dual-path action dispatch found. + +### #23 Paste flow +Already correct. Paste flows through responder chain via `paste(_:)` selector. + +### #24 NSUndoManager dual invocation +Cleaned in Phase 3. `DataChangeManager.undoLastChange()` and `redoLastChange()` removed. `RowOperationsManager.undoLastChange(tableRows:)` and `redoLastChange(tableRows:)` were dead wrappers, also removed. Production undo flows through Cmd+Z, the responder chain, NSUndoManager, registered undo blocks, and the `onUndoApplied` callback. + +### #32 Pasteboard format +Already correct. Pasteboard writes `.string`, custom TSV, `.html` types. + +### #33 Multi-cell paste +Already correct. Multi-cell paste implemented. Line 20 heuristic in `KeyHandlingTableView.paste` routes full-row pastes to `dataGridPasteRows` intentionally when no cell focus is active. + +### #34 Custom Home/End/PageUp/PageDown +Fixed in Phase 3. Custom helpers removed. NSTableView native via `interpretKeyEvents` routes to `scrollToBeginningOfDocument:`, `scrollToEndOfDocument:`, `scrollPageUp:`, `scrollPageDown:` with native `andModifySelection:` Shift variants. + +### #35 Shift+Tab in edit mode +Already correct. Shift+Tab works in browse and edit modes via `handleShiftTabKey`. + +### #49 Custom Shift+Arrow range selection +Fixed in Phase 3. Custom Shift+Up/Down handlers removed. NSTableView native via `interpretKeyEvents` handles `moveUpAndModifySelection:` and `moveDownAndModifySelection:` and maintains its own selection anchor internally. From 05a34e3c3bc6a0821f3ac3dc8faccec6c111b3fd Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Thu, 30 Apr 2026 18:41:24 +0700 Subject: [PATCH 02/11] docs(datagrid): remove Phase 3 audit doc --- docs/development/datagrid-audit.md | 57 ------------------------------ 1 file changed, 57 deletions(-) delete mode 100644 docs/development/datagrid-audit.md diff --git a/docs/development/datagrid-audit.md b/docs/development/datagrid-audit.md deleted file mode 100644 index 61a132f3c..000000000 --- a/docs/development/datagrid-audit.md +++ /dev/null @@ -1,57 +0,0 @@ -# DataGrid Audit - -Tracks issues found during the DataGrid refactor (54 total) and their resolution status across phases. - -## Phase 3 Status (2026-04-30) - -Refactor complete. Phase 3 audit re-evaluated, most issues found to be already correct in current code (Phase 1/2 era fixes). Real changes in Phase 3: - -- Removed Ctrl+HJKL Vim navigation (#12) -- Removed custom Shift+Arrow / Home / End / PageUp / PageDown, replaced with NSTableView native via `interpretKeyEvents` (#34, #49) -- NSUndoManager dual-invocation cleanup (#24) - -Issues marked "Already correct" had explorer findings that disagreed with audit. See per-issue annotations. - -## Issue Index - -### #6 Single-click to edit -Not applicable. Single-click edit conflicts with cell selection in data grid context. Double-click is correct native pattern (matches Numbers, TablePlus, DBeaver, DataGrip). - -### #10 Escape key behavior -Already correct. Escape only cancels active edit, does not deselect rows. - -### #11 Right-click responder chain -Already correct. `CellTextField.rightMouseDown` walks responder chain to `TableRowViewWithMenu`. - -### #12 Ctrl+HJKL Vim navigation -Fixed in Phase 3. Ctrl+HJKL block removed from `KeyHandlingTableView.keyDown`. NSTableView native handles arrow keys via `interpretKeyEvents` and the standard `moveLeft:`/`moveRight:`/`moveUp:`/`moveDown:` selectors. - -### #13 Context menu key equivalents -Already correct. Context menu items use `keyEquivalent: ""` so they do not shadow system shortcuts. - -### #14 Overlay editor dismiss -Already correct. `CellOverlayEditor` uses NSPanel + `resignKey`, not a global event monitor. - -### #16 Action dispatch -Already correct. No dual-path action dispatch found. - -### #23 Paste flow -Already correct. Paste flows through responder chain via `paste(_:)` selector. - -### #24 NSUndoManager dual invocation -Cleaned in Phase 3. `DataChangeManager.undoLastChange()` and `redoLastChange()` removed. `RowOperationsManager.undoLastChange(tableRows:)` and `redoLastChange(tableRows:)` were dead wrappers, also removed. Production undo flows through Cmd+Z, the responder chain, NSUndoManager, registered undo blocks, and the `onUndoApplied` callback. - -### #32 Pasteboard format -Already correct. Pasteboard writes `.string`, custom TSV, `.html` types. - -### #33 Multi-cell paste -Already correct. Multi-cell paste implemented. Line 20 heuristic in `KeyHandlingTableView.paste` routes full-row pastes to `dataGridPasteRows` intentionally when no cell focus is active. - -### #34 Custom Home/End/PageUp/PageDown -Fixed in Phase 3. Custom helpers removed. NSTableView native via `interpretKeyEvents` routes to `scrollToBeginningOfDocument:`, `scrollToEndOfDocument:`, `scrollPageUp:`, `scrollPageDown:` with native `andModifySelection:` Shift variants. - -### #35 Shift+Tab in edit mode -Already correct. Shift+Tab works in browse and edit modes via `handleShiftTabKey`. - -### #49 Custom Shift+Arrow range selection -Fixed in Phase 3. Custom Shift+Up/Down handlers removed. NSTableView native via `interpretKeyEvents` handles `moveUpAndModifySelection:` and `moveDownAndModifySelection:` and maintains its own selection anchor internally. From b70d6a4cd0f6174f8281d398d383da419af833cb Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Thu, 30 Apr 2026 18:45:46 +0700 Subject: [PATCH 03/11] fix(datagrid): route unhandled keys to super.keyDown so NSTableView handles arrow nav natively --- TablePro/Views/Results/KeyHandlingTableView.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/TablePro/Views/Results/KeyHandlingTableView.swift b/TablePro/Views/Results/KeyHandlingTableView.swift index 5971f246e..1d6d09341 100644 --- a/TablePro/Views/Results/KeyHandlingTableView.swift +++ b/TablePro/Views/Results/KeyHandlingTableView.swift @@ -148,7 +148,7 @@ final class KeyHandlingTableView: NSTableView { return } - interpretKeyEvents([event]) + super.keyDown(with: event) } @objc override func insertNewline(_ sender: Any?) { From edfbc07358451537dcef1c58eb9110538961cfde Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Thu, 30 Apr 2026 18:54:08 +0700 Subject: [PATCH 04/11] fix(datagrid): draw focus ring even when cell is on emphasized row --- TablePro/Views/Results/Cells/DataGridBaseCellView.swift | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/TablePro/Views/Results/Cells/DataGridBaseCellView.swift b/TablePro/Views/Results/Cells/DataGridBaseCellView.swift index 3f5225b6f..4bf353091 100644 --- a/TablePro/Views/Results/Cells/DataGridBaseCellView.swift +++ b/TablePro/Views/Results/Cells/DataGridBaseCellView.swift @@ -188,8 +188,8 @@ class DataGridBaseCellView: NSTableCellView { override var backgroundStyle: NSView.BackgroundStyle { didSet { - backgroundView.isHidden = (backgroundStyle == .emphasized) || (changeBackgroundColor == nil) - if isFocusedCell { updateFocusRing() } + backgroundView.isHidden = (changeBackgroundColor == nil) + if isFocusedCell { needsDisplay = true } } } @@ -201,7 +201,7 @@ class DataGridBaseCellView: NSTableCellView { override func draw(_ dirtyRect: NSRect) { super.draw(dirtyRect) - guard isFocusedCell, backgroundStyle != .emphasized else { return } + guard isFocusedCell else { return } NSGraphicsContext.saveGraphicsState() NSFocusRingPlacement.only.set() drawFocusRingMask() From b2a6f9411b688671ec23e96ac6a25cbbd55dedcf Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Thu, 30 Apr 2026 18:55:38 +0700 Subject: [PATCH 05/11] fix(datagrid): draw contrasting border for cell focus on emphasized row --- .../Results/Cells/DataGridBaseCellView.swift | 28 +++++++++++++++---- 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/TablePro/Views/Results/Cells/DataGridBaseCellView.swift b/TablePro/Views/Results/Cells/DataGridBaseCellView.swift index 4bf353091..274aa1164 100644 --- a/TablePro/Views/Results/Cells/DataGridBaseCellView.swift +++ b/TablePro/Views/Results/Cells/DataGridBaseCellView.swift @@ -193,19 +193,35 @@ class DataGridBaseCellView: NSTableCellView { } } - override var focusRingMaskBounds: NSRect { bounds } + override var focusRingMaskBounds: NSRect { + backgroundStyle == .emphasized ? .zero : bounds + } override func drawFocusRingMask() { + guard backgroundStyle != .emphasized else { return } NSBezierPath(rect: bounds).fill() } override func draw(_ dirtyRect: NSRect) { super.draw(dirtyRect) guard isFocusedCell else { return } - NSGraphicsContext.saveGraphicsState() - NSFocusRingPlacement.only.set() - drawFocusRingMask() - NSGraphicsContext.restoreGraphicsState() + if backgroundStyle == .emphasized { + drawEmphasizedFocusBorder() + } else { + NSGraphicsContext.saveGraphicsState() + NSFocusRingPlacement.only.set() + drawFocusRingMask() + NSGraphicsContext.restoreGraphicsState() + } + } + + private func drawEmphasizedFocusBorder() { + let inset: CGFloat = 1 + let rect = bounds.insetBy(dx: inset, dy: inset) + let path = NSBezierPath(rect: rect) + path.lineWidth = 2 + NSColor.alternateSelectedControlTextColor.setStroke() + path.stroke() } override func viewDidChangeEffectiveAppearance() { @@ -216,7 +232,7 @@ class DataGridBaseCellView: NSTableCellView { } private func updateFocusRing() { - focusRingType = isFocusedCell ? .exterior : .none + focusRingType = (isFocusedCell && backgroundStyle != .emphasized) ? .exterior : .none noteFocusRingMaskChanged() needsDisplay = true } From 2ba06c585afd23a74078a0ce55e9001b99c386f5 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Thu, 30 Apr 2026 18:59:39 +0700 Subject: [PATCH 06/11] refactor(datagrid): extract cell focus indicator into layer-backed CellFocusOverlay --- .../Results/Cells/CellFocusOverlay.swift | 50 ++++++++++++++++++ .../Results/Cells/DataGridBaseCellView.swift | 52 +++++++------------ 2 files changed, 68 insertions(+), 34 deletions(-) create mode 100644 TablePro/Views/Results/Cells/CellFocusOverlay.swift diff --git a/TablePro/Views/Results/Cells/CellFocusOverlay.swift b/TablePro/Views/Results/Cells/CellFocusOverlay.swift new file mode 100644 index 000000000..ed8ecda9f --- /dev/null +++ b/TablePro/Views/Results/Cells/CellFocusOverlay.swift @@ -0,0 +1,50 @@ +// +// CellFocusOverlay.swift +// TablePro +// + +import AppKit + +final class CellFocusOverlay: NSView { + enum Style { + case hidden + case contrastingBorder + } + + var style: Style = .hidden { + didSet { + guard oldValue != style else { return } + applyStyle() + } + } + + override init(frame frameRect: NSRect) { + super.init(frame: frameRect) + wantsLayer = true + translatesAutoresizingMaskIntoConstraints = false + isHidden = true + } + + required init?(coder: NSCoder) { + fatalError("init(coder:) has not been implemented") + } + + override func hitTest(_ point: NSPoint) -> NSView? { nil } + + override func viewDidChangeEffectiveAppearance() { + super.viewDidChangeEffectiveAppearance() + if !isHidden { applyStyle() } + } + + private func applyStyle() { + switch style { + case .hidden: + isHidden = true + layer?.borderWidth = 0 + case .contrastingBorder: + isHidden = false + layer?.borderWidth = 2 + layer?.borderColor = NSColor.alternateSelectedControlTextColor.cgColor + } + } +} diff --git a/TablePro/Views/Results/Cells/DataGridBaseCellView.swift b/TablePro/Views/Results/Cells/DataGridBaseCellView.swift index 274aa1164..0294de5eb 100644 --- a/TablePro/Views/Results/Cells/DataGridBaseCellView.swift +++ b/TablePro/Views/Results/Cells/DataGridBaseCellView.swift @@ -34,10 +34,22 @@ class DataGridBaseCellView: NSTableCellView { var isFocusedCell: Bool = false { didSet { guard oldValue != isFocusedCell else { return } - updateFocusRing() + updateFocusPresentation() } } + private lazy var focusOverlay: CellFocusOverlay = { + let overlay = CellFocusOverlay() + addSubview(overlay) + NSLayoutConstraint.activate([ + overlay.leadingAnchor.constraint(equalTo: leadingAnchor), + overlay.trailingAnchor.constraint(equalTo: trailingAnchor), + overlay.topAnchor.constraint(equalTo: topAnchor), + overlay.bottomAnchor.constraint(equalTo: bottomAnchor), + ]) + return overlay + }() + private(set) lazy var backgroundView: NSView = { let view = NSView() view.wantsLayer = true @@ -189,7 +201,7 @@ class DataGridBaseCellView: NSTableCellView { override var backgroundStyle: NSView.BackgroundStyle { didSet { backgroundView.isHidden = (changeBackgroundColor == nil) - if isFocusedCell { needsDisplay = true } + updateFocusPresentation() } } @@ -202,38 +214,10 @@ class DataGridBaseCellView: NSTableCellView { NSBezierPath(rect: bounds).fill() } - override func draw(_ dirtyRect: NSRect) { - super.draw(dirtyRect) - guard isFocusedCell else { return } - if backgroundStyle == .emphasized { - drawEmphasizedFocusBorder() - } else { - NSGraphicsContext.saveGraphicsState() - NSFocusRingPlacement.only.set() - drawFocusRingMask() - NSGraphicsContext.restoreGraphicsState() - } - } - - private func drawEmphasizedFocusBorder() { - let inset: CGFloat = 1 - let rect = bounds.insetBy(dx: inset, dy: inset) - let path = NSBezierPath(rect: rect) - path.lineWidth = 2 - NSColor.alternateSelectedControlTextColor.setStroke() - path.stroke() - } - - override func viewDidChangeEffectiveAppearance() { - super.viewDidChangeEffectiveAppearance() - if isFocusedCell { - needsDisplay = true - } - } - - private func updateFocusRing() { - focusRingType = (isFocusedCell && backgroundStyle != .emphasized) ? .exterior : .none + private func updateFocusPresentation() { + let onEmphasized = backgroundStyle == .emphasized + focusOverlay.style = (isFocusedCell && onEmphasized) ? .contrastingBorder : .hidden + focusRingType = (isFocusedCell && !onEmphasized) ? .exterior : .none noteFocusRingMaskChanged() - needsDisplay = true } } From a70b8272431b9463d29b903aca685e3e142bdc89 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Thu, 30 Apr 2026 19:02:33 +0700 Subject: [PATCH 07/11] fix(datagrid): route non-navigation keys through interpretKeyEvents so Delete/Return reach overrides --- TablePro/Views/Results/KeyHandlingTableView.swift | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/TablePro/Views/Results/KeyHandlingTableView.swift b/TablePro/Views/Results/KeyHandlingTableView.swift index 1d6d09341..d74fa14f8 100644 --- a/TablePro/Views/Results/KeyHandlingTableView.swift +++ b/TablePro/Views/Results/KeyHandlingTableView.swift @@ -134,6 +134,10 @@ final class KeyHandlingTableView: NSTableView { handleRightArrow(currentRow: row) return + case .upArrow, .downArrow, .home, .end, .pageUp, .pageDown: + super.keyDown(with: event) + return + default: break } @@ -148,7 +152,7 @@ final class KeyHandlingTableView: NSTableView { return } - super.keyDown(with: event) + interpretKeyEvents([event]) } @objc override func insertNewline(_ sender: Any?) { From 1918b9da3521ef6e488b020777508ddc8878c45b Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Thu, 30 Apr 2026 19:14:51 +0700 Subject: [PATCH 08/11] fix(datagrid): defer focus-reload to avoid reentrancy + track active end of multi-row selection --- .../Extensions/DataGridView+Selection.swift | 64 +++++++++++++++---- .../Views/Results/KeyHandlingTableView.swift | 32 ++++++++-- 2 files changed, 78 insertions(+), 18 deletions(-) diff --git a/TablePro/Views/Results/Extensions/DataGridView+Selection.swift b/TablePro/Views/Results/Extensions/DataGridView+Selection.swift index c5872a261..06ecc90a4 100644 --- a/TablePro/Views/Results/Extensions/DataGridView+Selection.swift +++ b/TablePro/Views/Results/Extensions/DataGridView+Selection.swift @@ -30,24 +30,60 @@ extension TableViewCoordinator { guard !isSyncingSelection else { return } guard let tableView = notification.object as? NSTableView else { return } + let previousSelection = selectedRowIndices let newSelection = Set(tableView.selectedRowIndexes.map { $0 }) - if newSelection != selectedRowIndices { + if newSelection != previousSelection { selectedRowIndices = newSelection } - if let keyTableView = tableView as? KeyHandlingTableView { - if newSelection.isEmpty { - keyTableView.focusedRow = -1 - keyTableView.focusedColumn = -1 - } else if keyTableView.focusedRow < 0, let firstRow = newSelection.min() { - keyTableView.focusedRow = firstRow - keyTableView.focusedColumn = 1 - } else if tableView.numberOfSelectedRows == 1 { - let newRow = tableView.selectedRow - if keyTableView.focusedRow != newRow { - keyTableView.focusedRow = newRow - } - } + guard let keyTableView = tableView as? KeyHandlingTableView else { return } + + let newFocus = resolvedFocus( + previous: previousSelection, + current: newSelection, + existingFocusedRow: keyTableView.focusedRow, + existingFocusedColumn: keyTableView.focusedColumn, + tableView: tableView + ) + + if keyTableView.focusedRow != newFocus.row { + keyTableView.focusedRow = newFocus.row + } + if keyTableView.focusedColumn != newFocus.column { + keyTableView.focusedColumn = newFocus.column + } + } + + private func resolvedFocus( + previous: Set, + current: Set, + existingFocusedRow: Int, + existingFocusedColumn: Int, + tableView: NSTableView + ) -> (row: Int, column: Int) { + if current.isEmpty { + return (-1, -1) + } + + let column = existingFocusedColumn >= 1 ? existingFocusedColumn : 1 + let added = current.subtracting(previous) + + if let tip = added.max() { + return (tip, column) + } + + let removed = previous.subtracting(current) + if let lostTip = removed.max(), + let currentMax = current.max(), + let currentMin = current.min() { + let row = lostTip > currentMax ? currentMax : currentMin + return (row, column) } + + if existingFocusedRow >= 0, current.contains(existingFocusedRow) { + return (existingFocusedRow, column) + } + + return (tableView.selectedRow, column) } } diff --git a/TablePro/Views/Results/KeyHandlingTableView.swift b/TablePro/Views/Results/KeyHandlingTableView.swift index d74fa14f8..42323a93e 100644 --- a/TablePro/Views/Results/KeyHandlingTableView.swift +++ b/TablePro/Views/Results/KeyHandlingTableView.swift @@ -10,13 +10,37 @@ final class KeyHandlingTableView: NSTableView { var selection = TableSelection() { didSet { guard let (rows, columns) = selection.reloadIndexes(from: oldValue) else { return } - let validRows = rows.filteredIndexSet { $0 < numberOfRows } - let validColumns = columns.filteredIndexSet { $0 < numberOfColumns } - guard !validRows.isEmpty, !validColumns.isEmpty else { return } - reloadData(forRowIndexes: validRows, columnIndexes: validColumns) + scheduleFocusReload(rows: rows, columns: columns) } } + private var pendingFocusReloadRows: IndexSet? + private var pendingFocusReloadColumns: IndexSet? + + private func scheduleFocusReload(rows: IndexSet, columns: IndexSet) { + if pendingFocusReloadRows != nil { + pendingFocusReloadRows?.formUnion(rows) + pendingFocusReloadColumns?.formUnion(columns) + return + } + pendingFocusReloadRows = rows + pendingFocusReloadColumns = columns + DispatchQueue.main.async { [weak self] in + self?.flushPendingFocusReload() + } + } + + private func flushPendingFocusReload() { + guard let pendingRows = pendingFocusReloadRows, + let pendingColumns = pendingFocusReloadColumns else { return } + pendingFocusReloadRows = nil + pendingFocusReloadColumns = nil + let validRows = pendingRows.filteredIndexSet { $0 < numberOfRows } + let validColumns = pendingColumns.filteredIndexSet { $0 < numberOfColumns } + guard !validRows.isEmpty, !validColumns.isEmpty else { return } + reloadData(forRowIndexes: validRows, columnIndexes: validColumns) + } + var focusedRow: Int { get { selection.focusedRow } set { selection.focusedRow = newValue } From bdeb7390773aa63222ae007bf6b4cc2fea681489 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Thu, 30 Apr 2026 19:28:25 +0700 Subject: [PATCH 09/11] fix(datagrid): review P0/P1 + header truncation + multi-sort weight + arrow nav skips hidden columns + Backspace-only row delete --- .../Results/Cells/DataGridBaseCellView.swift | 2 +- .../Extensions/DataGridView+Selection.swift | 2 +- .../Views/Results/KeyHandlingTableView.swift | 75 ++++++++++++++----- .../Views/Results/SortableHeaderCell.swift | 11 +++ 4 files changed, 71 insertions(+), 19 deletions(-) diff --git a/TablePro/Views/Results/Cells/DataGridBaseCellView.swift b/TablePro/Views/Results/Cells/DataGridBaseCellView.swift index 0294de5eb..369e88dee 100644 --- a/TablePro/Views/Results/Cells/DataGridBaseCellView.swift +++ b/TablePro/Views/Results/Cells/DataGridBaseCellView.swift @@ -200,7 +200,7 @@ class DataGridBaseCellView: NSTableCellView { override var backgroundStyle: NSView.BackgroundStyle { didSet { - backgroundView.isHidden = (changeBackgroundColor == nil) + backgroundView.isHidden = (backgroundStyle == .emphasized) || (changeBackgroundColor == nil) updateFocusPresentation() } } diff --git a/TablePro/Views/Results/Extensions/DataGridView+Selection.swift b/TablePro/Views/Results/Extensions/DataGridView+Selection.swift index 06ecc90a4..d8537dc49 100644 --- a/TablePro/Views/Results/Extensions/DataGridView+Selection.swift +++ b/TablePro/Views/Results/Extensions/DataGridView+Selection.swift @@ -84,6 +84,6 @@ extension TableViewCoordinator { return (existingFocusedRow, column) } - return (tableView.selectedRow, column) + return (current.min() ?? -1, column) } } diff --git a/TablePro/Views/Results/KeyHandlingTableView.swift b/TablePro/Views/Results/KeyHandlingTableView.swift index 42323a93e..d4bffcb87 100644 --- a/TablePro/Views/Results/KeyHandlingTableView.swift +++ b/TablePro/Views/Results/KeyHandlingTableView.swift @@ -148,6 +148,7 @@ final class KeyHandlingTableView: NSTableView { } let row = selectedRow + let modifiers = event.modifierFlags.intersection(.deviceIndependentFlagsMask) switch key { case .leftArrow: @@ -162,6 +163,12 @@ final class KeyHandlingTableView: NSTableView { super.keyDown(with: event) return + case .delete, .forwardDelete: + if modifiers.isEmpty || modifiers == .command { + deleteSelectedRowsIfPossible() + return + } + default: break } @@ -195,33 +202,67 @@ final class KeyHandlingTableView: NSTableView { editColumn(focusedColumn, row: row, with: nil, select: true) } - @objc override func deleteBackward(_ sender: Any?) { + @objc override func cancelOperation(_ sender: Any?) { + } + + private func deleteSelectedRowsIfPossible() { guard coordinator?.isEditable == true else { return } guard !selectedRowIndexes.isEmpty else { return } - delete(sender) + delete(nil) } - @objc override func cancelOperation(_ sender: Any?) { + private func handleLeftArrow(currentRow: Int) { + let target = focusedColumn < 0 + ? lastVisibleDataColumn() + : previousVisibleDataColumn(before: focusedColumn) + guard target >= 1 else { return } + focusedColumn = target + if currentRow >= 0 { scrollColumnToVisible(target) } } - private func handleLeftArrow(currentRow: Int) { - if focusedColumn > 1 { - focusedColumn -= 1 - if currentRow >= 0 { scrollColumnToVisible(focusedColumn) } - } else if focusedColumn == -1 && numberOfColumns > 1 { - focusedColumn = numberOfColumns - 1 - if currentRow >= 0 { scrollColumnToVisible(focusedColumn) } + private func handleRightArrow(currentRow: Int) { + let target = focusedColumn < 1 + ? firstVisibleDataColumn() + : nextVisibleDataColumn(after: focusedColumn) + guard target >= 1 else { return } + focusedColumn = target + if currentRow >= 0 { scrollColumnToVisible(target) } + } + + private func firstVisibleDataColumn() -> Int { + for index in 1..= 1 && focusedColumn < numberOfColumns - 1 { - focusedColumn += 1 - if currentRow >= 0 { scrollColumnToVisible(focusedColumn) } - } else if focusedColumn == -1 && numberOfColumns > 1 { - focusedColumn = 1 - if currentRow >= 0 { scrollColumnToVisible(focusedColumn) } + private func lastVisibleDataColumn() -> Int { + for index in stride(from: numberOfColumns - 1, through: 1, by: -1) where isVisibleDataColumn(at: index) { + return index + } + return -1 + } + + private func nextVisibleDataColumn(after current: Int) -> Int { + guard current + 1 < numberOfColumns else { return -1 } + for index in (current + 1).. Int { + guard current > 1 else { return -1 } + for index in stride(from: current - 1, through: 1, by: -1) where isVisibleDataColumn(at: index) { + return index + } + return -1 + } + + private func isVisibleDataColumn(at index: Int) -> Bool { + guard index >= 0, index < numberOfColumns else { return false } + let column = tableColumns[index] + return !column.isHidden && column.identifier != ColumnIdentitySchema.rowNumberIdentifier } private func handleTabKey() { diff --git a/TablePro/Views/Results/SortableHeaderCell.swift b/TablePro/Views/Results/SortableHeaderCell.swift index 33d6ee8e2..33b3336ab 100644 --- a/TablePro/Views/Results/SortableHeaderCell.swift +++ b/TablePro/Views/Results/SortableHeaderCell.swift @@ -16,10 +16,21 @@ final class SortableHeaderCell: NSTableHeaderCell { override init(textCell string: String) { super.init(textCell: string) + lineBreakMode = .byTruncatingTail + truncatesLastVisibleLine = true + wraps = false } required init(coder: NSCoder) { super.init(coder: coder) + lineBreakMode = .byTruncatingTail + truncatesLastVisibleLine = true + wraps = false + } + + override var state: NSControl.StateValue { + get { .off } + set { _ = newValue } } override func drawInterior(withFrame cellFrame: NSRect, in controlView: NSView) { From 7a5615fdc0c866be32eb85177eaa8510d7bdd50d Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Thu, 30 Apr 2026 19:30:50 +0700 Subject: [PATCH 10/11] fix(datagrid): bold all sorted column headers to match native macOS pattern --- TablePro/Views/Results/SortableHeaderCell.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/TablePro/Views/Results/SortableHeaderCell.swift b/TablePro/Views/Results/SortableHeaderCell.swift index 33b3336ab..994dd46ac 100644 --- a/TablePro/Views/Results/SortableHeaderCell.swift +++ b/TablePro/Views/Results/SortableHeaderCell.swift @@ -29,7 +29,7 @@ final class SortableHeaderCell: NSTableHeaderCell { } override var state: NSControl.StateValue { - get { .off } + get { sortDirection != nil ? .on : .off } set { _ = newValue } } From 5b0fca3eee16e0b5e3ad52e52270e0b9710308a6 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Thu, 30 Apr 2026 19:34:13 +0700 Subject: [PATCH 11/11] fix(datagrid): render bold sorted header manually to avoid state-on decoration --- .../Views/Results/SortableHeaderCell.swift | 33 +++++++++++++++---- 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/TablePro/Views/Results/SortableHeaderCell.swift b/TablePro/Views/Results/SortableHeaderCell.swift index 994dd46ac..41d20bf39 100644 --- a/TablePro/Views/Results/SortableHeaderCell.swift +++ b/TablePro/Views/Results/SortableHeaderCell.swift @@ -13,6 +13,7 @@ final class SortableHeaderCell: NSTableHeaderCell { private static let indicatorPadding: CGFloat = 4 private static let indicatorSpacing: CGFloat = 2 private static let priorityFontSize: CGFloat = 9 + private static let titleHorizontalPadding: CGFloat = 4 override init(textCell string: String) { super.init(textCell: string) @@ -28,11 +29,6 @@ final class SortableHeaderCell: NSTableHeaderCell { wraps = false } - override var state: NSControl.StateValue { - get { sortDirection != nil ? .on : .off } - set { _ = newValue } - } - override func drawInterior(withFrame cellFrame: NSRect, in controlView: NSView) { guard let direction = sortDirection else { super.drawInterior(withFrame: cellFrame, in: controlView) @@ -53,7 +49,7 @@ final class SortableHeaderCell: NSTableHeaderCell { width: max(0, cellFrame.width - reservedWidth), height: cellFrame.height ) - super.drawInterior(withFrame: titleFrame, in: controlView) + drawSortedTitle(in: titleFrame) let indicatorOriginX = cellFrame.maxX - Self.indicatorPadding - indicatorSize.width let indicatorOriginY = cellFrame.midY - indicatorSize.height / 2 @@ -77,6 +73,31 @@ final class SortableHeaderCell: NSTableHeaderCell { } } + private func drawSortedTitle(in rect: NSRect) { + let baseFont = font ?? NSFont.systemFont(ofSize: NSFont.smallSystemFontSize) + let boldFont = NSFontManager.shared.convert(baseFont, toHaveTrait: .boldFontMask) + + let paragraph = NSMutableParagraphStyle() + paragraph.alignment = alignment + paragraph.lineBreakMode = .byTruncatingTail + + let attributes: [NSAttributedString.Key: Any] = [ + .font: boldFont, + .foregroundColor: NSColor.headerTextColor, + .paragraphStyle: paragraph + ] + + let title = NSAttributedString(string: stringValue, attributes: attributes) + let textHeight = title.size().height + let drawRect = NSRect( + x: rect.minX + Self.titleHorizontalPadding, + y: rect.midY - textHeight / 2, + width: max(0, rect.width - Self.titleHorizontalPadding), + height: textHeight + ) + title.draw(in: drawRect) + } + override func drawSortIndicator( withFrame cellFrame: NSRect, in controlView: NSView,