fix(datagrid): give a structure row one contextual menu on both of AppKit's routes to it - #2592
Merged
Merged
Conversation
…pKit's routes to it Claude-Session: https://claude.ai/code/session_01Gy6Q4tzwG3bL9h15SMqep1
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follows #2591, which is merged. This branch is based on
main.A column row in the Structure tab raised two different contextual menus depending on how it was clicked. This gives it one.
The defect
AppKit takes two routes to a row's menu:
KeyHandlingTableView.rightMouseDownfalls through tosuper, which reaches the row view's ownmenu(for:). The Structure tab overrode this, so the click produced Copy Name, Copy Definition, Copy As, the referenced table, and Move Column Up/Down.rightMouseDownintercepts it and answers fromDataGridRowView.contextMenu(for:). The Structure tab did not override this, so a selected column row produced the data grid's row menu instead: Copy as INSERT Statement(s), Copy as UPDATE Statement(s), Paste, Set Value, Export Results…, Clear Results, and none of the structure commands.The second route is select-then-right-click, which is what most people do, and it is the one keyboard and assistive access take.
StructureRowViewWithMenunow overrides both entry points and both call onestructureMenu(for:).Getting the measurement right
The first probe I wrote was a UI test asserting the structure menu on both routes. It passed, which would have closed this as "not a bug". It was wrong twice over:
Export Results…is also a File menu item, so an app-rootedapp.menuItems[...]answered yes from the menu bar whatever the contextual menu held.app.children(matching: .menu)does not work either: measured, that query is empty while a contextual menu is up, so the scoped lookup silently answered no and the negative assertion passed vacuously.So a negative assertion about a contextual menu cannot be written in XCUITest here at all: an absent contextual item is indistinguishable from a present-but-unhittable menu bar one.
StructureRowMenuRouteTestsbuilds both menus directly and reads their items, which is what actually caught the defect (it failed oncontextMenu(for:)and passed onmenu(for:)).StructureRowMenuParityUITestskeeps the half that is assertable end to end:Copy Nameexists in no menu bar menu, so finding it proves the contextual menu carried it, and the test now also asserts the click really selected a row so it cannot silently test the same route twice.contextMenuItem(_:in:)moved toUITestCase(two suites need it), reduced to the branch that actually resolves, and documented with what was measured.Polish that came with it
Copy, for the cell under the pointer.
Cmd+Chas always copied the clicked cell in the Structure grid; the menu offered no way to do it, so the Type or the Default was unreachable from the pointer. The item construction moved toDataGridRowView.makeCopyItem(for:)so both menus build the same one, preserving the existing three-way cell / row / unresolved target.Two key equivalents were advertising commands that do not exist.
Cmd+C. That key copies the clicked cell, which is what the new Copy item does, not the column's name. Removed.Cmd+D.duplicateRowis bound toShift+Cmd+D, and that does not reach a structure grid either:MainContentCommandActions.duplicateRow()guards ondataGridOwnsSelectionand returns for.schemaGrid. There is no keystroke that duplicates a column, so the menu no longer claims one.⌫, because this one genuinely works:KeyHandlingTableView.keyDownroutes it to the delegate's row delete.Row commands act on the whole selection.
effectiveIndices()readselectedRowIndices, which holds only the anchor row of a dragged cell range, so Delete on a range would have shrunk to one row. It now goes throughcurrentRowSelection(fallbackRow:), the same resolution the data grid's menu uses. This mattered only once the in-selection route was owned, which is exactly the case where a range is what the user has.Verification
verify.sh buildPASSverify.sh test StructureRowMenuRouteTests DataGridRowViewCopyTests DataGridRowIdentityTestsPASS, 36 casesverify.sh uitest StructureRowMenuParityUITests StructureColumnMoveUITests CopyObjectsUITestsPASSswiftlint --strictclean on every file this branch touches. The twostorage_environment_defaultsviolations inUITestCase.swiftare pre-existing, confirmed by stashing the branch and re-running.docs/scripts/check-writing-style.shandcheck-docs-against-source.pyboth passforce_unwrappingviolation in the new test).Worth knowing:
verify.sh lint TablePro TableProTests TableProUITestsreported 0 violations on a file thatswiftlint --strict <file>rejects outright..swiftlint.ymlsetsincluded: [TablePro], and passing extra paths does not appear to widen that scope. A test-target lint violation can pass the wrapper and fail CI.One flake fixed on the way
The parity test crashed the runner once on this base, at
0.000 seconds, loggingDisplayManager: Could not find any displays containing rect (inf, inf, 0.0, 0.0). Existing is not laid out: a coordinate taken off a grid whose frame is still empty resolves to(inf, inf), andrightClickthen posts it at no display at all, so the runner dies rather than failing an assertion. Both structure suites now wait for a non-empty frame before taking a coordinate.Not covered by automation
The absence of the data grid's commands is asserted in the unit test rather than the UI test, for the reason above. The unit test builds the row view and both menus directly, so it covers the dispatch question precisely; what it cannot cover is AppKit's actual routing of a real right-click, which the UI test covers positively.