Skip to content

Apply Steel gauge subType, Digital Meter and decimals edits immediately - #573

Merged
mairas merged 3 commits into
mainfrom
fix/gauge-steel-option-changes
Aug 13, 2026
Merged

Apply Steel gauge subType, Digital Meter and decimals edits immediately#573
mairas merged 3 commits into
mainfrom
fix/gauge-steel-option-changes

Conversation

@mairas

@mairas mairas commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Why

GaugeSteelComponent.ngOnChanges enumerated value, zones, title, backgroundColor, frameColor, radialSize, units, minValue and maxValue. Three inputs had no branch at all — subType, barGauge (the Digital Meter setting) and decimals — and the steelseries library reads all three only while constructing a gauge. Editing any of them in widget options produced no visible effect, and then applied out of nowhere later when a window resize or the server's unit metadata happened to rebuild the face.

What

All three join the structural-rebuild condition. buildOptions/startGauge already re-read every input, so no new state was needed.

The batch now runs one rebuild instead of up to three. Previously zones, radialSize and units/min/max each called startGauge(true) in sequence, and the title/backgroundColor/frameColor setters ran in between — targeting whichever gauge object existed at that point, which a later rebuild in the same batch then discarded. Since buildOptions re-reads the title, background and frame from the inputs, the rebuild carries them; the setters now run only on a batch with no structural change in it.

Tests

Four added to gauge-steel.component.spec.ts — one per newly-honoured input (asserting the replacement is constructed as Linear, as LinearBargraph, and with the new lcdDecimals), plus one locking the batched-title behaviour above. The three input tests fail on main.

Fixes #558

@mairas

mairas commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Code review — testing persona, findings applied

The reviewer mutation-tested this branch in a scratch copy rather than reading it. Two of the PR's own claims turned out to be unpinned:

A regression to a rebuild per changed key passed all 14 tests. Every test batched exactly one structural key, so the central claim — one rebuild per batch, not up to three — had nothing asserting it. The symptom of a repeat rebuild is the one this file's own comment describes: the library cannot cancel a tween, so the discarded gauge keeps repainting the canvas with its stale scale and wins the last frame.

Deleting all three live setters outright passed all 14 tests. The PR changed when setTitleString / setBackgroundColor / setFrameDesign run — only on a batch with no structural change — and the only assertion about them was negative (not.toHaveBeenCalled() during a rebuild). Nothing checked they still fire on a standalone change, and nothing checked buildOptions carries the background and frame through a rebuild.

Added

  • A batch of three structural changes (maxValue + decimals + zones) asserting exactly one library construction.
  • A standalone title, background and frame change reaching their setters, with no rebuild.
  • Background and frame batched with a structural change: carried into gaugeOptions, setters not called.

Suite: 2036 passing.

Not applied

No parent-level test proving Angular delivers these changes through the host template, which renames two of them ([barGauge]="…digitalMeter", [decimals]="…numDecimal"). The reviewer is right that a dropped binding there would make this a no-op with the specs green. It needs a WidgetSteelGaugeComponent harness that does not exist yet, and the binding names are unchanged by this PR — noting it rather than growing the change.

setValueAnimated suppression on a structural batch is already pinned, by the pre-existing does not animate a gauge that a rebuild in the same batch is about to discard. The new structural keys share the one rebuilding boolean, so they cannot diverge from the maxValue case without an explicit code change. No test needed.

@mairas
mairas force-pushed the fix/gauge-steel-option-changes branch from 9384649 to c67a773 Compare August 12, 2026 21:41
@mairas

mairas commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Code review — 4 personas, findings applied

Reviewers: correctness, testing, maintainability, project-standards. Fresh contexts, isolated worktree.

Making subType a rebuild trigger exposed stale geometry

Two reviewers found it; one captured the constructor's actual options. onResized writes size when subType is radial and width/height otherwise, into a gaugeOptions object that is never cleaned. A gauge that has only been radial therefore has size and no width/height, and steelseries Linear ignores size and falls back to the canvas element's dimensions — which the outgoing Radial had set square. Probe output on a 400×150 tile:

LINEAR CTOR OPTIONS: [{…,"size":150}]   // no width, no height

So the linear gauge rendered 150×150 in a 400×150 tile. It cannot self-correct: lastSizeSignature is still radial:150, and the canvas sits position: absolute inside a 100%/100% wrapper, so changing canvas dimensions cannot trigger the wrapper's ResizeObserver. Since the default steel config is radial, this is the common direction of the flip the PR enables.

Fixed by deriving the geometry keys at build time from the last observed rect and the current subType, dropping the other class's keys. Mutation-checked: removing the re-derive fails the new test with expected undefined to be 400.

The rebuild list is inverted

Maintainability's argument: a hand-maintained list of what needs a rebuild makes "silently ignored" the default for every input added later — which is exactly #558. It is also already wrong: theme and widgetUUID were in neither the rebuild list nor the setter branches.

theme is not hypothetical. Zone section colours are resolved from it inside buildOptions and baked into the Section objects at construction; Host2 binds it to a child it creates once, and AppService swaps CSS variables and pushes a new ITheme without re-creating widgets. So a day/night switch left the alarm bands in the previous theme's colours until something unrelated rebuilt the face.

The predicate now lists the four inputs steelseries exposes a live setter for and rebuilds on everything else. Same line count; a new input costs a needless rebuild at worst.

Corrected

The new comment justified the decimals rebuild with "decimals reaches the LCD through buildOptions" — construction-only. The bundled library defines setLcdDecimals(n) on all four LCD classes, including the three Skip constructs. Behaviour was right either way, but the stated reason was false; the comment now says decimals is captured into a closure at construction, which is what the library actually does.

The CHANGELOG bolded Gauge type and Digital Meter; the widget options panel labels those controls "Gauge Type" and "Digital display", and the decimals control "Decimal Places". barGauge/digitalMeter is internal naming that leaked into release notes. Fixed, and the entry now covers the theme and geometry fixes.

Coverage, honestly

steelseries is a global script with only its enum constants shimmed, so each test installs its own constructor spy. The assertions therefore reach two surfaces: which constructor was called and how many times, and the contents of the private gaugeOptions at that moment. No canvas is drawn. A reviewer checked the real library rather than the shim and confirmed Linear, LinearBargraph and Radial are separate constructors with no live type switch, and that titleString, backgroundColor, frameDesign, lcdDecimals and valueColor are all read at construction — so "a rebuild is the only remedy" and "a rebuild re-applies the setters' values" both hold against the real library. Reverting the production change fails 6 of the now-19 tests.

Not actioned

pendingStructuralRebuild is dead state — both call sites pass forceRebuild: true, so it can never change an outcome, and the if (this.gauge && !structuralChange) block it guards is unreachable, including an if with an empty body. Pre-existing; removing it is a cleanup, not a fix.

Turning Digital display off leaves valueColor: GREEN in the shared options object, so the plain Linear bar stays green instead of the library default red. buildOptions has no else branch and the object never has keys removed. Pre-existing, but this PR does put it on a path a user now reaches with two clicks — worth its own issue if you want it tracked.

Two spec-quality points stand: the batched-carry tests assert the shared mutable gaugeOptions after ngOnChanges returns, so they would hold even if the constructor ignored the option; and every test hand-builds its SimpleChanges keys, pinning the branch body but not whether Angular delivers a record under that key.

Version reasoning confirmed: latest stable is v1.3.0+2, VERSION is already 1.4.0, cycle open, patch-level fix, no re-bump.

Gate on the head commit: 2045 tests pass, snc clean, lint clean.

mairas added 3 commits August 13, 2026 10:55
The library reads all three only while constructing a gauge, and
ngOnChanges enumerated neither, so editing Gauge type, Digital Meter or
the decimal places did nothing until an unrelated rebuild happened to
pick them up. Fold them into the structural-rebuild condition, which now
runs one rebuild per batch instead of up to three.

Fixes #558
A reviewer mutation-tested the branch: reverting to a rebuild per changed
key, and deleting all three live setters outright, each kept the suite
green. Both are behaviours this change introduced, so both now have tests
-- a batch of several structural changes asserting one construction, the
setters on a standalone change, and the background and frame carried
through a rebuild without their setters running.

Fixes #558
…e flip

Making subType a rebuild trigger exposed stale geometry. The radial writes
`size`; the linear pair writes `width`/`height` and, absent those, falls back to
the canvas element's dimensions — which the outgoing radial had set square. A
radial-to-linear flip therefore drew the new face at the tile's shorter side,
and no resize follows the flip to correct it.

The rebuild list is now inverted to the inputs the library exposes a live setter
for. A hand-maintained list of what needs a rebuild fails silent, which is the
bug it was written to fix; this way a new input costs a needless rebuild at
worst. It also picks up `theme`, whose zone-band colours were baked in at
construction and kept the previous theme's palette until something else rebuilt.
@mairas
mairas force-pushed the fix/gauge-steel-option-changes branch from ea3639f to 96e97f6 Compare August 13, 2026 07:56
@mairas
mairas merged commit 74846bb into main Aug 13, 2026
4 checks passed
@mairas
mairas deleted the fix/gauge-steel-option-changes branch August 13, 2026 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gauge-steel drops subType, digitalMeter and decimals changes entirely

1 participant