refactor(toolbar): make the connection window's chrome a function of what it is showing - #3041
Merged
Merged
Conversation
…w's pane state and preferences its own
…at follow the tab and the mode
…o what it is showing
…e only what the window is showing
… an answer per outcome
…able action and a mode-aware rule
…e commands that moved
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
…ane and one chooses its surface
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.
The connection window's titlebar carried the same seventeen controls whatever you were doing, and #2384 added two more to it. This makes the chrome a function of what the window is showing.
Root cause
The toolbar had no model of the window.
ValidationContextcarried one tab-shaped fact,isTableTab, and no content mode at all, so one list of fifteen identifiers expanding to seventeen hit targets was vended for all eight tab kinds, all four panes and both content modes, and the only lever anyone had was dimming. Every feature that arrived had to buy a permanent slot. The guard test written to stop exactly that counted identifiers rather than hit targets, so #2384's two-segment control passed it.The second half is that the toolbar was trying to be three things at once: the window's identity, the tab's verbs, and a mode switch, with no zone meaning anything.
Shape
One toolbar for the life of the window, one fixed identifier list, and context expressed by
NSToolbarItem.isHiddenandisEnabledand nothing else. Two pure resolvers decide it:ToolbarContextResolvermaps aToolbarContextto the hidden set and to each item's enablement, exhaustive overTabTypewith nodefault:arm.ConnectionActionsMenuResolvermaps the same context to one Actions pull-down carrying the long tail, built fresh on every open withtarget = nilso the responder chain andvalidateMenuItemgate it exactly as they gate the menu bar.The rule that makes it safe:
isHiddenis written only fromToolbarContext.VisibilityKey(tab kind, results mode, content mode, driver capability);isEnabledcarries everything transient. So the titlebar can change shape on a tab switch, a mode switch or a connection switch and at no other moment. A staged edit, a running query or a dropped connection dims a control and never moves one.An item you added yourself through Customize Toolbar is never hidden. Only identifiers in
defaultItemIdentifiersare hideable, so a button you placed stays where you put it and merely dims.Measured, not assumed
Probes compiled and run on macOS 27. Every one of these decided a design choice:
isHiddenreclaims the item's full width, writes nothing to disk, is not autosaved, and costs 0.003ms per toggle.insertItem/removeItemwrite bothTB Item IdentifiersandTB Default Item Identifiersimmediately, and a second window sharing the identifier sees it in the same run loop turn; every connection window shares one identifier, so that approach was out.setConfigurationFromDictionaryis deprecated 10.0 to 15.0 and unavailable from Swift.isHiddensetter runs no validation, and a hidden item'sisEnabledfreezes until something revalidates. Every write is followed byvalidateVisibleItems().NSToolbar.visibleItemsandNSToolbarItem.isVisible; both over-report and nothing repairs them.ToolbarSwitcherPresenter.anchorno longer reads either, and a source-scanning guard test fails the build if anything does.NSInvalidArgumentException. The centred pair is therefore two top-level items rather than a group: 0 raises across 16 presentations where the group raised 4 of 18, anchors within 2pt, and 249pt against the group's 257pt.That last one is visible in the screenshots below: this branch was photographed on a machine whose owner had dragged Back, Forward and Assistant into the toolbar by hand. Back and Forward survived;
contentModeItem,refreshSaveGroup,editorGroup,connectionGroupand the throughput readout were pruned; andconnection,database,refresh,saveChangesandactionswere spliced into their default positions. A bump would have thrown that arrangement away.Before and after
Same machine, same window size, same saved arrangement, only the code differs. Back and Forward are that user's own additions and survive. The Assistant button in the after shot was also theirs, and a later commit retired that item, so it is pruned on the next launch: the pane toggle opens the column and the picker in the pane's header chooses what it draws.
Before, 18 hit targets.
Chinook.sqliteis the container capsule, dim and inert on SQLite.After, 10 at the time of the shot, of which 3 were the user's own. A fresh install shows 7 here, and 9 remain for this user once the Assistant item is pruned.
Agent mode, with the Actions pull-down open on the session commands:
What moved, and where it went
Every command that left the default set has a menu home and a rebindable action. Nothing is only in the toolbar.
Defects fixed
Thirty, found while inventorying the old shape. The ones a user would notice:
saveChanges()already carried the branch that applies themhasPrincipalChangesjoinsupdateToolbarPendingState()andPendingChangeTrigger, through a newPendingChangeKindWorkspacePanes; the mode toggle is a reparent, not a rebuild+and−, Return, a per-row accessibility action, a context menu, and File > SessionisConnected && contentMode == .browseon both surfacesdefault:arm answers falseAppStorageEnvironmentand joinConnectionLocalState.purgeWindowTitleResolverand the same guarded sink as the titleThe full thirty are in the commit messages; the ones above are the released ones. Agent mode is unreleased, so its repairs carry no CHANGELOG
Fixedline.Verification
TableProTestsandTableProUITestsall compile, Debug, into a private DerivedData.ToolbarContextResolverTests,ConnectionActionsMenuResolverTests,ConnectionActionsMenuDelegateTests,ToolbarHiddenSetTests,ToolbarSourceAccessTests,MenuContentModeParityTests,SessionMenuDelegateTests,PendingChangeKindTests,TrailingPaneSurfaceResolverTests,TrailingPaneHeaderModelTests,TrailingPaneCommandTitleTests,TrailingPaneRevealTests,AgentModeWindowTests,AgentArtifactCacheTests,AgentSessionConfirmationTests,BrowseCollapseStateOwnershipTests,SidebarScopeControlTests,HistoryRowTintTests.swiftlint --strict, 0 violations.verify.sh docsPASS; 13 house-style checks, 8 source-claim checks, 135 pages with every link resolving.main, both driven against throwaway sandboxes.Not verified
TableProUITestswere compiled but never run. The runner cannot start on this machine: it dies before any test begins, on untouched suites too. The new cases inConnectionWindowChromeUITests,AgentSessionRailUITests,TrailingPaneSurfaceUITests,AgentModeRoundTripUITestsandAgentModeMenuUITestsare for CI. Two of the toolbar cases skip below a 1512pt screen, so on a 1024pt runner only the sidebar-scope case executes.isHiddenis macOS 15, so the fallback (the union of eight, dimming what cannot act) is derived from source and fromapply(_:)'s availability guard, not observed.insertItem(withItemIdentifier:at:)was, and it postswillAddItemNotificationonce with the item. If a real drop does not post, a dragged-in core item keeps the state it was vended with until the next tab switch, and the fresh-vend rule makes that state visible rather than invisible.saveChanges()has never run from the toolbar or from ⌘S before.Deliberately out of scope
QuickSwitcherPanelViewstill uses the banned.onTapGesture(count: 2). Outside this window's chrome.