fix(datagrid): make the JSON results view follow the grid as shown - #2050
Merged
Conversation
|
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.
Items B and C of the follow-up work from #2047. The JSON results view rendered something the grid was not showing.
What was wrong
It ignored the value filter entirely. With no selection,
computeJsonwalkedtableRows.rowsdirectly and never consulteddisplayIDs. So a filter that hid rows in the grid still emitted them as JSON. That is a set-membership bug, not just an ordering one.It was internally inconsistent. With rows selected it resolved through
DisplayRowMappingand therefore followed display order; with nothing selected it used fetch order. One control, two contracts, switched by invisible state.It ignored hidden columns and column order. It built its converter from the raw
tableRows.columns. A column you hid in the grid still appeared in the JSON pane, and columns came in query order rather than the order you dragged them into.docs/features/data-grid.mdxalready promises the opposite: "Copies follow the grid as shown: hidden columns are left out and columns keep their current order."It never re-rendered when only the filter changed.
displayIDsreaches SwiftUI through@ObservationIgnored weakhops onto a plain AppKit object, so a filter change produces no observation signal at all, and it was not in the render key either. The view was correct only by accident: switching into JSON mode remounts it, so it happened to read fresh values on the way in.The fix
One serializer.
ResultJsonSerializeris now the single place a result set becomes JSON, and both the JSON view and the grid's Copy as JSON go through it. Same rows in, same bytes out. This follows DataGrip, whose Text view is rendered by the same data extractors that power its copy, rather than by a second implementation that has to be kept in agreement.Row resolution is now uniform: an empty selection means every displayed row, a non-empty selection means those display positions, and both resolve through
DisplayRowMapping. The special case is gone rather than duplicated.Columns come from the persisted layout, not the live
NSTableView, because the grid is not mounted while the results pane is showing JSON.VisibleColumnProjection.fromColumnLayoutbuilds the projection fromColumnLayoutState. A result set with duplicate column names cannot be mapped back to single indices, so it keeps every column rather than guessing.An observable display revision.
TableViewCoordinatorticksdisplayRevisionfromdidSetonsortedIDsandvalueFilteredIDs, so a future write site cannot forget to bump it, and notifies through the delegate.DataTabGridDelegatealready holds the coordinator, so it mirrors the tick ontoMainContentCoordinator.gridDisplayRevision, which is observable and therefore actually invalidates the view. The render key holds thatIntrather than the id array: comparing up to 1000RowIDs on every body evaluation is the cost Apple's own guidance warns about ("keep your view bodies fast", "rely on limited dependencies"), and a fresh array is built on every filter recompute so the copy-on-write fast path never applies.This mirrors
TabSession.dataRevisionandTableStructureView.displayVersion, the two existing precedents for exactly this problem. Apple documents no guidance on version counters versus deep comparison, so this is an engineering call, not a platform rule.Sorting was already covered and is unchanged: data-tab sorting is a requery, which bumps
dataRevision.Tests
ResultsJsonViewTestsgains: no selection follows display order; no selection excludes rows a filter removed; a hidden column is left out; columns follow the user's order. The existing selection, out-of-range and empty-result cases still pass against the shared serializer.Lint clean,
ResultsJsonViewTests,MainContentCoordinatorSelectionResetTests,TabSessionRegistryTests,DisplayRowMappingTestsandJsonRowConverterTestsall green.Scope note
Export is untouched. If TablePro's export re-runs the query rather than serializing the view, that is a defensible split, but it should be stated in the export UI: DataGrip resolved the same question as Works as Intended and its users still file bugs about it, because nothing tells them.