fix: address PR #68 code review findings

HanSur94 · HanSur94 · commit 6b768cb1c5e4 · 2026-04-23T23:56:02.000+02:00
Blockers
  A1 IconCardWidget.deriveStateFromThreshold returned 'active' for
     healthy monitor-kind / CompositeThreshold branches; resolveIconColor
     only routes 'ok|warn|alarm|info', so those icons rendered mid-gray
     instead of green. Replaced both occurrences with 'ok'.
  A2 FastSenseWidget.autoScaleY_ clobbered user Y zoom on every live
     tick. Added a YLim PostSet listener + IsSettingYLim/UserZoomedY
     guards mirroring the existing X-zoom pattern so a mouse-scroll
     zoom stays pinned until the user explicitly re-resets.
  A3 MarkdownRenderer.getCSS still branched on legacy theme names
     ('industrial','ocean' → dark CSS), so info-popup contrast didn't
     match the aliased light theme struct. Collapsed to a single
     dark-vs-everything-else test matching the trimmed catalog.

Design concerns
  B1 FastSense.addLine datenum auto-promotion now respects an explicit
     'XType','numeric' NV-pair — numeric counters that happen to land
     in the datenum window (e.g. step counters starting at 700000)
     can opt out. Detected by walking raw varargin since parseOpts
     defaults XType to 'numeric' and can't distinguish
     default-from-explicit.
  B2 plantConfig MonitorDef field renamed to MinDurationSeconds so the
     unit lives in the name; the datenum-unit conversion is pulled
     into cfg.MonitorMinDurationFor (closure) and called once in
     registerPlantTags. cfg.TimeBase now documents the demo's
     time-base choice. Any future consumer that reads MonitorDefs
     directly has to acknowledge the unit in the field name.

Nits + follow-ups
  N1 StatusWidget label path guards isempty(obj.Sensor.Y) before
     indexing end.
  N3/N4 FastSenseDefaults / FastSenseToolbar docstrings updated to
     list only 'light'/'dark' with a note that legacy names alias.
  F2 FastSenseWidget.LiveViewMode is now a public property (default
     'reset') forwarded to FastSense. Callers in long-running
     deployments can set 'follow' for a sliding-window or 'preserve'
     for frozen axes, instead of the hard-coded 'reset'.
  F3 EventTimelineWidget severity colouring now keys off the numeric
     ev.Severity field (1=ok/info, 2=warn, 3=alarm, per EVENT-04)
     with a ThresholdLabel keyword fallback for pre-Severity events.
  F4 New tests/test_fastsense_datenum_autopromote.m covers the
     heuristic, the small-X no-promote path, the explicit-numeric
     opt-out, the explicit-datenum opt-in, the straddle case, and
     empty-X.
  Examples example_themes.m and example_toolbar.m updated to use the
  surviving 'light'/'dark' preset names.

N5 (suggested dead VisibleRows assignment) was rejected on closer
look — the assignment at allocatePanels:334 runs AFTER TotalRows has
been accumulated, so it refreshes the visible window against the
newly expanded total row count; ensureViewport's earlier call to
computeVisibleRows uses the pre-accumulation value.

Regression suite: 81/81 (new test file picked up automatically).
diff --git a/demo/industrial_plant/private/plantConfig.m b/demo/industrial_plant/private/plantConfig.m
@@ -107,6 +107,13 @@
     %   release threshold) as a pair of function handles.
     cfg.MonitorDefs = mkMonitorDefs_();
 
+    % Demo time-base: the data generator timestamps samples with MATLAB
+    % datenum (days since 0000-01-00). MonitorTag.MinDuration is measured
+    % in parent-X native units, so seconds → days.
+    cfg.TimeBase              = 'datenum';
+    cfg.SecondsPerTimeUnit    = 86400;  % days → seconds; inverse factor below
+    cfg.MonitorMinDurationFor = @(seconds) seconds / 86400;
+
     % ---- Display thresholds for FastSense plots -----------------------
     %   Parallel table to MonitorDefs that expresses the trip value in a
     %   form diagram widgets can draw as a horizontal line. Each entry is
@@ -146,16 +153,23 @@
                @(x,y) y < 20, @(x,y) y > 30, 2, 'medium', 'Cooling')];
 end
 
-function d = mkDef_(key, parentKey, condFn, offFn, minDur, crit, sub)
+function d = mkDef_(key, parentKey, condFn, offFn, minDurSeconds, crit, sub)
     %MKDEF_ Build one MonitorDef entry.
+    %   Debounce is authored in SECONDS (`MinDurationSeconds`) so the
+    %   config stays human-readable. The demo's parent SensorTags time-
+    %   stamp samples with MATLAB datenum (days), so registerPlantTags
+    %   converts via cfg.MonitorMinDurationDays(mDef.MinDurationSeconds)
+    %   before handing the value to MonitorTag.MinDuration. Any other
+    %   consumer that wants to read this directly must be aware of the
+    %   unit — the field name carries that contract.
     d = struct( ...
-        'Key',         key, ...
-        'ParentKey',   parentKey, ...
-        'ConditionFn', condFn, ...
-        'AlarmOffFn',  offFn, ...
-        'MinDuration', minDur, ...
-        'Criticality', crit, ...
-        'Subsystem',   sub);
+        'Key',                key, ...
+        'ParentKey',          parentKey, ...
+        'ConditionFn',        condFn, ...
+        'AlarmOffFn',         offFn, ...
+        'MinDurationSeconds', minDurSeconds, ...
+        'Criticality',        crit, ...
+        'Subsystem',          sub);
 end
 
 function t = mkDisplayThresholds_()
diff --git a/demo/industrial_plant/private/registerPlantTags.m b/demo/industrial_plant/private/registerPlantTags.m
@@ -94,18 +94,15 @@
 
     mDefs = cfg.MonitorDefs;  % expects 4 entries in fixed order, see plantConfig
 
-    % MonitorTag.MinDuration is expressed in parent-X native units. The
-    % demo's parent SensorTags carry time as MATLAB datenum (days since
-    % 0000-01-00), so a config value of "1 second" must be rescaled to
-    % 1/86400 days. Without this, a 3-second breach reads as 3.5e-5 days
-    % and always falls below the 1-day debounce window → monitor never
-    % fires and StatusWidget / IconCardWidget stay green.
-    secondsPerDay = 86400;
+    % MinDurationSeconds carries debounce in human-readable seconds;
+    % cfg.MonitorMinDurationFor converts to the parent-X native unit
+    % (datenum days in this demo). See plantConfig.mkDef_ docstring.
+    toMinDuration = cfg.MonitorMinDurationFor;
 
     mFeedlinePressureHigh = MonitorTag(mDefs(1).Key, ...
         TagRegistry.get(mDefs(1).ParentKey), mDefs(1).ConditionFn, ...
         'AlarmOffConditionFn', mDefs(1).AlarmOffFn, ...
-        'MinDuration',         mDefs(1).MinDuration / secondsPerDay, ...
+        'MinDuration',         toMinDuration(mDefs(1).MinDurationSeconds), ...
         'Criticality',         mDefs(1).Criticality, ...
         'EventStore',          store, ...
         'Name',                prettyName_(mDefs(1).Key));
@@ -114,7 +111,7 @@
     mReactorPressureCritical = MonitorTag(mDefs(2).Key, ...
         TagRegistry.get(mDefs(2).ParentKey), mDefs(2).ConditionFn, ...
         'AlarmOffConditionFn', mDefs(2).AlarmOffFn, ...
-        'MinDuration',         mDefs(2).MinDuration / secondsPerDay, ...
+        'MinDuration',         toMinDuration(mDefs(2).MinDurationSeconds), ...
         'Criticality',         mDefs(2).Criticality, ...
         'EventStore',          store, ...
         'Name',                prettyName_(mDefs(2).Key));
@@ -123,7 +120,7 @@
     mReactorTemperatureHigh = MonitorTag(mDefs(3).Key, ...
         TagRegistry.get(mDefs(3).ParentKey), mDefs(3).ConditionFn, ...
         'AlarmOffConditionFn', mDefs(3).AlarmOffFn, ...
-        'MinDuration',         mDefs(3).MinDuration / secondsPerDay, ...
+        'MinDuration',         toMinDuration(mDefs(3).MinDurationSeconds), ...
         'Criticality',         mDefs(3).Criticality, ...
         'EventStore',          store, ...
         'Name',                prettyName_(mDefs(3).Key));
@@ -132,7 +129,7 @@
     mCoolingFlowLow = MonitorTag(mDefs(4).Key, ...
         TagRegistry.get(mDefs(4).ParentKey), mDefs(4).ConditionFn, ...
         'AlarmOffConditionFn', mDefs(4).AlarmOffFn, ...
-        'MinDuration',         mDefs(4).MinDuration / secondsPerDay, ...
+        'MinDuration',         toMinDuration(mDefs(4).MinDurationSeconds), ...
         'Criticality',         mDefs(4).Criticality, ...
         'EventStore',          store, ...
         'Name',                prettyName_(mDefs(4).Key));
diff --git a/examples/01-basics/example_themes.m b/examples/01-basics/example_themes.m
@@ -11,8 +11,10 @@
 y2 = cos(x*2*pi/8) + 0.3*randn(1,n);
 y3 = 0.5*sin(x*2*pi/5) + 0.15*randn(1,n);
 
-%% 1. All 6 built-in theme presets
-themes = {'default', 'dark', 'light', 'industrial', 'scientific', 'ocean'};
+%% 1. Built-in theme presets ('light' and 'dark')
+%   Legacy preset names ('default', 'industrial', 'scientific', 'ocean')
+%   are accepted and aliased to 'light' for backward compatibility.
+themes = {'light', 'dark'};
 
 for i = 1:numel(themes)
     themeName = themes{i};
@@ -26,7 +28,7 @@
     title(fp.hAxes, sprintf('Theme: %s', themeName));
 end
 
-fprintf('All 6 themes rendered. Compare the styles!\n');
+fprintf('Both themes rendered. Compare the styles!\n');
 
 %% 2. Color palettes via LineColorOrder ('vibrant', 'muted', 'colorblind')
 palettes = {'vibrant', 'muted', 'colorblind'};
@@ -50,9 +52,9 @@
 title(fp.hAxes, 'Custom theme overrides (dark + larger font/lines)');
 
 %% 4. reapplyTheme — switch theme on a rendered plot
-fp.Theme = FastSenseTheme('ocean');
+fp.Theme = FastSenseTheme('light');
 fp.reapplyTheme();
-title(fp.hAxes, 'reapplyTheme: switched from dark to ocean');
+title(fp.hAxes, 'reapplyTheme: switched from dark to light');
 fprintf('reapplyTheme() demo: switched theme on an already-rendered plot.\n');
 
 %% 5. FastSenseDefaults — inspect global defaults
diff --git a/examples/01-basics/example_toolbar.m b/examples/01-basics/example_toolbar.m
@@ -25,7 +25,7 @@
 fprintf('Try: Data Cursor (click), Crosshair (hover), Grid, Legend, Autoscale Y, Export PNG\n');
 
 %% Dashboard with toolbar
-fig = FastSenseGrid(1, 2, 'Theme', 'industrial');
+fig = FastSenseGrid(1, 2, 'Theme', 'dark');
 fp1 = fig.tile(1);
 fp1.addLine(x, y1, 'DisplayName', 'Pressure');
 fp1.addThreshold(1.0, 'Direction', 'upper', 'ShowViolations', true);
diff --git a/libs/Dashboard/EventTimelineWidget.m b/libs/Dashboard/EventTimelineWidget.m
@@ -297,11 +297,16 @@ function refresh(obj)
                 if ~isempty(ev.ThresholdLabel)
                     lbl = [ev.SensorName ' — ' ev.ThresholdLabel];
                 end
-                % Color based on direction/severity hint in label
-                if ~isempty(strfind(lower(ev.ThresholdLabel), 'alarm'))
+                % Colour routing is driven by the numeric Severity field
+                % (1=ok/info, 2=warn, 3=alarm; see Event.m EVENT-04) with
+                % a ThresholdLabel keyword fallback for events authored
+                % before Severity existed.
+                clr = warnColor;
+                if isfield(ev, 'Severity') && ~isempty(ev.Severity) && ev.Severity >= 3
+                    clr = alarmColor;
+                elseif ~isfield(ev, 'Severity') && ~isempty(ev.ThresholdLabel) && ...
+                        ~isempty(strfind(lower(ev.ThresholdLabel), 'alarm'))
                     clr = alarmColor;
-                else
-                    clr = warnColor;
                 end
                 evts(end+1) = struct('startTime', ev.StartTime, ...
                     'endTime', ev.EndTime, 'label', lbl, 'color', clr); %#ok<AGROW>
diff --git a/libs/Dashboard/FastSenseWidget.m b/libs/Dashboard/FastSenseWidget.m
@@ -19,12 +19,23 @@
         YLabel       = ''    % Y-axis label (auto-set from Sensor if empty)
         YLimits             = []    % Fixed Y-axis range [min max]; empty = auto-scale
         ShowThresholdLabels = false % show inline name labels on threshold lines
+        % Forwarded to FastSense.LiveViewMode on render:
+        %   'reset'    — window covers the full X range every tick (default:
+        %                matches dashboard-demo expectation that users see
+        %                every sample since session start)
+        %   'follow'   — window of current width tracks the latest sample
+        %                (use for long-running deployments where the full
+        %                range would exhaust memory / downsampling budget)
+        %   'preserve' — frozen at the initial X range (legacy behaviour)
+        LiveViewMode = 'reset'
     end
     %   (Tag property now lives on the DashboardWidget base class — Plan 1009-02.)
 
     properties (SetAccess = private)
         FastSenseObj  = []
         IsSettingTime = false  % guard to distinguish programmatic vs user xlim change
+        IsSettingYLim = false  % guard so autoScaleY_ does not flip UserZoomedY
+        UserZoomedY   = false  % true after user mouse-zooms Y; suspends autoScaleY_
         CachedXMin    = inf    % cached minimum of X data for O(1) getTimeRange()
         CachedXMax    = -inf   % cached maximum of X data for O(1) getTimeRange()
         LastTagRef    = []     % Tag handle snapshot for cache-invalidation
@@ -71,14 +82,14 @@ function render(obj, parentPanel)
             obj.FastSenseObj = fp;
             fp.ShowThresholdLabels = obj.ShowThresholdLabels;
 
-            % Slide the X window as new samples arrive on updateData(). The
-            % default empty LiveViewMode leaves the window frozen at the
-            % initial render's data range, so later samples appear off the
-            % right edge of live widgets. 'reset' grows the window to cover
-            % the full current X range each tick — appropriate for
-            % dashboard use where users expect to see all data since the
-            % live pipeline started.
-            fp.LiveViewMode = 'reset';
+            % Slide the X window as new samples arrive on updateData().
+            % Forwarded from the widget-level LiveViewMode property so
+            % callers can swap between 'reset' (default: window grows to
+            % cover all samples — best for short demos), 'follow' (fixed-
+            % width window tracking the latest sample — best for long-
+            % running deployments), and 'preserve' (frozen at the initial
+            % X range — legacy behaviour).
+            fp.LiveViewMode = obj.LiveViewMode;
 
             % Bind data — Tag-first dispatch (v2.0).
             if ~isempty(obj.Tag)
@@ -148,6 +159,12 @@ function render(obj, parentPanel)
                 addlistener(ax, 'XLim', 'PostSet', @(~,~) obj.onXLimChanged());
             catch
             end
+            % Listen for manual Y zoom so autoScaleY_ stops fighting the
+            % user after a scroll / drag / programmatic ylim.
+            try
+                addlistener(ax, 'YLim', 'PostSet', @(~,~) obj.onYLimChanged());
+            catch
+            end
         end
 
         function refresh(obj)
@@ -205,10 +222,16 @@ function autoScaleY_(obj, y)
         %   samples outside the initial range would fall off the chart.
         %   This helper recomputes the Y extent every tick (including any
         %   threshold values so MonitorTag lines stay visible) and updates
-        %   the axes. Skipped when the widget has a user-pinned YLimits.
+        %   the axes. Skipped when:
+        %     - the widget has a user-pinned YLimits NV-pair, or
+        %     - the user manually zoomed Y via mouse (UserZoomedY),
+        %   so we never fight an explicit human interaction.
             if ~isempty(obj.YLimits)
                 return;
             end
+            if obj.UserZoomedY
+                return;
+            end
             if isempty(obj.FastSenseObj) || ~obj.FastSenseObj.IsRendered
                 return;
             end
@@ -241,7 +264,25 @@ function autoScaleY_(obj, y)
             else
                 pad = max(abs(yMax) * 0.1, 1);
             end
-            set(ax, 'YLim', [yMin - pad, yMax + pad]);
+            obj.IsSettingYLim = true;
+            try
+                set(ax, 'YLim', [yMin - pad, yMax + pad]);
+            catch
+            end
+            obj.IsSettingYLim = false;
+        end
+
+        function onYLimChanged(obj)
+        %ONYLIMCHANGED Detach widget from automatic Y rescale after user zoom.
+        %   Fired by the YLim PostSet listener. When the YLim change came
+        %   from inside autoScaleY_ (IsSettingYLim==true) we ignore it; any
+        %   other source — mouse scroll, drag, zoom toolbar, programmatic
+        %   ylim() from user code — counts as a manual override and
+        %   latches UserZoomedY so live ticks stop fighting the user.
+            if obj.IsSettingYLim
+                return;
+            end
+            obj.UserZoomedY = true;
         end
 
         function setTimeRange(obj, tStart, tEnd)
diff --git a/libs/Dashboard/IconCardWidget.m b/libs/Dashboard/IconCardWidget.m
@@ -383,7 +383,7 @@ function refresh(obj)
             % CompositeThreshold: delegate to computeStatus, no val needed (per D-04)
             if isa(obj.Threshold, 'CompositeThreshold')
                 cStatus = obj.Threshold.computeStatus();
-                if strcmp(cStatus, 'ok'), state = 'active'; else, state = 'alarm'; end
+                if strcmp(cStatus, 'ok'), state = 'ok'; else, state = 'alarm'; end
                 return;
             end
             % Monitor-kind tags are their own binary alarm signal.
@@ -398,7 +398,7 @@ function refresh(obj)
                 if val(end) > 0.5
                     state = 'alarm';
                 else
-                    state = 'active';
+                    state = 'ok';
                 end
                 return;
             end
diff --git a/libs/Dashboard/MarkdownRenderer.m b/libs/Dashboard/MarkdownRenderer.m
@@ -297,34 +297,29 @@ bodyHtml char(10) ...
         end
 
         function css = getCSS(themeName)
-            switch themeName
-                case {'dark', 'industrial', 'ocean'}
-                    bg = '#1a1a2e';
-                    fg = '#d4d4d4';
-                    codeBg = '#2d2d44';
-                    linkColor = '#5ca8e6';
-                    hrColor = '#3a3a5c';
-                    tableBorder = '#3a3a5c';
-                    tableHeadBg = '#2d2d44';
-                    tableStripeBg = '#22223a';
-                case {'light', 'scientific'}
-                    bg = '#ffffff';
-                    fg = '#2d2d2d';
-                    codeBg = '#f4f4f4';
-                    linkColor = '#0066cc';
-                    hrColor = '#ddd';
-                    tableBorder = '#ddd';
-                    tableHeadBg = '#f4f4f4';
-                    tableStripeBg = '#fafafa';
-                otherwise
-                    bg = '#ffffff';
-                    fg = '#2d2d2d';
-                    codeBg = '#f4f4f4';
-                    linkColor = '#0066cc';
-                    hrColor = '#ddd';
-                    tableBorder = '#ddd';
-                    tableHeadBg = '#f4f4f4';
-                    tableStripeBg = '#fafafa';
+            % Only 'dark' and 'light' remain as distinct presets. Every
+            % other name (empty, 'default', or a legacy alias like
+            % 'industrial' / 'scientific' / 'ocean') resolves to the light
+            % CSS palette, matching how FastSenseTheme / DashboardTheme
+            % alias those names to their 'light' structs.
+            if strcmp(themeName, 'dark')
+                bg = '#1a1a2e';
+                fg = '#d4d4d4';
+                codeBg = '#2d2d44';
+                linkColor = '#5ca8e6';
+                hrColor = '#3a3a5c';
+                tableBorder = '#3a3a5c';
+                tableHeadBg = '#2d2d44';
+                tableStripeBg = '#22223a';
+            else
+                bg = '#ffffff';
+                fg = '#2d2d2d';
+                codeBg = '#f4f4f4';
+                linkColor = '#0066cc';
+                hrColor = '#ddd';
+                tableBorder = '#ddd';
+                tableHeadBg = '#f4f4f4';
+                tableStripeBg = '#fafafa';
             end
             css = sprintf([ ...
                 'body { font-family: -apple-system, "Segoe UI", Helvetica, Arial, sans-serif; ' ...
diff --git a/libs/Dashboard/StatusWidget.m b/libs/Dashboard/StatusWidget.m
@@ -144,7 +144,7 @@ function refresh(obj)
                             lbl = sprintf('%s: %s', obj.Title, upper(obj.CurrentStatus));
                         end
                     end
-                elseif ~isempty(obj.Sensor)
+                elseif ~isempty(obj.Sensor) && ~isempty(obj.Sensor.Y)
                     val = obj.Sensor.Y(end);
                     units = '';
                     if ~isempty(obj.Sensor.Units)
diff --git a/libs/FastSense/FastSense.m b/libs/FastSense/FastSense.m
@@ -408,6 +408,20 @@ function addLine(obj, x, y, varargin)
             knownDefaults.DataStore = [];
             [known, passthrough] = parseOpts(knownDefaults, varargin, obj.Verbose);
 
+            % Detect whether the caller explicitly passed 'XType' as an
+            % NV-pair. parseOpts defaults it to 'numeric', so an
+            % unspecified call is indistinguishable from
+            % 'XType','numeric' at the parsed-options layer — we walk
+            % raw varargin here to tell them apart.
+            explicitXType = false;
+            for k = 1:2:numel(varargin)-1
+                if (ischar(varargin{k}) || (isstring(varargin{k}) && isscalar(varargin{k}))) ...
+                        && strcmpi(char(varargin{k}), 'XType')
+                    explicitXType = true;
+                    break;
+                end
+            end
+
             % Set XType if explicitly provided
             if strcmp(known.XType, 'datenum')
                 obj.XType = 'datenum';
@@ -416,11 +430,14 @@ function addLine(obj, x, y, varargin)
 
             % Auto-promote XType to 'datenum' when X values fall inside the
             % MATLAB serial-date range for years 1910-2100 (datenum 697000
-            % .. 769000). This makes Tag/SensorTag data that is timestamped
-            % with `now()` render with proper date ticks instead of raw
-            % scientific-notation numbers, without requiring every caller
-            % to pass 'XType', 'datenum' by hand. Skipped when X is empty.
-            if strcmp(obj.XType, 'numeric') && isnumeric(x) && ~isempty(x)
+            % .. 769000). Lets Tag/SensorTag data timestamped with `now()`
+            % render with proper date ticks instead of scientific-notation
+            % numbers, without requiring every caller to pass 'XType',
+            % 'datenum' by hand. Suppressed when the caller explicitly
+            % asked for 'numeric' (numeric counters / indices that happen
+            % to land in the datenum window must be able to opt out).
+            if ~explicitXType && strcmp(obj.XType, 'numeric') && ...
+                    isnumeric(x) && ~isempty(x)
                 xMinProbe = min(x);
                 xMaxProbe = max(x);
                 if isfinite(xMinProbe) && isfinite(xMaxProbe) && ...
diff --git a/libs/FastSense/FastSenseDefaults.m b/libs/FastSense/FastSenseDefaults.m
diff --git a/libs/FastSense/FastSenseToolbar.m b/libs/FastSense/FastSenseToolbar.m
diff --git a/tests/test_fastsense_datenum_autopromote.m b/tests/test_fastsense_datenum_autopromote.m
