fix(datagrid): give a structure row one contextual menu on both of AppKit's routes to it

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.

TablePro/Views/Results/DataGridRowView.swift

@@ -328,20 +328,55 @@ 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.
     func contextMenu(for event: NSEvent) -> NSMenu? {
         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()
 

TablePro/Views/Structure/StructureRowViewWithMenu.swift

@@ -27,7 +27,23 @@ final class StructureRowViewWithMenu: DataGridRowView {
     var onDelete: ((Set<Int>) -> 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<Int> {
-        if let selected = coordinator?.selectedRowIndices, !selected.isEmpty {
-            return selected
-        }
-        return [rowIndex]
+        coordinator?.currentRowSelection(fallbackRow: rowIndex) ?? [rowIndex]
     }
 
     @objc private func handleCopyName() { onCopyName?(effectiveIndices()) }

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)
+    }
+}

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
-    }
 }

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.