From ac4ce3eefd32cde23b6f4fc13cabfb3c5ccade34 Mon Sep 17 00:00:00 2001 From: Maciej Myszkiewicz Date: Sun, 11 Oct 2026 21:43:38 +0200 Subject: [PATCH] fix(gui): --history given on the command line beats the saved layout It did nothing whenever a layout existed, which since last week is always for the bundled demo: the station read the depth out of the layout file and overwrote whatever the flag said. A flag that looks honoured and is not. The decision the TODO was waiting on: the command line wins, the way it does over a config file everywhere else. Left out, the layout's depth stands. What made it possible to tell them apart is that `--history` is now an `Option` rather than a `default_value_t`: given as 4096 and defaulted to 4096 were the same value and had to stop being the same instruction. The gui's entry point takes a `Startup` carrying what the operator asked for this run, which is also where the screenshot request moved to. The depth in force is written back into the layout, so a save records it. A depth somebody asked for that reverted silently on the next start would be the same complaint one place along. TODO(gs-gui-history-precedence) is closed; the index counts move with it, and one of them was already stale. 538 tests, fmt and clippy clean. --- TODO.md | 3 +- crates/xtce-gs-cli/src/args.rs | 21 +++++++++---- crates/xtce-gs-cli/src/main.rs | 14 ++++++--- crates/xtce-gs-cli/src/run.rs | 7 ++--- crates/xtce-gs-gui/src/app.rs | 57 +++++++++++++++++++++++++++------- crates/xtce-gs-gui/src/lib.rs | 22 +++++++++---- 6 files changed, 89 insertions(+), 35 deletions(-) diff --git a/TODO.md b/TODO.md index 29f8c50..d02e26c 100644 --- a/TODO.md +++ b/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` diff --git a/crates/xtce-gs-cli/src/args.rs b/crates/xtce-gs-cli/src/args.rs index 191ece5..635ee83 100644 --- a/crates/xtce-gs-cli/src/args.rs +++ b/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, /// 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::()?, 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, /// 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, diff --git a/crates/xtce-gs-cli/src/main.rs b/crates/xtce-gs-cli/src/main.rs index a0165c7..65e7217 100644 --- a/crates/xtce-gs-cli/src/main.rs +++ b/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) } } diff --git a/crates/xtce-gs-cli/src/run.rs b/crates/xtce-gs-cli/src/run.rs index 92150aa..3269e5e 100644 --- a/crates/xtce-gs-cli/src/run.rs +++ b/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, -) -> 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. diff --git a/crates/xtce-gs-gui/src/app.rs b/crates/xtce-gs-gui/src/app.rs index c812809..c466460 100644 --- a/crates/xtce-gs-gui/src/app.rs +++ b/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, + 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); + } } diff --git a/crates/xtce-gs-gui/src/lib.rs b/crates/xtce-gs-gui/src/lib.rs index 343f5ad..f3ad33e 100644 --- a/crates/xtce-gs-gui/src/lib.rs +++ b/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, + /// Points of history per watched parameter. Given, this beats the layout's own depth. + pub history: Option, } /// [`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, -) -> 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))) }), );