fix(gui): --history given on the command line beats the saved layout

TODO.md

@@ -6,7 +6,7 @@ This file is the index: the identifier, where it is, and one line of what it is.
 
 ```console
 $ grep -rn "TODO(gs-link-rs-erasures)" crates/     # the full text of any entry
-$ grep -rn "TODO(" crates/ | wc -l                 # 32 of them, over 22 identifiers
+$ grep -rn "TODO(" crates/ | wc -l                 # 29 of them, over 21 identifiers
 ```
 
 Nothing here is a bug. The bugs found while building this are in
@@ -64,7 +64,6 @@ Ordered by how much you learn per hour spent, not by importance.
 | `gs-gui-app` | `app.rs:497` | Nothing in `app.rs` is tested against a running session, because `Session` has no constructor that does not spawn tasks. A trait over the four accessors `App` uses would fix it, at a vtable per call. |
 | `gs-gui-layout` | `layout.rs:159` | `tree_width` and `events_height` are written and never read back, so a panel the operator drags is narrow again next run. egui 0.35 has no public accessor for the dragged size. |
 | `gs-gui-layout` | `layout.rs:254` | At version 2 the loader will refuse every version-1 layout and the station will not start until the operator deletes a file they did not write. Discard, migrate, or keep refusing. |
-| `gs-gui-history-precedence` | `app.rs:172` | A saved layout's history depth silently overrides `--history`, so the flag does nothing whenever a layout exists — and one now ships with the bundled JPSS definition. Decide whose intent wins; needs clap's `value_source` to tell a given value from a defaulted one. |
 | `gs-gui-plots-monotonic` | `plots.rs:198` | The visible-range binary search assumes time never goes backwards. A spacecraft clock that steps back mid-pass truncates the window. Needs an ingest policy first. |
 
 ## `xtce-gs-cli`

crates/xtce-gs-cli/src/args.rs

@@ -14,6 +14,7 @@
 use std::path::PathBuf;
 
 use clap::{Args, Parser, Subcommand, ValueEnum};
+use xtce_gs_engine::config::DEFAULT_HISTORY_DEPTH;
 use xtce_gs_engine::{SessionConfig, TimeFormat, TimeSource};
 use xtce_gs_link::{CspVersion, Framing, PipelineConfig, RsConfig, SourceSpec};
 
@@ -115,8 +116,12 @@ pub struct RunArgs {
     pub time: TimeArgs,
 
     /// Points of history kept per watched parameter.
-    #[arg(long, default_value_t = xtce_gs_engine::config::DEFAULT_HISTORY_DEPTH)]
-    pub history: usize,
+    ///
+    /// Defaults to `xtce_gs_engine::config::DEFAULT_HISTORY_DEPTH`. An `Option` and not a
+    /// `default_value_t`, because a saved layout carries a depth too and the two have to be
+    /// told apart: given on the command line, this wins; left out, the layout's depth stands.
+    #[arg(long)]
+    pub history: Option<usize>,
 
     /// JSON limits file, keyed by qualified parameter name.
     #[arg(long)]
@@ -168,7 +173,7 @@ impl RunArgs {
             root_container: self.root.clone(),
             source: self.source.parse::<SourceSpec>()?,
             pipeline: self.pipeline.to_pipeline_config()?,
-            history_depth: self.history,
+            history_depth: self.history.unwrap_or(DEFAULT_HISTORY_DEPTH),
             event_capacity: xtce_gs_engine::config::DEFAULT_EVENT_CAPACITY,
             limits: self.limits.clone(),
             record: self.record.clone(),
@@ -209,8 +214,12 @@ pub struct ReplayArgs {
     pub time: TimeArgs,
 
     /// Points of history kept per watched parameter.
-    #[arg(long, default_value_t = xtce_gs_engine::config::DEFAULT_HISTORY_DEPTH)]
-    pub history: usize,
+    ///
+    /// Defaults to `xtce_gs_engine::config::DEFAULT_HISTORY_DEPTH`. An `Option` and not a
+    /// `default_value_t`, because a saved layout carries a depth too and the two have to be
+    /// told apart: given on the command line, this wins; left out, the layout's depth stands.
+    #[arg(long)]
+    pub history: Option<usize>,
 
     /// JSON limits file, keyed by qualified parameter name.
     #[arg(long)]
@@ -264,7 +273,7 @@ impl ReplayArgs {
                 repeat: self.repeat,
             },
             pipeline: self.pipeline.to_pipeline_config()?,
-            history_depth: self.history,
+            history_depth: self.history.unwrap_or(DEFAULT_HISTORY_DEPTH),
             event_capacity: xtce_gs_engine::config::DEFAULT_EVENT_CAPACITY,
             limits: self.limits.clone(),
             record: None,

crates/xtce-gs-cli/src/main.rs

@@ -104,8 +104,11 @@ fn dispatch(cli: Cli) -> Result<(), CliError> {
             if args.headless {
                 run::headless(config, verbose)
             } else {
-                let shot = screenshot(args.screenshot, args.screenshot_after);
-                run::gui(config, shot)
+                let startup = xtce_gs_gui::Startup {
+                    screenshot: screenshot(args.screenshot, args.screenshot_after),
+                    history: args.history,
+                };
+                run::gui(config, startup)
             }
         }
 
@@ -114,8 +117,11 @@ fn dispatch(cli: Cli) -> Result<(), CliError> {
             if args.headless {
                 run::headless(config, verbose)
             } else {
-                let shot = screenshot(args.screenshot, args.screenshot_after);
-                run::gui(config, shot)
+                let startup = xtce_gs_gui::Startup {
+                    screenshot: screenshot(args.screenshot, args.screenshot_after),
+                    history: args.history,
+                };
+                run::gui(config, startup)
             }
         }
 

crates/xtce-gs-cli/src/run.rs

@@ -76,11 +76,8 @@ const CCSDS_INTERLEAVE: [u64; 6] = [1, 2, 3, 4, 5, 8];
 ///
 /// [`CliError::Gui`] for a session that will not start, a layout that will not parse, or a
 /// platform that will not give the process a window.
-pub fn gui(
-    config: SessionConfig,
-    screenshot: Option<xtce_gs_gui::Screenshot>,
-) -> Result<(), CliError> {
-    xtce_gs_gui::run_capturing(config, screenshot).map_err(CliError::from)
+pub fn gui(config: SessionConfig, startup: xtce_gs_gui::Startup) -> Result<(), CliError> {
+    xtce_gs_gui::run_with(config, startup).map_err(CliError::from)
 }
 
 /// Runs with no window, printing counters once a second.

crates/xtce-gs-gui/src/app.rs

@@ -30,7 +30,8 @@ use xtce_gs_engine::Session;
 use xtce_gs_engine::session::log;
 use xtce_model::{ParamId, XtceDb};
 
-use crate::capture::{Capture, Next, Screenshot};
+use crate::Startup;
+use crate::capture::{Capture, Next};
 use crate::layout::Layout;
 use crate::panels::events::Events;
 use crate::panels::plots::{Plots, View};
@@ -165,7 +166,7 @@ impl App {
         cc: &eframe::CreationContext<'_>,
         session: Session,
         mut layout: Layout,
-        screenshot: Option<Screenshot>,
+        startup: Startup,
     ) -> Self {
         let db = Arc::clone(session.db());
         cc.egui_ctx.set_theme(layout.theme.preference());
@@ -177,15 +178,9 @@ impl App {
                 .store()
                 .write()
                 .unwrap_or_else(PoisonError::into_inner);
-            // TODO(gs-gui-history-precedence): this is the saved layout overriding
-            // `--history`, which the CLI documents as "points of history kept per watched
-            // parameter" and does not document as advisory. A flag that looks honoured and is
-            // not is the same failure as `gs-gui-layout` pointing the other way. The decision
-            // is whose intent wins — the flag typed this run, or the depth set in the
-            // interface last run — and it needs clap's `ArgMatches::value_source` threaded out
-            // of `args.rs`, because "given as 4096" and "defaulted to 4096" are the same value
-            // and must stop being the same thing.
-            store.set_depth(layout.history_depth.max(MIN_HISTORY_DEPTH));
+            let depth = depth_in_force(&layout, &startup);
+            layout.history_depth = depth;
+            store.set_depth(depth);
             layout.restore(&db, &mut store)
         };
         if named > 0 {
@@ -203,7 +198,7 @@ impl App {
             db,
             layout,
             status: Status::new(),
-            capture: screenshot.map(Capture::new),
+            capture: startup.screenshot.map(Capture::new),
             // `Tree::new` starts dirty, which is what forces the first frame's copy: the
             // store's generation can legitimately still be 0 at this point.
             tree: Tree::new(),
@@ -525,6 +520,20 @@ impl App {
 /// Split out so that the counting is testable without a window: the case worth getting right
 /// is the one where a definition was edited between two runs, and the operator has to be able
 /// to tell "nothing was saved" from "what was saved is no longer declared".
+/// The history depth in force: what the operator asked for this run, else what the layout
+/// remembers from the last one.
+///
+/// A command line beats a config file everywhere else and does here too. The result is written
+/// back into the layout as well as into the store, so that a save records what was in force —
+/// a depth the operator asked for that reverted silently on the next start would be the same
+/// complaint in a different place.
+fn depth_in_force(layout: &Layout, startup: &Startup) -> usize {
+    startup
+        .history
+        .unwrap_or(layout.history_depth)
+        .max(MIN_HISTORY_DEPTH)
+}
+
 fn restore_report(resolved: usize, named: usize) -> Event {
     if resolved == named {
         Event::info(
@@ -775,4 +784,28 @@ mod tests {
         assert_eq!(event.severity, Severity::Warning);
         assert!(event.message.contains('4'), "{}", event.message);
     }
+
+    #[test]
+    fn a_history_depth_given_on_the_command_line_beats_the_saved_one() {
+        let layout = Layout {
+            history_depth: 4096,
+            ..Layout::default()
+        };
+        let asked = Startup {
+            history: Some(250_000),
+            ..Startup::default()
+        };
+        assert_eq!(depth_in_force(&layout, &asked), 250_000);
+        assert_eq!(depth_in_force(&layout, &Startup::default()), 4096);
+    }
+
+    #[test]
+    fn a_depth_of_zero_is_lifted_to_the_smallest_legal_one() {
+        let layout = Layout::default();
+        let asked = Startup {
+            history: Some(0),
+            ..Startup::default()
+        };
+        assert_eq!(depth_in_force(&layout, &asked), MIN_HISTORY_DEPTH);
+    }
 }

crates/xtce-gs-gui/src/lib.rs

@@ -248,7 +248,20 @@ impl eframe::App for Failure {
 /// the operator has to fix, and a machine with no display is a machine where this is being
 /// read off a terminal anyway.
 pub fn run(config: SessionConfig) -> Result<(), GuiError> {
-    run_capturing(config, None)
+    run_with(config, Startup::default())
+}
+
+/// What the operator asked for on the command line this run, as against what the saved layout
+/// remembers from the last one.
+///
+/// Every field is an `Option` for the same reason: a depth of 4096 typed out and a depth of
+/// 4096 that nobody typed are the same number and not the same instruction.
+#[derive(Clone, Debug, Default, PartialEq, Eq)]
+pub struct Startup {
+    /// Where to write a picture of the window, and how long to fill it first.
+    pub screenshot: Option<Screenshot>,
+    /// Points of history per watched parameter. Given, this beats the layout's own depth.
+    pub history: Option<usize>,
 }
 
 /// [`run`], and a picture of the window on the way past.
@@ -263,10 +276,7 @@ pub fn run(config: SessionConfig) -> Result<(), GuiError> {
 /// fail the run: the session was real and its counters were real, and a station that exited
 /// non-zero over a picture would be the wrong trade for an operator who passed the flag by
 /// accident.
-pub fn run_capturing(
-    config: SessionConfig,
-    screenshot: Option<Screenshot>,
-) -> Result<(), GuiError> {
+pub fn run_with(config: SessionConfig, startup: Startup) -> Result<(), GuiError> {
     let Discarded { layout, note } = Layout::load(&config.definition)?;
     if let Some(note) = note {
         // Before the window, deliberately: this is the one moment the operator still has a
@@ -309,7 +319,7 @@ pub fn run_capturing(
             // The waker has been live since `Session::start`; every call it made before this
             // line did nothing, and nothing was lost by that — see [`RepaintSlot`].
             slot.fill(cc.egui_ctx.clone());
-            Ok(Box::new(App::new(cc, session, layout, screenshot)))
+            Ok(Box::new(App::new(cc, session, layout, startup)))
         }),
     );