Adopt Semi.Avalonia and a FieldWorks design-token system - #1083
Adopt Semi.Avalonia and a FieldWorks design-token system#1083johnml1135 wants to merge 4 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1083 +/- ##
=======================================
Coverage 38.35% 38.35%
=======================================
Files 1507 1512 +5
Lines 350634 350665 +31
Branches 40304 40310 +6
=======================================
+ Hits 134496 134515 +19
- Misses 186905 186911 +6
- Partials 29233 29239 +6
🚀 New features to boost your workflow:
|
jasonleenaylor
left a comment
jasonleenaylor
left a comment
There was a problem hiding this comment.
This is the most consequential change in the queue, so it got the most scrutiny — including
running your scanner against fixtures rather than reading it. The token architecture is
right, and I want it to land.
What is good, plainly. Aliasing Semi's semantic tier instead of inventing a palette is
the correct default and the ADR argues it well. GenerateTokenKeys turning a stale key into
a compile error is the right instinct and is the thing that makes "no exceptions" mean
something. DialogTheme.axaml + DialogThemeBootstrap are extended in place rather than
replaced. Deleting FwCheckBoxStyle/FwRadioButtonStyle (454 lines) instead of porting them
is right, because Semi exposes those sizes as overridable resources. New strings go through
.resx with no new L10NSharp. No test constructs a WinForms Form. The four rejected
alternatives in the body are each argued with evidence rather than taste, and the
live-UI-language-switching investigation that concluded "don't build it" is exactly the kind
of negative result worth writing down.
Timing. Before this lands, Hasso, Zach and Ken need to verify the timing — this is
foundational to everything downstream of it, and the sequencing is their call as much as the
content is mine.
1. Pin the theme variant
Nothing in the branch sets RequestedThemeVariant. A whole-branch search for
RequestedThemeVariant|ThemeVariant|ActualThemeVariant returns exactly one hit:
FwThemeResources.cs:49 reading app.ActualThemeVariant. Under Avalonia 11.3.17
(Directory.Packages.props:186), an unset RequestedThemeVariant means ActualThemeVariant
follows the OS app-theme setting, so a machine in dark mode resolves the Dark dictionary —
the one FwColorTokens.axaml:77-78 labels "unreviewed first-pass placeholders, not
design-approved", while the PR body says Light is the only reviewed variant.
The reason this is item 1 rather than a footnote: Dark is a complete 12-key palette, not a
stub. Every Light key has a counterpart, so it resolves silently instead of failing — Require
throws only on a missing key. An opt-in tester on a dark-mode machine sees an unreviewed UI
and has no way to know that is not the intended design, which makes their feedback misleading
as well.
Fix is one line at the top of FwAvaloniaApp.Initialize():
RequestedThemeVariant = ThemeVariant.Light;
Mirror it in PreviewHostApp so the two Initialize() bodies stay parallel as their comments
promise.
The test must assert RequestedThemeVariant, not ActualThemeVariant. Under headless the
latter reports Light regardless, so a test written the obvious way passes even unfixed.
2. The enforcement check stays blocking — but it has to be trustworthy first
I am keeping token-hygiene as a hard CI failure. The scope is narrow and real, and a check
that only ever runs advisory tends to stay advisory. But a blocking check must not misdescribe
itself, and this one does, in five places plus a bug.
2a. It can silently stop checking. token-hygiene.ps1:70-81 sets
FW_TOKEN_HYGIENE_REPORTED=1, and any later invocation in the same job exits 0 before
scanning, regardless of -Advisory. Reorder the CI steps so any advisory run precedes the
build and the enforcing run becomes a no-op pass that reports success. This is the most
important fix in the item: an enforcement mechanism that can quietly disable itself is worse
than none, because it is trusted.
2b. Four documentation claims are false. build.ps1:128-129 ("CI reports violations as
warning annotations either way"), build.ps1:227-228 and test.ps1:129-130 ("in CI it only
annotates the pull request") — CI.yml:73 runs -TokenHygiene with an explicit
exit $LASTEXITCODE and no continue-on-error, and annotations are emitted only under
$Advisory (token-hygiene.ps1:62), so CI emits none and hard-fails. test.ps1:46-47 omits
that test.ps1:131 force-runs it whenever CI=true. TokenHygiene.psm1:282-285 says comments
are "masked only when the comment is the whole line's content", which is not what the code
does. AGENTS.md is the only one that matches reality.
2c. ADR 0001's enforcement paragraph describes a feature that does not exist. It states the
check "requires every value in the FieldWorks token files to be either a Semi alias or a
literal on a declaration line with a written justification comment — a literal with no comment
... fails". The entire implementation is TokenHygiene.psm1:360:
if ($line -match '\bx:Key\s*=') { continue }. No comment is required or checked. Someone
reading the ADR would believe the token dictionaries are policed when they are the least
policed files in scope.
2d. Close the worst holes. I ran the scanner against fixtures; every one of these passes
clean today:
<Border x:Key="X" Background="#FF0000" Margin="40" Padding="12,8"/> (x:Key skips the line)
<Border Background="#00FF00"/> <!-- note --> (<!-- masks the line)
Opacity = 0.45, (not in the property list)
Padding = new Thickness(radio * 0.45) (arithmetic evades)
Height = (double)18 / MinWidth = 160.0m (cast/suffix evades)
var col = Avalonia.Media.Brushes.Red; (lookbehind exempts it)
<Border Margin="8 4 8 4"/> (numeric check is comma-only)
<Grid RowDefinitions="40,Auto" ColumnDefinitions="220,*"/> (names not listed)
The x:Key and comment-masking ones matter most because they skip the entire line, and
TokenHygiene.Tests.ps1:204,209,323 encode that behaviour as intended. Narrowing both to the
matched declaration rather than the whole line, adding Opacity/Margin/Padding/
BorderThickness/CornerRadius to the C# property list, and accepting space-separated
Thickness values would close most of it. I am not asking for a perfect scanner — I am asking
that it not be defeated by appending a comment.
2e. "Whole-tree, no grandfathering" is 170 files out of 2309 under Src. That is a fine
scope; it is just not what the phrase says. Please state the real scope in the ADR and the body.
3. One-use tokens belong next to their view — the check already allows this
DialogTheme.axaml:97-116 hoists roughly twenty per-dialog constants into the shared dialog
dictionary, and the file's own comment apologises for it: "one-off, content-driven, but still
named for the token-hygiene gate."
That apology is unnecessary, because the check does not require it. I verified this by running
the scanner over a fixture: a value declared in a view's own <UserControl.Resources> is not
flagged (the declaration line contains x:Key=), and consuming it via {StaticResource} is
not flagged either. Only the literal usage trips. So a one-use value can live in the file that
uses it.
All six of the clearest offenders are consumed from exactly one .axaml and never from C#:
| Key | Only consumer |
|---|---|
EntryGoAuxiliaryOptionsMaxHeight |
EntryGoDialogView.axaml:97 |
InsertEntryMatchesListMinHeight |
InsertEntryDlgView.axaml:114 |
LexOptionsComboMinWidth |
LexOptionsDlgView.axaml:59,134 |
MessageBoxIconGlyphSize |
MessageBoxView.axaml:42 |
AddNewSenseMinWidth |
AddNewSenseDlgView.axaml:8 (root element) |
MessageBoxMinHeight |
MessageBoxView.axaml:7 (root element) |
Please move them back and write the rule down somewhere durable: a value used by one view
lives in that view's own Resources; a value shared by two or more views, or consumed from
C#, goes in the shared dictionary. The two set on a root element referencing its own
Resources are worth a quick confirm rather than an assumption.
That restores "shared" to meaning shared, and removes the only place where the check is
visibly distorting the design.
4. Account for the values that changed, and re-measure two colours
The body says fwGroupBox "is the one real visual change". Seven values moved:
| Property | main | branch |
|---|---|---|
LabelColumnWidth |
96 | 150 |
WsAbbrevWidth |
28 | 60 |
FieldSpacing |
2 | 1 |
LabelBrush |
#6666B8 |
#696969 |
WsAbbrevBrush |
#4682B4 |
#404040 |
ValidationErrorBrush |
Firebrick #B22222 |
SemiColorDanger #F93920 |
PickerForegroundBrush |
#1A1A1A |
#1C1F23 |
plus three new: WsAbbrevMaxWidth 120, HotlinkBrush #0064FA, DisabledOptionBrush
#808080. FwColorTokenResolutionTests.cs:28,34 assert two of them, so they are intended —
but a 56% wider label column and a doubled writing-system gutter are not "no visual change",
and a reviewer reading the body would not go looking.
Please list each with a one-line reason under "Where to look". ValidationErrorBrush
especially: muted brick to vivid orange-red is a large perceptual jump, and the comment at
FwAvaloniaDensity.cs:205-208 says danger "is exactly what that role means, with no
FieldWorks-specific divergence to justify" — while FwColorTokens.axaml elsewhere declines
exactly this kind of drift ("so the measured value stays").
And the label colours need a real measurement, not a reworded claim. main
described #6666B8 as the "legacy label hue from the committed baseline pixels"; the branch
describes #696969 as "measured from the legacy baseline". Both claim the same source for
colours that are nowhere near each other (blue-violet versus grey; steel blue versus
near-black), FwColorTokens.axaml repeats the new claim, and ADR 0001 then cites those
comments as its provenance — so the evidence is circular and one half of it is false. You have
the baseline PNGs committed; please re-derive both values, state the method, and correct
whichever comment is wrong. For infrastructure like this the dictionary is the design
record, and the next person will treat whatever it says as measured fact.
5. Vendor-supplied strings must end up in Crowdin
What is here today is a net improvement, and worth saying so first. The only Ursa types used
anywhere are Form and FormItem (DataTree.cs), which render no text, and FwSemiLocale's
own docstring explains the real bug it prevents — both vendor themes reset to zh-CN on an
unrecognised locale, so without this class an Arabic user would get Chinese context menus.
Mapping the other locales to en-US is the right call.
The direction still needs to change. Semi now owns the wording of user-visible chrome — the
built-in TextBox context menu and validation furniture, in dialogs that do use text-bearing
controls (ChooserDialogView, CreateFeatureDialogView, EntryGoDialogView,
LexOptionsDlgView). A third-party vendor decides FieldWorks' vocabulary for six of the 29
locales in Installer.legacy.targets:527 and leaves the other 23 in English.
These strings need to come from a FieldWorks-owned .resx translated through Crowdin.
crowdin.json already globs Src/**/*.resx, so a resource file under Src/ flows to all
shipped locales with no config change — the work is enumerating Semi's localised keys and
overriding them. If that does not land in this PR, please record it as a Jira issue before
merge and reference it here, and label FwSemiLocale in its own summary as the interim
measure it is rather than the destination.
Either way, add a test pinning the locale lists against the shipped packages. They are
hand-transcribed from a version comment ("v11.3.14", "v1.15.1"), FwSemiLocale has no test
coverage at all, and drift in the wrong direction silently reintroduces the zh-CN default this
class exists to prevent.
6. Fix the layering inversion
FwAvalonia.csproj:83,93,97 feed ..\FwAvaloniaDialogs\DialogTheme.axaml into
GenerateFwTokenKeys, and FwAvaloniaDialogs.csproj:55 project-references FwAvalonia. So the
foundation's generated public API is determined by a file inside its own dependent. This
contradicts FwAvaloniaTheme.csproj:6-12, whose stated reason for existing is that the two
projects "share ONE token source without either depending on the other".
Concretely: GenerateTokenKeys.cs:114-116 throws InvalidDataException on an identifier
collision, so a Dialogs-only edit can fail the foundation's compile; and
FwAvaloniaApp.Initialize never merges DialogTheme.axaml, so the foundation publishes 23
Dialog* constants for keys its own app never registers.
The only consumer is CompactDialogStyles.cs (5 Dialog*Value uses), which is dialog styling
living in the foundation. Moving DialogTheme.axaml into FwAvaloniaTheme, or
CompactDialogStyles into Dialogs, removes the inversion entirely.
7. labelMaxWidth ignores the actual column width
DataTree.cs:439-440 computes labelMaxWidth from the token LabelColumnWidth (150) while
the real grid column comes from the host-supplied getLabelColumnWidth() (:92). This use is
new in this PR. Drag the splitter narrower than 150 and label wrapping stays capped at the
token, not the column. Either derive it from the same source the column uses, or say why the
token is correct here.
8. Pin the Ursa workarounds with tests
DataTree.cs fights Ursa's ControlTheme in three places — :107-110 (overriding
HorizontalAlignment=Left), :113-119 (a scoped style beating FormItem's Margin="0 8"),
:453-456 (local FontWeight.Normal beating a bold DynamicResource binding) — plus
:121-123/:152 depending on FormItem honouring only an absolute LabelWidth. All four
depend on undocumented internals of a 1.x dependency.
The only Ursa-aware test (DetailCustomFieldRenderingTests.cs:70) asserts none of them, and
VisualParityAndDensityTests.cs:141 asserts a constant rather than a rendered margin. An Ursa
upgrade that changes any of these regresses layout silently. Characterization tests asserting
the resolved alignment, margin and font weight would fail loudly instead, which is what you
want on a dependency you do not control.
9. Housekeeping
- Stranded Fluent.
FluentThemeis referenced by zero code on the branch but still
package-referenced inFwAvalonia.csproj:37,FwAvaloniaTests.csproj:28,
FwAvaloniaDialogs.csproj:35,FwAvaloniaDialogsTests.csproj:23,
FwAvaloniaPreviewHost.csproj:23, plus the pin atDirectory.Packages.props:192. Drop them —
otherwise a FluentDynamicResourcecan silently resolve again later. PrivateAssetsis backwards. OnlyFwAvalonia.csproj:39-41marks Semi/Ursa
PrivateAssets="all"— but FwAvalonia's runtime code needs Ursa, so the dependency does not
flow and every consumer must redeclare it or hit a runtimeFileNotFoundException. The other
four projects omit it. Pick one contract deliberately.VisualSnapshotTests.cs:105-114(DetailEditView_AtWindowWidth_RendersCleanly) asserts
nothing — it captures and returns, where the test above it calls
DialogLayoutAssert.AssertNoCrowding. It cannot fail on a rendering defect and it inflates
the 647-passed figure.- The word "gate". 22 newly-authored uses across
AGENTS.md,build.ps1,test.ps1,
TokenHygiene.psm1,token-hygiene.ps1and the three ADRs, plus the filename
0002-whole-tree-token-hygiene-gate.md. Please use "check"/"checker". This is all new text,
so there is no existing-prose exemption. - Broken ADR links. The body's three links point at
.../blob/semi-avalonia/docs/adr/...;
the files are atDocs/adr/..., and GitHub blob paths are case-sensitive. Docs/adr/is a third location.Docs/architecture/andopenspec/already exist for
this, andAGENTS.mdnamesDocs/lessons/README.mdas the durable-lessons index. Three
decisions of this weight should land in an existing system, or the new one should be
explained.Docs/adr/0002's central justification is an uncited appeal to "current design-token
literature" (twice). For a decision that changes build policy for every contributor, name the
source.- New group-box captions with no WinForms twin —
FwAvaloniaDialogsStrings.cs:41-45,56-57
adds "Interface", "Startup", "Automatic Updates". Thedialog-updateskill requires approved
divergences from the WinForms twin to be recorded in the conversion's Jira issue; noLT-
reference appears in the branch.
Questions, not change requests
- Brush identity. These properties now return the shared, mutable
SolidColorBrush
from the dictionary, wheremainreturned immutableBrushes.*singletons. You reason about
this forTransparentBrush(FwAvaloniaDensity.cs:230-236) but not forSliceRuleBrush,
SectionRuleBrushorPickerBorderBrush. Is anything relying on reference equality or
mutating one? - Does
FwBuildTasksneed Avalonia?FwBuildTasks.csproj:26-27takes the dependency solely
forThickness.Parse(GenerateTokenKeys.cs:127-133,180-188), which puts Avalonia.Base and
its closure intoBuildTools/for every build in the repo. Parsing one to four
comma-or-space-separated doubles by hand is a few lines. - Absolute paths in the generated file.
GenerateTokenKeys.cs:141,146writes the raw input
paths into the header, and those come from$(MSBuildProjectDirectory)\..\..., so
GeneratedTokenKeys.g.csdiffers byte-for-byte between machines. It is gitignored so the
impact is small, but a repo-relative path is a one-liner. - L10NSharp jumps ten betas (
SilVersions.props:22, beta0004 to beta0014) inside a theming
PR, with the body noting only that live language switching was investigated and dropped. What
else moved in those ten? FwSemiDensity.cs:35-39computesradio * 0.45while its own adjacent comment cites
Semi's ratio as "~0.375". Which is right?
Noted, not asked for
- High contrast. Not a regression — 150 WinForms files still use
SystemColorsand track
the OS scheme. But Avalonia maps a high-contrast scheme onto Light or Dark by whether its name
contains "White", so a High Contrast Black user lands in that unreviewed Dark palette. Pinning
Light (item 1) does not fix high contrast; it makes the failure consistent and reviewed rather
than arbitrary. Real support is out of scope here and worth its own ticket. - Dependency licences. Semi and Ursa are both MIT and compatible, and this PR records
neither — but the repo has no NuGet-licence convention at all (Docs/architecture/dependencies.md
is about repo dependencies, and no NOTICE file is tracked), so this PR is consistent with
existing practice. Raising it as a gap for someone to close, not as something you introduced.
Comments
FwAvaloniaDensity.cs appends the same sentence — "Resolved from the shared FwAvaloniaTheme
token dictionary (DataTree.X) at point-of-use, after the Application has started" — to roughly
25 properties, beside code that already reads
FwThemeResources.RequireDouble(GeneratedTokenKeys.DataTree_X). It restates the code, it is
HOW rather than WHAT, it names the collaborator, and it duplicates the type-level precondition
in every member. One statement on the type, none on the members.
Also: cross-file pointers (FwColorTokens.axaml:11-12, TokenHygiene.psm1:15-16); absence
narration (FwColorTokens.axaml:10-12, FwSemiDensity.cs:10-15, LexOptionsDlgView.axaml:34-38
and :115-119); DialogThemeBootstrap.cs:65-68, a four-line comment explaining why no style is
added, followed by nothing — delete it; a finding reference at DataTree.cs:121 ("Viewing
parity (11.15)"); and consumer instructions in FwThemeResources.cs:12-25. FwThemeResources
should carry an <exception> tag for the InvalidOperationException its whole design rests on
— which is also the one path Codecov shows uncovered.
The per-key rationale comments in DataTreeTokens.axaml and FwColorTokens.axaml are the good
ones: real WHY, one or two sentences, correctly using single hyphens inside XML comments. Keep
those.
Finally, dozens of the rewrapped comments break mid-phrase leaving orphan one-word lines
(FwAvaloniaApp.cs:17-18, FwThemeResources.cs:14-19, FwSemiDensity.cs:26-29,
DialogTheme.axaml:71-74, DialogLayoutAssert.cs:21-23, which also loses its bullet
indentation). Not a rule violation, but it reads as machine-mangled throughout.
Replace the Avalonia Fluent theme with Semi.Avalonia + Ursa app-wide, and rebuild the DataTree detail view on Ursa's Form/FormItem instead of a hand-built Grid. The theme swap surfaced (and this fixes) real layout regressions caught via actual screenshots: the pane not filling its width, labels breaking mid-word, writing-system abbreviations clipping, section headers centering instead of left-aligning, duplicated header text. Field visibility on collapse/expand now computes from the model (DetailVisibility) instead of toggling realized controls. Build a shared FieldWorks design-token system on top of that (new Src/Common/FwAvaloniaTheme project, Light/Dark ThemeDictionaries), replacing the color/spacing/font-size literals that used to be scattered across FwAvalonia/FwAvaloniaDialogs. Every token defaults to aliasing Semi's own semantic color/spacing roles (SemiColorText0, SemiColorBorder, SemiColorBackground0, the Semi spacing/radius scale, ...) rather than an independently-invented value; a FieldWorks-owned value requires a written, checkable reason (see FwColorTokens.axaml's comments) -- verified against the actual pinned Semi.Avalonia 11.3.14 resources, not assumed. Enforce this with Build/Agent/token-hygiene.ps1: unlike comment-hygiene.ps1, it is not diff-scoped and has no grandfathering -- every run scans the whole Avalonia surface (including the Src/LexText/Src/xWorks trees future conversions will land in) and fails on any hardcoded color/spacing literal. Wired into CI as a hard failure. A new Build/Src/FwBuildTasks GenerateTokenKeys task (following liblcm's LcmGenerate precedent, not a Roslyn source generator) turns a typo'd/renamed token key into a build error instead of a runtime throw, and bakes literal Thickness values for the few spots where Avalonia's compiled-XAML x:Static limitation previously forced a hand-duplicated literal. Add a reusable fwGroupBox titled-border primitive (the WinForms GroupBox analog) and apply it to the Options dialog, whose General/ Updates tabs previously applied one uniform spacing value to every sibling alike -- an unrelated setting boundary read identically to a label-to-its-field gap. Harden DialogLayoutAssert.AssertNoCrowding with two general checks that run automatically on every dialog: a readable-font-size floor, and a minimum gap between fwGroupBox siblings. Commit a small, curated set of baseline screenshots (Docs/migration/baseline-screenshots/) so a "this was reviewed and looks right" claim has a surviving, checkable artifact instead of living only in an ephemeral, gitignored capture. Record the load-bearing decisions in docs/adr/0001-0003: aliasing Semi's semantic tier by default, the whole-tree/no-grandfathering hygiene gate scoped to the Avalonia surface only, and geometric layout assertions plus reviewed screenshots instead of automated pixel-diff visual regression testing. Also upgrades L10NSharp 10.0.0-beta0004 to beta0014, a prerequisite for future UI-language work that was investigated and explicitly not built this branch: every UI-language-change path in FieldWorks, WinForms and Avalonia alike, already deliberately requires a restart rather than live-refreshing, so building live switching would be new engineering inconsistent with the rest of the app, not a gap this branch needed to close. Independently rebuilt, retested, and hygiene-checked after every commit throughout development, not just trusted from agent self-reports -- caught and fixed a recurring CRLF/LF corruption bug, an unauthorized subagent-forking-a-subagent race condition, a hygiene gate that silently scanned zero files off-root, a test that didn't test what it claimed to, a squash whose commit boundaries didn't match its own messages, and (via independent adversarial review) a hygiene gate that enforced a narrower slice than its commit message claimed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Found during PR review-summary alignment: the doc still described a now-removed architecture (three independent DialogFontSize copies that "must stay equal") and the pre-token-system value (12px). The actual, current state: one source (FwSurfaceFontSize in FwColorTokens.axaml, value 11), resolved by all three consumers directly, not hand-kept-equal. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Pin RequestedThemeVariant to Light in FwAvaloniaApp and PreviewHostApp. Left unset, ActualThemeVariant follows the OS app theme, so a machine in dark mode resolved FwColorTokens.axaml's Dark dictionary -- a complete palette, so it resolved silently rather than failing, and it is a first-pass placeholder rather than design-approved. The test asserts RequestedThemeVariant, because headless reports ActualThemeVariant as Light either way and a test written the obvious way passes against an unpinned app. Stop the token check from being able to skip its own scan. The FW_TOKEN_HYGIENE_REPORTED marker now suppresses duplicate annotations only, never the scan or the exit code. Previously any second invocation in a job exited 0 before scanning, which made enforcement depend on which CI step ran first. Close the ways the scanner could be defeated. An x:Key exempts a line only when it declares a primitive resource, so a layout element carrying a key is still scanned. A same-line comment no longer excuses the markup beside it; only lines wholly inside a comment are skipped. Thickness values separated by spaces are recognised, grid definitions are checked for literal lengths, and the C# assignment pattern covers Opacity, Margin, Padding, BorderThickness and CornerRadius, plus casts and numeric suffixes. Twelve fixture tests cover the shapes that previously passed clean. Correct five documentation claims that described behaviour the code does not have: CI annotating rather than failing, in build.ps1 twice and test.ps1 twice, and the comment-masking granularity in TokenHygiene.psm1. ADR 0001 claimed the check requires a justification comment beside every token literal; it requires none. ADR 0002 called the scope whole-tree without saying that means 170 files of roughly 2,300 under Src. Keep the writing-system gutter clickable. The form spans the splitter column, so its label column has to cover that column too: otherwise the value area starts underneath the splitter, which sits on top and swallowed right-clicks on the first pixels of every row -- including the whole abbreviation gutter, which starts exactly there.
41e46a1 to
ebb39be
Compare
|
Thank you — this is the review I most wanted on this branch, and running the 1. Theme variant — pinnedConfirmed: a whole-branch search returns the single hit you found, and nothing Your warning about the test was worth more than the fix. 2a. The skip — right diagnosis, but I did not apply your fixYou are right that the mechanism can stop checking: the early I did not reorder the steps, because doing that would create the hole rather So the marker now suppresses duplicate annotations only, never the scan and 2b. Five documentation claims — all confirmed false, all corrected
2c. ADR 0001 — correctedConfirmed: 2d. The evasions — closed, with your fixtures as testsAll eight passed clean; each is now a test.
One correction to the item: I did not attempt The scanner is still clean over all 170 files after the narrowing, so none of 2e. Real scope — stated170 files of roughly 2,300 The rebase turned up a real regressionThe branch was conflicting; it is now rebased onto main and That surfaced a genuine defect, and it is worth your attention more than
Fixed by having the form's label column span the splitter column as well. I got This also touches item 7 from the other direction: the label-column geometry has VerificationDebug build clean, comment-hygiene clean, token-hygiene clean over 170 files. Still openItems 3, 4, 5, 6, 7, 8, 9, the five questions and the comments pass are |
Move one-use tokens next to the views that use them. 29 of DialogTheme's 47 keys had exactly one consumer, no C# use, and no use by the file's own styles, so each now lives in that view's own Resources. Every root-element reference was already DynamicResource and every body reference StaticResource, so both resolve with the Resources block as the root's first child. The rule is written down in the shared dictionary's header: one view means the view, two or more views or any C# use means shared. DialogLabelFieldGapAbove had no consumer at all and is gone. Fix the layering inversion. FwAvalonia generated its public key constants from a file inside FwAvaloniaDialogs, its own dependent, so a Dialogs-only edit could fail the foundation's compile. The 17 genuinely shared tokens moved to FwAvaloniaTheme/Tokens/DialogTokens.axaml, which both projects already depend on, and both apps now merge it so the published constants match keys that are actually registered. Moving CompactDialogStyles instead was not possible: AvaloniaDialogHost and FwSurfaceStyles both consume it from the foundation. Re-derive the label colours instead of restating the claim. Measured from the committed DataTreeRender_multiws baseline: #696969 is the only ink in the label column (x 23-162) and #404040 the ink in the writing-system gutter (x 178-194), so both branch values are the measured ones and main's #6666B8 / #4682B4 appear nowhere in it. The comments now record the method and the pixel ranges. ValidationErrorBrush gets the justification it lacked: Firebrick #B22222 appears in none of the 17 committed baselines, so unlike the label colours there is no measured legacy value to preserve. Cap label wrapping from the live column, not the token. The cap came from LabelColumnWidth while the column came from getLabelColumnWidth, so dragging the splitter narrower left labels wrapping at the token width; the cap now tracks the column and is re-applied on a drag. Pin the Ursa workarounds and the locale lists. Four characterization tests assert the resolved alignment, margin, font weight and label width rather than the constants fed in, so an Ursa upgrade fails loudly. FwSemiLocale had no coverage: its lists are now compared against the locales actually shipped in Semi.Avalonia and Ursa.Themes.Semi, read from the assemblies. Both lists are correct as transcribed; the tests keep them that way. FwSemiLocale's own summary now says it is an interim measure, not the destination. Housekeeping. Drop the stranded FluentTheme references from five projects and its version pin, since no code referenced it. Remove PrivateAssets from Semi/Ursa in FwAvalonia, whose own controls need Ursa at runtime. Give Detail-07-wide the assertion it lacked. Rename "gate" to "check" in the new text and the ADR filename. Move the ADRs under Docs/architecture/adr with a README saying what belongs there, rather than adding a fourth top-level docs location. Replace ADR 0002's appeal to unnamed literature with the argument from this repository's own circumstances. Record the token key generator's source paths repo-relative so the generated file no longer differs between machines. Comments. Remove the precondition sentence repeated across FwAvaloniaDensity's members; it is stated once on the type. Delete DialogThemeBootstrap's comment explaining why it adds no style, followed by adding no style. Give FwThemeResources the exception tag its whole design rests on and drop the consumer instructions. Un-orphan the rewrapped comments this branch left breaking mid-phrase, and record why the radio glyph ratio is 0.45 rather than Semi's own 0.375.
|
Items 3, 4, 6, 7, 8, 9, the five questions and the comments pass are now in 3. One-use tokens — moved, and there were more than sixYou named six; measuring every key found 29 with exactly one consumer, no Your caution about the two on root elements was worth having, and the answer is The rule is written into the shared dictionary's header: one view means the One extra find: 4. The values, and the colours re-derivedThe body now lists all ten moved or new values with a reason each. On the
So this branch's comment was the true one and
6. Layering inversion — fixed, but not the way you offeredConfirmed: Your second option was not available: I also merged the new dictionary in both apps, which closes the second half of The 7.
|
|
Filed the deferred items rather than leaving them in a PR comment: new epic From this PR:
Two of your questions are decisions rather than deferred work, so they have no
|
3 participants
Replaces Avalonia's Fluent theme with Semi.Avalonia + Ursa and gives the Avalonia UI a real design-token system in place of colors/spacing scattered as literals — enforced by a new CI gate that fails the build on any hardcoded value in that surface. The Avalonia UI stays behind
FW_AVALONIA, opt-in only; nothing here changes default FieldWorks behavior.88 files is a lot for something with zero default-on behavior change — the honest reason: rebuilding the DataTree detail view on Ursa's layout primitive, and the token system it depends on, touch nearly everything already converted to Avalonia (9 dialogs + the detail-view foundation). The part actually worth scrutinizing is narrower than the diff: the token architecture and its enforcement gate. Most per-dialog changes are 1:1 literal→token swaps.
Where to look:
token-hygiene.ps1is now a hard CI failure — no grandfathering, across the whole scoped Avalonia surface (170 files of roughly 2,300 under Src), scoped to the Avalonia surface only. Why that scope, not global: docs/adr/0002.fwGroupBox(new titled-border primitive) is the largest deliberate visual change — applied only to the Options dialog, which had an actual reported defect; the other 8 were reviewed and left alone.LabelColumnWidthDataTree.cs m_sliceSplitPositionBase = 150); 96 was the narrower Avalonia-only guessWsAbbrevWidthSlice.MaxAbbrevWidthcap, so a long abbreviation is not clipped at the old 28WsAbbrevMaxWidthFieldSpacingLabelBrush#6666B8#696969WsAbbrevBrush#4682B4#404040ValidationErrorBrush#B22222SemiColorDanger#F93920PickerForegroundBrush#1A1A1ASemiColorText0#1C1F23HotlinkBrush#0066CC(local literal)SemiColorLink#0064FADisabledOptionBrush#808080Colour provenance, re-derived rather than restated. Review flagged that
mainand this branch cited the same source for colours nowhere near each other, so the evidence was circular and one half had to be false. Measured from the committedDataTreeRenderTests.DataTreeRender_multiws.verified.png:#696969is the only ink in the label column (x 23–162) — this branch'sLabelBrush.#404040is the ink in the writing-system gutter (x 178–194) — this branch'sWsAbbrevBrush.#6666B8and#4682B4,main's values, appear nowhere in that baseline.So this branch's "measured from the legacy baseline" comment is true and
main's was the false one. The token comments now carry the method and the pixel ranges, not just the claim.ValidationErrorBrushis the one colour with no measured legacy value at all: Firebrick#B22222appears in none of the 17 committed baseline images, so it was chosen rather than sampled. That is why this branch does not preserve it while it does preserve the label colours — the apparent inconsistency review spotted, explained.GenerateTokenKeys(newBuild/Src/FwBuildTaskscodegen) turns a stale/typo'd token key into a compile error — what makes "whole-tree, no exceptions" trustworthy rather than just strict-sounding.DialogLayoutAssertgained 2 general checks (readable-font floor, group-box minimum gap) that run automatically on every dialog test already in the suite, not just new ones.Deliberately not here: Dark/Compact/color-blind theming — the
ThemeDictionariesstructure supports all three later, none is built or visually verified now; Light is the only reviewed variant. No automated pixel-diff visual regression (why: docs/adr/0003) — verification is geometric assertions plus 9 committed baseline screenshots (Docs/migration/baseline-screenshots/). L10NSharp is a version bump only; live UI-language switching was investigated and explicitly not built (every language-change path in FieldWorks, old and new UI alike, already requires a restart).Verification: Not stacked. Build: 0 errors.
FwAvaloniaTests: 647 passed, 1 skipped (pre-existing).FwAvaloniaDialogsTests: 288 passed. Both hygiene gates clean. No native or installer files touched.Reading this a year from now -- start here
This PR's working history (grilling sessions, adversarial reviews, live corrections) lived
in a long agent conversation, not in tree files — there was nothing to evict from the repo
because none of it was ever committed as scratch docs. What follows synthesizes that
conversation's decisions and evidence directly into this record.
The three ADRs (
docs/adr/0001-0003) are the durable record of why; this section coversthe how it went, including the mistakes caught along the way.
The layer cake — token resolution, end to end
A view (C# in
FwAvalonia/FwAvaloniaDialogs, or.axamlin the dialogs project) asks fora value one of two ways:
{DynamicResource FwLabelBrush}or, for Semi's own roles now referenceddirectly,
{StaticResource SemiColorDanger}.FwThemeResources.RequireBrush(GeneratedTokenKeys.FwLabelBrush)— acompile-time-checked constant, not a raw string, resolved at point-of-use via
Application.Current.TryGetResource, never cached in a static field (Application.Currentis null under
beforefieldinitbefore the app starts).The key resolves through merged
Application.Resources:Src/Common/FwAvaloniaTheme'sFwColorTokens.axaml(Light/DarkThemeDictionaries— shared brushes + the oneFwSurfaceFontSize) andDataTreeTokens.axaml(flat, non-themed DataTree layoutdimensions), both merged by
FwAvaloniaApp/PreviewHostAppatInitialize(), plusDialogTheme.axaml's own localDialog*keys merged into that same dictionary and appliedper-dialog-body via
DialogThemeBootstrap.Apply.Underneath FieldWorks' tier sits Semi.Avalonia's own two-tier system: ~449 raw
color-ramp/spacing primitives (Layer 1, no meaning attached) and named semantic roles
(Layer 2:
SemiColorText0-3,SemiColorBorder,SemiColorBackground0-4, a flatspacing/radius/height scale) that alias them. FieldWorks' tier defaults to aliasing Layer 2
directly; a FieldWorks-owned value requires a written, checkable reason.
GenerateTokenKeys(aBuild/Src/FwBuildTasksMSBuild Task, not a Roslyn generator — seeDecisions below) reads the token
.axamlfiles'x:Keys at build time and emitsGeneratedTokenKeys.g.cs: the compile-time-checked constants above, plus bakedThicknessliteral values (via
Avalonia.Thickness.Parse) for the few spots(
CompactDialogStyles.cs/FwSurfaceStyles.cs) where Avalonia's compiled XAML rejectsx:Static, so a C# style builder can't read a token via{StaticResource}at all.Decisions, and why
Alias Semi's semantic tier by default, not an independent FieldWorks palette. Semi
already ships primitive→semantic aliasing (the pattern every mature design system — Fluent
2, Carbon, Atlassian, Adobe Spectrum — uses); FieldWorks previously ignored it and picked
every color independently by eye from old WinForms screenshots. Verified empirically that
{StaticResource}reaches Semi's own Layer-2 keys fine from ordinary view XAML — the knownDynamicResource-only landmine on this branch is narrower than first assumed: it's specificto
DialogTheme.axaml's own Setters (grafted onto a view's.Stylesat runtime), notThemeDictionariescrossing in general.token-hygiene.ps1is whole-tree and zero-grandfathering, deliberately unlikecomment-hygiene.ps1. The scoped tree is new code with nothing to grandfather; currentdesign-token practice treats that as the correct case for full-strictness-from-day-one, the
same literature is equally clear it's the wrong call for retrofitting legacy code — which is
why the WinForms surface stays out of scope. Consequence accepted deliberately: since only
agents are required to run
-TokenHygienelocally, one slipped-in violation onmainfailsevery unrelated PR touching the tree until fixed — no ratchet/baseline valve exists yet.
GenerateTokenKeysis a custom MSBuild Task, not a Roslyn source generator. Matches thiscodebase's own precedent for "generate typed C# from a declarative source" — liblcm's
LcmGenerate— rather than introducing tooling nobody on this codebase has used yet.Geometric layout assertions + reviewed screenshots, not automated pixel-diff. Real
current tooling for visual regression (Percy/Chromatic/Playwright) is a web/DOM-native
ecosystem with no mature managed equivalent for Avalonia/WPF; even mature web tooling needed
a dedicated AI-review layer to suppress anti-aliasing/font/DPI noise. A small, curated,
committed baseline set exists specifically so an "I looked, it's fine" claim survives past
the run that made it — previously all snapshots were ephemeral and gitignored.
Paths not taken
prerequisite for it. Investigated directly: every UI-language-change path in FieldWorks —
WinForms and the existing Avalonia port alike — deliberately sets
restartRequired = truerather than live-refreshing. Building live switching would be new, unrequested engineering
inconsistent with the rest of the app, not a gap this branch needed to close.
discussing the gate's escape valve. Rejected once the actual existing exception (the
x:Staticcompiled-XAML limitation) turned out to already be resolved better byGenerateTokenKeysgenerating the value outright, backed byDuplicateTokenPairConsistencyTests.csas a regression guard — stronger than an unverifiedsuppression comment would have been.
DialogLayoutAssert,rejected as too broad (would false-positive on deliberately tight pairs like a label over
its field) in favor of a rule scoped specifically to
fwGroupBoxsiblings.What this does NOT authorize
token-hygiene.ps1-style enforcement onto the WinFormssurface — that surface is out of scope by design (docs/adr/0002).
Not a decision that Dark/Compact/color-blind theming is "done" — only that the
ThemeDictionariesstructure won't need a rearchitecture to add them later; none has beendesigned, built, or visually verified.
pixel-for-pixel against Semi's real composited output — the 8 keys flagged in review were
checked directly; the remaining KEEP-AS-NEW dimension tokens were not individually
re-derived from Semi's spacing scale where no exact match existed.
Surprising findings
were factually backwards:
SemiColorBorderwas described as "opaque, too heavy" for a 1pxdivider when the real value (verified against the pinned
v11.3.14tag's source) is8%-opacity and nearly invisible; Semi's
Text0-3were described as "resolving identically"when they're four distinct brushes with different baked opacities (
0.8/0.62/0.35).Re-evaluating with the corrected facts didn't change any of the 8 affected keys' actual
values — every one turned out to already be independently grounded in real legacy-WinForms
pixel measurements — but the written reasoning in both
FwColorTokens.axamlanddocs/adr/0001was wrong until corrected during review.git commit-treeratherthan an interactive rebase) initially produced a commit whose message described work
actually contained in a different commit, because of a chronological mis-sequencing.
Caught by an adversarial review that diffed each commit's actual content against its
claimed content rather than trusting the message — re-sequenced and re-verified
byte-identical to the pre-squash tree before proceeding.
Evidence
.\build.ps1 -BuildTests -SkipNative -CommentHygiene -TokenHygiene— 0 errors, bothhygiene gates clean (170 files scanned by
token-hygiene.ps1, 0 violations).FwAvaloniaTests647 passed / 1 skipped (pre-existing, unrelated);FwAvaloniaDialogsTests288 passed / 0 failed (284 baseline + 4 new fixture tests for thehardened
DialogLayoutAssertchecks).gitlint --commits origin/main..HEADclean.SemiColorBorder,SemiColorText0-3, thespacing/radius/height scale) were confirmed against the pinned
11.3.14tag's real source(
src/Semi.Avalonia/Tokens/Palette/Light.axaml) and, separately, a live headless resourcewalk under this repo's own
TestAppBuilder/FwAvaloniaApp— not assumed fromdocumentation or an earlier/different vendor version.
DialogLayoutAssertchecks are mutation-tested: a fixture with the real defectpresent (unreadable font size; a
Margin="0"override defeating the themed groupseparation) fails, and the compliant case passes, for both.
git diff --name-only origin/main...HEAD, entirely managed C#/XAML/PowerShell/docs.This change is