diff --git a/CLAUDE.md b/CLAUDE.md index 6d056d86..b39fdf29 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -76,7 +76,7 @@ The mock serves Skip's full session/config surface (`loginStatus`, `applicationD **Runtime data pipeline** (`src/app/core/services/`): `SignalKConnectionService` (endpoint discovery) → `ConnectionStateMachine` (explicit connection lifecycle; registers callbacks so it carries no upward deps) → `SignalKDeltaService` (parses SK delta messages) → `DataService` (central hub mapping deltas to per-path observables; every value is written to both a `default` bucket and a per-`$source` bucket) → widgets. The DI graph is intentionally acyclic: `connection ← auth ← storage ← settings`, `data ← delta ← connection`. -**Widgets** (`src/app/widgets/`, ~46) are standalone components composed with three **host directives** that own runtime concerns: `WidgetRuntimeDirective` (config merge), `WidgetStreamsDirective` (diff-based path subscriptions), `WidgetMetadataDirective` (zones/meta). `WidgetService` is the registry — `kipWidgets` is a getter over the `_widgetDefinition` array; the full electrical family (bms, solar-charger, charger, alternator, inverter, ac) is registered and live. +**Widgets** (`src/app/widgets/`, ~46) are standalone components composed with three **host directives** that own runtime concerns: `WidgetRuntimeDirective` (config merge), `WidgetStreamsDirective` (diff-based path subscriptions), `WidgetMetadataDirective` (zones/meta). A widget that holds stream-derived presentation state owns clearing it: `WidgetStreamsDirective` rebuilds the subscription on a re-point, but `suppressBootstrapNull: true` filters the replayed leading null, so against a path that reports nothing the callback never runs and the previous path's reading stays on screen as a live reading of the new one. Compare `widgetPathSignature()` across effect runs and clear on a change — the three ng-gauges do; the other `streams.observe` callers do not yet (#585). `WidgetService` is the registry — `kipWidgets` is a getter over the `_widgetDefinition` array; the full electrical family (bms, solar-charger, charger, alternator, inverter, ac) is registered and live. **Config & persistence**: `SettingsService` holds in-memory config plus sync getters and observable getters. `StorageService` persists through the SK server's **applicationData REST API**, which has exactly two scopes — `user` (the authenticated user's private store) and `global` (a single shared bucket). All writes go through a **sequential JSON-Patch queue** because SK can't handle concurrent applicationData writes. `ConfigurationUpgradeService` migrates older config file versions (preserve the stored version on write). diff --git a/src/app/core/directives/widget-streams.directive.spec.ts b/src/app/core/directives/widget-streams.directive.spec.ts index 3d8cf85d..5c8fe26c 100644 --- a/src/app/core/directives/widget-streams.directive.spec.ts +++ b/src/app/core/directives/widget-streams.directive.spec.ts @@ -1,7 +1,7 @@ import { TestBed } from '@angular/core/testing'; import { beforeEach, afterEach, describe, expect, it, vi } from 'vitest'; import { Subject, BehaviorSubject, Observable } from 'rxjs'; -import { WidgetStreamsDirective } from './widget-streams.directive'; +import { WidgetStreamsDirective, widgetPathSignature, normalizeWidgetPath } from './widget-streams.directive'; import { DataService, IPathUpdate } from '../services/data.service'; import { UnitsService } from '../services/units.service'; import { IWidgetSvcConfig, IWidgetPath } from '../interfaces/widgets-interface'; @@ -901,3 +901,51 @@ describe('WidgetStreamsDirective TTL value reset (#1069)', () => { expect(hits[hits.length - 1]).toBeNull(); }); }); + +/** + * The identity a widget compares to tell a re-point apart from an unrelated reconfigure. The + * directive computes it to decide whether to rebuild a subscription; the gauges compute it to decide + * whether the reading on screen still describes the path being watched. Both must agree, which is + * why it is one exported function rather than two implementations. + */ +describe('widgetPathSignature', () => { + const base = { path: 'navigation.speedOverGround', pathType: 'number', convertUnitTo: 'knots', source: null, suppressBootstrapNull: true }; + + it('returns null for a config with no usable path', () => { + expect(widgetPathSignature(undefined)).toBeNull(); + expect(widgetPathSignature(null)).toBeNull(); + expect(widgetPathSignature({ ...base, path: null })).toBeNull(); + expect(widgetPathSignature({ ...base, path: '' })).toBeNull(); + // Whitespace passes the widget-options required check, so it has to normalize to "no path" + // here rather than becoming an identity of its own. + expect(widgetPathSignature({ ...base, path: ' ' })).toBeNull(); + }); + + it('treats a trimmed path and its padded form as the same reading', () => { + expect(widgetPathSignature({ ...base, path: ' navigation.speedOverGround ' })) + .toBe(widgetPathSignature(base)); + }); + + it('treats an unset source and the default source as the same reading', () => { + expect(widgetPathSignature({ ...base, source: null })).toBe(widgetPathSignature({ ...base, source: 'default' })); + expect(widgetPathSignature({ ...base, source: ' ' })).toBe(widgetPathSignature({ ...base, source: 'default' })); + }); + + it('separates readings that differ in path, source, type, unit or bootstrap-null policy', () => { + const sig = widgetPathSignature(base); + expect(widgetPathSignature({ ...base, path: 'navigation.speedThroughWater' })).not.toBe(sig); + expect(widgetPathSignature({ ...base, source: 'gps-2' })).not.toBe(sig); + expect(widgetPathSignature({ ...base, pathType: 'string' })).not.toBe(sig); + // A unit change re-expresses the number, so the displayed reading is no longer the same one. + expect(widgetPathSignature({ ...base, convertUnitTo: 'kph' })).not.toBe(sig); + expect(widgetPathSignature({ ...base, suppressBootstrapNull: false })).not.toBe(sig); + }); + + it('normalizeWidgetPath yields undefined for anything that is not a usable path', () => { + expect(normalizeWidgetPath(' a.b ')).toBe('a.b'); + expect(normalizeWidgetPath('')).toBeUndefined(); + expect(normalizeWidgetPath(' ')).toBeUndefined(); + expect(normalizeWidgetPath(null)).toBeUndefined(); + expect(normalizeWidgetPath(42)).toBeUndefined(); + }); +}); diff --git a/src/app/core/directives/widget-streams.directive.ts b/src/app/core/directives/widget-streams.directive.ts index 8635e94d..80c3bab4 100644 --- a/src/app/core/directives/widget-streams.directive.ts +++ b/src/app/core/directives/widget-streams.directive.ts @@ -8,6 +8,38 @@ import { takeUntilDestroyed } from '@angular/core/rxjs-interop'; /** Fixed stale-data TTL (ms) applied to every widget whose enableTimeout is on; not user-configurable. */ const FIXED_DATA_TIMEOUT_MS = 5000; +/** The subset of a path config that decides which reading a subscription delivers. */ +interface IPathIdentity { + path: string | null; + pathType?: string | null; + convertUnitTo?: string | null; + source?: string | null; + suppressBootstrapNull?: boolean; +} + +/** Trim a configured path to its canonical form; undefined when it is not a usable path. */ +export function normalizeWidgetPath(path: unknown): string | undefined { + const trimmed = typeof path === 'string' ? path.trim() : ''; + return trimmed.length ? trimmed : undefined; +} + +/** + * Identity of a configured path as the subscription diff computes it. Two configs that share a + * signature reuse one subscription; a different signature means the widget is now watching another + * reading. A widget that holds presentation state derived from the stream — a needle position, a + * last value, an alarm colour — needs this to tell a re-point apart from an unrelated reconfigure + * such as a theme change, because a rebuilt subscription may replay nothing at all and leave that + * state showing the previous path. + * + * Returns null for a config with no usable path, which has no identity to compare. + */ +export function widgetPathSignature(pathCfg: IPathIdentity | undefined | null): string | null { + const normalizedPath = normalizeWidgetPath(pathCfg?.path); + if (!pathCfg || !normalizedPath) return null; + const src = (pathCfg.source?.trim() || 'default'); + return [normalizedPath, pathCfg.pathType, pathCfg.convertUnitTo, src, pathCfg.suppressBootstrapNull ? '1' : '0'].join('|'); +} + @Directive({ selector: '[widget-streams]', exportAs: 'widgetStreams' @@ -63,9 +95,7 @@ export class WidgetStreamsDirective implements OnDestroy { } private computePathSignature(pathCfg: { path: string; pathType: string; convertUnitTo?: string; source?: string; suppressBootstrapNull?: boolean }): string { - const normalizedPath = this.normalizePath(pathCfg.path) ?? ''; - const src = (pathCfg.source?.trim() || 'default'); - return [normalizedPath, pathCfg.pathType, pathCfg.convertUnitTo, src, pathCfg.suppressBootstrapNull ? '1' : '0'].join('|'); + return widgetPathSignature(pathCfg) ?? ''; } private computeBaseKey(path: string, source?: string): string { @@ -75,9 +105,7 @@ export class WidgetStreamsDirective implements OnDestroy { } private normalizePath(path: unknown): string | undefined { - if (typeof path !== 'string') return undefined; - const trimmed = path.trim(); - return trimmed.length ? trimmed : undefined; + return normalizeWidgetPath(path); } private computeRootSignature(cfg: IWidgetSvcConfig | undefined): string { diff --git a/src/app/widgets/widget-gauge-ng-compass/widget-gauge-ng-compass.component.spec.ts b/src/app/widgets/widget-gauge-ng-compass/widget-gauge-ng-compass.component.spec.ts index 8dbb8b8e..9c69a756 100644 --- a/src/app/widgets/widget-gauge-ng-compass/widget-gauge-ng-compass.component.spec.ts +++ b/src/app/widgets/widget-gauge-ng-compass/widget-gauge-ng-compass.component.spec.ts @@ -1,4 +1,4 @@ -import { signal } from '@angular/core'; +import { WritableSignal, signal } from '@angular/core'; import { ComponentFixture, TestBed } from '@angular/core/testing'; import { beforeEach, describe, expect, it } from 'vitest'; import { WidgetGaugeNgCompassComponent } from './widget-gauge-ng-compass.component'; @@ -7,6 +7,7 @@ import { WidgetStreamsDirective } from '../../core/directives/widget-streams.dir import { UnitsService } from '../../core/services/units.service'; import { IPathUpdate } from '../../core/services/data.service'; import { IWidgetSvcConfig, IPathArray } from '../../core/interfaces/widgets-interface'; +import { States } from '../../core/interfaces/signalk-interfaces'; /** * A compass with no heading must still show its rose, with no needle and a '--' readout — a needle @@ -20,21 +21,25 @@ describe('WidgetGaugeNgCompassComponent no-data state', () => { let fixture: ComponentFixture; let internals: CompassInternals; let capturedNext: ((u: IPathUpdate) => void) | undefined; + let options: WritableSignal; + let observeCount: number; + let replayOnObserve: IPathUpdate | undefined; interface CompassInternals { value: () => number | null | undefined; textValue: () => string; dataAvailable: () => boolean; + currentState: () => string; optionsReady: () => boolean; gaugeOptions: { needle?: boolean }; } - const makeConfig = (): IWidgetSvcConfig => { + const makeConfig = (path: string | null = 'self.navigation.headingTrue'): IWidgetSvcConfig => { const dflt = WidgetGaugeNgCompassComponent.DEFAULT_CONFIG; const gaugePath = (dflt.paths as IPathArray)['gaugePath']; return { ...dflt, - paths: { gaugePath: { ...gaugePath, path: 'self.navigation.headingTrue' } } + paths: { gaugePath: { ...gaugePath, path } } }; }; @@ -43,9 +48,17 @@ describe('WidgetGaugeNgCompassComponent no-data state', () => { beforeEach(async () => { capturedNext = undefined; - const options = signal(makeConfig()); + options = signal(makeConfig()); + observeCount = 0; + replayOnObserve = undefined; const streamsFake = { - observe(_pathName: string, next: (u: IPathUpdate) => void) { capturedNext = next; } + observe(_pathName: string, next: (u: IPathUpdate) => void) { + capturedNext = next; + observeCount++; + // The real directive replays a BehaviorSubject, so a path holding a value delivers it + // synchronously inside the same effect run as the clear. + if (replayOnObserve) next(replayOnObserve); + } }; const unitsFake = { getUnitDisplaySymbol: (measure: string | null | undefined): string => measure ?? '', @@ -96,4 +109,78 @@ describe('WidgetGaugeNgCompassComponent no-data state', () => { expect(internals.dataAvailable()).toBe(false); expect(internals.textValue()).toBe('--'); }); + + // #534: a rebuilt subscription against a silent path replays nothing (the leading null is + // suppressed), so the callback never runs and the previous heading stayed on the rose. + it('clears the heading when re-pointed at a path that reports nothing', () => { + capturedNext?.(update(142)); + expect(internals.dataAvailable()).toBe(true); + + options.set(makeConfig('self.navigation.headingMagnetic')); + fixture.detectChanges(); + + expect(internals.dataAvailable()).toBe(false); + expect(internals.value()).toBeUndefined(); + expect(internals.textValue()).toBe('--'); + }); + + // Clearing the path entirely drops the subscription, so the heading has to go with it. This is + // what pins the clear ahead of the no-path bail-out rather than after it. + it('clears the heading when the path is cleared entirely', () => { + capturedNext?.(update(142)); + expect(internals.dataAvailable()).toBe(true); + + options.set(makeConfig(null)); + fixture.detectChanges(); + + expect(internals.dataAvailable()).toBe(false); + expect(internals.value()).toBeUndefined(); + expect(internals.textValue()).toBe('--'); + }); + + // currentState colours the heading text independently of dataAvailable, so without the reset an + // alarm-red readout from the old path survives onto the new one. + it('clears the zone state on a re-point', () => { + capturedNext?.({ ...update(142), state: States.Alarm } as IPathUpdate); + expect(internals.currentState()).toBe(States.Alarm); + + options.set(makeConfig('self.navigation.headingMagnetic')); + fixture.detectChanges(); + + expect(internals.currentState()).toBe(States.Normal); + }); + + // The same effect re-runs on a theme change, so an unconditional clear would blink the needle + // off and back on at every switch. + it('shows the new path\'s heading immediately when it has one, without surfacing the clear', () => { + capturedNext?.(update(142)); + + replayOnObserve = update(271); + options.set(makeConfig('self.navigation.headingMagnetic')); + fixture.detectChanges(); + + expect(internals.dataAvailable()).toBe(true); + expect(internals.value()).toBe(271); + expect(internals.textValue()).toBe('271'); + }); + + it('keeps the heading when the config changes without changing the path', () => { + capturedNext?.(update(142)); + const before = observeCount; + + fixture.componentRef.setInput('theme', { + contrast: 'rgba(0,0,0,1)', contrastDim: 'rgba(60,60,60,1)', + contrastDimmer: 'rgba(120,120,120,1)', cardColor: 'rgba(238,238,238,1)', + background: 'rgba(255,255,255,1)', zoneAlarm: 'rgba(255,0,0,1)', + zoneWarn: 'rgba(255,170,0,1)', zoneAlert: 'rgba(255,0,255,1)', + zoneEmergency: 'rgba(255,0,0,1)' + }); + fixture.detectChanges(); + + // Positive control: the effect really did re-run, so "no clear" is a decision, not a no-op. + expect(observeCount).toBeGreaterThan(before); + expect(internals.dataAvailable()).toBe(true); + expect(internals.value()).toBe(142); + expect(internals.textValue()).toBe('142'); + }); }); diff --git a/src/app/widgets/widget-gauge-ng-compass/widget-gauge-ng-compass.component.ts b/src/app/widgets/widget-gauge-ng-compass/widget-gauge-ng-compass.component.ts index 6ae6454f..c501b9b3 100644 --- a/src/app/widgets/widget-gauge-ng-compass/widget-gauge-ng-compass.component.ts +++ b/src/app/widgets/widget-gauge-ng-compass/widget-gauge-ng-compass.component.ts @@ -15,7 +15,7 @@ import { States } from '../../core/interfaces/signalk-interfaces'; import { getColors } from '../../core/utils/themeColors.utils'; import { SkipResizeObserverDirective } from '../../core/directives/skip-resize-observer.directive'; import { WidgetRuntimeDirective } from '../../core/directives/widget-runtime.directive'; -import { WidgetStreamsDirective } from '../../core/directives/widget-streams.directive'; +import { WidgetStreamsDirective, widgetPathSignature, normalizeWidgetPath } from '../../core/directives/widget-streams.directive'; import { ITheme } from '../../core/services/app-service'; import { UnitsService } from '../../core/services/units.service'; @@ -124,6 +124,38 @@ export class WidgetGaugeNgCompassComponent implements AfterViewInit { protected colorStrokeTicks = ''; private currentState = signal(States.Normal); private lastAppliedState: States | null = null; + /** + * Identity of the path the reading state describes. Three states: `undefined` before the first + * effect run (nothing has been shown, so there is nothing to clear), `null` for a config with no + * usable path, and the signature otherwise. `null` is a real identity rather than a second + * "not yet" — a cleared path must still compare unequal to the path that follows it. + */ + private lastPathSignature: string | null | undefined = undefined; + + /** + * Drop the reading when the widget is re-pointed at another path. + * + * The subscription is rebuilt on every run of the data effect, a theme change included, and + * `suppressBootstrapNull` gives each rebuild a fresh suppression closure. Against a path that + * reports nothing the replayed leading null is therefore filtered and the stream callback never + * runs — leaving the previous path's needle and value on screen, presented as a live reading of + * the new one. Clearing unconditionally here is wrong for the same reason: this effect re-runs on + * theme changes, which would blink the needle off and back on at every switch. + * + * The reading comes back because `observe()` below is passed a new closure on every effect run: + * the directive compares callback identity as well as the signature, so it rebuilds and replays + * the new path's value into this component. A stable callback reference would make that + * early-return instead, and the gauge would stay blank on a live path until the next delta. + */ + private clearReadingOnRepoint(signature: string | null): void { + if (this.lastPathSignature !== undefined && this.lastPathSignature !== signature) { + this.dataAvailable.set(false); + this.value.set(undefined); + this.textValue.set('--'); + this.currentState.set(States.Normal); + } + this.lastPathSignature = signature; + } private readonly negToPortPaths = [ "self.environment.wind.angleApparent", @@ -146,22 +178,32 @@ export class WidgetGaugeNgCompassComponent implements AfterViewInit { const theme = this.theme(); if (!cfg || !theme) return; const pCfg = cfg.paths?.['gaugePath']; - if (!pCfg?.path) return; - untracked(() => this.streams.observe('gaugePath', pkt => { + // Computed before the no-path bail-out, and null exactly when there is no usable path: the + // streams directive drops the subscription in that case, so the reading has to go with it. + const signature = widgetPathSignature(pCfg); + untracked(() => { + this.clearReadingOnRepoint(signature); + // Normalized, like the signature and the subscription: the widget-options required check + // accepts a padded path, which would subscribe fine and then miss this list, clamping a + // negative apparent wind angle to 0 instead of converting it to its 0-360 bearing. + const path = normalizeWidgetPath(pCfg?.path); + if (!signature || !path) return; + this.streams.observe('gaugePath', pkt => { let raw = (pkt?.data?.value as number) ?? null; this.dataAvailable.set(raw != null); if (raw == null) { this.value.set(0); this.textValue.set('--'); } else { - if (this.negToPortPaths.includes(pCfg.path)) raw = convertNegToPortDegree(raw); + if (this.negToPortPaths.includes(path)) raw = convertNegToPortDegree(raw); const clamped = Math.min(Math.max(raw, 0), 360); this.value.set(clamped); this.textValue.set(clamped.toFixed(0)); } const newState = (pkt?.state ?? States.Normal) as States; if (newState !== this.currentState()) this.currentState.set(newState); - })); + }); + }); }); // Build options when config/theme changes diff --git a/src/app/widgets/widget-gauge-ng-linear/widget-gauge-ng-linear.component.spec.ts b/src/app/widgets/widget-gauge-ng-linear/widget-gauge-ng-linear.component.spec.ts index f5a62d24..47056cea 100644 --- a/src/app/widgets/widget-gauge-ng-linear/widget-gauge-ng-linear.component.spec.ts +++ b/src/app/widgets/widget-gauge-ng-linear/widget-gauge-ng-linear.component.spec.ts @@ -9,6 +9,7 @@ import { WidgetMetadataDirective } from '../../core/directives/widget-metadata.d import { UnitsService } from '../../core/services/units.service'; import { IWidgetSvcConfig, IPathArray } from '../../core/interfaces/widgets-interface'; import { States } from '../../core/interfaces/signalk-interfaces'; +import { IPathUpdate } from '../../core/services/data.service'; /** * The label and the unit are rendered by the component as one header row on the card, so the gauge @@ -26,11 +27,15 @@ describe('WidgetGaugeNgLinearComponent header row and sizing', () => { let internals: LinearInternals; let options: WritableSignal; let sizeUpdates: LinearGaugeOptions[]; + let capturedNext: ((u: IPathUpdate) => void) | undefined; + let observeCount: number; + let replayOnObserve: IPathUpdate | undefined; interface LinearInternals { effectiveUnit: WritableSignal; dataAvailable: WritableSignal; currentState: WritableSignal; + value: () => number | null | undefined; barColor: (cfg: IWidgetSvcConfig, theme: unknown, state: string) => string; optionsReady: () => boolean; textValue: () => string; @@ -45,17 +50,20 @@ describe('WidgetGaugeNgLinearComponent header row and sizing', () => { cardColor: '#111', background: '#000' }; - const makeConfig = (subType = 'vertical'): IWidgetSvcConfig => { + const makeConfig = (subType = 'vertical', path: string | null = 'self.navigation.speedOverGround'): IWidgetSvcConfig => { const dflt = WidgetGaugeNgLinearComponent.DEFAULT_CONFIG; const gaugePath = (dflt.paths as IPathArray)['gaugePath']; return { ...dflt, ignoreZones: true, gauge: { ...dflt.gauge, type: 'ngLinear', subType }, - paths: { gaugePath: { ...gaugePath, path: 'self.navigation.speedOverGround', convertUnitTo: 'knots' } } + paths: { gaugePath: { ...gaugePath, path, convertUnitTo: 'knots' } } }; }; + const update = (value: unknown, measure?: string): IPathUpdate => + ({ data: { value, timestamp: null, measure }, state: States.Normal }) as unknown as IPathUpdate; + const unitsFake = { convertBetweenMeasures: (from: string, to: string, value: number): number => from === to ? value : value, getUnitDisplaySymbol: (measure: string | null | undefined): string => measure ?? '', @@ -70,12 +78,21 @@ describe('WidgetGaugeNgLinearComponent header row and sizing', () => { beforeEach(async () => { options = signal(makeConfig()); sizeUpdates = []; + capturedNext = undefined; + observeCount = 0; + replayOnObserve = undefined; await TestBed.configureTestingModule({ imports: [WidgetGaugeNgLinearComponent], providers: [ { provide: WidgetRuntimeDirective, useValue: { options } }, - { provide: WidgetStreamsDirective, useValue: { observe: () => undefined } }, + { provide: WidgetStreamsDirective, useValue: { observe: (_p: string, next: (u: IPathUpdate) => void) => { + capturedNext = next; + observeCount++; + // The real directive replays a BehaviorSubject, so a path holding a value delivers it + // synchronously inside the same effect run as the clear. + if (replayOnObserve) next(replayOnObserve); + } } }, { provide: WidgetMetadataDirective, useValue: { zones: () => [], observe: () => undefined } }, { provide: UnitsService, useValue: unitsFake } ] @@ -216,4 +233,78 @@ describe('WidgetGaugeNgLinearComponent header row and sizing', () => { expect(internals.barColor(cfg, themed, States.Alarm)).toBe('rgba(0,0,0,0)'); }); }); + + // #534: a rebuilt subscription against a silent path replays nothing (the leading null is + // suppressed), so the callback never runs and the previous path's reading stayed on the bar. + describe('re-point', () => { + it('clears the reading when re-pointed at a path that reports nothing', () => { + capturedNext?.(update(6.5, 'knots')); + expect(internals.dataAvailable()).toBe(true); + expect(internals.value()).toBe(6.5); + + options.set(makeConfig('vertical', 'self.environment.depth.belowTransducer')); + fixture.detectChanges(); + + expect(internals.dataAvailable()).toBe(false); + expect(internals.value()).toBeUndefined(); + expect(internals.textValue()).toBe('--'); + expect(internals.effectiveUnit()).toBe(''); + }); + + // The same effect re-runs on a theme change, so an unconditional clear would blink the bar off + // and back on at every switch. + it('shows the new path\'s reading immediately when it has one, without surfacing the clear', () => { + capturedNext?.(update(6.5, 'knots')); + + replayOnObserve = update(31.2, 'm'); + options.set(makeConfig('vertical', 'self.environment.depth.belowTransducer')); + fixture.detectChanges(); + + expect(internals.dataAvailable()).toBe(true); + expect(internals.value()).toBe(31.2); + // The rendered header, not just the signal: this is what the user reads. + expect(fixture.nativeElement.querySelector('.gaugeUnit').textContent.trim()).toBe('m'); + }); + + // Clearing the path entirely drops the subscription, so the reading has to go with it. This is + // what pins the clear ahead of the no-path bail-out rather than after it. + it('clears the reading when the path is cleared entirely', () => { + capturedNext?.(update(6.5, 'knots')); + expect(internals.dataAvailable()).toBe(true); + + options.set(makeConfig('vertical', null)); + fixture.detectChanges(); + + expect(internals.dataAvailable()).toBe(false); + expect(internals.value()).toBeUndefined(); + expect(internals.textValue()).toBe('--'); + }); + + // currentState drives the value text's colour independently of dataAvailable, so without the + // reset an alarm-red readout from the old path survives onto the new one. + it('clears the zone state on a re-point', () => { + capturedNext?.({ ...update(6.5, 'knots'), state: States.Alarm }); + expect(internals.currentState()).toBe(States.Alarm); + + options.set(makeConfig('vertical', 'self.environment.depth.belowTransducer')); + fixture.detectChanges(); + + expect(internals.currentState()).toBe(States.Normal); + }); + + it('keeps the reading when the config changes without changing the path', () => { + capturedNext?.(update(6.5, 'knots')); + expect(internals.dataAvailable()).toBe(true); + const before = observeCount; + + fixture.componentRef.setInput('theme', { ...theme, cardColor: '#eee', background: '#fff' }); + fixture.detectChanges(); + + // Positive control: the effect really did re-run, so "no clear" is a decision, not a no-op. + expect(observeCount).toBeGreaterThan(before); + expect(internals.dataAvailable()).toBe(true); + expect(internals.value()).toBe(6.5); + expect(internals.effectiveUnit()).toBe('knots'); + }); + }); }); diff --git a/src/app/widgets/widget-gauge-ng-linear/widget-gauge-ng-linear.component.ts b/src/app/widgets/widget-gauge-ng-linear/widget-gauge-ng-linear.component.ts index d6220ce6..0c7df2ca 100644 --- a/src/app/widgets/widget-gauge-ng-linear/widget-gauge-ng-linear.component.ts +++ b/src/app/widgets/widget-gauge-ng-linear/widget-gauge-ng-linear.component.ts @@ -16,7 +16,7 @@ import { getColors } from '../../core/utils/themeColors.utils'; import { States } from '../../core/interfaces/signalk-interfaces'; import { SkipResizeObserverDirective } from '../../core/directives/skip-resize-observer.directive'; import { WidgetRuntimeDirective } from '../../core/directives/widget-runtime.directive'; -import { WidgetStreamsDirective } from '../../core/directives/widget-streams.directive'; +import { WidgetStreamsDirective, widgetPathSignature } from '../../core/directives/widget-streams.directive'; import { WidgetMetadataDirective } from '../../core/directives/widget-metadata.directive'; import { UnitsService } from '../../core/services/units.service'; import { ITheme } from '../../core/services/app-service'; @@ -103,6 +103,39 @@ export class WidgetGaugeNgLinearComponent implements AfterViewInit { private currentState = signal(States.Normal); /** Measure the incoming value was converted to (server-resolved for this display path). '' = boot placeholder. */ private effectiveUnit = signal(''); + /** + * Identity of the path the reading state describes. Three states: `undefined` before the first + * effect run (nothing has been shown, so there is nothing to clear), `null` for a config with no + * usable path, and the signature otherwise. `null` is a real identity rather than a second + * "not yet" — a cleared path must still compare unequal to the path that follows it. + */ + private lastPathSignature: string | null | undefined = undefined; + + /** + * Drop the reading when the widget is re-pointed at another path. + * + * The subscription is rebuilt on every run of the data effect, a theme change included, and + * `suppressBootstrapNull` gives each rebuild a fresh suppression closure. Against a path that + * reports nothing the replayed leading null is therefore filtered and the stream callback never + * runs — leaving the previous path's bar, value and unit on screen, presented as a live reading + * of the new one. Clearing unconditionally here is wrong for the same reason: this effect re-runs + * on theme changes, which would blink the bar off and back on at every switch. + * + * The reading comes back because `observe()` below is passed a new closure on every effect run: + * the directive compares callback identity as well as the signature, so it rebuilds and replays + * the new path's value into this component. A stable callback reference would make that + * early-return instead, and the gauge would stay blank on a live path until the next delta. + */ + private clearReadingOnRepoint(signature: string | null): void { + if (this.lastPathSignature !== undefined && this.lastPathSignature !== signature) { + this.dataAvailable.set(false); + this.value.set(undefined); + this.textValue.set('--'); + this.effectiveUnit.set(''); + this.currentState.set(States.Normal); + } + this.lastPathSignature = signature; + } protected adjustedScale = computed(() => { const cfg = this.runtime.options(); @@ -143,11 +176,13 @@ export class WidgetGaugeNgLinearComponent implements AfterViewInit { const cfg = this.runtime.options(); const theme = this.theme(); if (!cfg || !theme) return; - if (!cfg.paths?.['gaugePath'].path) return; + // Computed before the no-path bail-out, and null exactly when there is no usable path: the + // streams directive drops the subscription in that case, so the reading has to go with it. + const signature = widgetPathSignature(cfg.paths?.['gaugePath']); untracked(() => { - // Reset the tagged measure so a stale unit never paints the new subscription's value. - this.effectiveUnit.set(''); + this.clearReadingOnRepoint(signature); + if (!signature) return; this.streams.observe('gaugePath', path => { const raw = (path?.data?.value as number) ?? null; const measure = path.data.measure ?? ''; diff --git a/src/app/widgets/widget-gauge-ng-radial/widget-gauge-ng-radial.component.spec.ts b/src/app/widgets/widget-gauge-ng-radial/widget-gauge-ng-radial.component.spec.ts index 29ed4e42..9507a29b 100644 --- a/src/app/widgets/widget-gauge-ng-radial/widget-gauge-ng-radial.component.spec.ts +++ b/src/app/widgets/widget-gauge-ng-radial/widget-gauge-ng-radial.component.spec.ts @@ -9,6 +9,7 @@ import { UnitsService } from '../../core/services/units.service'; import { IPathUpdate } from '../../core/services/data.service'; import { IWidgetSvcConfig, IPathArray } from '../../core/interfaces/widgets-interface'; import { IScale } from '../../core/utils/dataScales.util'; +import { States } from '../../core/interfaces/signalk-interfaces'; /** * Regression tests for the gauge's displayScale reinterpretation — the P2b unit-flip mechanic. @@ -33,6 +34,7 @@ describe('WidgetGaugeNgRadialComponent displayScale reinterpretation (P2b flip)' let capturedNext: ((u: IPathUpdate) => void) | undefined; let observeCount: number; let lastObservedPath: string; + let replayOnObserve: IPathUpdate | undefined; interface GaugeInternals { effectiveUnit: WritableSignal; @@ -41,10 +43,11 @@ describe('WidgetGaugeNgRadialComponent displayScale reinterpretation (P2b flip)' textValue: () => string; dataAvailable: () => boolean; optionsReady: () => boolean; + pathDataState: () => States | null; } // convertUnitTo is the stored authoring unit; a tagged measure that differs is the flip target. - const makeConfig = (): IWidgetSvcConfig => { + const makeConfig = (path = 'self.test.soc'): IWidgetSvcConfig => { const dflt = WidgetGaugeNgRadialComponent.DEFAULT_CONFIG; const gaugePath = (dflt.paths as IPathArray)['gaugePath']; return { @@ -53,7 +56,7 @@ describe('WidgetGaugeNgRadialComponent displayScale reinterpretation (P2b flip)' displayScale: { lower: 10, upper: 100, type: 'linear' }, gauge: { ...dflt.gauge, type: 'ngRadial', subType: 'capacity' }, paths: { - gaugePath: { ...gaugePath, path: 'self.test.soc', convertUnitTo: 'ratio' } + gaugePath: { ...gaugePath, path, convertUnitTo: 'ratio' } } }; }; @@ -75,11 +78,15 @@ describe('WidgetGaugeNgRadialComponent displayScale reinterpretation (P2b flip)' observeCount = 0; lastObservedPath = ''; + replayOnObserve = undefined; const streamsFake = { observe(pathName: string, next: (u: IPathUpdate) => void) { lastObservedPath = pathName; capturedNext = next; observeCount++; + // The real directive's base is a BehaviorSubject, so a path that already holds a value + // replays it synchronously, inside the same effect run as the clear. + if (replayOnObserve) next(replayOnObserve); } }; const metadataFake = { zones: () => [], observe: () => undefined }; @@ -175,4 +182,109 @@ describe('WidgetGaugeNgRadialComponent displayScale reinterpretation (P2b flip)' // With the tag cleared, the scale falls back to the stored convertUnitTo bounds again. expect(internals.adjustedScale()).toEqual({ min: 10, max: 100, majorTicks: [] }); }); + + // #534: a rebuilt subscription against a silent path replays nothing (the leading null is + // suppressed), so the stream callback never runs and the previous path's reading stayed on the + // dial, presented as a live reading of the new path. + it('clears the reading when re-pointed at a path that reports nothing', () => { + capturedNext?.(update(42, 'percent')); + expect(internals.dataAvailable()).toBe(true); + expect(internals.value()).toBe(42); // within the reinterpreted 20..200 scale, so unclamped + + options.set(makeConfig('self.test.silent')); + fixture.detectChanges(); + + // The new subscription delivers nothing at all — exactly the case that used to leave the old + // needle in place. + expect(internals.dataAvailable()).toBe(false); + expect(internals.value()).toBeUndefined(); + expect(internals.textValue()).toBe('--'); + expect(internals.effectiveUnit()).toBe(''); + }); + + // The clear is only correct because it runs in the same synchronous block as the resubscribe: the + // directive's base is a BehaviorSubject, so a path that already holds a value replays it at once. + // Separating the two would blank the gauge on every re-point, which is the difference between + // clearing STALE data and clearing ALL data. + it('shows the new path\'s reading immediately when it has one, without surfacing the clear', () => { + capturedNext?.(update(42, 'percent')); + expect(internals.value()).toBe(42); + + replayOnObserve = update(71, 'percent'); + options.set(makeConfig('self.test.live')); + fixture.detectChanges(); + + expect(internals.dataAvailable()).toBe(true); + expect(internals.value()).toBe(71); + expect(internals.textValue()).toBe(''); + }); + + // Zone colours are driven by the path's state, so carrying an old path's alarm onto a new one is + // the same lie as carrying its value. + it('clears the zone state on a re-point, so an old alarm colour cannot carry over', () => { + capturedNext?.({ data: { value: 42, timestamp: null, measure: 'percent' }, state: States.Alarm }); + expect(internals.pathDataState()).toBe(States.Alarm); + + options.set(makeConfig('self.test.silent')); + fixture.detectChanges(); + + expect(internals.pathDataState()).toBeNull(); + }); + + // Clearing the path tears the subscription down in the directive, so the reading has to go too — + // otherwise the dial keeps a number with nothing feeding it. + it('clears the reading when the path is cleared entirely', () => { + capturedNext?.(update(42, 'percent')); + expect(internals.dataAvailable()).toBe(true); + + const cleared = makeConfig(); + (cleared.paths as IPathArray)['gaugePath'].path = null; + options.set(cleared); + fixture.detectChanges(); + + expect(internals.dataAvailable()).toBe(false); + expect(internals.value()).toBeUndefined(); + expect(internals.textValue()).toBe('--'); + }); + + // A path-less config has no signature, so it must not read as "nothing has been shown yet" and + // suppress the clear on the re-point after it. + it('still clears on the re-point that follows a cleared path', () => { + capturedNext?.(update(42, 'percent')); + + const cleared = makeConfig(); + (cleared.paths as IPathArray)['gaugePath'].path = null; + options.set(cleared); + fixture.detectChanges(); + + options.set(makeConfig('self.test.live')); + fixture.detectChanges(); + capturedNext?.(update(7, 'percent')); + expect(internals.dataAvailable()).toBe(true); + + options.set(makeConfig('self.test.silent')); + fixture.detectChanges(); + expect(internals.dataAvailable()).toBe(false); + }); + + // The same effect re-runs on a theme change, so an unconditional clear would blink the needle off + // and back on at every switch. + it('keeps the reading when the config changes without changing the path', () => { + capturedNext?.(update(42, 'percent')); + expect(internals.dataAvailable()).toBe(true); + + const before = observeCount; + fixture.componentRef.setInput('theme', { + contrast: '#000', contrastDim: '#333', contrastDimmer: '#666', + cardColor: '#eee', background: '#fff' + }); + fixture.detectChanges(); + + // Positive control: the effect really did re-run, so "no clear" is a decision, not a no-op. + expect(observeCount).toBeGreaterThan(before); + expect(internals.dataAvailable()).toBe(true); + expect(internals.value()).toBe(42); + expect(internals.textValue()).toBe(''); + expect(internals.effectiveUnit()).toBe('percent'); + }); }); diff --git a/src/app/widgets/widget-gauge-ng-radial/widget-gauge-ng-radial.component.ts b/src/app/widgets/widget-gauge-ng-radial/widget-gauge-ng-radial.component.ts index a8addda8..1e2418a6 100644 --- a/src/app/widgets/widget-gauge-ng-radial/widget-gauge-ng-radial.component.ts +++ b/src/app/widgets/widget-gauge-ng-radial/widget-gauge-ng-radial.component.ts @@ -16,7 +16,7 @@ import { getHighlights } from '../../core/utils/zones-highlight.utils'; import { getColors } from '../../core/utils/themeColors.utils'; import { SkipResizeObserverDirective } from '../../core/directives/skip-resize-observer.directive'; import { WidgetRuntimeDirective } from '../../core/directives/widget-runtime.directive'; -import { WidgetStreamsDirective } from '../../core/directives/widget-streams.directive'; +import { WidgetStreamsDirective, widgetPathSignature } from '../../core/directives/widget-streams.directive'; import { WidgetMetadataDirective } from '../../core/directives/widget-metadata.directive'; import { UnitsService } from '../../core/services/units.service'; import { ITheme } from '../../core/services/app-service'; @@ -143,6 +143,38 @@ export class WidgetGaugeNgRadialComponent implements AfterViewInit { private pathDataState = signal(null); private viewReady = signal(false); protected gaugeOptions: RadialGaugeOptions = {} as RadialGaugeOptions; + /** + * Identity of the path the reading state describes. Three states: `undefined` before the first + * effect run (nothing has been shown, so there is nothing to clear), `null` for a config with no + * usable path, and the signature otherwise. `null` is a real identity rather than a second + * "not yet" — a cleared path must still compare unequal to the path that follows it. + */ + private lastPathSignature: string | null | undefined = undefined; + + /** + * Drop the reading when the widget is re-pointed at another path. + * + * The subscription is rebuilt on every run of the data effect, a theme change included, and + * `suppressBootstrapNull` gives each rebuild a fresh suppression closure. Against a path that + * reports nothing the replayed leading null is therefore filtered and the stream callback never + * runs — leaving the previous path's needle and value on screen, presented as a live reading of + * the new one. Clearing unconditionally here is wrong for the same reason: this effect re-runs on + * theme changes, which would blink the needle off and back on at every switch. + * + * The reading comes back because `observe()` below is passed a new closure on every effect run: + * the directive compares callback identity as well as the signature, so it rebuilds and replays + * the new path's value into this component. A stable callback reference would make that + * early-return instead, and the gauge would stay blank on a live path until the next delta. + */ + private clearReadingOnRepoint(signature: string | null): void { + if (this.lastPathSignature !== undefined && this.lastPathSignature !== signature) { + this.dataAvailable.set(false); + this.value.set(undefined); + this.effectiveUnit.set(''); + this.pathDataState.set(null); + } + this.lastPathSignature = signature; + } constructor() { // Data subscription effect @@ -150,9 +182,14 @@ export class WidgetGaugeNgRadialComponent implements AfterViewInit { const cfg = this.runtime.options(); const theme = this.theme(); if (!cfg || !theme) return; - if (!cfg.paths?.['gaugePath'].path) return; + // Computed before the no-path bail-out, and null exactly when there is no usable path: the + // streams directive drops the subscription in that case, so the reading has to go with it. + const signature = widgetPathSignature(cfg.paths?.['gaugePath']); - untracked(() => this.streams.observe('gaugePath', path => { + untracked(() => { + this.clearReadingOnRepoint(signature); + if (!signature) return; + this.streams.observe('gaugePath', path => { if (path.state !== this.pathDataState()) { this.pathDataState.set((path.state as States) || null); } @@ -172,7 +209,8 @@ export class WidgetGaugeNgRadialComponent implements AfterViewInit { // clamp this.value.set(Math.min(Math.max(raw, lower), upper)); } - })); + }); + }); }); // Metadata observation (idempotent) – only when zones not ignored