Settings: hand-rolled sidebar chrome in place of the TabView - #199
Open
T0mSIlver wants to merge 3 commits into
Open
Settings: hand-rolled sidebar chrome in place of the TabView#199T0mSIlver wants to merge 3 commits into
T0mSIlver wants to merge 3 commits into
Conversation
The tabbed Settings window had run out of room: five tabs of dense rows in a 580x420 frame, with no place to say what a pane is for. This replaces the TabView with a two-column layout — a fixed 208pt sidebar of plain buttons and a detail column that leads with the pane's title and a one-line subtitle. Chrome only: every pane renders exactly the content it did before. Hand-rolled rather than NavigationSplitView/.sidebarAdaptable on purpose. Those bring a sidebar-collapse toolbar button that can only be removed with private-API hacks, and their rows surface to accessibility as table cells. Plain AXButton rows keep the window under our control and give the AX drills something stable to press. The Settings scene is kept (openSettings(), Cmd-comma and restorationBehavior(.disabled) all ride on it). Width is fixed at 780 and height stays free: .windowResizability(.contentSize) would resize the window under the pointer on every tab switch. Both AX drills had to move with it: `button "<name>" of toolbar 1 of window 1` no longer exists. The probe is now shared (scripts/lib/ax-probe.swift) and gained --press <identifier> and --scope <identifier>. Scoping closes a real hole rather than being cosmetic: pane content asserts now have to match inside settings.pane.<tab>, so a sidebar row's AXStaticText label can no longer satisfy them.
…ute matched The open question is whether SwiftUI surfaces .accessibilityIdentifier on a container in a plain-styled hierarchy. If it does not, every scoped pane assertion in the drill would fail for a reason that has nothing to do with the pane. The fallback is the window's first AXScrollArea, which keeps the property the scope exists for: the sidebar rows and the pane header sit outside it, so a navigation label still cannot satisfy a pane assertion. The success line names the route it took, so one drill run answers the question.
1 task
… pane scope Review follow-ups on the drill's two shape-matched fallbacks. Both of them — an AXButton with this title for --press, the first AXScrollArea for --scope — searched every window the app had open. The enrollment sheet alone contains two scroll views, so a presented sheet could have answered a pane assertion, and any window with a "General" button could have answered a tab press. `--window <title-substring>` restricts the search; both drills pass "Settings". A hint that matches nothing searches everything and says so, so a window that stops carrying its title degrades loudly instead of silently matching nothing. ui-smoke now also dumps the tree once before the per-tab assertions and asserts that the pane scope is reachable at all — either a settings.pane.* AXIdentifier or a scroll area. Losing AX identity was previously six 10s timeouts, each blaming its own needle; it is now one named failure, and the pass line records which of the two routes the run used. capture-readme-assets.sh: the three tab tables are indexed together, so a mismatch would have written one tab's screenshot into another tab's file. Length guard, loud exit.
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.
What
Replaces the five-tab
TabViewSettings window with a hand-rolled two-columnlayout: a fixed 208pt sidebar of plain buttons, and a detail column that leads
with the pane's title and a one-line subtitle. Chrome only — every pane
renders exactly the content it did before, from the same call sites.
Stacked PR 1 of 2. #201 (
settings-row-idiom) targets this branch and rewritesthe row/group primitives inside the panes — review this one first, and do not
delete this branch until #201 is retargeted (GitHub auto-closes children when a
base branch is deleted).
Why hand-rolled instead of
NavigationSplitView/.sidebarAdaptable: both bringa sidebar-collapse toolbar button that can only be removed with private-API
hacks, and their rows surface to accessibility as table cells. Plain
AXButtonrows keep the window under our control and give the AX drills a stable element
with a stable identifier to press.
Details:
Settingsscene is kept, soopenSettings(), Cmd-comma andrestorationBehavior(.disabled)all keep working.not
.windowResizability(.contentSize): that resizes the window under thepointer on every tab switch. Each pane keeps its own
ScrollView.WindowAccessor(
SettingsWindowChrome), applied onviewDidMoveToWindowand ondidBecomeKey— SwiftUI re-asserts its own titlebar config when the scenerebuilds the window.
NSVisualEffectView.sidebarmaterial,.behindWindow, 28pt topinset clearing the traffic lights, 34pt rows with an r8 selection fill, 22×22
tinted icon tiles, About pinned to the bottom over a
v{version} ({build})footer.
Sources/localvoxtral/Settings/{SettingsSidebarView,SettingsPaneHeader,SettingsWindowChrome}.swift.SettingsTabgainstitle/subtitle/systemImage/tintand the two AXidentifiers. Raw values are unchanged — the scripts key off them.
Script updates (mandatory, not optional polish)
Both AX drills clicked
button "<tab>" of toolbar 1 of window 1, which no longerexists. The probe moved to
scripts/lib/ax-probe.swift(shared byui-smoke.shand
capture-readme-assets.sh) and gained:--press <identifier>— find by AXIdentifier,AXUIElementPerformActionkAXPressAction;--title <text>is accepted as an AXTitle fallback, and theprobe prints which route it used, so the log answers the "does
.accessibilityIdentifiersurface on a.plainButton?" question empirically.--scope <identifier>— assert needles ONLY inside that subtree. This closes areal hole: sidebar row labels are
AXStaticTextand could vacuously satisfy apane assert. Every
assert_tabnow has to match insidesettings.pane.<tab>.--window <title-substring>— both fallbacks match by shape (an AXButtonwith this title; the first AXScrollArea), so they must not wander into another
window: the enrollment sheet alone contains two
ScrollViews. Both drills passSettings. A hint that matches no window searches all of them and says so, soa window that stops carrying its title degrades loudly rather than matching
nothing.
--dump-on-failoutput.ui-smoke.shdumps the tree once before the per-tab assertions and asserts thepane scope is reachable at all (a
settings.pane.*identifier or a scrollarea). Losing AX identity used to be six 10s timeouts each blaming its own
needle; it is now one named failure, and the pass line records which route the
run used.
capture-readme-assets.sh: its three tab tables are indexed together, so alength mismatch would have written one tab's screenshot into another tab's
file. Length guard, loud exit.
No assert was weakened; the pane asserts are strictly stronger than before.
Proof
./scripts/remote-build.shRe-run after the review hardening commit (
da5d653), focused then full:CI
build-teston this branch: pass (4m7s).Build is warning-free for the new/changed files (the only warnings in
.build/last-remote.logare the pre-existingClaudePluginInstallService.swift:226try?one and two inClaudeHookPublisherTests).New test —
Tests/localvoxtralTests/SettingsTabTests.swift(6 cases, part of the2021 above): the primary + meta sidebar arrays cover
SettingsTab.allCasesexactly once with no overlap and no duplicates; every case has a non-empty
title/subtitle/symbol/identifier; subtitles are single-line sentences; and both
identifier schemes are pinned to the literal
settings.tab.<rawValue>/settings.pane.<rawValue>strings the scripts hardcode, as are the raw valuesthemselves.
(runs 30309569624
and 30310237279).
Both stopped before any AX assertion ran, at the drill's pre-launch guard:
That is the runner's GUI session having a localvoxtral instance that would not
quit (it is the owner's personal Mac, ~00:20 local). The drill deliberately
refuses to kill it rather than force-quitting someone's running app, and I did
not weaken that guard to get a green. The AX selectors are therefore verified
by code-reading only — please re-dispatch
ui-smoke.ymlon this branch once theMac is free.
capture-assets.yml(which would have produced screenshots andthe same evidence) was deliberately NOT dispatched: it
pkills the running appand switches the Mac to dark mode, which is not something to do to an owner who
is evidently at the keyboard
two GUI drill scripts. Nothing in the diff touches prompts, model pins,
sampling, the polish request shape, or anything else that reaches the model
(
scripts/ci/llm-lane-filter.shmatches nothing here).UI change — what was verified, and what was not
Verified here: compiles clean under Swift 6.2 strict concurrency; the full unit
suite; the sidebar/pane data model by test; and the drill selectors by
code-reading plus a
deliberate belt-and-braces in the probe:
--pressfalls back to the row's AXTitleand
--scopefalls back to the window's first AXScrollArea, each printing whichroute it took. So the drill answers "does
.accessibilityIdentifiersurface on a.plainButton / on a pane container?" on its first real run without beingable to break on the answer. The fallback keeps the anti-vacuous property: the
sidebar rows and pane header are outside the pane's scroll area.
Not verified — needs the owner's eyes on a real window (
./scripts/try-pr.sh <this PR>):.behindWindowsidebar material vibrates or renders flat in thiswindow. If it is flat, the documented one-line fallback is
.withinWindowinSettingsSidebarBackground.Settings window (the
didBecomeKeyre-apply is there for exactly that). If itever fights the scene irrecoverably, the documented fallback is to keep the
standard titlebar.
scale, and that the 780×560 default is comfortable for the longest pane
(Text Processing).