From 5f016e119214367792f8b55efc073aac291a2589 Mon Sep 17 00:00:00 2001 From: Karim Jerbi Date: Mon, 5 Oct 2026 22:45:37 +0100 Subject: [PATCH] fix(layers): carry layer name instead of row index in Message::Layer* Closes the view-to-dispatch resort race: the nine Layer* variants plus ColorPickTarget::Layer now carry the layer name and resolve to the current panel row at dispatch time via layer_panel_index; unknown names no-op. View sites (layer panel, ribbon dropdown) and the CLI/plugin path send names, so the CLI position lookup and the ribbon enumerate are deleted. Panel-internal selection state stays index-based. Tests: layer_toggle_visible_targets_layer_by_name_after_resort pins name targeting across a re-sort; layer_toggle_visible_unknown_name_is_noop pins the graceful no-op. Full suite: 1974 passed / 0 failed / 25 ignored. --- src/app/control/actions.rs | 26 ++++++----- src/app/mod.rs | 24 +++++----- src/app/update/command.rs | 94 ++++++++++++++++++++++++++++++++++++++ src/app/update/mod.rs | 47 +++++++++++++++---- src/app/update/style.rs | 12 +++-- src/ui/ribbon/mod.rs | 11 ++--- src/ui/window/layers.rs | 24 ++++++---- 7 files changed, 186 insertions(+), 52 deletions(-) diff --git a/src/app/control/actions.rs b/src/app/control/actions.rs index 0cfbf624c..951c0ef1d 100644 --- a/src/app/control/actions.rs +++ b/src/app/control/actions.rs @@ -1226,20 +1226,24 @@ impl OpenCADStudio { "undo" => Message::Undo, "redo" => Message::Redo, "layer_visible" | "layer_locked" | "layer_frozen" | "layer_current" => { - let layer = string(req, "layer")?; - // The Layer* messages index the panel's (sortable) row list. - let index = self.tabs[self.active_tab] + let layer = string(req, "layer")?.to_string(); + // The Layer* messages carry the layer name and resolve it to + // the current panel row at dispatch time (#22) — no index + // lookup here, so panel sort order cannot mistarget the CLI. + if !self.tabs[self.active_tab] + .scene + .document .layers - .layers - .iter() - .position(|l| l.name == layer) - .ok_or_else(|| failure("unknown_layer", "Layer does not exist"))?; + .contains(&layer) + { + return Err(failure("unknown_layer", "Layer does not exist")); + } match name { - "layer_visible" => Message::LayerToggleVisible(index), - "layer_locked" => Message::LayerToggleLock(index), - "layer_frozen" => Message::LayerToggleFreeze(index), + "layer_visible" => Message::LayerToggleVisible(layer), + "layer_locked" => Message::LayerToggleLock(layer), + "layer_frozen" => Message::LayerToggleFreeze(layer), _ => { - let select = self.update(Message::LayerSelect(index)); + let select = self.update(Message::LayerSelect(layer)); let current = self.update(Message::LayerSetCurrent); return Ok(Task::batch([select, current])); } diff --git a/src/app/mod.rs b/src/app/mod.rs index 755735f17..807f5a46e 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -1583,8 +1583,8 @@ pub enum ColorPickTarget { PropertiesField(String), /// Current creation colour (ribbon). Ribbon, - /// A layer's colour, by panel row index. - Layer(usize), + /// A layer's colour, by layer name. + Layer(String), /// A saved layer-state layer's colour, by editor row index. LayerState(usize), /// The MText editor's selection (or global) colour. @@ -2669,28 +2669,28 @@ pub enum Message { XrefRowFindReplacePrompt(usize), XrefRowChangePathEnter, XrefRowChangePathLeave, - LayerToggleVisible(usize), - LayerToggleLock(usize), - LayerToggleFreeze(usize), - LayerTogglePlot(usize), + LayerToggleVisible(String), + LayerToggleLock(String), + LayerToggleFreeze(String), + LayerTogglePlot(String), /// Sort the Layer Manager table by a clicked column header. LayerSort(crate::ui::window::layers::LayerSortCol), - /// Toggle per-viewport freeze: (layer_index, vp_col_index) - LayerToggleVpFreeze(usize, usize), + /// Toggle per-viewport freeze: (layer_name, vp_col_index) + LayerToggleVpFreeze(String, usize), LayerNew, LayerDelete, /// Confirm deleting a non-empty layer (erases its objects too). LayerDeleteConfirm, LayerSetCurrent, - LayerSelect(usize), - LayerRenameStart(usize), + LayerSelect(String), + LayerRenameStart(String), LayerRenameEdit(String), - LayerColorPickerToggle(usize), + LayerColorPickerToggle(String), LayerColorMorePalette, LayerColorSet(codec::types::Color), LayerLinetypeSet(String), LayerLineweightSet(LineWeight), - LayerTransparencyEdit(usize, String), + LayerTransparencyEdit(String, String), LayerRenameCommit, // ── Layer State Manager ───────────────────────────────────────────── LayerStateManagerOpen, diff --git a/src/app/update/command.rs b/src/app/update/command.rs index 420fa65c6..dfd004c20 100644 --- a/src/app/update/command.rs +++ b/src/app/update/command.rs @@ -1449,6 +1449,18 @@ pub(super) fn on_tab_close(&mut self, idx: usize) -> Task { Task::none() } + /// Resolve a layer name to its current panel-row index at dispatch time. + /// `Message::Layer*` carries the name (not the row) so a resort between + /// view() and handler dispatch cannot mistarget; `None` + /// (deleted/renamed meanwhile) lets the caller no-op gracefully (#22). + pub(super) fn layer_panel_index(&self, i: usize, name: &str) -> Option { + self.tabs[i] + .layers + .layers + .iter() + .position(|l| l.name == name) + } + /// Layers a Layer-manager row toggle (visibility / lock / freeze / /// transparency) should affect: the whole multi-selection when the clicked /// row is part of it, otherwise just the clicked row — so those toggles work @@ -4540,6 +4552,88 @@ mod layer_rename_tests { } } +#[cfg(test)] +mod layer_name_target_tests { + // RED: `Message::LayerToggleVisible` currently carries a panel-row `usize`; + // these tests pass a layer `String` name and fail to compile until the + // #22 migration lands. + use crate::app::{Message, OpenCADStudio}; + use crate::ui::window::layers::LayerSortCol; + + fn rename_layer(app: &mut OpenCADStudio, old_name: &str, new_name: &str) { + let i = app.active_tab; + let idx = app.tabs[i] + .layers + .layers + .iter() + .position(|layer| layer.name == old_name) + .expect("layer exists in panel"); + app.tabs[i].layers.editing = Some(idx); + app.tabs[i].layers.edit_buf = new_name.to_string(); + let _ = app.on_layer_rename_commit(); + } + + fn app_with_zulu_alpha() -> OpenCADStudio { + let mut app = OpenCADStudio::new_for_test(); + app.automation_op(r#"{"op":"new"}"#); + let _ = app.on_layer_new(); + let first = app.tabs[app.active_tab].layers.edit_buf.clone(); + rename_layer(&mut app, &first, "ZULU"); + let _ = app.on_layer_new(); + let second = app.tabs[app.active_tab].layers.edit_buf.clone(); + rename_layer(&mut app, &second, "ALPHA"); + app + } + + #[test] + fn layer_toggle_visible_targets_layer_by_name_after_resort() { + let mut app = app_with_zulu_alpha(); + // Flip the panel order; a stale row index would now point at the + // wrong layer, but the name still resolves to ZULU. + let _ = app.update(Message::LayerSort(LayerSortCol::Name)); + let _ = app.update(Message::LayerToggleVisible("ZULU".to_string())); + let i = app.active_tab; + let zulu = app.tabs[i] + .layers + .layers + .iter() + .find(|l| l.name == "ZULU") + .expect("ZULU in panel"); + let alpha = app.tabs[i] + .layers + .layers + .iter() + .find(|l| l.name == "ALPHA") + .expect("ALPHA in panel"); + assert!(!zulu.visible, "ZULU should be toggled off"); + assert!(alpha.visible, "ALPHA must be untouched"); + assert!( + app.tabs[i].scene.document.layers.get("ZULU").unwrap().flags.off, + "document ZULU should be off" + ); + } + + #[test] + fn layer_toggle_visible_unknown_name_is_noop() { + let mut app = app_with_zulu_alpha(); + let i = app.active_tab; + let before: Vec<(String, bool)> = app.tabs[i] + .layers + .layers + .iter() + .map(|l| (l.name.clone(), l.visible)) + .collect(); + let _ = app.update(Message::LayerToggleVisible("DELETED".to_string())); + let after: Vec<(String, bool)> = app.tabs[i] + .layers + .layers + .iter() + .map(|l| (l.name.clone(), l.visible)) + .collect(); + assert_eq!(before, after, "unknown name must change nothing"); + } +} + #[cfg(test)] mod plot_style_ctb_guard_tests { use crate::app::OpenCADStudio; diff --git a/src/app/update/mod.rs b/src/app/update/mod.rs index 597c77c7c..9d97758cb 100644 --- a/src/app/update/mod.rs +++ b/src/app/update/mod.rs @@ -3316,8 +3316,11 @@ impl OpenCADStudio { } // ── Layer panel messages ─────────────────────────────────────── - Message::LayerToggleVisible(idx) => { + Message::LayerToggleVisible(name) => { let i = self.active_tab; + let Some(idx) = self.layer_panel_index(i, &name) else { + return Task::none(); + }; // New state = toggle of the clicked row, applied to every target // (the whole selection when the clicked row is part of it) (#236). let on = self.tabs[i].layers.layers.get(idx).map(|l| !l.visible); @@ -3364,8 +3367,11 @@ impl OpenCADStudio { Task::none() } - Message::LayerToggleLock(idx) => { + Message::LayerToggleLock(name) => { let i = self.active_tab; + let Some(idx) = self.layer_panel_index(i, &name) else { + return Task::none(); + }; let locked = self.tabs[i].layers.layers.get(idx).map(|l| !l.locked); let targets = self.layer_row_action_targets(i, idx); if let Some(locked) = locked { @@ -3402,8 +3408,11 @@ impl OpenCADStudio { Task::none() } - Message::LayerToggleFreeze(idx) => { + Message::LayerToggleFreeze(name) => { let i = self.active_tab; + let Some(idx) = self.layer_panel_index(i, &name) else { + return Task::none(); + }; let frozen = self.tabs[i].layers.layers.get(idx).map(|l| !l.frozen); let targets = self.layer_row_action_targets(i, idx); if let Some(frozen) = frozen { @@ -3442,8 +3451,11 @@ impl OpenCADStudio { } Task::none() } - Message::LayerTogglePlot(idx) => { + Message::LayerTogglePlot(name) => { let i = self.active_tab; + let Some(idx) = self.layer_panel_index(i, &name) else { + return Task::none(); + }; let plottable = self.tabs[i] .layers @@ -3493,8 +3505,11 @@ impl OpenCADStudio { Task::none() } - Message::LayerToggleVpFreeze(layer_idx, vp_col_idx) => { - self.on_layer_toggle_vp_freeze(layer_idx, vp_col_idx) + Message::LayerToggleVpFreeze(name, vp_col_idx) => { + let Some(idx) = self.layer_panel_index(self.active_tab, &name) else { + return Task::none(); + }; + self.on_layer_toggle_vp_freeze(idx, vp_col_idx) } Message::LayerNew => self.on_layer_new(), @@ -3505,8 +3520,11 @@ impl OpenCADStudio { Message::LayerSetCurrent => self.on_layer_set_current(), - Message::LayerSelect(idx) => { + Message::LayerSelect(name) => { let i = self.active_tab; + let Some(idx) = self.layer_panel_index(i, &name) else { + return Task::none(); + }; if self.tabs[i].layers.editing.is_some() { return Task::done(Message::LayerRenameCommit); } @@ -3535,8 +3553,11 @@ impl OpenCADStudio { Task::none() } - Message::LayerRenameStart(idx) => { + Message::LayerRenameStart(name) => { let i = self.active_tab; + let Some(idx) = self.layer_panel_index(i, &name) else { + return Task::none(); + }; self.tabs[i].layers.selected = Some(idx); self.tabs[i].layers.selected_multi = vec![idx]; if let Some(layer) = self.tabs[i].layers.layers.get(idx) { @@ -3554,8 +3575,11 @@ impl OpenCADStudio { Message::LayerRenameCommit => self.on_layer_rename_commit(), - Message::LayerColorPickerToggle(idx) => { + Message::LayerColorPickerToggle(name) => { let i = self.active_tab; + let Some(idx) = self.layer_panel_index(i, &name) else { + return Task::none(); + }; let panel = &mut self.tabs[i].layers; if panel.color_picker_row == Some(idx) { panel.color_picker_row = None; @@ -3659,8 +3683,11 @@ impl OpenCADStudio { Task::none() } - Message::LayerTransparencyEdit(idx, s) => { + Message::LayerTransparencyEdit(name, s) => { let i = self.active_tab; + let Some(idx) = self.layer_panel_index(i, &name) else { + return Task::none(); + }; let val = if let Ok(v) = s.parse::() { Some(v.clamp(0, 90)) } else if s.is_empty() { diff --git a/src/app/update/style.rs b/src/app/update/style.rs index 3313a9a3f..329f5d17e 100644 --- a/src/app/update/style.rs +++ b/src/app/update/style.rs @@ -1160,9 +1160,15 @@ pub(super) fn on_text_style_dialog_open(&mut self) -> Task { Some(crate::app::ColorPickTarget::Ribbon) => { Some(Message::RibbonColorChanged(color)) } - Some(crate::app::ColorPickTarget::Layer(idx)) => { - self.tabs[self.active_tab].layers.selected = Some(idx); - Some(Message::LayerColorSet(color)) + Some(crate::app::ColorPickTarget::Layer(name)) => { + let i = self.active_tab; + match self.layer_panel_index(i, &name) { + Some(idx) => { + self.tabs[i].layers.selected = Some(idx); + Some(Message::LayerColorSet(color)) + } + None => None, + } } Some(crate::app::ColorPickTarget::LayerState(idx)) => { Some(Message::LayerStateEditorLayerColor(idx, color)) diff --git a/src/ui/ribbon/mod.rs b/src/ui/ribbon/mod.rs index fdbb9e95c..0cc1e04ee 100644 --- a/src/ui/ribbon/mod.rs +++ b/src/ui/ribbon/mod.rs @@ -992,11 +992,10 @@ impl Ribbon { let rows: Vec> = self .layer_infos .iter() - .enumerate() - .filter(|(_, info)| { + .filter(|info| { filter.is_empty() || info.name.to_lowercase().contains(&filter) }) - .map(|(index, info)| { + .map(|info| { let is_active = info.name == self.active_layer; let lc = info.color; let lv = info.visible; @@ -1019,15 +1018,15 @@ impl Ribbon { let vis = icon_btn( crate::ui::icons::layer_visible(lv), - Message::LayerToggleVisible(index), + Message::LayerToggleVisible(info.name.clone()), ); let freeze = icon_btn( crate::ui::icons::layer_freeze(lf), - Message::LayerToggleFreeze(index), + Message::LayerToggleFreeze(info.name.clone()), ); let lock = icon_btn( crate::ui::icons::layer_lock(ll), - Message::LayerToggleLock(index), + Message::LayerToggleLock(info.name.clone()), ); let checkmark: Element<'_, Message> = crate::ui::icons::themed_check_cell(is_active); diff --git a/src/ui/window/layers.rs b/src/ui/window/layers.rs index d6a4011c5..f28447dcf 100644 --- a/src/ui/window/layers.rs +++ b/src/ui/window/layers.rs @@ -673,6 +673,9 @@ fn layer_row<'a>( vp_cols: &'a [VpCol], name_col_w: f32, ) -> Element<'a, Message> { + // #22: messages carry the layer name (resolved at dispatch time) so a + // resort between view() and the handler cannot mistarget the row. + let lname = layer.name.clone(); let svg_btn = |bytes: &'static [u8], on_press: Message| -> Element<'a, Message> { button(crate::ui::icons::semantic(bytes, ICON_SZ)) .on_press(on_press) @@ -703,7 +706,7 @@ fn layer_row<'a>( }; let plot_btn: Element<'_, Message> = button(plot_icon) - .on_press(Message::LayerTogglePlot(index)) + .on_press(Message::LayerTogglePlot(lname.clone())) .style(cell_button_style(is_selected, index)) .padding(Padding { top: COMBO_PAD_V, @@ -742,7 +745,7 @@ fn layer_row<'a>( text(crate::ui::text_util::elide(&layer.name, name_budget)) .size(FONT_SZ), ) - .on_press(Message::LayerRenameStart(index)) + .on_press(Message::LayerRenameStart(lname.clone())) .style(cell_button_style(is_selected, index)) .padding(Padding { top: COMBO_PAD_V, @@ -774,9 +777,9 @@ fn layer_row<'a>( ..Default::default() }, Message::LayerColorSet, - Message::LayerColorPickerToggle(index), + Message::LayerColorPickerToggle(lname.clone()), Message::OpenColorWindow( - crate::app::ColorPickTarget::Layer(index), + crate::app::ColorPickTarget::Layer(lname.clone()), layer.color, ), )) @@ -841,8 +844,9 @@ fn layer_row<'a>( // Transparency cell let trans_str = layer.transparency.to_string(); + let lname_trans = lname.clone(); let trans_cell = text_input("0", &trans_str) - .on_input(move |s| Message::LayerTransparencyEdit(index, s)) + .on_input(move |s| Message::LayerTransparencyEdit(lname_trans.clone(), s)) .size(FONT_SZ) .padding(Padding { top: COMBO_PAD_V, @@ -864,13 +868,13 @@ fn layer_row<'a>( .align_y(iced::Center), name_cell, iced::widget::Space::new().width(2), - container(svg_btn(vis_svg, Message::LayerToggleVisible(index))) + container(svg_btn(vis_svg, Message::LayerToggleVisible(lname.clone()))) .width(Length::Fixed(COL_ICON)) .align_x(iced::Center), - container(svg_btn(frz_svg, Message::LayerToggleFreeze(index))) + container(svg_btn(frz_svg, Message::LayerToggleFreeze(lname.clone()))) .width(Length::Fixed(COL_ICON)) .align_x(iced::Center), - container(svg_btn(lck_svg, Message::LayerToggleLock(index))) + container(svg_btn(lck_svg, Message::LayerToggleLock(lname.clone()))) .width(Length::Fixed(COL_ICON)) .align_x(iced::Center), container(plot_btn) @@ -892,7 +896,7 @@ fn layer_row<'a>( row_content = row_content.push( container(svg_btn( vp_frz_svg, - Message::LayerToggleVpFreeze(index, vp_idx), + Message::LayerToggleVpFreeze(lname.clone(), vp_idx), )) .width(Length::Fixed(COL_ICON)) .align_x(iced::Center), @@ -925,7 +929,7 @@ fn layer_row<'a>( .height(Length::Fixed(ROW_H)) .width(Fill), ) - .on_press(Message::LayerSelect(index)) + .on_press(Message::LayerSelect(lname)) .into() }