From 6d6a8b14392bcf0272fa7d79d10f5ce96150cbb2 Mon Sep 17 00:00:00 2001 From: Karim Jerbi Date: Fri, 9 Oct 2026 21:20:21 +0100 Subject: [PATCH] fix(layers): key rename and picker state by name, not row LayerPanel.editing and color_picker_row were positional indices, so a sort/sync/filter between start and commit retargeted renames and swatches. Both are Option now; commit resolves fresh with graceful no-op on deleted layers. Existing rename tests migrated. Tests: layer_rename_commit_follows_name_across_resort (RED first) plus deleted/empty no-op and picker-resort pins. Full suite: 2019 passed / 0 failed / 25 ignored. --- src/app/update/command.rs | 173 ++++++++++++++++++++++++++++++++------ src/app/update/mod.rs | 10 +-- src/ui/window/layers.rs | 12 ++- 3 files changed, 158 insertions(+), 37 deletions(-) diff --git a/src/app/update/command.rs b/src/app/update/command.rs index 46cdcf40f..e8ede541f 100644 --- a/src/app/update/command.rs +++ b/src/app/update/command.rs @@ -1378,7 +1378,7 @@ pub(super) fn on_tab_close(&mut self, idx: usize) -> Task { if let Some(idx) = new_idx { self.tabs[i].layers.selected = Some(idx); self.tabs[i].layers.selected_multi = vec![idx]; - self.tabs[i].layers.editing = Some(idx); + self.tabs[i].layers.editing = Some(new_name.clone()); self.tabs[i].layers.edit_buf = new_name.clone(); } self.sync_ribbon_layers(); @@ -1558,16 +1558,18 @@ pub(super) fn on_tab_close(&mut self, idx: usize) -> Task { pub(super) fn on_layer_rename_commit(&mut self) -> Task { let i = self.active_tab; - let editing_idx = self.tabs[i].layers.editing.take(); - if let Some(idx) = editing_idx { + let editing_name = self.tabs[i].layers.editing.take(); + if let Some(old_name) = editing_name { let new_name = self.tabs[i].layers.edit_buf.trim().to_string(); - let old_name = self.tabs[i] + // The name was captured at RenameStart; look it up fresh so + // a sort/sync/filter in between cannot retarget the rename. + // Deleted meanwhile → clear and no-op. + let still_there = self.tabs[i] .layers .layers - .get(idx) - .map(|l| l.name.clone()) - .unwrap_or_default(); - if !new_name.is_empty() && new_name != old_name { + .iter() + .any(|l| l.name == old_name); + if !new_name.is_empty() && new_name != old_name && still_there { self.push_undo_snapshot(i, "LAYER RENAME"); if !self.tabs[i].rename_layer(&old_name, &new_name) { self.discard_last_undo_entry(i); @@ -4445,13 +4447,15 @@ mod layer_rename_tests { 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); + assert!( + app.tabs[i] + .layers + .layers + .iter() + .any(|layer| layer.name == old_name), + "layer exists in panel" + ); + app.tabs[i].layers.editing = Some(old_name.to_string()); app.tabs[i].layers.edit_buf = new_name.to_string(); let _ = app.on_layer_rename_commit(); } @@ -4490,8 +4494,13 @@ mod layer_rename_tests { fn current_layer_rename_updates_name_and_allocated_handle() { let mut app = app_with_editing_layer(); let i = app.active_tab; - let idx = app.tabs[i].layers.editing.expect("new layer is being edited"); - let old_name = app.tabs[i].layers.layers[idx].name.clone(); + let old_name = app.tabs[i].layers.editing.clone().expect("new layer is being edited"); + let idx = app.tabs[i] + .layers + .layers + .iter() + .position(|l| l.name == old_name) + .expect("editing layer in panel"); app.tabs[i] .scene .document @@ -4528,8 +4537,13 @@ mod layer_rename_tests { fn layer_rename_undo_redo_restores_active_layer() { let mut app = app_with_editing_layer(); let i = app.active_tab; - let idx = app.tabs[i].layers.editing.expect("new layer is being edited"); - let old_name = app.tabs[i].layers.layers[idx].name.clone(); + let old_name = app.tabs[i].layers.editing.clone().expect("new layer is being edited"); + let idx = app.tabs[i] + .layers + .layers + .iter() + .position(|l| l.name == old_name) + .expect("editing layer in panel"); app.tabs[i].layers.selected = Some(idx); let _ = app.on_layer_set_current(); @@ -4554,7 +4568,13 @@ mod layer_rename_tests { let old_name = app.tabs[i].layers.edit_buf.clone(); rename_layer(&mut app, &old_name, "A"); let _ = app.on_layer_new(); - let b_idx = app.tabs[i].layers.editing.expect("new layer is being edited"); + let b_name = app.tabs[i].layers.editing.clone().expect("new layer is being edited"); + let b_idx = app.tabs[i] + .layers + .layers + .iter() + .position(|l| l.name == b_name) + .expect("editing layer in panel"); app.tabs[i].layers.selected = Some(b_idx); let _ = app.on_layer_set_current(); let header_name = app.tabs[i].scene.document.header.current_layer_name.clone(); @@ -4617,13 +4637,15 @@ mod layer_name_target_tests { 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); + assert!( + app.tabs[i] + .layers + .layers + .iter() + .any(|layer| layer.name == old_name), + "layer exists in panel" + ); + app.tabs[i].layers.editing = Some(old_name.to_string()); app.tabs[i].layers.edit_buf = new_name.to_string(); let _ = app.on_layer_rename_commit(); } @@ -4668,6 +4690,103 @@ mod layer_name_target_tests { ); } + #[test] + fn layer_rename_commit_follows_name_across_resort() { + // #44: rename state keyed by name survives a panel re-sort; a stale + // row index would rename the wrong layer. + let mut app = app_with_zulu_alpha(); + let i = app.active_tab; + app.tabs[i].layers.editing = Some("ZULU".to_string()); + app.tabs[i].layers.edit_buf = "ZULU_NEW".to_string(); + let _ = app.update(Message::LayerSort(LayerSortCol::Name)); + let _ = app.update(Message::LayerRenameCommit); + let names: Vec = app.tabs[i] + .layers + .layers + .iter() + .map(|l| l.name.clone()) + .collect(); + assert!( + names.contains(&"ZULU_NEW".to_string()), + "ZULU must be renamed, got {names:?}" + ); + assert!( + !names.contains(&"ZULU".to_string()), + "old name must be gone, got {names:?}" + ); + assert!( + app.tabs[i].scene.document.layers.contains("ZULU_NEW"), + "document must follow the rename" + ); + } + + #[test] + fn layer_rename_commit_deleted_layer_is_noop() { + // Renaming a layer deleted mid-edit must clear state without + // renaming anything or pushing undo. + let mut app = app_with_zulu_alpha(); + let i = app.active_tab; + app.finish_pending_history(i); + let undo_before = app.tabs[i].history.undo_stack.len(); + app.tabs[i].layers.editing = Some("GONE".to_string()); + app.tabs[i].layers.edit_buf = "NEW".to_string(); + let _ = app.update(Message::LayerRenameCommit); + assert!(app.tabs[i].layers.editing.is_none(), "stale edit must clear"); + assert!( + app.tabs[i].layers.edit_buf.is_empty(), + "stale edit buffer must clear" + ); + assert_eq!( + app.tabs[i].history.undo_stack.len(), + undo_before, + "no-op commit must not push undo" + ); + assert!( + !app.tabs[i].scene.document.layers.contains("NEW"), + "nothing must be renamed" + ); + } + + #[test] + fn layer_rename_commit_empty_name_is_noop() { + let mut app = app_with_zulu_alpha(); + let i = app.active_tab; + app.finish_pending_history(i); + let undo_before = app.tabs[i].history.undo_stack.len(); + app.tabs[i].layers.editing = Some("ZULU".to_string()); + app.tabs[i].layers.edit_buf = " ".to_string(); + let _ = app.update(Message::LayerRenameCommit); + assert!(app.tabs[i].layers.editing.is_none(), "edit must clear"); + assert!( + app.tabs[i].scene.document.layers.contains("ZULU"), + "blank commit must not rename" + ); + assert_eq!( + app.tabs[i].history.undo_stack.len(), + undo_before, + "blank commit must not push undo" + ); + } + + #[test] + fn layer_color_picker_toggle_follows_name_across_resort() { + // Same name-keying guarantee as rename, for the swatch row. + let mut app = app_with_zulu_alpha(); + let _ = app.update(Message::LayerColorPickerToggle("ZULU".to_string())); + let _ = app.update(Message::LayerSort(LayerSortCol::Name)); + let i = app.active_tab; + assert_eq!( + app.tabs[i].layers.color_picker_row, + Some("ZULU".to_string()), + "picker must stay on ZULU across a resort" + ); + let _ = app.update(Message::LayerColorPickerToggle("ZULU".to_string())); + assert!( + app.tabs[i].layers.color_picker_row.is_none(), + "second toggle must close the picker" + ); + } + #[test] fn layer_toggle_visible_unknown_name_is_noop() { let mut app = app_with_zulu_alpha(); diff --git a/src/app/update/mod.rs b/src/app/update/mod.rs index 202eff1b6..985df0ebb 100644 --- a/src/app/update/mod.rs +++ b/src/app/update/mod.rs @@ -3612,10 +3612,8 @@ impl OpenCADStudio { }; 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) { - self.tabs[i].layers.edit_buf = layer.name.clone(); - } - self.tabs[i].layers.editing = Some(idx); + self.tabs[i].layers.edit_buf = name.clone(); + self.tabs[i].layers.editing = Some(name); Task::none() } @@ -3633,11 +3631,11 @@ impl OpenCADStudio { return Task::none(); }; let panel = &mut self.tabs[i].layers; - if panel.color_picker_row == Some(idx) { + if panel.color_picker_row.as_deref() == Some(name.as_str()) { panel.color_picker_row = None; panel.color_full_palette = false; } else { - panel.color_picker_row = Some(idx); + panel.color_picker_row = Some(name); panel.color_full_palette = false; panel.selected = Some(idx); // Opening the swatch on a row outside the current diff --git a/src/ui/window/layers.rs b/src/ui/window/layers.rs index f28447dcf..0a4d62c39 100644 --- a/src/ui/window/layers.rs +++ b/src/ui/window/layers.rs @@ -127,11 +127,15 @@ pub struct LayerPanel { /// All selected rows (Ctrl/Shift extend it). Bulk property changes and /// deletion act on every row here. Empty ⇔ nothing selected. pub selected_multi: Vec, - pub editing: Option, + /// Layer being renamed (row text-input open). Keyed by NAME so a + /// rebuild/sort between start and commit cannot retarget it (#44); + /// `selected`/`selected_multi` below stay index-based and are re-resolved. + pub editing: Option, pub edit_buf: String, pub current_layer: String, pub linetype_items: Vec, - pub color_picker_row: Option, + /// Row whose color swatch is open. Name-keyed for the same reason (#44). + pub color_picker_row: Option, pub color_full_palette: bool, pub linetype_combo: combo_box::State, pub lw_combo: combo_box::State, @@ -459,8 +463,8 @@ impl LayerPanel { let is_anchor = self.selected == Some(i); let is_sel = is_anchor || self.selected_multi.contains(&i); let is_current = layer.name == self.current_layer; - let is_editing = self.editing == Some(i); - let color_open = self.color_picker_row == Some(i); + let is_editing = self.editing.as_deref() == Some(layer.name.as_str()); + let color_open = self.color_picker_row.as_deref() == Some(layer.name.as_str()); let (ltc, lwc) = if is_anchor { (Some(&self.linetype_combo), Some(&self.lw_combo))