From 2f5bbbd68ddb71f79aa454797d90847d09ddd480 Mon Sep 17 00:00:00 2001 From: mattlafy Date: Tue, 13 Oct 2026 05:21:19 -0400 Subject: [PATCH] doc: Space::tab_name / layout / is_paper replace repeated hand-written matches (#888) * doc: Space::is_paper/layout/tab_name helpers replace hand-written matches Twenty-seven call sites in 15 files across engine, io and ui-egui matched Space by hand to get "is this a layout", "which layout" or "Model or the layout's name". Route them through three small helpers on Space instead. No behaviour change. This also makes adding a space kind a few-file change. Co-Authored-By: Claude Opus 5.5 * doc: keep Space::Model comparisons that no helper shortens; test the helpers Only the sites that repeated the same match (tab name, layout name, paper lineweights) go through the helpers; plain Model comparisons and plot_window's match on (space, plotArea) stay as they were. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 Co-authored-by: Brandon Thomas --- crates/doc/src/lib.rs | 20 ++++++++++++++++++++ crates/doc/src/tests.rs | 11 +++++++++++ crates/engine/src/cmd/exchange.rs | 16 +++++----------- crates/engine/src/cmd/inquiry.rs | 2 +- crates/engine/src/cmd/inquiry/count.rs | 5 +---- crates/engine/src/cmd/layout.rs | 20 +++++++++----------- crates/engine/src/cmd/layout/prompts.rs | 15 +++------------ crates/engine/src/cmd/modify2.rs | 2 +- crates/engine/src/cmd/plotters.rs | 6 +++--- crates/engine/src/cmd/view/named_views.rs | 6 +++--- crates/engine/src/sysvars.rs | 9 +-------- crates/io/src/pdf.rs | 16 +++++----------- crates/io/src/plotter.rs | 5 +---- crates/ui-egui/src/managers.rs | 10 ++-------- crates/ui-egui/src/palettes.rs | 6 ++---- crates/ui-egui/src/plotstyles.rs | 6 ++---- 16 files changed, 70 insertions(+), 85 deletions(-) diff --git a/crates/doc/src/lib.rs b/crates/doc/src/lib.rs index d063a83..91fdad3 100644 --- a/crates/doc/src/lib.rs +++ b/crates/doc/src/lib.rs @@ -61,6 +61,26 @@ pub enum Space { Paper(String), } +impl Space { + /// Whether this is a layout's paper space. + pub fn is_paper(&self) -> bool { + matches!(self, Space::Paper(_)) + } + + /// The layout whose paper space this is; `None` for model space. + pub fn layout(&self) -> Option<&str> { + match self { + Space::Paper(n) => Some(n), + Space::Model => None, + } + } + + /// The tab name (CTAB): `Model`, or the layout's name. + pub fn tab_name(&self) -> &str { + self.layout().unwrap_or("Model") + } +} + #[derive(Clone, Debug, PartialEq)] pub struct Drawing { pub header: Header, diff --git a/crates/doc/src/tests.rs b/crates/doc/src/tests.rs index 7c46d8e..87ff828 100644 --- a/crates/doc/src/tests.rs +++ b/crates/doc/src/tests.rs @@ -549,3 +549,14 @@ fn invisible_dimblock_entity_is_ignored_in_dimension_extents() { dim(&mut d, far_line("0")); assert_eq!(d.extents(&Space::Model).max, Vec2::new(6000.0, 6000.0)); } + +#[test] +fn space_names_its_tab() { + assert!(!Space::Model.is_paper()); + assert_eq!(Space::Model.layout(), None); + assert_eq!(Space::Model.tab_name(), "Model"); + let p = Space::Paper("Sheet A".into()); + assert!(p.is_paper()); + assert_eq!(p.layout(), Some("Sheet A")); + assert_eq!(p.tab_name(), "Sheet A"); +} diff --git a/crates/engine/src/cmd/exchange.rs b/crates/engine/src/cmd/exchange.rs index 46c34af..c6b60d6 100644 --- a/crates/engine/src/cmd/exchange.rs +++ b/crates/engine/src/cmd/exchange.rs @@ -292,9 +292,9 @@ fn plot_scale(b: &Bounds2, sheet: &Sheet, fit: bool, scale: Option) -> f64 fn run_preview(s: &mut Session, p: &Value) -> Result { let space = plot_space("preview", s, p)?; let d = s.doc()?; - let mut page = match &space { - Space::Paper(n) => d.layout(n).map(|l| l.page.clone()).unwrap_or_default(), - Space::Model => { + let mut page = match space.layout() { + Some(n) => d.layout(n).map(|l| l.page.clone()).unwrap_or_default(), + None => { // Model space plots on A4 (metric) or ANSI A, like the PDF plotter. let mut page = PageSetup::default(); if paper::paper_unit_mm(d) == 1.0 @@ -317,10 +317,7 @@ fn run_preview(s: &mut Session, p: &Value) -> Result { page.landscape = l; } let sheet = Sheet::from_page(&page, paper::paper_unit_mm(d)); - let lineweights = p.get("lineweights").and_then(Value::as_bool).unwrap_or(match space { - Space::Paper(_) => page.lineweights, - Space::Model => true, - }); + let lineweights = p.get("lineweights").and_then(Value::as_bool).unwrap_or(if space.is_paper() { page.lineweights } else { true }); let fit = p.get("fit").and_then(Value::as_bool).unwrap_or(matches!(space, Space::Model)); let scale = p.get("scale").and_then(Value::as_f64).filter(|v| v.is_finite() && *v > 0.0); let width = p.get("width").and_then(Value::as_u64).unwrap_or(PREVIEW_PX).clamp(16, MAX_PREVIEW_PX); @@ -360,10 +357,7 @@ fn run_preview(s: &mut Session, p: &Value) -> Result { antialias: true, }; let png = cadcraft_render::raster::render_png(&list, &view, &ro).ok_or_else(|| bad("preview", "rendering the preview failed"))?; - let layout = match &space { - Space::Model => "Model".to_string(), - Space::Paper(n) => n.clone(), - }; + let layout = space.tab_name(); Ok(json!({ "data": base64_encode(&png), "width": w, "height": h, "layout": layout, "paper": page.paper, "landscape": page.landscape })) } diff --git a/crates/engine/src/cmd/inquiry.rs b/crates/engine/src/cmd/inquiry.rs index e21e92f..7804946 100644 --- a/crates/engine/src/cmd/inquiry.rs +++ b/crates/engine/src/cmd/inquiry.rs @@ -283,7 +283,7 @@ fn run_inspect(s: &mut Session, p: &Value) -> Result { "title": st.title, "path": st.path, "dirty": st.is_dirty(), - "space": match &space { cadcraft_doc::Space::Model => "Model".to_string(), cadcraft_doc::Space::Paper(n) => n.clone() }, + "space": space.tab_name(), "entityCount": d.entity_count(), "counts": counts, "extents": if ext.is_empty() { Value::Null } else { json!([[ext.min.x, ext.min.y], [ext.max.x, ext.max.y]]) }, diff --git a/crates/engine/src/cmd/inquiry/count.rs b/crates/engine/src/cmd/inquiry/count.rs index ec63253..cce9285 100644 --- a/crates/engine/src/cmd/inquiry/count.rs +++ b/crates/engine/src/cmd/inquiry/count.rs @@ -74,10 +74,7 @@ impl Scope { } /// The top-level objects in scope, each with the name of its space. fn objects<'a>(&self, d: &'a Drawing) -> Vec<(String, &'a std::sync::Arc)> { - let name = |sp: &Space| match sp { - Space::Model => "Model".to_string(), - Space::Paper(n) => n.clone(), - }; + let name = |sp: &Space| sp.tab_name().to_string(); let mut out = Vec::new(); let mut add = |sp: &Space| { if let Some(store) = d.space(sp) { diff --git a/crates/engine/src/cmd/layout.rs b/crates/engine/src/cmd/layout.rs index d2f3da6..2e4c67c 100644 --- a/crates/engine/src/cmd/layout.rs +++ b/crates/engine/src/cmd/layout.rs @@ -260,10 +260,10 @@ fn run_delete(s: &mut Session, p: &Value) -> Result { } fn run_rename(s: &mut Session, p: &Value) -> Result { - let from = match (str_param(p, "from"), s.layout_space()) { + let from = match (str_param(p, "from"), s.layout_space().layout()) { (Some(f), _) => f.to_string(), - (None, Space::Paper(n)) => n, - (None, Space::Model) => return Err(bad("layout.rename", "`from` is required in the Model tab")), + (None, Some(n)) => n.to_string(), + (None, None) => return Err(bad("layout.rename", "`from` is required in the Model tab")), }; let from = layout_name("layout.rename", s.doc()?, &from)?; let to = str_param(p, "to").ok_or_else(|| bad("layout.rename", "`to` is required"))?; @@ -636,10 +636,9 @@ fn run_pagesetup(s: &mut Session, p: &Value) -> Result { "displayPlotStyles", ]; let changes = keys.iter().any(|k| p.get(k).is_some()); - let name = match (str_param(p, "layout"), s.space()) { - (Some(n), _) => Some(layout_name(cmd, s.doc()?, n)?), - (None, Space::Paper(n)) => Some(n), - (None, Space::Model) => None, + let name = match str_param(p, "layout") { + Some(n) => Some(layout_name(cmd, s.doc()?, n)?), + None => s.space().layout().map(str::to_string), }; let Some(name) = name else { if changes { @@ -806,10 +805,9 @@ fn run_plot(s: &mut Session, p: &Value, cmd: &str) -> Result { } } let bytes = hook(s.doc()?, &space, &opts).map_err(|e| bad(cmd, e))?; - let layout = match (&sheets, &space) { - (Some(names), _) => names.first().cloned().unwrap_or_default(), - (None, Space::Model) => "Model".to_string(), - (None, Space::Paper(n)) => n.clone(), + let layout = match &sheets { + Some(names) => names.first().cloned().unwrap_or_default(), + None => space.tab_name().to_string(), }; let pages = sheets.as_ref().map_or(1, Vec::len); match str_param(p, "path") { diff --git a/crates/engine/src/cmd/layout/prompts.rs b/crates/engine/src/cmd/layout/prompts.rs index a7b2e7f..d25b40c 100644 --- a/crates/engine/src/cmd/layout/prompts.rs +++ b/crates/engine/src/cmd/layout/prompts.rs @@ -87,10 +87,7 @@ impl LayoutM { let pats: Vec<&str> = pat.split(',').map(str::trim).filter(|p| !p.is_empty()).take(1000).collect(); let pats = if pats.is_empty() { vec!["*"] } else { pats }; let names: Vec = ls.iter().filter(|l| pats.iter().any(|p| wildcard(p, &l.name))).map(|l| l.name.clone()).collect(); - let current = match s.layout_space() { - Space::Paper(n) => n, - Space::Model => "Model".into(), - }; + let current = s.layout_space().tab_name().to_string(); s.echo(format!("Active Layout: {current}")); if names.is_empty() { s.echo(format!("No layout matches \"{pat}\".")); @@ -131,10 +128,7 @@ impl Interactive for LayoutM { Stage::RenameFrom => with_cur(Prompt::new("Enter layout to rename", Accept::TEXT)), Stage::RenameTo(_) => Prompt::new("Enter new layout name", Accept::TEXT), Stage::Set => { - let tab = match s.layout_space() { - Space::Paper(n) => n, - Space::Model => "Model".into(), - }; + let tab = s.layout_space().tab_name().to_string(); Prompt::new("Enter layout to make current", Accept::TEXT).default(tab) } Stage::List => Prompt::new("Enter layout name(s) to list", Accept::TEXT).default("*"), @@ -199,10 +193,7 @@ impl Interactive for LayoutM { Stage::Set => { let n = match name { Some(n) => n, - None => match s.layout_space() { - Space::Paper(n) => n, - Space::Model => "Model".into(), - }, + None => s.layout_space().tab_name().to_string(), }; let set = find_command("layout.set").ok_or_else(|| EngineError::UnknownCommand("layout.set".into()))?; (set.run)(s, &json!({ "name": n })).map_err(retry)?; diff --git a/crates/engine/src/cmd/modify2.rs b/crates/engine/src/cmd/modify2.rs index 79d74e4..1ce6623 100644 --- a/crates/engine/src/cmd/modify2.rs +++ b/crates/engine/src/cmd/modify2.rs @@ -1452,7 +1452,7 @@ fn chspace(s: &mut Session, hs: &[Handle], to: Option<&str>, vp: Option) fn run_chspace(s: &mut Session, p: &Value) -> Result { let hs = targets(s, p)?; let (n, _, target) = chspace(s, &hs, str_param(p, "to"), curves::handle_param(p, "viewport"))?; - Ok(json!({ "moved": n, "to": match target { Space::Model => "model".to_string(), Space::Paper(n) => n } })) + Ok(json!({ "moved": n, "to": target.layout().unwrap_or("model") })) } #[derive(Clone, Copy, Debug, PartialEq, Eq)] diff --git a/crates/engine/src/cmd/plotters.rs b/crates/engine/src/cmd/plotters.rs index a682728..7cc9521 100644 --- a/crates/engine/src/cmd/plotters.rs +++ b/crates/engine/src/cmd/plotters.rs @@ -286,9 +286,9 @@ pub(crate) fn plot_options(s: &Session, p: &Value, cmd: &str) -> Result { let layout = match str_param(p, "layout") { Some(n) if n.eq_ignore_ascii_case("model") => None, Some(n) => s.doc()?.layout(n).map(|l| l.page.clone()), - None => match s.space() { - cadcraft_doc::Space::Paper(n) => s.doc()?.layout(&n).map(|l| l.page.clone()), - cadcraft_doc::Space::Model => None, + None => match s.space().layout() { + Some(n) => s.doc()?.layout(n).map(|l| l.page.clone()), + None => None, }, }; let dev = match given { diff --git a/crates/engine/src/cmd/view/named_views.rs b/crates/engine/src/cmd/view/named_views.rs index 680e440..40c2563 100644 --- a/crates/engine/src/cmd/view/named_views.rs +++ b/crates/engine/src/cmd/view/named_views.rs @@ -152,10 +152,10 @@ fn restore(s: &mut Session, name: &str) -> Result { match &v.layout { Some(l) => { // A paper space view: its layout (an unknown one: the current or first layout). - let target = match (l.is_empty(), s.layout_space()) { + let target = match (l.is_empty(), s.layout_space().layout()) { (false, _) => l.clone(), - (true, Space::Paper(cur)) => cur, - (true, Space::Model) => s.doc()?.layouts.iter().min_by_key(|x| x.tab_order).map(|x| x.name.clone()).unwrap_or_default(), + (true, Some(cur)) => cur.to_string(), + (true, None) => s.doc()?.layouts.iter().min_by_key(|x| x.tab_order).map(|x| x.name.clone()).unwrap_or_default(), }; if s.doc()?.layout(&target).is_none() { return Err(EngineError::Other(format!("View \"{}\" belongs to layout \"{target}\", which does not exist.", v.name))); diff --git a/crates/engine/src/sysvars.rs b/crates/engine/src/sysvars.rs index 7001c59..2914f61 100644 --- a/crates/engine/src/sysvars.rs +++ b/crates/engine/src/sysvars.rs @@ -116,14 +116,7 @@ pub fn get(s: &Session, name: &str) -> Option { "DBMOD" => json!(s.state().map(|d| i32::from(d.is_dirty())).unwrap_or(0)), // The template NEW/QNEW start from (a profile setting, like FONTALT; "" = none). "QNEWTEMPLATE" => json!(crate::cmd::file::qnew_template()), - "CTAB" => json!( - s.state() - .map(|d| match &d.space { - cadcraft_doc::Space::Model => "Model".to_string(), - cadcraft_doc::Space::Paper(n) => n.clone(), - }) - .unwrap_or_default() - ), + "CTAB" => json!(s.state().map(|d| d.space.tab_name().to_string()).unwrap_or_default()), // UNDO state of the drawing: Control/Auto/group bits and the number of marks. "UNDOCTL" => json!(s.state().ok()?.undo_ctl.bits()), "UNDOMARKS" => json!(s.state().ok()?.undo_marks()), diff --git a/crates/io/src/pdf.rs b/crates/io/src/pdf.rs index 2962b75..c43aa98 100644 --- a/crates/io/src/pdf.rs +++ b/crates/io/src/pdf.rs @@ -265,9 +265,9 @@ fn links_json(v: Option<&Value>) -> Vec<(Handle, String)> { /// The page setup a plot of `space` uses, with overrides applied. pub fn page_for(d: &Drawing, space: &Space, o: &PdfOptions) -> Result { - let mut page = match space { - Space::Paper(n) => d.layout(n).map(|l| l.page.clone()).ok_or_else(|| IoError::Format(format!("no layout `{n}`")))?, - Space::Model => { + let mut page = match space.layout() { + Some(n) => d.layout(n).map(|l| l.page.clone()).ok_or_else(|| IoError::Format(format!("no layout `{n}`")))?, + None => { let mut p = PageSetup::default(); if paper::paper_unit_mm(d) == 1.0 && let Some(a4) = paper::paper_size("A4") @@ -328,10 +328,7 @@ pub fn pdf(d: &Drawing, space: &Space, o: &PdfOptions) -> Result> { /// The name of a sheet: `Model` or the layout's. fn sheet_name(space: &Space) -> String { - match space { - Space::Model => "Model".to_string(), - Space::Paper(n) => n.clone(), - } + space.tab_name().to_string() } /// Write a vector PDF with one page per space (at most [`MAX_PAGES`]), each labelled with its @@ -394,10 +391,7 @@ fn sheet(d: &Drawing, space: &Space, o: &PdfOptions, links_to: &[(Handle, String let page = page_for(d, space, o)?; let unit_mm = paper::paper_unit_mm(d); let sheet = Sheet::from_page(&page, unit_mm); - let lineweights = o.lineweights.unwrap_or(match space { - Space::Paper(_) => page.lineweights, - Space::Model => true, - }); + let lineweights = o.lineweights.unwrap_or(if space.is_paper() { page.lineweights } else { true }); let plot_style_table = cadcraft_doc::plot_style_table(d, &page.plot_style_table).map(std::sync::Arc::new); let ropts = cadcraft_render::Options { tolerance: 0.001, diff --git a/crates/io/src/plotter.rs b/crates/io/src/plotter.rs index cc6a077..f5133e4 100644 --- a/crates/io/src/plotter.rs +++ b/crates/io/src/plotter.rs @@ -76,10 +76,7 @@ fn png(d: &Drawing, space: &Space, opts: &Value) -> Result> { k *= shrink; let px = |v: f64| (v * k).round().clamp(1.0, 16384.0) as u32; let (width, height) = (px(sheet.size.x), px(sheet.size.y)); - let lineweights = o.lineweights.unwrap_or(match space { - Space::Paper(_) => page.lineweights, - Space::Model => true, - }); + let lineweights = o.lineweights.unwrap_or(if space.is_paper() { page.lineweights } else { true }); let table = cadcraft_doc::plot_style_table(d, &page.plot_style_table).map(Arc::new); // Paper units per drawing unit: 1 for a layout at 1:1, the fitted scale for model space. let fit = matches!(space, Space::Model) || o.fit == Some(true); diff --git a/crates/ui-egui/src/managers.rs b/crates/ui-egui/src/managers.rs index b2f7a91..781dde3 100644 --- a/crates/ui-egui/src/managers.rs +++ b/crates/ui-egui/src/managers.rs @@ -474,10 +474,7 @@ fn page_setup(app: &mut CadApp, ctx: &egui::Context, open: &mut bool) -> Action }; let mut layouts: Vec<(u32, String, cadcraft_doc::PageSetup)> = d.layouts.iter().map(|l| (l.tab_order, l.name.clone(), l.page.clone())).collect(); layouts.sort_by_key(|l| l.0); - let shown = match app.session.layout_space() { - cadcraft_doc::Space::Paper(n) => Some(n), - cadcraft_doc::Space::Model => None, - }; + let shown = app.session.layout_space().layout().map(str::to_string); let mut sel = temp::(ctx, "pagesetup_sel") .filter(|s| layouts.iter().any(|l| l.1 == *s)) .or(shown) @@ -900,10 +897,7 @@ fn plot(app: &mut CadApp, ctx: &egui::Context, open: &mut bool) -> Action { targets.extend(layouts.into_iter().map(|l| l.1)); let generation = PLOT_GEN.load(Ordering::Relaxed); let mut st = temp::(ctx, "plot_state").filter(|s| s.generation == generation && targets.contains(&s.target)).unwrap_or_else(|| { - let target = match app.session.layout_space() { - cadcraft_doc::Space::Paper(n) => n, - cadcraft_doc::Space::Model => "Model".into(), - }; + let target = app.session.layout_space().tab_name().to_string(); let mut ext = d.extents(&cadcraft_doc::Space::Model); if ext.is_empty() { ext = cadcraft_geom::Bounds2::new(cadcraft_geom::Vec2::ZERO, cadcraft_geom::Vec2::new(12.0, 9.0)); diff --git a/crates/ui-egui/src/palettes.rs b/crates/ui-egui/src/palettes.rs index e06dc0b..849e0f7 100644 --- a/crates/ui-egui/src/palettes.rs +++ b/crates/ui-egui/src/palettes.rs @@ -749,10 +749,8 @@ pub(crate) fn properties_section(app: &mut CadApp, ui: &mut egui::Ui) { } }); // The current layout's plot style table; named tables give the current plot style. - let layout = match app.session.layout_space() { - cadcraft_doc::Space::Paper(n) => d.layout(&n).map(|l| (n.clone(), l.page.plot_style_table.clone())), - cadcraft_doc::Space::Model => None, - }; + let space = app.session.layout_space(); + let layout = space.layout().and_then(|n| d.layout(n).map(|l| (n.to_string(), l.page.plot_style_table.clone()))); let table = layout.as_ref().and_then(|(_, t)| cadcraft_doc::plot_style_table(d, t)); match table.as_ref().filter(|t| !t.is_color_dependent()) { Some(t) => prop_row(ui, "Plot style", |ui| { diff --git a/crates/ui-egui/src/plotstyles.rs b/crates/ui-egui/src/plotstyles.rs index 5240c6d..614e729 100644 --- a/crates/ui-egui/src/plotstyles.rs +++ b/crates/ui-egui/src/plotstyles.rs @@ -51,10 +51,8 @@ fn window(app: &mut CadApp, ctx: &egui::Context, open: &mut bool) -> Action { return None; }; let names = cadcraft_doc::plot_style_table_names(d); - let layout = match app.session.layout_space() { - cadcraft_doc::Space::Paper(n) => d.layout(&n).map(|l| (n.clone(), l.page.plot_style_table.clone(), l.page.show_plot_styles)), - cadcraft_doc::Space::Model => None, - }; + let space = app.session.layout_space(); + let layout = space.layout().and_then(|n| d.layout(n).map(|l| (n.to_string(), l.page.plot_style_table.clone(), l.page.show_plot_styles))); let mut sel = temp::(ctx, "pstyle_sel") .filter(|s| names.iter().any(|n| n.eq_ignore_ascii_case(s))) .or_else(|| layout.as_ref().map(|l| l.1.clone()).filter(|n| names.iter().any(|x| x.eq_ignore_ascii_case(n))))