forked from 1b-agent/opencadstudio-upstream
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.
This commit is contained in:
1 parent
c5f8eb2921
commit
5f016e1192
7 files changed
+186
-52
No files matched your search
+15
-11
@@ -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]));
|
||||
}
|
||||
|
||||
+12
-12
@@ -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,
|
||||
|
||||
@@ -1449,6 +1449,18 @@ pub(super) fn on_tab_close(&mut self, idx: usize) -> Task<Message> {
|
||||
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<usize> {
|
||||
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;
|
||||
|
||||
+37
-10
@@ -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::<i32>() {
|
||||
Some(v.clamp(0, 90))
|
||||
} else if s.is_empty() {
|
||||
|
||||
@@ -1160,9 +1160,15 @@ pub(super) fn on_text_style_dialog_open(&mut self) -> Task<Message> {
|
||||
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))
|
||||
|
||||
@@ -992,11 +992,10 @@ impl Ribbon {
|
||||
let rows: Vec<Element<Message>> = 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);
|
||||
|
||||
+14
-10
@@ -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()
|
||||
}
|
||||
|
||||
|
||||
Reference in new issue
Block a user