diff --git a/CHANGELOG.md b/CHANGELOG.md index 5800bee66..08d3be896 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Copy To and Duplicate Database in the sidebar and the Database menu, carrying structure, data or both to any connection. (#2487) - Search in the connection, database and schema picker that Copy To and Compare & Sync share. (#2487) - Move Column Up and Move Column Down on a column's right-click menu, with the reason where the engine cannot. (#2479) +- Copy on a column's right-click menu in Structure, for the cell under the pointer. - `Up` and `Down` while editing a cell, moving the editor to the same column of the row above or below. (#2569) - Tab rows in Settings > General > Tabs, wrapping the strip instead of scrolling it. (#2438) - Autoscrolling while dragging a tab, so a tab can be moved past the run currently on screen. (#2438) @@ -36,6 +37,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- The data grid's row commands on a column's right-click menu in Structure, when the column was already selected. +- Wrong keyboard shortcuts shown beside Copy Name and Duplicate in the Structure right-click menu. - Plugin download reporting no progress at all when a connection or a file needs a driver installed. - Parse error on any MongoDB filter written in shell syntax, such as `db.orders.find({status: 1})`. - MongoDB `.sort()` and `.projection()` silently ignored when written with unquoted keys. diff --git a/TablePro/Views/Results/DataGridRowView.swift b/TablePro/Views/Results/DataGridRowView.swift index b6018ab1e..c606d58a3 100644 --- a/TablePro/Views/Results/DataGridRowView.swift +++ b/TablePro/Views/Results/DataGridRowView.swift @@ -328,6 +328,47 @@ class DataGridRowView: NSTableRowView { menu.addItem(navInNewTabItem) } + /// Where a right-click landed: the table column index it hit, and the data column that + /// resolves to. Both are -1 when the click missed, and they are different misses: no column at + /// all is not the same as a column that carries no data, such as the row number. + private func clickedColumns(for event: NSEvent) -> (table: Int, data: Int) { + guard let coordinator, let tableView = coordinator.tableView else { return (-1, -1) } + let locationInRow = convert(event.locationInWindow, from: nil) + let locationInTable = tableView.convert(locationInRow, from: self) + let clickedColumn = tableView.column(at: locationInTable) + guard clickedColumn >= 0 else { return (-1, -1) } + let dataColumn = DataGridView.dataColumnIndex( + for: clickedColumn, in: tableView, schema: coordinator.identitySchema + ) ?? -1 + return (clickedColumn, dataColumn) + } + + /// The data column a right-click landed on, or -1 when it missed one. + private func clickedDataColumnIndex(for event: NSEvent) -> Int { + clickedColumns(for: event).data + } + + /// Copy, meaning the cell under the pointer. Shared so a grid that builds its own row menu + /// offers the same item rather than leaving the pointer with no route to a value the keyboard + /// can already copy: the Structure tab had `Cmd+C` copying the clicked cell and no menu item + /// for it at all. + internal func makeCopyItem(for event: NSEvent) -> NSMenuItem { + let columns = clickedColumns(for: event) + let target: CopyContextTarget = if columns.data >= 0 { + .cell(columns.data) + } else if columns.table >= 0 { + .row + } else { + .unresolved + } + let item = NSMenuItem( + title: String(localized: "Copy"), action: #selector(copyFromContextMenu(_:)), keyEquivalent: "" + ) + item.representedObject = target + item.target = self + return item + } + /// Deliberately not `menu(for:)`. The table view owns context-menu handling because it /// is the only level that can re-target the selection to the clicked row first; a row /// view answering `menuForEvent:` would swallow the event and act on the old selection. @@ -335,13 +376,7 @@ class DataGridRowView: NSTableRowView { guard let coordinator = coordinator, let tableView = coordinator.tableView else { return nil } - let locationInRow = convert(event.locationInWindow, from: nil) - let locationInTable = tableView.convert(locationInRow, from: self) - let clickedColumn = tableView.column(at: locationInTable) - - let dataColumnIndex: Int = clickedColumn >= 0 - ? DataGridView.dataColumnIndex(for: clickedColumn, in: tableView, schema: coordinator.identitySchema) ?? -1 - : -1 + let dataColumnIndex = clickedDataColumnIndex(for: event) let menu = NSMenu() @@ -352,19 +387,7 @@ class DataGridRowView: NSTableRowView { return menu } - let copyTarget: CopyContextTarget = if dataColumnIndex >= 0 { - .cell(dataColumnIndex) - } else if clickedColumn >= 0 { - .row - } else { - .unresolved - } - - let copyItem = NSMenuItem( - title: String(localized: "Copy"), action: #selector(copyFromContextMenu(_:)), keyEquivalent: "") - copyItem.representedObject = copyTarget - copyItem.target = self - menu.addItem(copyItem) + menu.addItem(makeCopyItem(for: event)) let copyAsMenu = NSMenu() diff --git a/TablePro/Views/Structure/StructureRowViewWithMenu.swift b/TablePro/Views/Structure/StructureRowViewWithMenu.swift index 4e0acf265..25f4a2ae0 100644 --- a/TablePro/Views/Structure/StructureRowViewWithMenu.swift +++ b/TablePro/Views/Structure/StructureRowViewWithMenu.swift @@ -27,7 +27,23 @@ final class StructureRowViewWithMenu: DataGridRowView { var onDelete: ((Set) -> Void)? var onUndoDelete: ((Int) -> Void)? + /// AppKit takes two routes to a row's menu and this row owns both. + /// + /// `KeyHandlingTableView.rightMouseDown` intercepts a click that lands inside the selection and + /// answers from `contextMenu(for:)`; a click outside it falls through to `super`, which reaches + /// `menu(for:)`. Overriding only the second is what left the Structure tab showing the data + /// grid's row commands, Copy as INSERT and Paste and Set Value and Export Results, over a + /// schema row, and none of Copy Name, Copy Definition or the referenced table, for the + /// select-then-right-click path that most people take. override func menu(for event: NSEvent) -> NSMenu? { + structureMenu(for: event) + } + + override func contextMenu(for event: NSEvent) -> NSMenu? { + structureMenu(for: event) + } + + private func structureMenu(for event: NSEvent) -> NSMenu? { guard structureTab != .ddl, structureTab != .parts, structureTab != .triggers else { return nil } let menu = NSMenu() @@ -43,12 +59,17 @@ final class StructureRowViewWithMenu: DataGridRowView { return menu } + /// The clicked cell, so the Type or the Default is reachable from the pointer and not only + /// from `Cmd+C`, which has copied it all along. + menu.addItem(makeCopyItem(for: event)) + + /// No `Cmd+C` on this one. That key copies the clicked cell, which is what the item above + /// does; advertising it here promised a shortcut that has never copied a column's name. let copyNameItem = NSMenuItem( title: String(localized: "Copy Name"), action: #selector(handleCopyName), - keyEquivalent: "c" + keyEquivalent: "" ) - copyNameItem.keyEquivalentModifierMask = .command copyNameItem.target = self menu.addItem(copyNameItem) @@ -110,24 +131,30 @@ final class StructureRowViewWithMenu: DataGridRowView { if isStructureEditable { menu.addItem(NSMenuItem.separator()) + /// No key equivalent. This showed `Cmd+D`, which is not the binding `duplicateRow` + /// carries, and the real one does not reach here either: `MainContentCommandActions` + /// guards it on `dataGridOwnsSelection` and returns for a schema grid. There is no + /// keystroke that duplicates a column, so the menu stops claiming one. let dupItem = NSMenuItem( title: String(localized: "Duplicate"), action: #selector(handleDuplicate), - keyEquivalent: "d" + keyEquivalent: "" ) - dupItem.keyEquivalentModifierMask = .command dupItem.target = self menu.addItem(dupItem) + /// Read from the binding rather than typed in, because this one genuinely works: + /// `KeyHandlingTableView.keyDown` routes it to the delegate's row delete, and it can + /// be rebound in Settings, at which point a literal here would start lying. let delItem = NSMenuItem( title: String(localized: "Delete"), action: #selector(handleDelete), - keyEquivalent: String( - UnicodeScalar(NSBackspaceCharacter).map { Character($0) } ?? "\u{8}" - ) + keyEquivalent: "" ) - delItem.keyEquivalentModifierMask = [] delItem.target = self + MenuItemFactory.apply( + shortcut: .delete, keyboard: AppSettingsManager.shared.keyboard, to: delItem + ) menu.addItem(delItem) } @@ -148,11 +175,15 @@ final class StructureRowViewWithMenu: DataGridRowView { } } + /// The rows a row command acts on, resolved the way the data grid's own menu resolves them. + /// + /// A cell range dragged across several rows lives in `selectionController`, and the table view + /// keeps only its anchor in `selectedRowIndices`, so reading the latter alone shrank Delete + /// from every row the range covered to one. That was invisible while this menu was reachable + /// only from a click outside the selection; owning the in-selection route as well is exactly + /// the case where a range is what the user has. private func effectiveIndices() -> Set { - if let selected = coordinator?.selectedRowIndices, !selected.isEmpty { - return selected - } - return [rowIndex] + coordinator?.currentRowSelection(fallbackRow: rowIndex) ?? [rowIndex] } @objc private func handleCopyName() { onCopyName?(effectiveIndices()) } diff --git a/TableProTests/Views/Structure/StructureRowMenuRouteTests.swift b/TableProTests/Views/Structure/StructureRowMenuRouteTests.swift new file mode 100644 index 000000000..e2075685d --- /dev/null +++ b/TableProTests/Views/Structure/StructureRowMenuRouteTests.swift @@ -0,0 +1,112 @@ +// +// StructureRowMenuRouteTests.swift +// TableProTests +// + +import AppKit +import Foundation +import SwiftUI +@testable import TablePro +import TableProPluginKit +import Testing + +@MainActor +private final class StructureRouteLayoutPersister: ColumnLayoutPersisting { + func load(for key: ColumnLayoutTableKey) -> ColumnLayoutState? { nil } + func save(_ layout: ColumnLayoutState, for key: ColumnLayoutTableKey) {} + func clear(for key: ColumnLayoutTableKey) {} +} + +/// A column row raises the same menu whichever way AppKit reaches it. +/// +/// There are two routes and they used to end in two different menus. +/// `KeyHandlingTableView.rightMouseDown` intercepts a click that lands inside the selection and +/// answers from `DataGridRowView.contextMenu(for:)`; a click outside the selection falls through to +/// `super`, which reaches the row view's own `menu(for:)`. The Structure tab overrode only the +/// second, so selecting a column and right-clicking it produced the data grid's row commands over a +/// schema row. +/// +/// Asserted here rather than in a UI test because XCUITest cannot tell the two menus apart in this +/// runner: an open contextual menu is not a child of the application element, and the titles that +/// would discriminate (`Export Results…`) also sit in the menu bar, so an app-rooted query answers +/// from there whatever the contextual menu holds. +@Suite("Structure row menu route") +@MainActor +struct StructureRowMenuRouteTests { + /// Only the structure menu builds this. + private let structureOnly = "Copy Name" + /// Only the data grid's row menu builds this. + private let dataGridOnly = "Export Results…" + + private func makeRowView(tab: StructureTab = .columns) -> StructureRowViewWithMenu { + let coordinator = TableViewCoordinator( + changeManager: AnyChangeManager(DataChangeManager()), + isEditable: true, + selectedRowIndices: .constant([]), + delegate: nil, + layoutPersister: StructureRouteLayoutPersister() + ) + let tableRows = TableRows.from( + queryRows: [[.text("id")]], columns: ["Name"], columnTypes: [.text(rawType: "TEXT")] + ) + coordinator.tableRowsProvider = { tableRows } + coordinator.rebuildColumnMetadataCache(from: tableRows) + coordinator.updateCache() + + let tableView = KeyHandlingTableView() + tableView.coordinator = coordinator + tableView.addTableColumn(DataGridView.makeRowNumberColumn()) + coordinator.tableView = tableView + + let rowView = StructureRowViewWithMenu() + rowView.coordinator = coordinator + rowView.rowIndex = 0 + rowView.structureTab = tab + return rowView + } + + private func rightClick() throws -> NSEvent { + try #require(NSEvent.mouseEvent( + with: .rightMouseDown, + location: NSPoint(x: 10, y: 10), + modifierFlags: [], + timestamp: 0, + windowNumber: 0, + context: nil, + eventNumber: 0, + clickCount: 1, + pressure: 1 + )) + } + + private func titles(_ menu: NSMenu?) -> [String] { + (menu?.items ?? []).map(\.title) + } + + @Test("The route a click outside the selection takes builds the structure menu") + func theUnselectedRouteBuildsTheStructureMenu() throws { + let rowView = makeRowView() + let items = titles(rowView.menu(for: try rightClick())) + + #expect(items.contains(structureOnly)) + #expect(!items.contains(dataGridOnly)) + } + + @Test("The route a click inside the selection takes builds the structure menu too") + func theSelectedRouteBuildsTheStructureMenu() throws { + let rowView = makeRowView() + let items = titles(rowView.contextMenu(for: try rightClick())) + + #expect(items.contains(structureOnly)) + #expect(!items.contains(dataGridOnly)) + } + + /// Not a column list, so neither route offers a menu at all. + @Test("A DDL tab row raises no menu on either route") + func theDdlTabRaisesNoMenu() throws { + let rowView = makeRowView(tab: .ddl) + + #expect(rowView.menu(for: try rightClick()) == nil) + #expect(rowView.contextMenu(for: try rightClick()) == nil) + } +} diff --git a/TableProUITests/CopyObjectsUITests.swift b/TableProUITests/CopyObjectsUITests.swift index b63991ccb..4159895eb 100644 --- a/TableProUITests/CopyObjectsUITests.swift +++ b/TableProUITests/CopyObjectsUITests.swift @@ -107,18 +107,4 @@ final class CopyObjectsUITests: UITestCase { private func dismissMenu(in app: XCUIApplication) { app.typeKey(.escape, modifierFlags: []) } - - /// Scoped to the menu the right-click raised. **Database > Copy To…** carries the same title, - /// on purpose, and a closed menu bar submenu is still in the accessibility tree, so an - /// app-rooted `menuItems[title]` matches two elements and refuses to click either. - /// - /// An open contextual menu is a direct child of the application; the menu bar is a - /// `.menuBar` and its submenus hang under that. Where the runner's tree does not agree, - /// hittability separates them: only the open menu's items can be clicked. - private func contextMenuItem(_ title: String, in app: XCUIApplication) -> XCUIElement { - let scoped = app.children(matching: .menu).firstMatch.menuItems[title].firstMatch - if scoped.exists { return scoped } - let matches = app.menuItems.matching(NSPredicate(format: "title == %@", title)) - return matches.allElementsBoundByIndex.first { $0.isHittable } ?? matches.firstMatch - } } diff --git a/TableProUITests/StructureColumnMoveUITests.swift b/TableProUITests/StructureColumnMoveUITests.swift index f9e31fba5..f5b7cbfcd 100644 --- a/TableProUITests/StructureColumnMoveUITests.swift +++ b/TableProUITests/StructureColumnMoveUITests.swift @@ -27,6 +27,13 @@ final class StructureColumnMoveUITests: UITestCase { waitForPredicate(timeout: 30) { grid.tableRows.count > 1 }, "Album has more than one column, so both directions have somewhere to go" ) + /// Existing is not laid out. A coordinate taken off a grid whose frame is still empty + /// resolves to `(inf, inf)`, which `rightClick` then posts at no display at all and the + /// runner dies rather than failing an assertion. + XCTAssertTrue( + waitForPredicate(timeout: 30) { grid.frame.width > 0 && grid.frame.height > 0 }, + "The grid must be laid out before a coordinate is taken off it" + ) /// A point offset from the grid, never a row or cell element: the grid's columns are /// siblings of its rows and later in the tree, so XCUITest reads both as obscured. diff --git a/TableProUITests/StructureRowMenuParityUITests.swift b/TableProUITests/StructureRowMenuParityUITests.swift new file mode 100644 index 000000000..8209af8c4 --- /dev/null +++ b/TableProUITests/StructureRowMenuParityUITests.swift @@ -0,0 +1,79 @@ +// +// StructureRowMenuParityUITests.swift +// TableProUITests +// + +import XCTest + +/// A column row raises its own menu whichever way it was clicked. +/// +/// AppKit takes two routes to a row's menu. `KeyHandlingTableView.rightMouseDown` intercepts a +/// click that lands inside the selection and answers from `DataGridRowView.contextMenu(for:)`; a +/// click outside it falls through to `super`, which reaches the row view's own `menu(for:)`. The +/// Structure tab overrode only the second, so selecting a column and right-clicking it produced +/// the data grid's row commands over a schema row. +/// +/// Only the positive half is assertable here. `Copy Name` exists in no menu bar menu, so finding +/// it proves the contextual menu carried it; the absence of the data grid's commands cannot be +/// asserted through XCUITest at all, because their titles do sit in the menu bar. That half is +/// `StructureRowMenuRouteTests`, which builds both menus and reads their items. +final class StructureRowMenuParityUITests: UITestCase { + private let structureOnlyItem = "Copy Name" + + func testAColumnRowRaisesTheStructureMenuWhetherOrNotItIsSelected() throws { + let app = try launchWithSampleDatabase() + let window = app.windows.firstMatch + + let row = objectBrowserRow("Album", in: window) + XCTAssertTrue(row.waitToExist(timeout: 20), "The object browser must list Album") + clickAtCenter(row) + + showStructure(in: window) + let grid = window.tables.matching(identifier: "data-grid").firstMatch + XCTAssertTrue(grid.waitToExist(timeout: 30), "The structure editor must draw its column grid") + XCTAssertTrue( + waitForPredicate(timeout: 30) { grid.tableRows.count > 1 }, + "Album must report its columns" + ) + /// Existing is not laid out. A coordinate taken off a grid whose frame is still empty + /// resolves to `(inf, inf)`, which `rightClick` then posts at no display at all and the + /// runner dies rather than failing an assertion. + XCTAssertTrue( + waitForPredicate(timeout: 30) { grid.frame.width > 0 && grid.frame.height > 0 }, + "The grid must be laid out before a coordinate is taken off it" + ) + + /// A point offset from the grid, never a row or cell element: the grid's columns are + /// siblings of its rows and later in the tree, so XCUITest reads both as obscured. + let target = grid.coordinate(withNormalizedOffset: .zero).withOffset(CGVector(dx: 80, dy: 40)) + + target.rightClick() + assertStructureMenu(in: app, path: "an unselected column row") + app.typeKey(.escape, modifierFlags: []) + + /// Selecting first is the route that was broken, and the one most people take. + target.click() + XCTAssertTrue( + waitForPredicate(timeout: 10) { grid.tableRows.allElementsBoundByIndex.contains { $0.isSelected } }, + "The click must select a column row, or this asserts the same route twice" + ) + target.rightClick() + assertStructureMenu(in: app, path: "a selected column row") + app.typeKey(.escape, modifierFlags: []) + } + + private func assertStructureMenu(in app: XCUIApplication, path: String) { + XCTAssertTrue( + contextMenuItem(structureOnlyItem, in: app).waitToExist(timeout: 15), + "\(path) must offer \(structureOnlyItem), which only the structure menu builds" + ) + } + + private func showStructure(in window: XCUIElement) { + let modePicker = window.radioGroups["results-view-mode-picker"].firstMatch + XCTAssertTrue(modePicker.waitToExist(timeout: 20), "The result must expose its view modes") + let structure = modePicker.radioButtons["Structure"].firstMatch + XCTAssertTrue(structure.waitToExist(timeout: 20), "Structure must be one of them") + structure.click() + } +} diff --git a/TableProUITests/Support/UITestCase.swift b/TableProUITests/Support/UITestCase.swift index 0d8ac8c0e..afc4861e7 100644 --- a/TableProUITests/Support/UITestCase.swift +++ b/TableProUITests/Support/UITestCase.swift @@ -217,6 +217,25 @@ internal class UITestCase: XCTestCase { element.coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)).click() } + /// An item of the contextual menu a right-click just raised, picked out from the menu bar's + /// copy of the same title. + /// + /// A closed menu bar submenu is still in the accessibility tree, so an app-rooted + /// `menuItems[title]` matches **Database > Copy To…** as readily as the menu under the pointer + /// and then refuses to click either. Hittability is what separates them: only the open menu's + /// items can be clicked. + /// + /// Scoping by container does not work here, measured: `app.children(matching: .menu)` is empty + /// while a contextual menu is up, so a query built on it silently answers no. That is why this + /// takes a title the menu bar also has and narrows it, rather than asking a container what it + /// holds. **A negative assertion cannot be written this way at all**: an absent contextual + /// item is indistinguishable from a present-but-unhittable menu bar one. Assert the absence in + /// a unit test over the menu-building code instead. + internal func contextMenuItem(_ title: String, in app: XCUIApplication) -> XCUIElement { + let matches = app.menuItems.matching(NSPredicate(format: "title == %@", title)) + return matches.allElementsBoundByIndex.first { $0.isHittable } ?? matches.firstMatch + } + /// The app removes its own defaults domain as it terminates, which is the only point that /// reliably comes after `cfprefsd` has written it. This sweep is the backstop for a run that /// crashed or was killed before it got there, and it runs before the class's tests so a diff --git a/docs/features/table-structure.mdx b/docs/features/table-structure.mdx index bbe358b5c..98a054d56 100644 --- a/docs/features/table-structure.mdx +++ b/docs/features/table-structure.mdx @@ -75,7 +75,7 @@ Everything that runs goes to query history rather than the change queue. | **Ref Schema** | Referenced schema, for cross-schema references | | **On Delete / On Update** | Dropdowns: NO ACTION, RESTRICT, CASCADE, SET NULL, SET DEFAULT | -Right-click a foreign key and choose **Open [table]** to jump to the referenced table. Right-click any row in these three grids for **Copy Name**, **Copy Definition**, **Copy As** (CSV, JSON, SQL), **Duplicate** (`Cmd+D`), and **Delete**. A row already marked for deletion offers **Undo Delete**. +Right-click a foreign key and choose **Open [table]** to jump to the referenced table. Right-click any row in these three grids for **Copy** (the cell under the pointer), **Copy Name**, **Copy Definition**, **Copy As** (CSV, JSON, SQL), **Duplicate**, and **Delete** (`Delete`). A row already marked for deletion offers **Undo Delete**. The same menu appears whether or not the row was already selected, and its row commands act on the whole selection. ## Constraints tab