fix(editor): close the query tab review findings - #2895
Merged
Merged
Conversation
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.
Follow-up to #2892. A second model reviewed that branch after it merged and found ten defects in it;
this closes all ten. Nothing here is new behaviour, it is the rewrite finished correctly.
Correctness
A pinned failure read as a success.
resolveSettledResulttested onlyexecution.errorMessage,which the tab clears on the next run, while the pinned
ResultSetkeeps its own. Pin a failedquery, run something that works, switch back, and the result reported no columns and no error, so it
resolved to the "statement succeeded" view over a query that had failed.
QueryResultInputsnowcarries
activeResultErrorMessage, and oneresolvedErrorhelper answers for the banner and for thesuccess gate together.
The find bar outlived the grid it searches.
showsFindBartested the tab type and not the viewmode, so switching a table tab from Data to JSON, Chart, Map or Structure left the find bar mounted
over an unmounted data-grid coordinator.
ResultsViewMode.showsFindBaralready existed and is nowpart of the condition. The old code got this right by accident, because the find bar was built
inside the
.dataarm of the switch this PR's parent flattened.Clear Results was unreachable at the moment it was wanted. Clear Query deliberately leaves the
results standing, and it takes
canRunaway with it. Because Run was aMenu(primaryAction:)disabled on
canRun, the whole menu went with it, including the Clear Results entry inside it. Runis now a real split button: a
ControlGroupwhose Button and Menu are separately enabled, withcanOpenRunMenucovering "anything in here is live". The same bug hid both clears whenever theconnection was unavailable.
The two new Query menu commands were always enabled.
MainSplitViewController.isEnabledhad noarm for
clearQuery(_:)orclearResults(_:), so they fell through totrueand did nothing whenclicked on a table tab, an empty editor, or a window with no command actions.
MenuValidationContextgainscanClearQueryandcanClearResults.Clear Query left the dirty dot stale. The editor's text binding recomputes
window.isDocumentEditedon every keystroke; a command that empties the tab without going throughthat binding did not. A scratch tab kept a dot it no longer deserved and a cleared file-backed tab
was not marked modified. Both now call
MainContentCoordinator.refreshUnsavedIndicator(), which isthe binding's own logic given a name.
Trailing pane
History was replaced by a row click.
revealInspectorForSelectionguarded onlyisAssistantVisible, so with Auto-show inspector on, clicking any grid row closed a deliberatelyopened History pane and persisted Inspector as that connection's surface. The guard now covers both.
History could mount deactivated.
HistoryPanelViewkeys its activation onHistoryPanelState.isVisible, and the synchroniser ran only from explicit reveal and hide. Aworkspace switch onto a connection whose remembered surface was
.history, or a divider drag thatcollapsed the pane, left the flag disagreeing with the pane: a mounted, permanently blank History.
syncHistoryPanelVisibility()now runs fromshowSelectedTrailingPane(), which every reveal, hide,workspace switch and availability change already ends in, and from
splitViewDidResizeSubviews.An upgrading user lost their open history. #2892 moved the drawer into the trailing pane without
carrying the persisted state across, so anyone whose drawer was open at shutdown got Inspector
instead.
TrailingPaneStateadopts.historywhen no surface has ever been stored for theconnection and the history preferences say it was visible. Deterministic and one-time without a
migration flag: the branch is reachable only while no surface is stored, and storing one is what
changing it does.
Layout and onboarding
A long result name set the bar's width.
compactTitlereturned the full label for a singleresult, and the chooser is
fixedSizeinside everyViewThatFitstier, so a result named after along identifier could push the grid out of a 400pt pane. Bounded to 16 characters; the full name
stays on the menu entry and the accessibility label.
The Find Past Queries tip had no anchor. The rewrite dropped the
FeatureTipPopoverAnchor, andnothing else instantiates
FindPastQueriesTip, so its donated rule could never present. Reattachedto the Run menu, which is the control that produces the history the tip points at.
Verification
verify.sh build: PASSverify.sh test QueryResultPresentationTests QueryCommandAvailabilityTests ResultSetMenuModelTests ResultSetPolicyTests: PASS, 41 executed, 41 passedverify.sh lint TablePro: PASS, 0 violationsTwo regression tests added for the two findings that are pure logic:
pinnedFailureStaysAFailureand
findBarFollowsModeinQueryResultPresentationTests, andrunMenuOutlivesRuninQueryCommandAvailabilityTests. The trailing-pane and dirty-indicator fixes are AppKit window stateand are covered by the existing UI suites rather than by new unit tests.
No CHANGELOG entry: #2892 is still unreleased, and per the repo's rules a fix to something unreleased
folds into the original entry rather than adding a "Fixed" line. The entries #2892 added already
describe the behaviour this PR makes true.