diff --git a/CHANGELOG.md b/CHANGELOG.md index e965cd41b..3335c1094 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Connection with two transports enabled reaching the database directly, with neither transport applied. +- Delete Connection missing from the connection editor since 0.39.0. +- Continue dimmed after filtering the database chooser down to one driver. +- Down arrow not reaching the list from the database chooser's search field. +- VoiceOver reading a database chooser row's icon before the driver's name. - Cost badge on every plan node of a query ending in `LIMIT`, where the share it reads could exceed 100%. (#2633) - Green "low cost" badge on plans that report no cost at all, such as SQLite and ClickHouse. (#2633) - Empty Cost, Rows and Actual Time columns in the EXPLAIN tree for engines that report none of them. (#2633) diff --git a/CLAUDE.md b/CLAUDE.md index af9fdea48..cac2ed21a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -217,7 +217,7 @@ To ship one: add the record type or field in CloudKit Console (or `xcrun cktool **The data grid's column window measures the viewport against the whole column run, so its rebase counts chrome only**: `NSTableColumn.isHidden` costs O(attached columns) per write, because AppKit walks every row view and re-sorts its subviews to rebuild the key view loop. Hiding the columns outside a viewport is therefore quadratic in the column count: measured at 233ms for 100 columns, 8.1s for 500 and 34s for 1000, with the relayout debris of those writes never reclaimed, which is where a 500-column table's 837MB went. No AppKit knob helps: `autorecalculatesKeyViewLoop = false`, `beginUpdates`/`endUpdates` and hiding before any row exists were all measured and none of them changed it. The grid therefore keeps every column attached and visible, and pays nothing for the ones off screen by not building a view for them: `tableView(_:viewFor:row:)` returns nil for every data column and `DataGridRowView` draws the cells the viewport touches with CoreText. Measured on a 500-column table: opening it went from 12.4s to 26ms and from 837MB to 3.9MB, and the whole table holds 26 views rather than 12,500. Never reintroduce a column window built on `isHidden`. -**A drawn cell can only speak through a mounted view, so accessibility changes the grid's shape**: `NSTableView` builds its `AXCell` tree from cell views and from nothing else. An `NSAccessibilityElement` published by the row never reaches that tree, whichever attribute carries the text, whether the attribute is set or overridden, and however the element is parented; AppKit puts its own placeholder there instead, so the grid publishes a full set of correctly placed, permanently blank cells and reads as empty. That shipped as the drawn-cell rewrite's accessibility (#2381), where every value in every result was silent to VoiceOver. `DataGridCellAccessibilityView` is the answer: one view per data cell that draws nothing, takes no clicks (`hitTest` returns nil) and reads its text through the coordinator, so an edit is spoken with nothing to invalidate. It is mounted only once `DataGridAccessibility.isActive`, which the first accessibility question asked of the grid sets, because nothing reports an attached client: `NSWorkspace.isVoiceOverEnabled` covers VoiceOver alone and nothing at all reports Switch Control, Voice Control, an inspector or XCUITest. And it is mounted only for the rows in the viewport: walking the tree makes `NSTableView` prepare every row of the page, not the twenty-five on screen, and ask for a view for each one, so mounting on preparation alone put 9,000 views and a 21,000 element tree behind a 1,000-row page and starved the app of the main thread badly enough that a sample database never finished opening. A row prepared while it was off screen is answered with no view and is never asked again, so `remountAccessibilityCells()` reloads the rows a scroll reveals; it runs off the clip view's `boundsDidChangeNotification` rather than the scroll view's live-scroll pair, because that is the only one that also reports the programmatic `scrollRowToVisible` VoiceOver uses to reach an off-screen row. Activation remounts every grid, deferred off the query that raised it so the remount cannot re-enter the tree AppKit is walking. Two consequences for tests. A row is as wide as the grid however narrow the result is, so its centre is empty width no cell covers; and a table publishes its columns as siblings of its rows, each as tall as every row it spans and later in the tree, so XCUITest reads every row and every cell in the grid as obscured and refuses to click either. A UI test clicks a point offset from the `data-grid` element, never a row or cell element. +**A drawn cell can only speak through a mounted view, so accessibility changes the grid's shape**: `NSTableView` builds its `NSAccessibility.Role.cell` tree from cell views and from nothing else. An `NSAccessibilityElement` published by the row never reaches that tree, whichever attribute carries the text, whether the attribute is set or overridden, and however the element is parented; AppKit puts its own placeholder there instead, so the grid publishes a full set of correctly placed, permanently blank cells and reads as empty. That shipped as the drawn-cell rewrite's accessibility (#2381), where every value in every result was silent to VoiceOver. `DataGridCellAccessibilityView` is the answer: one view per data cell that draws nothing, takes no clicks (`hitTest` returns nil) and reads its text through the coordinator, so an edit is spoken with nothing to invalidate. It is mounted only once `DataGridAccessibility.isActive`, which the first accessibility question asked of the grid sets, because nothing reports an attached client: `NSWorkspace.isVoiceOverEnabled` covers VoiceOver alone and nothing at all reports Switch Control, Voice Control, an inspector or XCUITest. And it is mounted only for the rows in the viewport: walking the tree makes `NSTableView` prepare every row of the page, not the twenty-five on screen, and ask for a view for each one, so mounting on preparation alone put 9,000 views and a 21,000 element tree behind a 1,000-row page and starved the app of the main thread badly enough that a sample database never finished opening. A row prepared while it was off screen is answered with no view and is never asked again, so `remountAccessibilityCells()` reloads the rows a scroll reveals; it runs off the clip view's `boundsDidChangeNotification` rather than the scroll view's live-scroll pair, because that is the only one that also reports the programmatic `scrollRowToVisible` VoiceOver uses to reach an off-screen row. Activation remounts every grid, deferred off the query that raised it so the remount cannot re-enter the tree AppKit is walking. Two consequences for tests. A row is as wide as the grid however narrow the result is, so its centre is empty width no cell covers; and a table publishes its columns as siblings of its rows, each as tall as every row it spans and later in the tree, so XCUITest reads every row and every cell in the grid as obscured and refuses to click either. A UI test clicks a point offset from the `data-grid` element, never a row or cell element. **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). diff --git a/TablePro/Views/Connection/TypeChooser/DatabaseTypeChooserModel.swift b/TablePro/Views/Connection/TypeChooser/DatabaseTypeChooserModel.swift index 87eea0928..47ba40ea8 100644 --- a/TablePro/Views/Connection/TypeChooser/DatabaseTypeChooserModel.swift +++ b/TablePro/Views/Connection/TypeChooser/DatabaseTypeChooserModel.swift @@ -9,7 +9,10 @@ import Observation @MainActor @Observable final class DatabaseTypeChooserModel { - var searchText: String = "" + var searchText: String = "" { + didSet { settleHighlight() } + } + var highlightedType: DatabaseType? private let allTypes: [DatabaseType] @@ -44,4 +47,55 @@ final class DatabaseTypeChooserModel { .map { (category: $0.key, types: $0.value.sorted { $0.rawValue < $1.rawValue }) } .sorted { $0.category.sortOrder < $1.category.sortOrder } } + + /// The rows as the list draws them, which is category order and then alphabetical, not the + /// order `filteredTypes` happens to produce. Arrowing has to follow what is on screen. + var orderedTypes: [DatabaseType] { + groupedTypes.flatMap(\.types) + } + + /// The match a query most plausibly meant, or nil when the query matched nothing. + /// + /// Ranked, and that is the whole point: the filter also matches taglines and category names, so + /// the first row on screen is routinely not the best answer. "PostgreSQL" matches CockroachDB + /// ("Distributed SQL, PostgreSQL-compatible") and PGlite, and CockroachDB sorts ahead of + /// PostgreSQL alphabetically inside Relational. Arming the first row would let Return commit a + /// driver the user never looked at, and on an existing connection a type change resets its + /// credentials, SSL, driver options and transport. + var bestMatch: DatabaseType? { + let trimmed = searchText.trimmingCharacters(in: .whitespacesAndNewlines).lowercased() + guard !trimmed.isEmpty else { return nil } + let candidates = orderedTypes + if let exact = candidates.first(where: { $0.rawValue.lowercased() == trimmed }) { + return exact + } + if let prefixed = candidates.first(where: { $0.rawValue.lowercased().hasPrefix(trimmed) }) { + return prefixed + } + if let named = candidates.first(where: { $0.rawValue.lowercased().contains(trimmed) }) { + return named + } + return candidates.count == 1 ? candidates.first : nil + } + + func moveHighlight(by offset: Int) { + let items = orderedTypes + guard !items.isEmpty else { return } + guard let current = highlightedType, let index = items.firstIndex(of: current) else { + highlightedType = offset > 0 ? items.first : items.last + return + } + let target = index + offset + guard items.indices.contains(target) else { return } + highlightedType = items[target] + } + + /// Keeps a highlight the query still shows, and otherwise arms the best match, or nothing. + /// + /// Never falls back to the first row: a query that matched only taglines leaves the highlight + /// nil and Continue dimmed, which is correct, because no row is a defensible default there. + private func settleHighlight() { + if let current = highlightedType, orderedTypes.contains(current) { return } + highlightedType = bestMatch + } } diff --git a/TablePro/Views/Connection/TypeChooser/DatabaseTypeChooserSheet.swift b/TablePro/Views/Connection/TypeChooser/DatabaseTypeChooserSheet.swift index 4942162fb..78533a0ca 100644 --- a/TablePro/Views/Connection/TypeChooser/DatabaseTypeChooserSheet.swift +++ b/TablePro/Views/Connection/TypeChooser/DatabaseTypeChooserSheet.swift @@ -52,8 +52,19 @@ struct DatabaseTypeChooserSheet: View { Spacer() - NativeSearchField(text: $model.searchText, placeholder: String(localized: "Search")) - .frame(width: 180) + /// Arrow keys are handed over by the search field's own `NSSearchFieldDelegate`, which + /// is the only route: a focused field editor consumes them as `doCommandBySelector:` + /// before `onMoveCommand` or `onKeyPress` can see them, and `searchFocused` is + /// macOS 15. `onSubmit` stays unwired, or Return would commit through both the field + /// editor and Continue's `.defaultAction`. + NativeSearchField( + text: $model.searchText, + placeholder: String(localized: "Search"), + onMoveUp: { model.moveHighlight(by: -1) }, + onMoveDown: { model.moveHighlight(by: 1) }, + accessibilityIdentifier: "database-type-chooser-search" + ) + .frame(width: 180) } .padding(20) } @@ -94,6 +105,10 @@ struct DatabaseTypeChooserSheet: View { proxy.scrollTo(initialType, anchor: .center) } } + .onChange(of: model.highlightedType) { _, highlighted in + guard let highlighted else { return } + proxy.scrollTo(highlighted) + } } } } @@ -142,10 +157,15 @@ private struct DatabaseTypeChooserRow: View { var body: some View { HStack(spacing: 12) { + /// The row's icon is decoration the name already carries, and left unhidden it + /// publishes its own element, so VoiceOver reads "Cylinder Shape, Filled" ahead of the + /// driver. `Image(decorative:)` cannot cover it, because `DatabaseType.iconImage` also + /// returns an SF Symbol. type.iconImage .renderingMode(.template) .foregroundStyle(type.themeColor) .frame(width: 26, height: 26) + .accessibilityHidden(true) VStack(alignment: .leading, spacing: 2) { Text(type.rawValue) diff --git a/TablePro/Views/ConnectionForm/Components/ConnectionFormActionBar.swift b/TablePro/Views/ConnectionForm/Components/ConnectionFormActionBar.swift index bb29809df..0c5248ba2 100644 --- a/TablePro/Views/ConnectionForm/Components/ConnectionFormActionBar.swift +++ b/TablePro/Views/ConnectionForm/Components/ConnectionFormActionBar.swift @@ -3,6 +3,7 @@ // TablePro // +import AppKit import SwiftUI /// The window's commit actions, and the reason they are unavailable. @@ -23,6 +24,7 @@ struct ConnectionFormActionBar: View { let canCommit = issues.isEmpty && !coordinator.isInstallingPlugin return HStack(spacing: 12) { + deleteButton validationMessage(issues) Spacer(minLength: 12) TestConnectionStatusButton(coordinator: coordinator) @@ -47,6 +49,36 @@ struct ConnectionFormActionBar: View { .padding(.vertical, 12) } + /// Restores the Delete the editor shipped through 0.38.0. + /// + /// It sat at `ToolbarItemPlacement.destructiveAction` until #995 rebuilt the form and dropped + /// the button while porting its body into `ConnectionFormCoordinator.deleteCurrent()`, which + /// has had no caller since. The window has no toolbar now, so it takes the bottom bar's leading + /// edge, which is where `SSHProfileEditorView` already puts the same action. + @ViewBuilder + private var deleteButton: some View { + if !coordinator.isNew { + Button(String(localized: "Delete"), role: .destructive) { + confirmDelete() + } + .accessibilityIdentifier("connection-form-delete") + } + } + + private func confirmDelete() { + Task { + let confirmed = await AlertHelper.confirmDestructive( + title: String(localized: "Delete Connection"), + message: String(localized: "Are you sure you want to delete this connection? This cannot be undone."), + confirmButton: String(localized: "Delete"), + window: NSApp.keyWindow + ) + if confirmed { + coordinator.deleteCurrent() + } + } + } + private var defaultActionTitle: String { coordinator.isNew ? String(localized: "Save & Connect") diff --git a/TableProTests/ViewModels/DatabaseTypeChooserModelTests.swift b/TableProTests/ViewModels/DatabaseTypeChooserModelTests.swift new file mode 100644 index 000000000..cbffd7518 --- /dev/null +++ b/TableProTests/ViewModels/DatabaseTypeChooserModelTests.swift @@ -0,0 +1,172 @@ +// +// DatabaseTypeChooserModelTests.swift +// TableProTests +// +// The chooser's filter matches taglines and category names as well as driver names, so the first +// row on screen is routinely not the best answer. Arming it would let Return commit a driver the +// user never looked at, and on an existing connection a type change resets its credentials, SSL, +// driver options and transport. +// + +import Foundation +import Testing + +@testable import TablePro + +@MainActor +@Suite("Database type chooser model") +struct DatabaseTypeChooserModelTests { + /// Named explicitly rather than taken from `PluginManager`, which loads no plugins under XCTest. + private func model() -> DatabaseTypeChooserModel { + DatabaseTypeChooserModel(types: [.postgresql, .cockroachdb, .pglite, .mysql, .mariadb, .sqlite]) + } + + @Test("a query naming a driver exactly arms that driver, not an alphabetically earlier tagline hit") + func exactNameOutranksATaglineMatch() { + let model = model() + model.searchText = "PostgreSQL" + + #expect( + model.filteredTypes.contains(.cockroachdb), + "CockroachDB's tagline mentions PostgreSQL, so it is still a visible result" + ) + #expect(model.orderedTypes.first != .postgresql, "CockroachDB sorts ahead of it on screen") + #expect(model.highlightedType == .postgresql, "The armed row must be the driver that was named") + } + + @Test("MySQL is armed over MariaDB, whose tagline names it") + func exactNameOutranksAForkTagline() { + let model = model() + model.searchText = "MySQL" + #expect(model.highlightedType == .mysql) + } + + @Test("a prefix arms the driver it begins") + func prefixArmsTheDriver() { + let model = model() + model.searchText = "postgre" + #expect(model.highlightedType == .postgresql) + } + + @Test("a sole match is armed even when only its tagline matched") + func soleTaglineMatchIsArmed() { + let model = model() + model.searchText = "Distributed" + + #expect(model.orderedTypes == [.cockroachdb], "Only CockroachDB's tagline says Distributed") + #expect( + model.highlightedType == .cockroachdb, + "One row is unambiguous, which is the whole point of arming it" + ) + } + + @Test("several matches with no driver-name hit arm nothing") + func ambiguousNonNameMatchArmsNothing() { + let model = model() + model.searchText = DatabaseCategory.relational.displayName + + #expect(model.orderedTypes.count > 1, "Every relational driver matches on category alone") + #expect( + model.orderedTypes.allSatisfy { + !$0.rawValue.lowercased().contains(DatabaseCategory.relational.displayName.lowercased()) + }, + "None of them matched on its own name" + ) + #expect( + model.highlightedType == nil, + "No row is a defensible default among many, so Continue stays dimmed" + ) + } + + @Test("filtering to a single result arms it") + func soleMatchIsArmed() { + let model = model() + model.searchText = "SQLite" + #expect(model.orderedTypes == [.sqlite]) + #expect(model.highlightedType == .sqlite) + } + + @Test("a query matching nothing clears the highlight") + func noMatchClearsTheHighlight() { + let model = model() + model.searchText = "SQLite" + #expect(model.highlightedType == .sqlite) + + model.searchText = "nothing matches this" + #expect(model.orderedTypes.isEmpty) + #expect(model.highlightedType == nil) + } + + @Test("a highlight the query still shows survives further typing") + func visibleHighlightSurvives() { + let model = model() + model.highlightedType = .postgresql + model.searchText = "postgres" + #expect(model.highlightedType == .postgresql) + } + + @Test("clearing the query keeps the driver the user had narrowed to") + func clearingKeepsAVisibleHighlight() { + let model = model() + model.searchText = "SQLite" + #expect(model.highlightedType == .sqlite) + + model.searchText = "" + #expect( + model.highlightedType == .sqlite, + "Every driver is visible again and this one still is, so the choice stands" + ) + } + + @Test("a fresh model with no query arms nothing") + func emptyQueryOnOpenArmsNothing() { + let model = model() + model.searchText = "" + #expect(model.highlightedType == nil, "Opening the chooser must not pre-arm a driver") + } + + @Test("arrowing walks the rows in the order the list draws them") + func arrowingFollowsDisplayOrder() { + let model = model() + let rows = model.orderedTypes + #expect(rows.count > 2) + + model.moveHighlight(by: 1) + #expect(model.highlightedType == rows.first) + + model.moveHighlight(by: 1) + #expect(model.highlightedType == rows[1]) + + model.moveHighlight(by: -1) + #expect(model.highlightedType == rows.first) + } + + @Test("arrowing stops at the ends rather than wrapping") + func arrowingClampsAtTheEnds() { + let model = model() + let rows = model.orderedTypes + + model.highlightedType = rows.first + model.moveHighlight(by: -1) + #expect(model.highlightedType == rows.first) + + model.highlightedType = rows.last + model.moveHighlight(by: 1) + #expect(model.highlightedType == rows.last) + } + + @Test("arrowing up from nothing arms the last row") + func arrowingUpFromNothingArmsTheLast() { + let model = model() + model.moveHighlight(by: -1) + #expect(model.highlightedType == model.orderedTypes.last) + } + + @Test("preselecting an initial type is left alone by the filter it still matches") + func preselectSurvivesAMatchingFilter() { + let model = model() + model.preselect(.mariadb) + model.searchText = "Maria" + #expect(model.highlightedType == .mariadb) + } +} diff --git a/TableProUITests/RedisConnectionModeUITests.swift b/TableProUITests/RedisConnectionModeUITests.swift index 709170190..a52cc3bad 100644 --- a/TableProUITests/RedisConnectionModeUITests.swift +++ b/TableProUITests/RedisConnectionModeUITests.swift @@ -65,16 +65,28 @@ final class RedisConnectionModeUITests: UITestCase { XCTAssertTrue(newConnection.waitToExist(timeout: 10)) newConnection.click() - let chooser = app.windows.firstMatch - let search = chooser.searchFields.firstMatch + /// Scoped to the sheet, not the window: the chooser is a `.sheet` on the welcome window, so + /// a window-scoped `searchFields.firstMatch` sees the welcome list's own filter first, and + /// both carry the identifier `sidebar-filter`. The driver name then goes into the + /// connection filter and the chooser list is never filtered. + let sheet = app.sheets.firstMatch + XCTAssertTrue(sheet.waitToExist(timeout: 10), "New Connection… should open the chooser sheet") + + let search = sheet.searchFields.firstMatch XCTAssertTrue(search.waitToExist(timeout: 10), "The chooser should offer its search field") + XCTAssertTrue(waitUntilHittable(search, timeout: 10)) search.click() - app.typeText("Redis") + search.typeText("Redis") + XCTAssertTrue( + waitForPredicate(timeout: 10) { (search.value as? String) == "Redis" }, + "Typing should reach the chooser's search field" + ) - let redis = chooser.outlines.firstMatch.staticTexts + let redis = sheet.outlines.firstMatch.staticTexts .matching(NSPredicate(format: "value == %@", "Redis")) .firstMatch XCTAssertTrue(redis.waitToExist(timeout: 10), "The chooser should list Redis") + XCTAssertTrue(waitUntilHittable(redis, timeout: 10)) redis.coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)).doubleClick() let form = app.windows["connection-form"]