From df103575b948d5b52477cc78e2e1ab2e4181e671 Mon Sep 17 00:00:00 2001 From: Karim Jerbi Date: Sun, 9 Aug 2026 21:51:05 +0100 Subject: [PATCH 1/8] feat(scene): REDRAW refresh request resolved to per-viewport instance ids --- src/scene/mod.rs | 151 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 150 insertions(+), 1 deletion(-) diff --git a/src/scene/mod.rs b/src/scene/mod.rs index 79c30d228..319cbd3fa 100644 --- a/src/scene/mod.rs +++ b/src/scene/mod.rs @@ -1381,6 +1381,16 @@ struct SceneLight { web_enabled: bool, } +/// One-shot presentation-cache refresh scope for REDRAW/REDRAWALL. Resolved to +/// concrete `ViewportInstance::instance_id`s at request time; `All` is the +/// superset and dominates. `Active` = current-input viewport (paper sheet in +/// paper space with nothing entered, D6). +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum ViewportRefreshScope { + Active, + All, +} + pub struct Scene { pub camera: Rc>, /// View saved immediately before the latest navigation operation. ZOOM @@ -1729,7 +1739,11 @@ pub struct Scene { /// without depending on the shader widget's internal `Program::State` /// (which can miss events under overlapping overlays). pub viewcube_hover: std::cell::Cell>, - /// Wall time (ms) of the most recent wire re-tessellation — the work done + /// Pending REDRAW force set, keyed by viewport `instance_id`. Populated by + /// `request_refresh` and drained per-id by `refresh_consume` from whichever + /// builder owns that instance. Never touches geometry_epoch/block_epoch. + pending_refresh: std::cell::RefCell>, + /// Wall time (ms) of the most recent wire re-tessellation /// on a wire-cache miss in `model_tile_wires_arc` / `paper_sheet_wires_arc`. /// Stays at the last value while the cache is hit (idle pan/zoom on a warm /// cache reads ~0). Surfaced by the frame-budget HUD (Phase 5.3). @@ -1921,6 +1935,7 @@ impl Scene { last_render_aspect: std::cell::Cell::new(16.0 / 9.0), last_world_per_pixel: std::cell::Cell::new(0.0), viewcube_hover: std::cell::Cell::new(None), + pending_refresh: std::cell::RefCell::new(rustc_hash::FxHashSet::default()), last_tess_ms: std::cell::Cell::new(0.0), last_tess_wires: std::cell::Cell::new(0), last_model_wire_gen: std::cell::Cell::new(0), @@ -2499,6 +2514,63 @@ impl Scene { self.push_geometry_delta(epoch, Vec::new(), true); } + /// Stable per-viewport identity (mirrors the tag used by the renderer so + /// request resolution and builder consumption can never disagree — see + /// render.rs `instance_id` encoding). + pub fn instance_id_for(&self, inst: &ViewportInstance) -> u64 { + if let Some(t) = inst.tile_idx { + 0x1000_0000_0000_0000 | (t as u64) + } else if inst.paper_sheet { + 0x2000_0000_0000_0000 + } else { + 0x3000_0000_0000_0000 | inst.handle.value() + } + } + + /// REDRAW core: resolve a scope to the concrete `instance_id`s it targets + /// and add them to the pending force set. `All` unions over `Active` so the + /// superset always wins (never shrinks a prior `All`). No DB / undo touch. + pub fn request_refresh(&self, scope: ViewportRefreshScope) { + let mut set = self.pending_refresh.borrow_mut(); + if let ViewportRefreshScope::All = scope { + set.clear(); + // All: every current tile in Model, else sheet + every paper content vp. + if self.current_layout == "Model" { + let tiles = self.model_tiles.borrow(); + for (t, _) in tiles.iter().enumerate() { + set.insert(0x1000_0000_0000_0000 | (t as u64)); + } + } else { + set.insert(0x2000_0000_0000_0000); // sheet + let (_sheet, _sheet_h, content) = self.paper_viewport_handles(); + for h in content.iter() { + set.insert(0x3000_0000_0000_0000 | h.value()); + } + } + } else if self.current_layout != "Model" { + // Active in paper: entered content vp, else the sheet (D6(a)). + match self.active_viewport { + Some(h) => { set.insert(0x3000_0000_0000_0000 | h.value()); } + None => { set.insert(0x2000_0000_0000_0000); } + } + } else { + // Active in Model: the active tile (always resolvable). + let active = self.active_model_tile.get(); + set.insert(0x1000_0000_0000_0000 | (active as u64)); + } + } + + /// True while any force is still pending (never cleared falsely). + pub fn refresh_pending_any(&self) -> bool { + !self.pending_refresh.borrow().is_empty() + } + + /// Builder hook: if `instance_id` has a pending force, mark it forced and + /// remove it (one-shot per id). Returns whether to set force_rasterize. + pub(crate) fn refresh_consume(&self, instance_id: u64) -> bool { + self.pending_refresh.borrow_mut().remove(&instance_id) + } + /// Drop a single entity from the tessellation memo so the next render /// re-tessellates just that entity while reusing every other. Pair with /// [`bump_geometry_no_blocks`] for an incremental single-entity edit. @@ -9967,3 +10039,80 @@ mod delta_undo_tests { assert_eq!(ms_occurrences(&scene, copy_h), 1); } } + +#[cfg(test)] +mod redraw_tests { + use super::*; + + fn tile_inst(idx: usize, active: bool) -> ViewportInstance { + ViewportInstance { + handle: Handle::NULL, + tile_idx: Some(idx), + screen_rect: iced::Rectangle { x: 0.0, y: 0.0, width: 1.0, height: 1.0 }, + camera: Camera::default(), + render_mode: acadrust::entities::ViewportRenderMode::Wireframe2D, + active, + grid_on: false, + paper_sheet: false, + } + } + + #[test] + fn request_resolves_to_instance_ids_and_consumes_per_id() { + let scene = Scene::new(); // default: one model tile, index 0, active + let geom_before = scene.geometry_epoch; + let block_before = scene.block_epoch; + + // Active -> the active model tile's instance_id. + scene.request_refresh(ViewportRefreshScope::Active); + let active_id = scene.instance_id_for(&tile_inst(0, true)); + assert!(scene.refresh_pending_any(), "a request must be pending"); + assert!(scene.refresh_consume(active_id), "active tile id must be consumed"); + assert!(!scene.refresh_consume(active_id), "second consume for the same id is empty (one-shot per id)"); + assert!(!scene.refresh_pending_any(), "id set empty after its only target consumed it"); + + // REDRAW never touches geometry / block epochs. + assert_eq!(scene.geometry_epoch, geom_before, "REDRAW must not bump geometry_epoch"); + assert_eq!(scene.block_epoch, block_before, "REDRAW must not bump block_epoch"); + } + + #[test] + fn all_extinguishes_and_then_resurrects_active_after_consumption() { + let scene = Scene::new(); + let t0 = scene.instance_id_for(&tile_inst(0, false)); + + scene.request_refresh(ViewportRefreshScope::Active); + assert!(scene.refresh_consume(t0), "Active targeted tile 0"); + assert!(!scene.refresh_pending_any(), "Active consumed: set now empty"); + + // A subsequent All must re-establish membership even after the Active + // request was fully drained (the superset is resolved at request time). + scene.request_refresh(ViewportRefreshScope::All); + assert!(scene.refresh_pending_any(), "All re-pends after Active was consumed"); + assert!(scene.refresh_consume(t0), "All must re-force tile 0 after Active drained"); + } + + #[test] + fn all_targets_every_existing_model_tile() { + let mut scene = Scene::new(); + // Create MULTIPLE real model tiles through the public split API so the + // test exercises the per-id REDRAWALL guarantee (the very reason for the + // FxHashSet design): every EXISTING tile id must be targeted and drained. + scene.split_active_pane(false); // 2 tiles + scene.split_active_pane(false); // 3 tiles + assert!(scene.model_tiles.borrow().len() >= 3, "split must have produced tiles"); + + scene.request_refresh(ViewportRefreshScope::All); + for idx in 0..scene.model_tiles.borrow().len() { + let id = scene.instance_id_for(&tile_inst(idx, false)); + assert!( + scene.refresh_consume(id), + "All must target EXISTING tile {idx}, not just tile 0" + ); + } + assert!(!scene.refresh_pending_any(), "All drained every existing tile"); + // A tile index that does not exist must NOT be drained. + let ghost = scene.instance_id_for(&tile_inst(99, false)); + assert!(!scene.refresh_consume(ghost), "nonexistent tile 99 has no membership"); + } +} From 7ace7fef9e87733aef7fc4128cd5d76d07b64300 Mon Sep 17 00:00:00 2001 From: Karim Jerbi Date: Sun, 9 Aug 2026 21:56:46 +0100 Subject: [PATCH 2/8] feat(render): per-viewport REDRAW force_rasterize through builders to prepare gate --- src/scene/view/render.rs | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/scene/view/render.rs b/src/scene/view/render.rs index c6ead7099..62370efb8 100644 --- a/src/scene/view/render.rs +++ b/src/scene/view/render.rs @@ -168,6 +168,10 @@ pub struct ViewportData { pub(in crate::scene) compass_rotation: Mat4, pub(in crate::scene) hover_region: Option, pub(in crate::scene) show_viewcube: bool, + /// Set when REDRAW/REDRAWALL requested a forced re-rasterize of this + /// viewport this frame — bypasses the scene-render cache even if the + /// signature is unchanged. Consumed (reset) in `prepare`. + pub(in crate::scene) force_rasterize: bool, /// Header.fill_mode (FILLMODE): when false, hatch / wipeout / face3d-fill /// uploads short-circuit so the renderer draws only wireframe. pub(in crate::scene) fill_mode: bool, @@ -433,6 +437,13 @@ impl shader::Primitive for Primitive { // keeps updating in its own always-on pass, so cube hover still // tracks while the scene is cached. let sig = render_signature(vp, clip_size.width, clip_size.height); + // REDRAW bypass: pin render_sig to force a full pass this frame + // even if the signature is otherwise unchanged. The request is + // one-shot per viewport (consumed by the builder that produced + // this `ViewportData`), so this frame only. + if vp.force_rasterize { + inner.render_sig = u64::MAX; + } let skip = inner.render_sig != u64::MAX && sig == inner.render_sig; inner.render_sig = sig; inner.skip_geometry = skip; @@ -3070,7 +3081,10 @@ impl Scene { let bg_color = [0.0, 0.0, 0.0, 0.0]; let viewports: Vec = instances .iter() - .filter_map(|inst| self.viewport_data_for(inst, canvas, hover_region, show_viewcube)) + .filter_map(|inst| { + let force = self.refresh_consume(self.instance_id_for(inst)); + self.viewport_data_for(inst, canvas, hover_region, show_viewcube, force) + }) .collect(); // Empty viewports → blit nothing; the container background (model bg // or the paper desk colour) stays visible. @@ -3149,8 +3163,9 @@ impl Scene { grid_on: tile.grid_on, paper_sheet: false, }; + let force = self.refresh_consume(self.instance_id_for(&inst)); let viewports = self - .viewport_data_for(&inst, canvas, hover_region, show_viewcube) + .viewport_data_for(&inst, canvas, hover_region, show_viewcube, force) .into_iter() .collect(); let perf_nav = perf_nav.map(|mut sample| { @@ -3175,6 +3190,7 @@ impl Scene { canvas: (f32, f32), hover_region: Option, show_viewcube: bool, + force_rasterize: bool, ) -> Option { let display = self.viewport_display_settings(inst); let mut flags = render_mode_flags(inst.render_mode); @@ -3571,6 +3587,7 @@ impl Scene { }; Some(ViewportData { instance_id, + force_rasterize, wires: Arc::downgrade(&all_wires), clip_boundary_ndc, preview_wires, From 692bb3babf5db2118dbad3fd6652d9b8437a4b4e Mon Sep 17 00:00:00 2001 From: Karim Jerbi Date: Sun, 9 Aug 2026 22:03:17 +0100 Subject: [PATCH 3/8] feat(commands): REDRAW/REDRAWALL/REGEN/REGENALL --- src/app/commands/display.rs | 80 +++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/src/app/commands/display.rs b/src/app/commands/display.rs index 27463534a..b7d8d16a0 100644 --- a/src/app/commands/display.rs +++ b/src/app/commands/display.rs @@ -214,6 +214,37 @@ impl OpenCADStudio { return Some(Task::done(Message::ToggleLayoutTabs)); } + // ── REDRAW / REGEN ────────────────────────────────────────────── + // REDRAW — force a full re-rasterize of the current viewport next + // frame, WITHOUT touching the DB (never bumps geometry_epoch / + // block_epoch, never pushes undo). Scope: Active. This arm does not + // itself clear previews or cancel commands (the normal non-transparent + // dispatch teardown, commands/mod.rs:90-96, governs that separately); + // it only queues a per-viewport cache invalidation. + "REDRAW" => { + use crate::scene::ViewportRefreshScope; + self.tabs[i].scene.request_refresh(ViewportRefreshScope::Active); + self.command_line.push_output("REDRAW: viewport refreshed."); + return Some(Task::none()); + } + // REDRAWALL — force re-rasterize of every generated viewport. + "REDRAWALL" => { + use crate::scene::ViewportRefreshScope; + self.tabs[i].scene.request_refresh(ViewportRefreshScope::All); + self.command_line.push_output("REDRAWALL: viewports refreshed."); + return Some(Task::none()); + } + // REGEN — full model regeneration (bump_geometry: geometry_epoch AND + // block_epoch; C4). No undo, no DB mutation, so do NOT touch + // self.tabs[i].dirty — a newly opened drawing must not become + // "modified" merely because tessellation caches were invalidated (C7). + // REGENALL is functionally identical (C5). + "REGEN" | "REGENALL" => { + self.tabs[i].scene.bump_geometry(); + self.command_line.push_output("REGEN: regenerated model."); + return Some(Task::none()); + } + // ── Drafting aids — same toggles the status-bar pills drive, also // reachable by name from the command line. ───────────────────────── // GRID — show / hide the reference grid. @@ -1223,3 +1254,52 @@ fn parse_landxml_cgpoints(xml: &str) -> Vec<[f64; 3]> { } out } + +#[cfg(test)] +mod tests { + use crate::app::OpenCADStudio; + + fn fresh_app() -> OpenCADStudio { + let mut app = OpenCADStudio::new_for_test(); + app.automation_op(r#"{"op":"new"}"#); + app + } + + #[test] + fn redraw_requests_and_leaves_geometry_untouched() { + let mut app = fresh_app(); + let i = app.active_tab; + let geom_before = app.tabs[i].scene.geometry_epoch; + let block_before = app.tabs[i].scene.block_epoch; + let _ = app.run_command_line("REDRAW"); + assert!( + app.tabs[i].scene.refresh_pending_any(), + "REDRAW must leave a pending force request" + ); + assert_eq!(app.tabs[i].scene.geometry_epoch, geom_before, "REDRAW must not regen"); + assert_eq!(app.tabs[i].scene.block_epoch, block_before, "REDRAW must not regen blocks"); + } + + #[test] + fn redrawall_marks_all_tiles() { + let mut app = fresh_app(); + let i = app.active_tab; + let _ = app.run_command_line("REDRAWALL"); + assert!(app.tabs[i].scene.refresh_pending_any(), "REDRAWALL must leave a force request"); + } + + #[test] + fn regen_rebuilds_but_does_not_dirty_document() { + let mut app = fresh_app(); + let i = app.active_tab; + let geom_before = app.tabs[i].scene.geometry_epoch; + let block_before = app.tabs[i].scene.block_epoch; + app.tabs[i].dirty = false; + let _ = app.run_command_line("REGEN"); + assert_ne!(app.tabs[i].scene.geometry_epoch, geom_before, "REGEN must regenerate geometry"); + assert_ne!(app.tabs[i].scene.block_epoch, block_before, "REGEN must regenerate block epoch"); + assert!(!app.tabs[i].dirty, "REGEN must NOT mark the document as modified (no DB change)"); + let _ = app.run_command_line("REGENALL"); + assert!(!app.tabs[i].dirty, "REGENALL must not dirty the document either"); + } +} From d2e18ead381d1d18a9a0d8dad570bc034d481bbf Mon Sep 17 00:00:00 2001 From: Karim Jerbi Date: Sun, 9 Aug 2026 22:14:11 +0100 Subject: [PATCH 4/8] feat(aliases): R/RA/RE/REA for REDRAW/REGEN; register verbs --- assets/ocad.pgp | 4 ++++ src/app/commands/mod.rs | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/assets/ocad.pgp b/assets/ocad.pgp index c2be1de94..7c1598cd7 100644 --- a/assets/ocad.pgp +++ b/assets/ocad.pgp @@ -130,6 +130,10 @@ UN, *UNITS TP, *TOOLPALETTES SSM, *SHEETSET HI, *HIDE +R, *REDRAW +RA, *REDRAWALL +RE, *REGEN +REA, *REGENALL 3O, *3DORBIT ORBIT, *3DORBIT MV, *MVIEW diff --git a/src/app/commands/mod.rs b/src/app/commands/mod.rs index 6843a5cda..1966d0706 100644 --- a/src/app/commands/mod.rs +++ b/src/app/commands/mod.rs @@ -423,6 +423,11 @@ inventory::submit!(crate::command::CommandRegistration { "XOPEN", "REGION", "REG", + // Redraw / regenerate the display caches. + "REDRAW", + "REDRAWALL", + "REGEN", + "REGENALL", "ARCHIVE", "ETRANSMIT", "FLATSHOT", From 319710f36b67d584470c802abe9680930c7b5f1f Mon Sep 17 00:00:00 2001 From: Karim Jerbi Date: Sun, 9 Aug 2026 22:21:38 +0100 Subject: [PATCH 5/8] test(commands): alias R routes like REDRAW --- src/app/commands/display.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/app/commands/display.rs b/src/app/commands/display.rs index b7d8d16a0..2ad05a00d 100644 --- a/src/app/commands/display.rs +++ b/src/app/commands/display.rs @@ -1280,6 +1280,21 @@ mod tests { assert_eq!(app.tabs[i].scene.block_epoch, block_before, "REDRAW must not regen blocks"); } + #[test] + fn aliases_route_like_full_verbs() { + let mut full = fresh_app(); + let mut short = fresh_app(); + let _ = full.run_command_line("REDRAW"); + let _ = short.run_command_line("R"); + let i = full.active_tab; + assert!(short.tabs[i].scene.refresh_pending_any(), "'R' must trigger REDRAW"); + assert_eq!( + short.tabs[i].scene.refresh_pending_any(), + full.tabs[i].scene.refresh_pending_any(), + "'R' and 'REDRAW' must leave the same refresh state" + ); + } + #[test] fn redrawall_marks_all_tiles() { let mut app = fresh_app(); From de1c1798a0426d8196f1670899ef9bbe6ff6a41f Mon Sep 17 00:00:00 2001 From: Karim Jerbi Date: Mon, 10 Aug 2026 06:20:44 +0100 Subject: [PATCH 6/8] fix(scene): cfg-gate test-only refresh_pending_any to silence dead-code warning --- src/scene/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/scene/mod.rs b/src/scene/mod.rs index 319cbd3fa..3b77126c9 100644 --- a/src/scene/mod.rs +++ b/src/scene/mod.rs @@ -2561,6 +2561,7 @@ impl Scene { } /// True while any force is still pending (never cleared falsely). + #[cfg(test)] pub fn refresh_pending_any(&self) -> bool { !self.pending_refresh.borrow().is_empty() } From 7013aa8cbbb7fdb1c152c38d7f137d0546035eef Mon Sep 17 00:00:00 2001 From: Karim Jerbi Date: Mon, 10 Aug 2026 23:39:46 +0100 Subject: [PATCH 7/8] fix(alias): migrate new default aliases into existing profiles Existing installs only got newly-added default aliases when storage was absent; load_aliases parsed an already-present ocad.pgp or web alias table verbatim, so R/RA/RE/REA remained unknown to profiles seeded before version 2. Introduce a versioned default merge: track the default-table version each profile has seen (wordless ocad.pgp.version marker / localStorage key) and fold in only the aliases introduced after that version, never overwriting user-defined mappings or resurrecting deliberately removed aliases. On first migration the merged table and the advanced marker are persisted so later launches skip the already-delivered aliases. --- src/app/alias.rs | 221 +++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 206 insertions(+), 15 deletions(-) diff --git a/src/app/alias.rs b/src/app/alias.rs index 6888af69a..66c479cf5 100644 --- a/src/app/alias.rs +++ b/src/app/alias.rs @@ -38,9 +38,85 @@ fn alias_file_path() -> Option { Some(crate::config::config_dir()?.join("ocad.pgp")) } +/// Path to the marker recording which default-alias version this profile has +/// seen, `/ocad.pgp.version`. Kept alongside the alias file; its absence +/// means "predates versioning" (treated as version 1). +#[cfg(not(target_arch = "wasm32"))] +fn alias_version_file_path() -> Option { + Some(crate::config::config_dir()?.join("ocad.pgp.version")) +} + +/// Read the default-version a profile has seen. `0` when no marker exists — +/// indistinguishable from a first-run of an already-seeded file, so migrate only +/// neutral additions (never removals) in that case. +#[cfg(not(target_arch = "wasm32"))] +fn read_alias_version(path: &PathBuf) -> u32 { + std::fs::read_to_string(path) + .ok() + .and_then(|s| s.trim().parse::().ok()) + .unwrap_or(0) +} + +/// Read the default-version a profile has seen from web `localStorage`. +#[cfg(target_arch = "wasm32")] +fn read_alias_version_web() -> u32 { + web_sys::window() + .and_then(|window| window.local_storage().ok().flatten()) + .and_then(|storage| storage.get_item(WEB_ALIAS_VERSION_KEY).ok().flatten()) + .and_then(|s| s.parse::().ok()) + .unwrap_or(0) +} + #[cfg(target_arch = "wasm32")] const WEB_ALIAS_KEY: &str = "opencadstudio.aliases"; +/// Version of the shipped default alias table. Bump this whenever the embedded +/// defaults add aliases or change a default target, so existing profiles can be +/// migrated forward (see `introduced_at` / `migrate_aliases`). Version 1 shipped +/// the pre-REDRAW table; version 2 introduced the REDRAW-family aliases. +const DEFAULT_ALIASES_VERSION: u32 = 2; + +#[cfg(target_arch = "wasm32")] +const WEB_ALIAS_VERSION_KEY: &str = "opencadstudio.aliases.version"; + +/// The aliases introduced by a given version of the default table (1-indexed). +/// A profile that last saw version `V` receives each alias in versions `V+1..` +/// that it does not already define. Aliases introduced before `V`, even if the +/// user later removed them, are left alone so deliberate deletions survive. +fn introduced_at(version: u32) -> &'static [(&'static str, &'static str)] { + match version { + 2 => &[ + ("R", "REDRAW"), + ("RA", "REDRAWALL"), + ("RE", "REGEN"), + ("REA", "REGENALL"), + ], + _ => &[], + } +} + +/// Fold the defaults introduced after `from_version` into `stored`, adding only +/// aliases the user does not already define (so overrides and custom targets +/// are preserved, and nothing already present is overwritten). Returns whether +/// any alias was added. +fn migrate_aliases(stored: &mut FxHashMap, from_version: u32) -> bool { + if from_version >= DEFAULT_ALIASES_VERSION { + return false; + } + let mut changed = false; + for version in from_version + 1..=DEFAULT_ALIASES_VERSION { + for (alias, cmd) in introduced_at(version) { + let key = alias.to_uppercase(); + if stored.contains_key(&key) { + continue; + } + stored.insert(key, cmd.to_uppercase()); + changed = true; + } + } + changed +} + /// Parse `.pgp` text into an `alias → command` map, both uppercased. Skips blank /// lines and `;` comments; tolerates a leading `*` on the command and arbitrary /// whitespace padding. @@ -90,31 +166,76 @@ fn default_map() -> FxHashMap { /// text from `localStorage`. Missing or unavailable storage falls back to the /// embedded defaults. pub(super) fn load_aliases() -> FxHashMap { + // After any migration the caller's key differs; we shadow `path` here so + // the version marker maps one-to-one with the alias file that produced it. #[cfg(not(target_arch = "wasm32"))] { - match alias_file_path() { - Some(path) => match std::fs::read_to_string(&path) { - Ok(body) => parse_pgp(&body), - Err(_) => { - // No file yet — copy the shipped default file, best-effort. + let Some(path) = alias_file_path() else { + return default_map(); + }; + return match std::fs::read_to_string(&path) { + Ok(body) => { + let mut map = parse_pgp(&body); + let seen = alias_version_file_path() + .map(|vp| read_alias_version(&vp)) + .unwrap_or(0); + if migrate_aliases(&mut map, seen) { + // Persist the merged table and advance the marker so the + // next boot skips the already-delivered aliases. if let Some(dir) = path.parent() { let _ = std::fs::create_dir_all(dir); } - let _ = std::fs::write(&path, DEFAULT_ALIASES_PGP); - default_map() + let _ = std::fs::write(&path, to_pgp(&map)); + if let Some(vp) = alias_version_file_path() { + let _ = std::fs::write(&vp, DEFAULT_ALIASES_VERSION.to_string()); + } } - }, - None => default_map(), - } + map + } + Err(_) => { + // No file yet — copy the shipped default file, best-effort, and + // mark it as current so a later upgrade merges only new aliases. + if let Some(dir) = path.parent() { + let _ = std::fs::create_dir_all(dir); + } + let _ = std::fs::write(&path, DEFAULT_ALIASES_PGP); + if let Some(vp) = alias_version_file_path() { + let _ = std::fs::write(&vp, DEFAULT_ALIASES_VERSION.to_string()); + } + default_map() + } + }; } #[cfg(target_arch = "wasm32")] { - web_sys::window() - .and_then(|window| window.local_storage().ok().flatten()) - .and_then(|storage| storage.get_item(WEB_ALIAS_KEY).ok().flatten()) - .map(|body| parse_pgp(&body)) - .unwrap_or_else(default_map) + let Some(storage) = + web_sys::window().and_then(|window| window.local_storage().ok().flatten()) + else { + return default_map(); + }; + let stored = storage.get_item(WEB_ALIAS_KEY).ok().flatten(); + return match stored { + Some(body) => { + let mut map = parse_pgp(&body); + let seen = read_alias_version_web(); + if migrate_aliases(&mut map, seen) { + let _ = storage.set_item(WEB_ALIAS_KEY, &to_pgp(&map)); + let _ = storage.set_item( + WEB_ALIAS_VERSION_KEY, + &DEFAULT_ALIASES_VERSION.to_string(), + ); + } + map + } + None => { + let _ = storage.set_item( + WEB_ALIAS_VERSION_KEY, + &DEFAULT_ALIASES_VERSION.to_string(), + ); + default_map() + } + }; } } @@ -187,3 +308,73 @@ impl OpenCADStudio { self.set_command_aliases(map); } } + +#[cfg(test)] +mod tests { + use super::*; + + fn map(pairs: &[(&str, &str)]) -> FxHashMap { + pairs + .iter() + .map(|(a, c)| (a.to_string(), c.to_string())) + .collect() + } + + /// An existing profile that predates the REDRAW aliases gains exactly the + /// new defaults, with every pre-existing mapping kept. + #[test] + fn migration_adds_new_defaults_preserving_existing() { + let mut stored = map(&[("L", "LINE"), ("Z", "ZOOM"), ("HI", "HIDE")]); + let changed = migrate_aliases(&mut stored, 0); + assert!(changed, "migrating from v0 to v2 must add defaults"); + assert_eq!(stored.get("R").map(String::as_str), Some("REDRAW")); + assert_eq!(stored.get("RA").map(String::as_str), Some("REDRAWALL")); + assert_eq!(stored.get("RE").map(String::as_str), Some("REGEN")); + assert_eq!(stored.get("REA").map(String::as_str), Some("REGENALL")); + // Existing entries must survive untouched. + assert_eq!(stored.get("L").map(String::as_str), Some("LINE")); + assert_eq!(stored.get("Z").map(String::as_str), Some("ZOOM")); + assert_eq!(stored.get("HI").map(String::as_str), Some("HIDE")); + } + + /// A user-defined mapping for an alias that collides with a new default is + /// never overwritten by the migration. + #[test] + fn migration_preserves_user_override_of_new_alias() { + let mut stored = map(&[("R", "REGEN")]); + let changed = migrate_aliases(&mut stored, 0); + assert!(changed); + assert_eq!( + stored.get("R").map(String::as_str), + Some("REGEN"), + "user's R=REGEN override must win over default R=REDRAW" + ); + } + + /// A profile already at the current version is left untouched — deliberate + /// removals are not resurrected on later launches. + #[test] + fn migration_is_noop_at_current_version() { + let mut stored = map(&[("L", "LINE")]); + let changed = migrate_aliases(&mut stored, DEFAULT_ALIASES_VERSION); + assert!( + !changed, + "a current profile must not be re-migrated (preserves deletions)" + ); + assert_eq!(stored.len(), 1); + assert_eq!(stored.get("L").map(String::as_str), Some("LINE")); + } + + /// Migrating a mid-range profile (one that saw v1) only pulls in v2's + /// additions, not hypothetical earlier ones. + #[test] + fn migration_from_version_one_adds_v2_aliases() { + let mut stored = map(&[("L", "LINE")]); + let changed = migrate_aliases(&mut stored, 1); + assert!(changed); + assert_eq!(stored.get("R").map(String::as_str), Some("REDRAW")); + assert_eq!(stored.get("RA").map(String::as_str), Some("REDRAWALL")); + assert_eq!(stored.get("RE").map(String::as_str), Some("REGEN")); + assert_eq!(stored.get("REA").map(String::as_str), Some("REGENALL")); + } +} From f3d8904fc7e8169eb04bd47900cffdcb43e06de4 Mon Sep 17 00:00:00 2001 From: Karim Jerbi Date: Mon, 10 Aug 2026 23:49:43 +0100 Subject: [PATCH 8/8] fix(alias): only advance the migration marker after the merge persists Advancing ocad.pgp.version (or the web counterpart) could succeed while the merged alias table write failed, so a later launch skipped migration against a stale file and permanently lost the new aliases. Gate the version write on the alias-table write (native fs::write / web set_item) succeeding. --- src/app/alias.rs | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/src/app/alias.rs b/src/app/alias.rs index 66c479cf5..fc305430c 100644 --- a/src/app/alias.rs +++ b/src/app/alias.rs @@ -180,14 +180,17 @@ pub(super) fn load_aliases() -> FxHashMap { .map(|vp| read_alias_version(&vp)) .unwrap_or(0); if migrate_aliases(&mut map, seen) { - // Persist the merged table and advance the marker so the - // next boot skips the already-delivered aliases. + // Persist the merged table, and only if that succeeds advance + // the version marker so a later launch retries the merge + // (otherwise the new aliases would be lost for good while the + // marker claimed they were delivered). if let Some(dir) = path.parent() { let _ = std::fs::create_dir_all(dir); } - let _ = std::fs::write(&path, to_pgp(&map)); - if let Some(vp) = alias_version_file_path() { - let _ = std::fs::write(&vp, DEFAULT_ALIASES_VERSION.to_string()); + if std::fs::write(&path, to_pgp(&map)).is_ok() { + if let Some(vp) = alias_version_file_path() { + let _ = std::fs::write(&vp, DEFAULT_ALIASES_VERSION.to_string()); + } } } map @@ -220,11 +223,18 @@ pub(super) fn load_aliases() -> FxHashMap { let mut map = parse_pgp(&body); let seen = read_alias_version_web(); if migrate_aliases(&mut map, seen) { - let _ = storage.set_item(WEB_ALIAS_KEY, &to_pgp(&map)); - let _ = storage.set_item( - WEB_ALIAS_VERSION_KEY, - &DEFAULT_ALIASES_VERSION.to_string(), - ); + // Only advance the version marker once the migrated table is + // actually stored, so a failed set_item leaves the marker + // behind and the merge is retried next launch. + if storage + .set_item(WEB_ALIAS_KEY, &to_pgp(&map)) + .is_ok() + { + let _ = storage.set_item( + WEB_ALIAS_VERSION_KEY, + &DEFAULT_ALIASES_VERSION.to_string(), + ); + } } map }