fix(datagrid): make the controls JSON result mode offers do what they say - #2250
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
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.
Fixes #2244.
JSON result mode offered three controls that did nothing. Each is fixed at its own cause, and the investigation changed what one of those causes turned out to be.
The Filters toggle drew nothing
showsRowFiltersis.data || .json, so the funnel button andCmd+Option+Fwere live in JSON mode, butFilterPanelViewonly existed inside thecase .data:arm. The panel is now arowFilterChromebuilder both arms call.Filtering rebuilds the query and re-runs it, so once the panel renders, JSON shows filtered rows with no further plumbing. The builder is deliberately not hoisted above the
switch: that would paint it over explain plans and the statement-succeeded view in.data.Row editing appeared to do nothing, for a different reason than the issue says
The issue predicted a missing pending-change overlay. That is not what happens. Pending inserts are real rows in
TableRows(appendInsertedRow) and cell edits are written into the same buffer (tableRows.edit), so JSON already carries every pending value.The actual cause is the selection. Add Row, Duplicate Row, Paste and Delete each set
selectionState.indicesto the row they just touched so the grid can highlight it and scroll to it, andResultsJsonViewreads that same selection as "show only these rows". So in JSON mode Add Row collapsed the document to one all-null object, Paste collapsed it to the pasted rows, and Delete collapsed it to a single surviving neighbour. It looked like nothing happened; in fact everything happened and the view narrowed to it.Those four writes are now guarded by
selectionPointsTheGrid. The grid calls immediately after them (applyDelta,beginEditing) already no-op through a nil coordinator when the grid is unmounted, so the selection write was the only line with an effect in JSON mode, which is what makes this safe. Undo and redo index adjustments are untouched, since those keep the selection valid rather than pointing at anything.Cmd+F opened a find bar that could not find
FindCoordinator.runSearch()reads the data grid's cells throughdataTabDelegate?.tableViewCoordinator, which exists only while the grid is mounted. In JSON mode the bar rendered and reported no matches whatever you typed.ResultsViewModegainsshowsFindBar, true only for.data, and the find commands and the Edit menu's Find validation now read it instead ofshowsRowFilters. The two predicates were doing one job for two different things: the filter panel is chrome above the results and works in any mode that shows rows, while the find bar searches one specific view's cells. JSON's Tree view keeps its own search field.Verification
MainStatusBarLayoutTests,ResultsJsonViewTests,MainMenuBuilderTests,RowEditingCoordinatorCopyTests,GridSelectionOwnerTests: 33 executed, 33 passed. Log grepped forFailing tests:,TEST FAILEDandCrash:, 0 hits.swiftlint --strictclean over the app target and over the changed test file.findBarVisibilityByModepins the new predicate and asserts the invariant that any mode which finds must also filter, so the two cannot be split the wrong way later.No UI automation
The three fixes are a view-tree branch, a menu validation arm, and a guarded assignment. Driving them through XCUITest needs a live connection with a loaded table, which
TableProUITestshas no deterministic fixture for. The behaviour is pinned by the predicate test and by reading; a UI test here would assert the app launched.Deliberately not in this PR
Two defects share one cause and need their own design, so I have filed them rather than half-fixing them here.
Display order does not survive the grid unmounting.
activeGridDisplayIDsresolves throughdataTabDelegate?.tableViewCoordinator, which isweakon an object owned by the mounted grid, andsortedIDs/valueFilteredIDslive on that same view-layer coordinator. So switching a sorted or value-filtered result to JSON drops to storage order, and a selection made in Data mode then maps to different rows. That is the display-position invariantCLAUDE.mdrecords from #1837.Pending deletions are not shown, and cannot be until the above is fixed, because a deletion is recorded by display position rather than by
RowID.