From d6eacafee1db0798377721ed4510de9af8088d34 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Sat, 22 Aug 2026 20:18:17 +0700 Subject: [PATCH 1/3] fix(datagrid): keep the column window with the viewport on a wide result (#2381) --- CHANGELOG.md | 8 + .../MainContentCoordinator+FKNavigation.swift | 2 +- .../Views/Results/ColumnWindowResolver.swift | 20 ++ .../Views/Results/DataGridColumnPool.swift | 92 +++++- .../Views/Results/DataGridCoordinator.swift | 52 +++- TablePro/Views/Results/DataGridRowView.swift | 2 +- TablePro/Views/Results/DataGridView.swift | 6 - .../Extensions/DataGridView+Editing.swift | 53 ++-- .../Extensions/DataGridView+Selection.swift | 4 +- .../Extensions/DataGridView+Sort.swift | 5 + .../Views/Results/KeyHandlingTableView.swift | 74 ++--- .../Results/ColumnWindowResolverTests.swift | 35 +++ .../Results/DataGridColumnPoolTests.swift | 278 +++++++++++++++++- .../Results/DataGridRowViewCopyTests.swift | 26 +- .../FocusedColumnResolutionTests.swift | 89 +++++- 15 files changed, 643 insertions(+), 103 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 08b8665e6..d690c45eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Flickering columns, blank columns, and an unpainted gap while scrolling a result with about 100 columns sideways. (#2381) +- Find, arrow keys, and the inline editor unable to reach a column scrolled off the side of a wide result. +- Return opening no editor on a row selected with the arrow keys. +- Tab out of a row's last cell and Shift+Tab out of its first doing nothing. +- Size All Columns to Fit leaving the far columns of a wide result unreachable. + ## [0.67.1] - 2026-08-22 ### Added diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+FKNavigation.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+FKNavigation.swift index 600d6b941..f3ca8df6d 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+FKNavigation.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+FKNavigation.swift @@ -117,7 +117,7 @@ extension MainContentCoordinator { guard let tableView = NSApp.keyWindow?.firstResponder as? KeyHandlingTableView, let coordinator = tableView.coordinator, tableView.selectedRow >= 0, - DataGridView.isDataTableColumn(tableView.focusedColumn), + tableView.presentsDataColumn(at: tableView.focusedColumn), let columnIndex = DataGridView.dataColumnIndex( for: tableView.focusedColumn, in: tableView, diff --git a/TablePro/Views/Results/ColumnWindowResolver.swift b/TablePro/Views/Results/ColumnWindowResolver.swift index 5cc3f58e8..29494f041 100644 --- a/TablePro/Views/Results/ColumnWindowResolver.swift +++ b/TablePro/Views/Results/ColumnWindowResolver.swift @@ -68,6 +68,26 @@ internal enum ColumnWindowResolver { return window(for: desired, columnWidths: columnWidths) } + /// A window over `index`, for a caller that needs a column's frame before the viewport has + /// reached it. + /// + /// Re-centres rather than stretching the mounted range out to reach the target. Spanning from + /// the current range to a far column mounts every column in between, which is the whole cost + /// the window exists to avoid: measured at 848ms and 3,081 cell views for one Find match 90 + /// columns away, and 4.8s at 500 columns. The caller scrolls in the same turn, so the columns + /// this drops were never drawn again anyway. + /// + /// - Returns: `nil` when the range already covers `index` and there is nothing to mount. + internal static func window( + containing index: Int, + columnWidths: [CGFloat], + current: Range? + ) -> Window? { + guard columnWidths.indices.contains(index) else { return nil } + if let current, current.contains(index) { return nil } + return window(for: padded(index..<(index + 1), count: columnWidths.count), columnWidths: columnWidths) + } + /// The columns the viewport actually intersects. Always at least one column, so a viewport /// narrower than a single column still mounts the one under it. private static func visibleRange( diff --git a/TablePro/Views/Results/DataGridColumnPool.swift b/TablePro/Views/Results/DataGridColumnPool.swift index e506f56a6..8b517b9c9 100644 --- a/TablePro/Views/Results/DataGridColumnPool.swift +++ b/TablePro/Views/Results/DataGridColumnPool.swift @@ -154,9 +154,7 @@ final class DataGridColumnPool { /// Hiding the far ones is measurably close to free; the spacers keep the document width and /// therefore the scroll extent identical to mounting everything. func applyColumnWindow(in tableView: NSTableView) { - let candidates = tableView.tableColumns.filter { - activeIdentifiers.contains($0.identifier) && !userHiddenIdentifiers.contains($0.identifier) - } + let candidates = presentedColumns(in: tableView) guard !candidates.isEmpty else { hideSpacers() return @@ -168,36 +166,106 @@ final class DataGridColumnPool { return } - // The document starts at the row-number column, but the resolver measures from the first - // data column, so the viewport has to be rebased before it can index into these widths. - let leadingChrome = tableView.tableColumns - .prefix { $0.identifier != candidates[0].identifier } - .filter { !$0.isHidden } - .reduce(0) { $0 + $1.width + tableView.intercellSpacing.width } - // A mounted column contributes its width plus one intercell gap; a hidden one contributes // nothing. A spacer standing in for N columns has to carry their gaps too, or the document // ends up short and the last columns cannot be reached. let spacing = tableView.intercellSpacing.width let window = ColumnWindowResolver.resolve( columnWidths: candidates.map { $0.width + spacing }, - viewportMinX: viewport.minX - leadingChrome, + viewportMinX: viewport.minX - leadingChromeWidth(in: tableView, before: candidates[0]), viewportWidth: viewport.width, current: windowedRange ) guard window.range != windowedRange else { return } - windowedRange = window.range + mount(window, over: candidates, in: tableView) + } + + /// Mounts a column the window left out, so anything that reads its frame gets a real rect. + /// + /// `rect(ofColumn:)` and `frameOfCell(atColumn:row:)` are both empty for a hidden column, so + /// `scrollColumnToVisible` scrolls to the document origin instead of the column, and the inline + /// editor's own empty-frame guard makes it open nothing at all (#2381). + /// - Returns: whether the window had to widen, so the caller can drop it and let the next + /// resolve pick a tight one instead of leaving the widened range mounted. + @discardableResult + func mountColumn(_ column: NSTableColumn, in tableView: NSTableView) -> Bool { + guard presentsColumn(column) else { return false } + let candidates = presentedColumns(in: tableView) + guard let position = candidates.firstIndex(of: column) else { return false } + + let spacing = tableView.intercellSpacing.width + guard let window = ColumnWindowResolver.window( + containing: position, + columnWidths: candidates.map { $0.width + spacing }, + current: windowedRange + ) else { return false } + mount(window, over: candidates, in: tableView) + return true + } + + /// The first and last columns the result presents, in display order. The pool owns these + /// because the spacers are attached columns too and one of them sits immediately before the + /// first data column, so no fixed position can name either end. + func firstPresentedColumnIndex(in tableView: NSTableView) -> Int? { + tableView.tableColumns.firstIndex { presentsColumn($0) } + } + + func lastPresentedColumnIndex(in tableView: NSTableView) -> Int? { + tableView.tableColumns.lastIndex { presentsColumn($0) } + } + func nextPresentedColumnIndex(after index: Int, in tableView: NSTableView) -> Int? { + let start = max(0, index + 1) + guard start < tableView.tableColumns.count else { return nil } + return tableView.tableColumns[start...].firstIndex { presentsColumn($0) } + } + + func previousPresentedColumnIndex(before index: Int, in tableView: NSTableView) -> Int? { + let end = min(max(0, index), tableView.tableColumns.count) + guard end > 0 else { return nil } + return tableView.tableColumns[.. Bool { + guard index >= 0, index < tableView.tableColumns.count else { return false } + return presentsColumn(tableView.tableColumns[index]) + } + + private func presentedColumns(in tableView: NSTableView) -> [NSTableColumn] { + tableView.tableColumns.filter { presentsColumn($0) } + } + + private func mount( + _ window: ColumnWindowResolver.Window, + over candidates: [NSTableColumn], + in tableView: NSTableView + ) { + windowedRange = window.range for (index, column) in candidates.enumerated() { let mounted = window.range.contains(index) if column.isHidden == mounted { column.isHidden = !mounted } } + let spacing = tableView.intercellSpacing.width applySpacer(leadingSpacer, width: spacerWidth(window.leadingWidth, spacing: spacing), in: tableView) applySpacer(trailingSpacer, width: spacerWidth(window.trailingWidth, spacing: spacing), in: tableView) } + /// The document starts at the row-number column, but the resolver measures from the first data + /// column, so the viewport has to be rebased before it can index into those widths. + /// + /// Only chrome counts. The leading spacer sits ahead of the first data column and holds exactly + /// the width of the columns the window left out, which the resolver's own model already carries, + /// so counting it here subtracts that width twice and walks the window left while the reader + /// scrolls right, until it parks off screen and the grid paints nothing (#2381). + private func leadingChromeWidth(in tableView: NSTableView, before firstColumn: NSTableColumn) -> CGFloat { + tableView.tableColumns + .prefix { $0.identifier != firstColumn.identifier } + .filter { !$0.isHidden && !ColumnIdentitySchema.isSpacer($0.identifier) } + .reduce(0) { $0 + $1.width + tableView.intercellSpacing.width } + } + /// The resolver works in per-column slots that already include one gap each. A spacer is a /// single column, so it keeps one gap of its own and absorbs the rest as width. private func spacerWidth(_ slotWidth: CGFloat, spacing: CGFloat) -> CGFloat { diff --git a/TablePro/Views/Results/DataGridCoordinator.swift b/TablePro/Views/Results/DataGridCoordinator.swift index 740a09453..32f83be5c 100644 --- a/TablePro/Views/Results/DataGridCoordinator.swift +++ b/TablePro/Views/Results/DataGridCoordinator.swift @@ -132,6 +132,52 @@ final class TableViewCoordinator: NSObject, NSTableViewDelegate, NSTableViewData columnPool.presentsColumn(column) } + /// Whether this position in `tableColumns` holds one of the columns the result presents. + /// + /// The row-number column and the window's two spacers are attached columns as well, and one + /// spacer sits immediately before the first data column, so no fixed position answers this. + func presentsColumn(atTableColumnIndex index: Int) -> Bool { + guard let tableView else { return false } + return columnPool.presentsColumn(atTableColumnIndex: index, in: tableView) + } + + func firstPresentedColumnIndex() -> Int? { + guard let tableView else { return nil } + return columnPool.firstPresentedColumnIndex(in: tableView) + } + + func lastPresentedColumnIndex() -> Int? { + guard let tableView else { return nil } + return columnPool.lastPresentedColumnIndex(in: tableView) + } + + func nextPresentedColumnIndex(after index: Int) -> Int? { + guard let tableView else { return nil } + return columnPool.nextPresentedColumnIndex(after: index, in: tableView) + } + + func previousPresentedColumnIndex(before index: Int) -> Int? { + guard let tableView else { return nil } + return columnPool.previousPresentedColumnIndex(before: index, in: tableView) + } + + /// The single way to reach a column, for Find, cell navigation and the inline editor alike. + /// + /// A column the window left out has no frame at all, so `scrollColumnToVisible` scrolls to the + /// document origin instead of the column and the editor's own empty-frame guard opens nothing. + /// Mounting first gives it one. Only a mount that had to widen the window drops it afterwards, + /// so stepping column by column keeps the resolver's hysteresis instead of re-windowing on + /// every keystroke. + func scrollColumnToVisible(tableColumnIndex index: Int) { + guard let tableView, index >= 0, index < tableView.numberOfColumns else { return } + let widened = columnPool.mountColumn(tableView.tableColumns[index], in: tableView) + tableView.scrollColumnToVisible(index) + if widened { + columnPool.invalidateColumnWindow() + } + updateColumnWindow() + } + /// The columns the user is looking at, which is every presented column and not merely the /// mounted ones. Copy, find and size-all all read this, so narrowing it to the window would /// silently drop the columns off screen from a copied row or a search. @@ -1080,9 +1126,8 @@ final class TableViewCoordinator: NSObject, NSTableViewDelegate, NSTableViewData guard let match, match.displayRow >= 0, match.displayRow < tableView.numberOfRows else { return } tableView.scrollRowToVisible(match.displayRow) - if let displayColumn = tableColumnIndex(for: match.columnIndex), - displayColumn < tableView.numberOfColumns { - tableView.scrollColumnToVisible(displayColumn) + if let displayColumn = tableColumnIndex(for: match.columnIndex) { + scrollColumnToVisible(tableColumnIndex: displayColumn) } tableView.selectRowIndexes(IndexSet(integer: match.displayRow), byExtendingSelection: false) } @@ -1094,6 +1139,7 @@ final class TableViewCoordinator: NSObject, NSTableViewDelegate, NSTableViewData guard displayRow >= 0, displayRow < tableView.numberOfRows else { return } tableView.scrollRowToVisible(displayRow) tableView.selectRowIndexes(IndexSet(integer: displayRow), byExtendingSelection: false) + scrollColumnToVisible(tableColumnIndex: displayCol) beginCellEdit(row: displayRow, tableColumnIndex: displayCol) } diff --git a/TablePro/Views/Results/DataGridRowView.swift b/TablePro/Views/Results/DataGridRowView.swift index e331360b0..3e2ce0c00 100644 --- a/TablePro/Views/Results/DataGridRowView.swift +++ b/TablePro/Views/Results/DataGridRowView.swift @@ -462,7 +462,7 @@ class DataGridRowView: NSTableRowView { private func focusedDataColumnIndex(in coordinator: TableViewCoordinator) -> Int? { guard let tableView = coordinator.tableView as? KeyHandlingTableView, tableView.focusedRow == rowIndex, - DataGridView.isDataTableColumn(tableView.focusedColumn) else { return nil } + tableView.presentsDataColumn(at: tableView.focusedColumn) else { return nil } return DataGridView.dataColumnIndex( for: tableView.focusedColumn, in: tableView, diff --git a/TablePro/Views/Results/DataGridView.swift b/TablePro/Views/Results/DataGridView.swift index b5a12a3fb..5d5e4528c 100644 --- a/TablePro/Views/Results/DataGridView.swift +++ b/TablePro/Views/Results/DataGridView.swift @@ -427,12 +427,6 @@ struct DataGridView: NSViewRepresentable { tableView.selectionOverlay = overlay } - static let firstDataTableColumnIndex: Int = 1 - - static func isDataTableColumn(_ tableColumnIndex: Int) -> Bool { - tableColumnIndex >= firstDataTableColumnIndex - } - static func dataColumnIndex( for tableColumnIndex: Int, in tableView: NSTableView, diff --git a/TablePro/Views/Results/Extensions/DataGridView+Editing.swift b/TablePro/Views/Results/Extensions/DataGridView+Editing.swift index d69bd27ae..2e40cca0a 100644 --- a/TablePro/Views/Results/Extensions/DataGridView+Editing.swift +++ b/TablePro/Views/Results/Extensions/DataGridView+Editing.swift @@ -117,32 +117,14 @@ extension TableViewCoordinator { } func handleOverlayTabNavigation(row: Int, column: Int, forward: Bool) { - guard let tableView = tableView else { return } - - var nextColumn = forward ? column + 1 : column - 1 - var nextRow = row - - if forward { - if nextColumn >= tableView.numberOfColumns { - nextColumn = DataGridView.firstDataTableColumnIndex - nextRow += 1 - } - if nextRow >= tableView.numberOfRows { - nextRow = tableView.numberOfRows - 1 - nextColumn = tableView.numberOfColumns - 1 - } - } else { - if !DataGridView.isDataTableColumn(nextColumn) { - nextColumn = tableView.numberOfColumns - 1 - nextRow -= 1 - } - if nextRow < 0 { - nextRow = 0 - nextColumn = DataGridView.firstDataTableColumnIndex - } - } + guard let tableView = tableView, + let target = tabNavigationTarget(from: (row, column), forward: forward, in: tableView) + else { return } + let nextRow = target.row + let nextColumn = target.column tableView.selectRowIndexes(IndexSet(integer: nextRow), byExtendingSelection: false) + scrollColumnToVisible(tableColumnIndex: nextColumn) guard let nextColumnIndex = DataGridView.dataColumnIndex( for: nextColumn, @@ -161,4 +143,27 @@ extension TableViewCoordinator { value: value ) } + + /// Tab walks the presented columns and wraps onto the next row's first, Shift+Tab onto the + /// previous row's last. Both ends are resolved rather than assumed: the window's spacers and + /// the pool's surplus slots are attached columns too, so neither end of `tableColumns` holds a + /// data column and a fixed position lands on a spacer that swallows the keystroke. + private func tabNavigationTarget( + from cell: (row: Int, column: Int), + forward: Bool, + in tableView: NSTableView + ) -> (row: Int, column: Int)? { + if forward { + if let next = nextPresentedColumnIndex(after: cell.column) { + return (cell.row, next) + } + guard cell.row + 1 < tableView.numberOfRows, let first = firstPresentedColumnIndex() else { return nil } + return (cell.row + 1, first) + } + if let previous = previousPresentedColumnIndex(before: cell.column) { + return (cell.row, previous) + } + guard cell.row > 0, let last = lastPresentedColumnIndex() else { return nil } + return (cell.row - 1, last) + } } diff --git a/TablePro/Views/Results/Extensions/DataGridView+Selection.swift b/TablePro/Views/Results/Extensions/DataGridView+Selection.swift index d771788c1..e8bab696c 100644 --- a/TablePro/Views/Results/Extensions/DataGridView+Selection.swift +++ b/TablePro/Views/Results/Extensions/DataGridView+Selection.swift @@ -102,7 +102,9 @@ extension TableViewCoordinator { return (-1, -1) } - let column = existingFocusedColumn >= 1 ? existingFocusedColumn : 1 + let column = presentsColumn(atTableColumnIndex: existingFocusedColumn) + ? existingFocusedColumn + : (firstPresentedColumnIndex() ?? -1) let added = current.subtracting(previous) if let tip = added.max() { diff --git a/TablePro/Views/Results/Extensions/DataGridView+Sort.swift b/TablePro/Views/Results/Extensions/DataGridView+Sort.swift index d886e5fc2..ca81bbe46 100644 --- a/TablePro/Views/Results/Extensions/DataGridView+Sort.swift +++ b/TablePro/Views/Results/Extensions/DataGridView+Sort.swift @@ -371,6 +371,11 @@ extension TableViewCoordinator { fittedColumnCount: fittedColumns.count ) } + // Sizing reaches the columns the window unmounted as well, and the spacers stand in for + // those at the width they had before, so the document is short by the whole delta until + // the window is resolved again. + columnPool.invalidateColumnWindow() + updateColumnWindow() scheduleLayoutPersist() } diff --git a/TablePro/Views/Results/KeyHandlingTableView.swift b/TablePro/Views/Results/KeyHandlingTableView.swift index 37aebb9bf..81cae8026 100644 --- a/TablePro/Views/Results/KeyHandlingTableView.swift +++ b/TablePro/Views/Results/KeyHandlingTableView.swift @@ -214,7 +214,7 @@ final class KeyHandlingTableView: NSTableView { } private func clampDataColumn(_ value: Int, schema: ColumnIdentitySchema) -> Int { - let firstData = DataGridView.firstDataTableColumnIndex + let firstData = firstVisibleDataColumn() let candidate = value < firstData ? firstData : value guard candidate >= 0, candidate < numberOfColumns else { return -1 } return DataGridView.dataColumnIndex(for: candidate, in: self, schema: schema) ?? -1 @@ -258,7 +258,7 @@ final class KeyHandlingTableView: NSTableView { private func focusedDataCell() -> (row: Int, columnIndex: Int)? { guard selectedRowIndexes.count == 1, focusedRow >= 0, - DataGridView.isDataTableColumn(focusedColumn), + presentsDataColumn(at: focusedColumn), let schema = coordinator?.identitySchema, let dataColumn = DataGridView.dataColumnIndex(for: focusedColumn, in: self, schema: schema) else { return nil @@ -279,7 +279,7 @@ final class KeyHandlingTableView: NSTableView { /// requires a single selected row: a paste anchors on the focused cell alone. private func pasteAnchorCell() -> (row: Int, column: Int)? { guard focusedRow >= 0, - DataGridView.isDataTableColumn(focusedColumn), + presentsDataColumn(at: focusedColumn), let schema = coordinator?.identitySchema, let dataCol = DataGridView.dataColumnIndex(for: focusedColumn, in: self, schema: schema) else { return nil @@ -312,7 +312,7 @@ final class KeyHandlingTableView: NSTableView { case #selector(paste(_:)): return canPaste case #selector(insertNewline(_:)): - return selectedRow >= 0 && DataGridView.isDataTableColumn(focusedColumn) + return selectedRow >= 0 && presentsDataColumn(at: focusedColumn) case #selector(selectAll(_:)): return numberOfRows > 0 default: @@ -358,7 +358,7 @@ final class KeyHandlingTableView: NSTableView { !fkCombo.isCleared, fkCombo.matches(event), selectedRow >= 0, - DataGridView.isDataTableColumn(focusedColumn), + presentsDataColumn(at: focusedColumn), let schema = coordinator?.identitySchema, let columnIndex = DataGridView.dataColumnIndex(for: focusedColumn, in: self, schema: schema) { coordinator?.toggleForeignKeyPreview( @@ -418,12 +418,16 @@ final class KeyHandlingTableView: NSTableView { @objc override func insertNewline(_ sender: Any?) { let row = selectedRow guard row >= 0, - DataGridView.isDataTableColumn(focusedColumn), + presentsDataColumn(at: focusedColumn), let schema = coordinator?.identitySchema, let columnIndex = DataGridView.dataColumnIndex(for: focusedColumn, in: self, schema: schema), let coordinator else { return } + // The cell cursor can sit on a column the window left out, and a cell with no view behind + // it opens nothing at all. Reaching it first also puts it on screen, where the editor the + // keystroke is about to open belongs. + coordinator.scrollColumnToVisible(tableColumnIndex: focusedColumn) coordinator.handleCellInteraction(row: row, tableColumn: focusedColumn, columnIndex: columnIndex, tableView: self) } @@ -445,64 +449,46 @@ final class KeyHandlingTableView: NSTableView { let target = focusedColumn < 0 ? lastVisibleDataColumn() : previousVisibleDataColumn(before: focusedColumn) - guard DataGridView.isDataTableColumn(target) else { return } + guard presentsDataColumn(at: target) else { return } focusedColumn = target coordinator?.dismissFKPreviewOnColumnChange() - if currentRow >= 0 { scrollColumnToVisible(target) } + if currentRow >= 0 { coordinator?.scrollColumnToVisible(tableColumnIndex: target) } } private func handleRightArrow(currentRow: Int) { - let target = DataGridView.isDataTableColumn(focusedColumn) + let target = presentsDataColumn(at: focusedColumn) ? nextVisibleDataColumn(after: focusedColumn) : firstVisibleDataColumn() - guard DataGridView.isDataTableColumn(target) else { return } + guard presentsDataColumn(at: target) else { return } focusedColumn = target coordinator?.dismissFKPreviewOnColumnChange() - if currentRow >= 0 { scrollColumnToVisible(target) } + if currentRow >= 0 { coordinator?.scrollColumnToVisible(tableColumnIndex: target) } } private func firstVisibleDataColumn() -> Int { - for index in DataGridView.firstDataTableColumnIndex.. Int { - for index in stride( - from: numberOfColumns - 1, - through: DataGridView.firstDataTableColumnIndex, - by: -1 - ) where isVisibleDataColumn(at: index) { - return index - } - return -1 + coordinator?.lastPresentedColumnIndex() ?? -1 } private func nextVisibleDataColumn(after current: Int) -> Int { - guard current + 1 < numberOfColumns else { return -1 } - for index in (current + 1).. Int { - guard current > DataGridView.firstDataTableColumnIndex else { return -1 } - for index in stride( - from: current - 1, - through: DataGridView.firstDataTableColumnIndex, - by: -1 - ) where isVisibleDataColumn(at: index) { - return index - } - return -1 + coordinator?.previousPresentedColumnIndex(before: current) ?? -1 } - private func isVisibleDataColumn(at index: Int) -> Bool { + /// Whether this position in `tableColumns` holds one of the columns the result presents. + /// + /// The row-number column and the window's two spacers are attached columns as well, and one + /// spacer sits immediately before the first data column, so no fixed position answers this. + func presentsDataColumn(at index: Int) -> Bool { guard index >= 0, index < numberOfColumns else { return false } - let column = tableColumns[index] - return coordinator?.presentsColumn(column) ?? !column.isHidden + guard let coordinator else { return !tableColumns[index].isHidden } + return coordinator.presentsColumn(atTableColumnIndex: index) } /// `NSResponder` declares these two but does not implement them, so calling `super` raises @@ -516,7 +502,7 @@ final class KeyHandlingTableView: NSTableView { /// element itself stays the table: `NSTableView`'s own focused-element resolution already /// walks to the cell, and overriding it in Swift is not available on this type. internal func postCellCursorMoved() { - guard selectedRow >= 0, DataGridView.isDataTableColumn(focusedColumn) else { return } + guard selectedRow >= 0, presentsDataColumn(at: focusedColumn) else { return } guard let cell = view(atColumn: focusedColumn, row: selectedRow, makeIfNecessary: false) else { return } NSAccessibility.post(element: cell, notification: .focusedUIElementChanged) } @@ -551,7 +537,7 @@ final class KeyHandlingTableView: NSTableView { private func moveFocusToNextCell() -> Bool { let row = selectedRow - guard row >= 0, DataGridView.isDataTableColumn(focusedColumn) else { return false } + guard row >= 0, presentsDataColumn(at: focusedColumn) else { return false } var nextColumn = nextVisibleDataColumn(after: focusedColumn) var nextRow = row @@ -567,7 +553,7 @@ final class KeyHandlingTableView: NSTableView { private func moveFocusToPreviousCell() -> Bool { let row = selectedRow - guard row >= 0, DataGridView.isDataTableColumn(focusedColumn) else { return false } + guard row >= 0, presentsDataColumn(at: focusedColumn) else { return false } var previousColumn = previousVisibleDataColumn(before: focusedColumn) var previousRow = row @@ -586,7 +572,7 @@ final class KeyHandlingTableView: NSTableView { focusedRow = row focusedColumn = column scrollRowToVisible(row) - scrollColumnToVisible(column) + coordinator?.scrollColumnToVisible(tableColumnIndex: column) postCellCursorMoved() } diff --git a/TableProTests/Views/Results/ColumnWindowResolverTests.swift b/TableProTests/Views/Results/ColumnWindowResolverTests.swift index 5e8fdc1d1..792f09121 100644 --- a/TableProTests/Views/Results/ColumnWindowResolverTests.swift +++ b/TableProTests/Views/Results/ColumnWindowResolverTests.swift @@ -20,6 +20,41 @@ struct ColumnWindowResolverTests { return window.leadingWidth + mounted + window.trailingWidth } + @Test("A range that already covers the column needs no widening") + func containingIsNilWhenAlreadyMounted() { + #expect(ColumnWindowResolver.window(containing: 20, columnWidths: wide, current: 10..<40) == nil) + } + + @Test("An out-of-bounds column widens nothing") + func containingIsNilOutOfBounds() { + #expect(ColumnWindowResolver.window(containing: 500, columnWidths: wide, current: nil) == nil) + } + + /// Stretching the mounted range out to reach a far column would mount every column in between, + /// which is the cost the window exists to avoid. The caller scrolls in the same turn, so the + /// columns it leaves behind are never drawn again. + @Test("Reaching a far column mounts a window around it, not everything in between") + func containingRecentresRatherThanStretches() throws { + let window = try #require( + ColumnWindowResolver.window(containing: 400, columnWidths: wide, current: 0..<30) + ) + + #expect(window.range.contains(400)) + #expect(window.range.count <= ColumnWindowResolver.overscan * 2 + 1) + #expect(totalWidth(window, widths: wide) == wide.reduce(0, +)) + } + + @Test("Reaching a column with no window yet mounts a window around it") + func containingFromNoWindow() throws { + let window = try #require( + ColumnWindowResolver.window(containing: 250, columnWidths: wide, current: nil) + ) + + #expect(window.range.contains(250)) + #expect(window.range.count < 50) + #expect(totalWidth(window, widths: wide) == wide.reduce(0, +)) + } + @Test("No columns resolves to an empty window") func emptyColumns() { let window = ColumnWindowResolver.resolve( diff --git a/TableProTests/Views/Results/DataGridColumnPoolTests.swift b/TableProTests/Views/Results/DataGridColumnPoolTests.swift index b6918b216..3e1a9ab8c 100644 --- a/TableProTests/Views/Results/DataGridColumnPoolTests.swift +++ b/TableProTests/Views/Results/DataGridColumnPoolTests.swift @@ -15,8 +15,12 @@ struct DataGridColumnPoolTests { private func makeTableView() -> NSTableView { let tableView = NSTableView() // Mirrors DataGridView. The default style redistributes column widths on resize, which - // would silently rewrite the widths these tests assert on. + // would silently rewrite the widths these tests assert on, and the default style and + // intercell spacing put the columns at different document positions than the grid's, which + // is the geometry the window is resolved against. tableView.columnAutoresizingStyle = .noColumnAutoresizing + tableView.style = .plain + tableView.intercellSpacing = NSSize(width: 1, height: 0) let rowNumberColumn = NSTableColumn(identifier: ColumnIdentitySchema.rowNumberIdentifier) rowNumberColumn.width = 40 tableView.addTableColumn(rowNumberColumn) @@ -809,4 +813,276 @@ struct DataGridColumnPoolTests { let hasSpacer = tableView.tableColumns.contains(where: { ColumnIdentitySchema.isSpacer($0.identifier) }) #expect(!hasSpacer) } + + // MARK: - Window geometry while scrolling + + private func scroll(_ scrollView: NSScrollView, to offsetX: CGFloat, tableView: NSTableView) { + scrollView.contentView.scroll(to: NSPoint(x: offsetX, y: scrollView.contentView.bounds.origin.y)) + scrollView.reflectScrolledClipView(scrollView.contentView) + tableView.layoutSubtreeIfNeeded() + } + + private func documentWidth(of tableView: NSTableView) -> CGFloat { + tableView.layoutSubtreeIfNeeded() + return tableView.frame.width + } + + /// How much of the viewport, past the row-number column, no mounted data column paints into. + /// + /// Measured through `rect(ofColumn:)`, which is `NSTableView`'s own answer for where a column + /// sits and is `NSZeroRect` for an unmounted one. Asking the resolver instead would only prove + /// the resolver agrees with itself, which is exactly how #2381 shipped. + private func unpaintedViewportWidth(in scrollView: NSScrollView, tableView: NSTableView) -> CGFloat { + let viewport = scrollView.contentView.bounds + let rowNumber = tableView.column(withIdentifier: ColumnIdentitySchema.rowNumberIdentifier) + let contentStart = rowNumber >= 0 + ? max(viewport.minX, tableView.rect(ofColumn: rowNumber).maxX) + : viewport.minX + + var painted: CGFloat = 0 + for column in dataColumns(in: tableView) where !column.isHidden { + let index = tableView.column(withIdentifier: column.identifier) + guard index >= 0 else { continue } + let rect = tableView.rect(ofColumn: index) + painted += max(0, min(rect.maxX, viewport.maxX) - max(rect.minX, contentStart)) + } + return max(0, (viewport.maxX - contentStart) - painted) + } + + private func mountedIdentifiers(in tableView: NSTableView) -> Set { + Set(dataColumns(in: tableView).filter { !$0.isHidden }.map(\.identifier)) + } + + private func scrollOffsets(in scrollView: NSScrollView, tableView: NSTableView) -> [CGFloat] { + let maximum = documentWidth(of: tableView) - scrollView.contentView.bounds.width + guard maximum > 0 else { return [0] } + let forward = Array(stride(from: 0, through: maximum, by: 150)) + [maximum] + return forward + forward.reversed() + } + + /// The reported bug. The window is resolved against a model of the whole column run, so the + /// viewport has to be rebased into that model before it can pick a range. Counting the leading + /// spacer as chrome subtracted the columns it stands in for a second time, and the window + /// walked left while the reader scrolled right until it painted nothing at all. + @Test("The mounted columns cover the viewport at every horizontal scroll offset") + func windowCoversTheViewportWhileScrolling() { + let pool = DataGridColumnPool() + let (scrollView, tableView) = makeScrolledTableView(viewportWidth: 800) + reconcileWide(pool, tableView: tableView, count: 100) + + var worstGap: CGFloat = 0 + for offset in scrollOffsets(in: scrollView, tableView: tableView) { + scroll(scrollView, to: offset, tableView: tableView) + pool.applyColumnWindow(in: tableView) + tableView.layoutSubtreeIfNeeded() + worstGap = max(worstGap, unpaintedViewportWidth(in: scrollView, tableView: tableView)) + } + + #expect(worstGap == 0) + } + + /// A window that alternates between two ranges re-mounts columns on every scroll event, which + /// is what the reader sees as flicker. + @Test("Resolving again at the same scroll offset settles rather than alternating") + func windowSettlesAtOneOffset() { + let pool = DataGridColumnPool() + let (scrollView, tableView) = makeScrolledTableView(viewportWidth: 800) + reconcileWide(pool, tableView: tableView, count: 100) + + scroll(scrollView, to: documentWidth(of: tableView) / 2, tableView: tableView) + pool.applyColumnWindow(in: tableView) + tableView.layoutSubtreeIfNeeded() + let settled = mountedIdentifiers(in: tableView) + + for _ in 0..<5 { + pool.applyColumnWindow(in: tableView) + tableView.layoutSubtreeIfNeeded() + } + + #expect(mountedIdentifiers(in: tableView) == settled) + } + + /// The spacers exist to keep the scroll extent, so no window position may change it. + @Test("The document keeps its width at every window position") + func documentWidthSurvivesEveryWindowPosition() { + let pool = DataGridColumnPool() + let (scrollView, tableView) = makeScrolledTableView(viewportWidth: 800) + reconcileWide(pool, tableView: tableView, count: 100) + let expected = documentWidth(of: tableView) + + for offset in scrollOffsets(in: scrollView, tableView: tableView) { + scroll(scrollView, to: offset, tableView: tableView) + pool.applyColumnWindow(in: tableView) + #expect(documentWidth(of: tableView) == expected) + } + } + + @Test("Scrolled to the end, the last column is mounted") + func lastColumnIsMountedAtTheEnd() throws { + let pool = DataGridColumnPool() + let (scrollView, tableView) = makeScrolledTableView(viewportWidth: 800) + reconcileWide(pool, tableView: tableView, count: 100) + + scroll( + scrollView, + to: documentWidth(of: tableView) - scrollView.contentView.bounds.width, + tableView: tableView + ) + pool.applyColumnWindow(in: tableView) + + let last = try #require(dataColumns(in: tableView).last) + #expect(!last.isHidden) + } + + // MARK: - Reaching a column the window left out + + /// `rect(ofColumn:)` and `frameOfCell(atColumn:row:)` are both empty for an unmounted column, + /// so Find scrolled to the document origin instead of the match and the inline editor opened + /// nothing at all. + @Test("A column the window left out can be mounted on demand") + func mountColumnReachesAnUnmountedColumn() throws { + let pool = DataGridColumnPool() + let (scrollView, tableView) = makeScrolledTableView(viewportWidth: 800) + reconcileWide(pool, tableView: tableView, count: 100) + scroll(scrollView, to: 0, tableView: tableView) + pool.applyColumnWindow(in: tableView) + + let last = try #require(dataColumns(in: tableView).last) + #expect(last.isHidden) + + pool.mountColumn(last, in: tableView) + tableView.layoutSubtreeIfNeeded() + + #expect(!last.isHidden) + #expect(tableView.rect(ofColumn: tableView.column(withIdentifier: last.identifier)).width > 0) + } + + /// Stretching the window out to reach a far column mounts every column in between, which is the + /// cost the window exists to avoid: measured at 848ms and 3,081 cell views for one match 90 + /// columns away, and 4.8s at 500 columns. + @Test("Mounting a far column does not mount everything in between") + func mountColumnStaysBounded() throws { + let pool = DataGridColumnPool() + let (scrollView, tableView) = makeScrolledTableView(viewportWidth: 800) + reconcileWide(pool, tableView: tableView, count: 100) + scroll(scrollView, to: 0, tableView: tableView) + pool.applyColumnWindow(in: tableView) + let mountedBefore = mountedIdentifiers(in: tableView).count + + let last = try #require(dataColumns(in: tableView).last) + pool.mountColumn(last, in: tableView) + tableView.layoutSubtreeIfNeeded() + + #expect(!last.isHidden) + #expect(mountedIdentifiers(in: tableView).count <= mountedBefore) + } + + @Test("Mounting a far column keeps the document width") + func mountColumnKeepsTheDocumentWidth() throws { + let pool = DataGridColumnPool() + let (scrollView, tableView) = makeScrolledTableView(viewportWidth: 800) + reconcileWide(pool, tableView: tableView, count: 100) + scroll(scrollView, to: 0, tableView: tableView) + pool.applyColumnWindow(in: tableView) + let expected = documentWidth(of: tableView) + + let last = try #require(dataColumns(in: tableView).last) + pool.mountColumn(last, in: tableView) + + #expect(documentWidth(of: tableView) == expected) + } + + @Test("A column the user hid is never mounted on demand") + func mountColumnRefusesAUserHiddenColumn() throws { + let pool = DataGridColumnPool() + let (_, tableView) = makeScrolledTableView(viewportWidth: 800) + reconcileWide(pool, tableView: tableView, count: 100, hidden: ["c99"]) + + let hidden = try #require(dataColumns(in: tableView).last) + pool.mountColumn(hidden, in: tableView) + + #expect(hidden.isHidden) + } + + // MARK: - Naming the ends of the data run + + /// The window's spacers are attached columns too, and the leading one sits immediately before + /// the first data column, so a fixed position names a spacer rather than data. + @Test("The first and last presented columns are data columns, not spacers") + func presentedEndsSkipTheSpacers() throws { + let pool = DataGridColumnPool() + let (_, tableView) = makeScrolledTableView(viewportWidth: 800) + reconcileWide(pool, tableView: tableView, count: 100) + + let first = try #require(pool.firstPresentedColumnIndex(in: tableView)) + let last = try #require(pool.lastPresentedColumnIndex(in: tableView)) + + #expect(!ColumnIdentitySchema.isSpacer(tableView.tableColumns[first].identifier)) + #expect(!ColumnIdentitySchema.isSpacer(tableView.tableColumns[last].identifier)) + #expect(tableView.tableColumns[first].identifier == dataColumns(in: tableView).first?.identifier) + #expect(tableView.tableColumns[last].identifier == dataColumns(in: tableView).last?.identifier) + } + + @Test("Walking forward and back from an end stays inside the data run") + func presentedNeighboursStayInsideTheDataRun() throws { + let pool = DataGridColumnPool() + let (_, tableView) = makeScrolledTableView(viewportWidth: 800) + reconcileWide(pool, tableView: tableView, count: 20) + + let first = try #require(pool.firstPresentedColumnIndex(in: tableView)) + let last = try #require(pool.lastPresentedColumnIndex(in: tableView)) + + #expect(pool.previousPresentedColumnIndex(before: first, in: tableView) == nil) + #expect(pool.nextPresentedColumnIndex(after: last, in: tableView) == nil) + #expect(pool.nextPresentedColumnIndex(after: first, in: tableView) != nil) + #expect(pool.previousPresentedColumnIndex(before: last, in: tableView) != nil) + } + + /// The create-table grid opens with no columns at all, where every attached column is chrome. + @Test("A result with no columns presents no column at either end") + func emptyResultHasNoPresentedEnds() { + let pool = DataGridColumnPool() + let (_, tableView) = makeScrolledTableView(viewportWidth: 800) + + reconcileWide(pool, tableView: tableView, count: 0) + + #expect(pool.firstPresentedColumnIndex(in: tableView) == nil) + #expect(pool.lastPresentedColumnIndex(in: tableView) == nil) + } + + @Test("A single-column result presents that column at both ends") + func singleColumnResultHasOneEnd() { + let pool = DataGridColumnPool() + let (_, tableView) = makeScrolledTableView(viewportWidth: 800) + + reconcileWide(pool, tableView: tableView, count: 1) + + #expect(pool.firstPresentedColumnIndex(in: tableView) == pool.lastPresentedColumnIndex(in: tableView)) + #expect(pool.firstPresentedColumnIndex(in: tableView) != nil) + } + + /// Size All Columns to Fit reaches the columns the window unmounted as well, so the spacers + /// stand in at the width those columns used to have and the document ends up short. + @Test("Resizing unmounted columns restores the full document width") + func widthChangeOutsideTheWindowRestoresDocumentWidth() { + let pool = DataGridColumnPool() + let (scrollView, tableView) = makeScrolledTableView(viewportWidth: 800) + reconcileWide(pool, tableView: tableView, count: 100) + scroll(scrollView, to: 0, tableView: tableView) + pool.applyColumnWindow(in: tableView) + + for column in dataColumns(in: tableView) { + column.width = 300 + } + pool.invalidateColumnWindow() + pool.applyColumnWindow(in: tableView) + + let gap = tableView.intercellSpacing.width + let everyColumnSlot = dataColumns(in: tableView).reduce(0) { $0 + $1.width + gap } + let occupied = tableView.tableColumns + .filter { !$0.isHidden && $0.identifier != ColumnIdentitySchema.rowNumberIdentifier } + .reduce(0) { $0 + $1.width + gap } + + #expect(occupied == everyColumnSlot) + } } diff --git a/TableProTests/Views/Results/DataGridRowViewCopyTests.swift b/TableProTests/Views/Results/DataGridRowViewCopyTests.swift index 1b9b5a17b..f2eac8664 100644 --- a/TableProTests/Views/Results/DataGridRowViewCopyTests.swift +++ b/TableProTests/Views/Results/DataGridRowViewCopyTests.swift @@ -66,17 +66,33 @@ struct DataGridRowViewCopyTests { return coordinator } + /// Built through the column pool rather than by attaching columns by hand, so the positions + /// these tests focus are the ones the grid really has: the pool also attaches the window's two + /// spacers, one of them ahead of the first data column. private func makeTableView(for coordinator: TableViewCoordinator) -> KeyHandlingTableView { let tableView = KeyHandlingTableView() tableView.coordinator = coordinator tableView.addTableColumn(DataGridView.makeRowNumberColumn()) - for identifier in coordinator.identitySchema.identifiers { - tableView.addTableColumn(NSTableColumn(identifier: identifier)) - } coordinator.tableView = tableView + coordinator.columnPool.reconcile( + tableView: tableView, + schema: coordinator.identitySchema, + columnTypes: [], + savedLayout: nil, + isEditable: true, + hiddenColumnNames: [], + widthCalculator: { _, _ in 100 } + ) return tableView } + private func tableColumnIndex(of dataIndex: Int, in tableView: KeyHandlingTableView) -> Int { + guard let coordinator = tableView.coordinator, + let identifier = coordinator.identitySchema.identifier(for: dataIndex) + else { return -1 } + return tableView.column(withIdentifier: identifier) + } + private func invokeCopy( on rowView: DataGridRowView, target: DataGridRowView.CopyContextTarget = .unresolved @@ -243,7 +259,7 @@ struct DataGridRowViewCopyTests { ) let tableView = makeTableView(for: coordinator) tableView.focusedRow = 0 - tableView.focusedColumn = 2 + tableView.focusedColumn = tableColumnIndex(of: 1, in: tableView) let rowView = DataGridRowView() rowView.coordinator = coordinator @@ -266,7 +282,7 @@ struct DataGridRowViewCopyTests { ) let tableView = makeTableView(for: coordinator) tableView.focusedRow = 0 - tableView.focusedColumn = 2 + tableView.focusedColumn = tableColumnIndex(of: 1, in: tableView) let rowView = DataGridRowView() rowView.coordinator = coordinator diff --git a/TableProTests/Views/Results/FocusedColumnResolutionTests.swift b/TableProTests/Views/Results/FocusedColumnResolutionTests.swift index 2cb05165e..7946ce292 100644 --- a/TableProTests/Views/Results/FocusedColumnResolutionTests.swift +++ b/TableProTests/Views/Results/FocusedColumnResolutionTests.swift @@ -4,11 +4,19 @@ // import AppKit +import SwiftUI import TableProPluginKit import Testing @testable import TablePro +@MainActor +private final class FocusedColumnLayoutPersister: ColumnLayoutPersisting { + func load(for key: ColumnLayoutTableKey) -> ColumnLayoutState? { nil } + func save(_ layout: ColumnLayoutState, for key: ColumnLayoutTableKey) {} + func clear(for key: ColumnLayoutTableKey) {} +} + /// `focusedColumn` is a position in `tableView.tableColumns`, which carries the row-number column /// and a hidden spacer ahead of the data and which the reader can reorder. Preview FK Reference /// used to turn it into a data index by subtracting 1, so the menu command previewed the wrong @@ -36,6 +44,43 @@ struct FocusedColumnResolutionTests { return (tableView, schema) } + private func makeCoordinator(columns: [String]) -> TableViewCoordinator { + let coordinator = TableViewCoordinator( + changeManager: AnyChangeManager(DataChangeManager()), + isEditable: true, + selectedRowIndices: .constant([]), + delegate: nil, + layoutPersister: FocusedColumnLayoutPersister() + ) + let rows = columns.map { PluginCellValue.text($0) } + let tableRows = TableRows.from( + queryRows: [rows], + columns: columns, + columnTypes: Array(repeating: ColumnType.text(rawType: "TEXT"), count: columns.count) + ) + coordinator.tableRowsProvider = { tableRows } + coordinator.rebuildColumnMetadataCache(from: tableRows) + coordinator.updateCache() + + let tableView = KeyHandlingTableView() + tableView.coordinator = coordinator + tableView.dataSource = coordinator + tableView.delegate = coordinator + tableView.addTableColumn(DataGridView.makeRowNumberColumn()) + coordinator.tableView = tableView + coordinator.columnPool.reconcile( + tableView: tableView, + schema: coordinator.identitySchema, + columnTypes: [], + savedLayout: nil, + isEditable: true, + hiddenColumnNames: [], + widthCalculator: { _, _ in 100 } + ) + tableView.reloadData() + return coordinator + } + private func tableColumnIndex(of name: String, in grid: (tableView: NSTableView, schema: ColumnIdentitySchema)) -> Int? { guard let identifier = grid.schema.identifier(for: grid.schema.dataIndex(forColumnName: name) ?? -1) else { return nil @@ -44,6 +89,32 @@ struct FocusedColumnResolutionTests { return index >= 0 ? index : nil } + /// Moving the selection with the keyboard leaves no cell cursor behind, so the grid seeds one + /// from the selection change. Seeding it with a fixed position lands on the window's leading + /// spacer, and every command that reads the cursor then resolves it to no column at all: Return + /// opens no editor while the menu item still validates as enabled (#2381). + @Test("A selection with no cell cursor seeds one on a data column, not a spacer") + func selectionSeedsTheCursorOnADataColumn() throws { + let coordinator = makeCoordinator(columns: ["id", "name"]) + let tableView = try #require(coordinator.tableView as? KeyHandlingTableView) + tableView.focusedRow = -1 + tableView.focusedColumn = -1 + + tableView.selectRowIndexes(IndexSet(integer: 0), byExtendingSelection: false) + coordinator.tableViewSelectionDidChange( + Notification(name: NSTableView.selectionDidChangeNotification, object: tableView) + ) + + #expect(coordinator.presentsColumn(atTableColumnIndex: tableView.focusedColumn)) + #expect( + DataGridView.dataColumnIndex( + for: tableView.focusedColumn, + in: tableView, + schema: coordinator.identitySchema + ) == 0 + ) + } + @Test("A data column does not sit one place after its data index") func dataColumnsAreNotOffsetByOne() throws { let grid = makeGrid(columns: ["id", "name", "customer_id"]) @@ -70,18 +141,26 @@ struct FocusedColumnResolutionTests { func reorderingKeepsTheMapping() throws { let grid = makeGrid(columns: ["id", "name", "customer_id"]) let before = try #require(tableColumnIndex(of: "customer_id", in: grid)) + let firstData = try #require(tableColumnIndex(of: "id", in: grid)) - grid.tableView.moveColumn(before, toColumn: DataGridView.firstDataTableColumnIndex) + grid.tableView.moveColumn(before, toColumn: firstData) let after = try #require(tableColumnIndex(of: "customer_id", in: grid)) #expect(after != before) #expect(DataGridView.dataColumnIndex(for: after, in: grid.tableView, schema: grid.schema) == 2) } - @Test("The row-number column is not a data column") - func rowNumberColumnIsNotData() { + /// The row-number column and the window's two spacers all sit in `tableColumns`, and one spacer + /// sits ahead of the first data column, so a fixed position names chrome rather than data. + @Test("Neither the row-number column nor the window's spacers are data columns") + func chromeColumnsAreNotData() { let grid = makeGrid(columns: ["id", "name"]) - #expect(DataGridView.isDataTableColumn(0) == false) - #expect(DataGridView.dataColumnIndex(for: 0, in: grid.tableView, schema: grid.schema) == nil) + + let chrome = grid.tableView.tableColumns.indices.filter { + DataGridView.dataColumnIndex(for: $0, in: grid.tableView, schema: grid.schema) == nil + } + + #expect(chrome.contains(0)) + #expect(chrome.count == 3) } } From 9f09dcbd41058ddc3084abab7163b68dcf552cdf Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Sat, 22 Aug 2026 20:55:38 +0700 Subject: [PATCH 2/3] fix(datagrid): classify a clicked column by what the result presents --- .../Views/Results/KeyHandlingTableView.swift | 3 +-- .../FocusedColumnResolutionTests.swift | 27 +++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/TablePro/Views/Results/KeyHandlingTableView.swift b/TablePro/Views/Results/KeyHandlingTableView.swift index 81cae8026..e48dad970 100644 --- a/TablePro/Views/Results/KeyHandlingTableView.swift +++ b/TablePro/Views/Results/KeyHandlingTableView.swift @@ -127,8 +127,7 @@ final class KeyHandlingTableView: NSTableView { return } - let column = tableColumns[clickedColumn] - let isDataColumn = column.identifier != ColumnIdentitySchema.rowNumberIdentifier + let isDataColumn = presentsDataColumn(at: clickedColumn) let modifiers = event.modifierFlags.intersection(.deviceIndependentFlagsMask) if event.clickCount >= 2 { diff --git a/TableProTests/Views/Results/FocusedColumnResolutionTests.swift b/TableProTests/Views/Results/FocusedColumnResolutionTests.swift index 7946ce292..73edc7b86 100644 --- a/TableProTests/Views/Results/FocusedColumnResolutionTests.swift +++ b/TableProTests/Views/Results/FocusedColumnResolutionTests.swift @@ -115,6 +115,33 @@ struct FocusedColumnResolutionTests { ) } + /// The whole keystroke, not just the seed: with no cell cursor, a selection change seeds one and + /// Return has to open the editor on it. Seeded onto a spacer, every step past the seed resolved + /// to no column and the keystroke was swallowed (#2381). + @Test("Return opens the editor on the column a keyboard selection seeded") + func returnOpensTheEditorOnTheSeededColumn() throws { + let coordinator = makeCoordinator(columns: ["id", "name"]) + let tableView = try #require(coordinator.tableView as? KeyHandlingTableView) + let window = NSWindow( + contentRect: NSRect(x: 0, y: 0, width: 600, height: 400), + styleMask: [.titled], + backing: .buffered, + defer: false + ) + window.contentView = tableView + tableView.layoutSubtreeIfNeeded() + tableView.focusedRow = -1 + tableView.focusedColumn = -1 + + tableView.selectRowIndexes(IndexSet(integer: 0), byExtendingSelection: false) + coordinator.tableViewSelectionDidChange( + Notification(name: NSTableView.selectionDidChangeNotification, object: tableView) + ) + tableView.insertNewline(nil) + + #expect(coordinator.overlayEditor != nil) + } + @Test("A data column does not sit one place after its data index") func dataColumnsAreNotOffsetByOne() throws { let grid = makeGrid(columns: ["id", "name", "customer_id"]) From d6b04847aa792a17b4f545336a10b664b9a8dffd Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Sat, 22 Aug 2026 20:55:42 +0700 Subject: [PATCH 3/3] docs(claude-md): record the data grid column window invariants --- CLAUDE.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index 084b18cb2..4341eec86 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -208,6 +208,10 @@ To ship one: add the record type or field in CloudKit Console (or `xcrun cktool **The data grid header owns all of its own chrome, so nothing may ask AppKit to paint any of it**: `NSTableHeaderCell` and `NSTableHeaderView` both paint a fixed 28pt band that they centre vertically in whatever frame they are given, a 16pt column divider on `midY` and a 1pt rule at `midY + 13`. The data grid grows its header to 42pt for a column comment, so that band lands mid-cell: the rule crosses the comment's descenders and sits 8pt above the real bottom edge. `SortableHeaderChrome` is therefore the single owner of header geometry and colours, `SortableHeaderCell.draw(withFrame:in:)` never calls `super`, and `SortableHeaderView.draw(_:)` fills the background and rules the bottom edge itself. The trap is that the header view paints a second copy of that same band for `NSTableView.highlightedTableColumn`, driven by *state* rather than by a drawing call, so no cell override can reach it: setting it gives the sorted column a stray divider and a rule no other column has. TablePro already draws the sorted-column affordance itself (bold title, chevron, priority number, with `drawSortIndicator` overridden to nothing), so `highlightedTableColumn` is a redundant second channel and must stay unset. All sorted-column presentation goes through `SortableHeaderView.applySortState(_:schema:)`, which publishes the order natively through `tableView.sortDescriptors` (for accessibility; it paints nothing) and updates the cells. `SortableHeaderRenderingTests` rasterises the header and guards this. This shipped as a rule through the comment line and a stray divider on the sorted column (#2017). +**The data grid's column window measures the viewport against the whole column run, so its rebase counts chrome only**: `ColumnWindowResolver` is pure and models every column from the first data column onward, and `DataGridColumnPool.applyColumnWindow` has to move the live viewport into that space before it can pick a range. Only chrome may be subtracted there. The leading spacer sits ahead of the first data column and holds exactly the width of the columns the window left out, which the resolver's model already carries, so counting it subtracts that width twice: the window walks left while the reader scrolls right, alternates between two ranges, and finally parks off screen, where the grid paints nothing until the table is reopened. That shipped as #2381, and a frame-by-frame capture of one scroll sweep across a 100-column table shows it plainly: the worst frame left 100% of the grid unpainted before the fix and 40pt, one column gutter, after it. Two measured AppKit facts hold the design up. A visible column occupies exactly `width + intercellSpacing.width` and a hidden one occupies nothing, spacing included, which is what lets the spacers keep the document width and therefore the scroll extent. And `rect(ofColumn:)` and `frameOfCell(atColumn:row:)` are both empty for a hidden column, so anything that reads a column's frame has to mount it first through `TableViewCoordinator.scrollColumnToVisible(tableColumnIndex:)`, which re-centres a bounded window instead of stretching the mounted range out to the target, because stretching mounts every column in between (848ms and 3,081 cell views for a match 90 columns away, 4.8s at 500). A test here measures through `rect(ofColumn:)` and never through the resolver, or it only proves the resolver agrees with itself. + +**No fixed position in `tableColumns` names a data column**: the attached order is `[__rowNumber__, __leadingSpacer__, data columns, surplus pool slots, __trailingSpacer__]`, so `presentsColumn` is the question to ask, with `firstPresentedColumnIndex` and its neighbours beside it on `DataGridColumnPool`. `DataGridView.firstDataTableColumnIndex` was a hardcoded `1` that the leading spacer took over when windowing landed, and `isDataTableColumn` accepted the trailing spacer at the other end. The cell cursor was seeded onto a spacer whenever the selection moved without a click, so Down then Return did nothing on any table while the Edit menu item still validated as enabled, Tab out of a row's last cell and Shift+Tab out of its first were swallowed, and `scrollColumnToVisible` on a column the window had unmounted scrolled to the document origin instead of the column (#2381). + **Decoding a MongoDB binary UUID is a per-column decision, and the column's type name is load-bearing**: BSON binary subtype 3 is the legacy UUID format, and the Java, C# and Python drivers each wrote it with a different byte order with nothing in the stored bytes to say which. `MongoDBUuidCodec` therefore decodes subtype 3 only when the connection names one (`mongoUuidRepresentation`); subtype 4 is unambiguous and always decodes. The choice is made once per column from `BsonDocumentFlattener.columnKinds`' majority vote, never per value, because a decoded cell is `.text` and an undecoded one is `.bytes`, and `CellDisplayFormatter` runs blob formatting over a `.text` cell whenever its column type is BLOB. One UUID decoded inside a column the app still types `BLOB` renders as `0x4c65676163...`. For the same reason `BsonDocumentFlattener.typeName` must keep `BLOB` as the base name for undecoded binary: `ColumnTypeClassifier` splits a type name at the first `(` and looks the base up, so `BLOB` and `BLOB(3)` both classify as `.blob`, and that classification is the only thing keeping a binary cell out of the inline editor. The parenthesised part carries the BSON subtype so MQL export can write it back; `MongoDBUuidCodec.columnTypeName(forSubtype:)` and `binarySubtype(fromColumnTypeName:)` are the only two places that spelling is produced or read, and MQL export is `supportedDatabaseTypeIds = ["MongoDB"]`, so it never sees another driver's `BLOB`. Once a column does decode, both edit guards (`isBlobType` and `asBytes != nil`) fall together, so every write path must parse the wrapper back to `$binary`: `MongoDBStatementGenerator.jsonValue` and `idValueJson`, `MongoDBQueryBuilder.jsonValue` plus its `=`, `!=` and `IN` arms (a case-insensitive regex can never match a binary field), and `MQLExportHelpers.mqlJsonValue`. An `_id` filter left as wrapper text matches zero documents while the UI reports the save succeeded. (#2086) **A pooled metadata read assumes a second connection reaches the same database, and an embedded engine breaks that assumption**: `MetadataConnectionPool` builds a whole new driver, so it is only correct when the database lives on a server the driver reconnects to. When the database lives *inside* the driver instance, the pool gets a different database: a second `duckdb_open(":memory:")` is a fresh empty database, and a second `duckdb_open` on the same *file* is a second independent read-write instance that the first never sees (DuckDB's file lock does not conflict within one process). The failure is silent, because an empty catalog is indistinguishable from "no tables", which is why #2108 survived a manual refresh. `supportsConnectionPooling` is the opt-out, and it is read only by `DatabaseManager.canPool`; DuckDB and PGlite set it `false`. SQLite-family engines keep pooling, because multi-connection access to one file is what they are built for. Two rules follow. First, every metadata read goes through `DatabaseManager.withMetadataDriver` so `metadataRoute` can apply the rule; reaching for `MetadataConnectionPool.shared.withDriver` directly bypasses it, which is how routines kept pooling after the sidebar stopped. Second, a capability with no `DriverPlugin` static is curated per type and `buildMetadataSnapshot` must carry it over from the built-in snapshot, or `register(snapshot:forTypeId:)` resets it to the struct default the moment the plugin loads. That is not hypothetical: it silently disabled MongoDB's `authenticationIsDatabaseScoped` (#1970) for every build that had the plugin installed. `registerVariant` already treats the curated entry as authoritative, which is the only reason PGlite's flag ever worked.