fix(layers): resolve review feedback for row key ownership, others actions, and delete gating

This commit is contained in:
ruchirguitar
2026-11-14 13:25:54 +05:30
parent d909659b45
commit b0379b0309
2 changed files with 154 additions and 71 deletions
+106 -71
View File
@@ -268,6 +268,66 @@ fn disclosure(ui: &Ui, view: &View, n: &Node, (r, x): (egui::Rect, f32), open: b
}
}
/// The row's name label or inline rename field. Returns (name_end, name_rect).
fn row_name(ui: &mut Ui, view: &View, item: &Row, r: egui::Rect, x: f32, out: &mut Out) -> (f32, egui::Rect) {
let n = &*item.node;
let doc = view.doc;
let t = &view.t;
let xml_id = view.identify_xml_id;
let name = if doc.mask_edit.is_some_and(|m| m.layer == n.id) { "<Opacity Mask>".to_string() } else { n.identify_label(xml_id) };
let font = egui::FontId::proportional(13.0);
let renaming: Option<(u64, String)> = ui.data(|d| d.get_temp(rename_id()));
let name_rect = egui::Rect::from_min_max(egui::pos2(x - 2.0, r.center().y - 10.0), egui::pos2(r.right() - 44.0, r.center().y + 10.0));
let name_end = match renaming {
Some((rid, mut buf)) if rid == n.id.0 => {
let field = rename_id().with("field");
let kept = crate::widgets::own_field_shift_enter(ui, field, &mut buf, &name);
let mut child = ui.new_child(egui::UiBuilder::new().max_rect(name_rect));
let te = child
.add(egui::TextEdit::singleline(&mut buf).id(field).align(egui::Align2::LEFT_CENTER).desired_width(name_rect.width()).font(font));
// Focus the field as it opens. Asking every frame took the focus back from Enter,
// Escape or a click elsewhere, so the rename never ended.
if !te.has_focus() && !te.lost_focus() {
te.request_focus();
}
if te.lost_focus() {
ui.data_mut(|d| d.remove::<(u64, String)>(rename_id()));
if child.input(|i| !i.key_pressed(egui::Key::Escape)) && buf != name {
out.actions.push(("layer.setProps".into(), json!({"ids": [n.id.0], "name": buf})));
}
} else {
// Shift+Enter renames it and keeps the field open.
if kept && buf != name {
out.actions.push(("layer.setProps".into(), json!({"ids": [n.id.0], "name": buf.clone()})));
}
ui.data_mut(|d| d.insert_temp(rename_id(), (n.id.0, buf)));
}
name_rect.right()
}
_ => {
let painter = ui.painter().with_clip_rect(name_rect);
// Generated names ("<Path>", "<Opacity Mask>") are translated where painted; the stored name stays English.
// XML IDs stay as ids (not translated).
let shown = if doc.mask_edit.is_some_and(|m| m.layer == n.id) {
tl!("<Opacity Mask>").to_string()
} else if xml_id {
name
} else {
painted_name(n, &n.display_name(), crate::i18n::current())
};
let text = paint_name(&painter, egui::pos2(x, r.center().y), &shown, font, if item.dim { t.text_dim } else { t.text }, prints_italic(n));
// A clipping path's name is underlined, a masked object's with a dashed line.
if item.clip_path {
painter.line_segment([text.left_bottom(), text.right_bottom()], Stroke::new(1.0, t.text));
} else if n.extras().mask.is_some() {
painter.extend(egui::Shape::dashed_line(&[text.left_bottom(), text.right_bottom()], Stroke::new(1.0, t.text), 3.0, 2.0));
}
text.right()
}
};
(name_end, name_rect)
}
/// The widgets of row `item`.
pub(super) fn row(ui: &mut Ui, view: &View, item: &Row, out: &mut Out) {
if let Some(i) = item.guide {
@@ -277,8 +337,7 @@ pub(super) fn row(ui: &mut Ui, view: &View, item: &Row, out: &mut Out) {
return spine_row(ui, view, item, out);
}
let n = &*item.node;
let (depth, clip_path, has_children, open, color) = (item.depth, item.clip_path, item.opens, item.open, item.colour);
let doc = view.doc;
let (depth, has_children, open, color) = (item.depth, item.opens, item.open, item.colour);
let t = &view.t;
let h = view.h;
let (r, resp) = ui.allocate_exact_size(vec2(ui.available_width(), h), Sense::click_and_drag());
@@ -309,60 +368,7 @@ pub(super) fn row(ui: &mut Ui, view: &View, item: &Row, out: &mut Out) {
th
});
// Name (Preferences › Units › Identify Objects By: Object Name or XML ID).
let xml_id = view.identify_xml_id;
let name = if doc.mask_edit.is_some_and(|m| m.layer == n.id) { "<Opacity Mask>".to_string() } else { n.identify_label(xml_id) };
let font = egui::FontId::proportional(13.0);
let renaming: Option<(u64, String)> = ui.data(|d| d.get_temp(rename_id()));
let name_rect = egui::Rect::from_min_max(egui::pos2(x - 2.0, r.center().y - 10.0), egui::pos2(r.right() - 44.0, r.center().y + 10.0));
let name_end;
match renaming {
Some((rid, mut buf)) if rid == n.id.0 => {
let field = rename_id().with("field");
let kept = crate::widgets::own_field_shift_enter(ui, field, &mut buf, &name);
let mut child = ui.new_child(egui::UiBuilder::new().max_rect(name_rect));
let te = child.add(
egui::TextEdit::singleline(&mut buf).id(field).align(egui::Align2::LEFT_CENTER).desired_width(name_rect.width()).font(font.clone()),
);
// Focus the field as it opens. Asking every frame took the focus back from Enter,
// Escape or a click elsewhere, so the rename never ended.
if !te.has_focus() && !te.lost_focus() {
te.request_focus();
}
if te.lost_focus() {
ui.data_mut(|d| d.remove::<(u64, String)>(rename_id()));
if child.input(|i| !i.key_pressed(egui::Key::Escape)) && buf != name {
out.actions.push(("layer.setProps".into(), json!({"ids": [n.id.0], "name": buf})));
}
} else {
// Shift+Enter renames it and keeps the field open.
if kept && buf != name {
out.actions.push(("layer.setProps".into(), json!({"ids": [n.id.0], "name": buf.clone()})));
}
ui.data_mut(|d| d.insert_temp(rename_id(), (n.id.0, buf)));
}
name_end = name_rect.right();
}
_ => {
let painter = ui.painter().with_clip_rect(name_rect);
// Generated names ("<Path>", "<Opacity Mask>") are translated where painted; the stored name stays English.
// XML IDs stay as ids (not translated).
let shown = if doc.mask_edit.is_some_and(|m| m.layer == n.id) {
tl!("<Opacity Mask>").to_string()
} else if xml_id {
name.clone()
} else {
painted_name(n, &n.display_name(), crate::i18n::current())
};
let text = paint_name(&painter, egui::pos2(x, r.center().y), &shown, font, if item.dim { t.text_dim } else { t.text }, prints_italic(n));
name_end = text.right();
// A clipping path's name is underlined, a masked object's with a dashed line.
if clip_path {
painter.line_segment([text.left_bottom(), text.right_bottom()], Stroke::new(1.0, t.text));
} else if n.extras().mask.is_some() {
painter.extend(egui::Shape::dashed_line(&[text.left_bottom(), text.right_bottom()], Stroke::new(1.0, t.text), 3.0, 2.0));
}
}
}
let (name_end, name_rect) = row_name(ui, view, item, r, x, out);
// Target circle and selection square.
let col_x = r.right() - 43.5;
ui.painter().line_segment([egui::pos2(col_x, r.top()), egui::pos2(col_x, r.bottom())], Stroke::new(1.0, t.input_border));
@@ -409,6 +415,10 @@ pub(super) fn row(ui: &mut Ui, view: &View, item: &Row, out: &mut Out) {
// Right-clicking an unselected row selects it; a highlighted row keeps multiselection.
if resp.secondary_clicked() && !view.rows.contains(&n.id) {
out.actions.push(("layer.setCurrent".into(), json!({"id": n.id.0})));
out.rows_after_click = Some(vec![n.id]);
}
if resp.secondary_clicked() {
crate::panels::set_row_keys(ui.ctx(), ROW_KEYS, true);
}
resp.context_menu(|ui| {
row_context_menu(ui, view, n, &collect_ids, out);
@@ -462,21 +472,19 @@ fn row_context_menu(ui: &mut Ui, view: &View, n: &Node, collect_ids: &[u64], out
}
// "Delete "<name>"" or "Delete Selection"
let del_label = if is_multi { tl!("Delete Selection").to_string() } else { format!("{} \"{display}\"", tl!("Delete")) };
if widgets::menu_item_name(ui, &del_label, true, false) {
let del_enabled = {
let listed: std::collections::HashSet<NodeId> = collect_ids.iter().map(|&id| NodeId(id)).collect();
!view.doc.layers.iter().all(|l| listed.contains(&l.id))
};
if widgets::menu_item_name(ui, &del_label, del_enabled, false) {
out.actions.push(("layer.delete".into(), json!({ "ids": collect_ids })));
ui.close();
}
ui.separator();
// Lock Others / Unlock All Layers
{
let (lock_cmd, lock_params) = others_action(view.doc, n, false);
let (lock_cmd, lock_params, lock_enabled) = multi_others_action(view.doc, collect_ids, false);
let lock_label = if lock_cmd == "layer.lockOthers" { "Lock Others" } else { "Unlock All Layers" };
let own = view.doc.layer_of(n.id);
let lock_enabled = if lock_cmd == "layer.lockOthers" {
view.doc.layers.iter().any(|l| Some(l.id) != own && !l.locked)
} else {
view.doc.layers.iter().any(|l| l.locked)
};
if widgets::menu_item(ui, lock_label, lock_enabled, false) {
out.actions.push((lock_cmd, lock_params));
ui.close();
@@ -484,14 +492,8 @@ fn row_context_menu(ui: &mut Ui, view: &View, n: &Node, collect_ids: &[u64], out
}
// Hide Others / Show All Layers
{
let (hide_cmd, hide_params) = others_action(view.doc, n, true);
let (hide_cmd, hide_params, hide_enabled) = multi_others_action(view.doc, collect_ids, true);
let hide_label = if hide_cmd == "layer.hideOthers" { "Hide Others" } else { "Show All Layers" };
let own = view.doc.layer_of(n.id);
let hide_enabled = if hide_cmd == "layer.hideOthers" {
view.doc.layers.iter().any(|l| Some(l.id) != own && l.visible)
} else {
view.doc.layers.iter().any(|l| !l.visible)
};
if widgets::menu_item(ui, hide_label, hide_enabled, false) {
out.actions.push((hide_cmd, hide_params));
ui.close();
@@ -511,6 +513,39 @@ fn row_context_menu(ui: &mut Ui, view: &View, n: &Node, collect_ids: &[u64], out
});
}
/// Context menu Others action for `ids`: Lock Others / Unlock All Layers or Hide Others / Show All Layers.
/// When any other layer is unlocked (visible), returns `lockOthers` (`hideOthers`) preserving all layers holding
/// `ids`. When all other layers are locked (hidden) already, returns `unlockAll` (`showAll`).
pub(crate) fn multi_others_action(doc: &Document, ids: &[u64], eye: bool) -> (String, Value, bool) {
let own_layers: std::collections::HashSet<NodeId> = ids
.iter()
.filter_map(|&id| {
let nid = NodeId(id);
let own = doc.layer_of(nid);
doc.node(nid).map(|n| layer_row(n, own))
})
.collect();
let has_other_active = if eye {
doc.layers.iter().any(|l| !own_layers.contains(&l.id) && l.visible)
} else {
doc.layers.iter().any(|l| !own_layers.contains(&l.id) && !l.locked)
};
let cmd = match (eye, has_other_active) {
(true, true) => "layer.hideOthers",
(true, false) => "layer.showAll",
(false, true) => "layer.lockOthers",
(false, false) => "layer.unlockAll",
};
let enabled = match (eye, has_other_active) {
(true, true) => true,
(true, false) => doc.layers.iter().any(|l| !l.visible),
(false, true) => true,
(false, false) => doc.layers.iter().any(|l| l.locked),
};
let params = if cmd.ends_with("Others") { json!({ "ids": ids }) } else { json!({}) };
(cmd.into(), params, enabled)
}
/// Alt-clicking row `n`'s eye (`eye`) or lock: Hide Others or Lock Others, every top-level layer
/// but the one holding the row; when they all are hidden (locked) already, Show All Layers (Unlock
/// All Layers). Each is one undo step.
+48
View File
@@ -2462,6 +2462,7 @@ fn context_menu_right_click_selects_unselected_row() {
let highlighted = rows_of(&app);
let layer1 = app.session.active().unwrap().doc.layers[0].id;
assert!(highlighted.contains(&layer1), "right-click selected the unselected row");
assert!(crate::panels::has_row_keys(&ctx, super::ROW_KEYS), "secondary click claims row keys");
}
/// Right-clicking a row that IS highlighted preserves the multiselection.
@@ -2525,3 +2526,50 @@ fn context_menu_locate_object_locates_by_id() {
assert!(result.is_ok(), "layer.locate with id succeeds without selection: {:?}", result.err());
assert_eq!(app.session.active().unwrap().highlighted_rows(), vec![a]);
}
/// In a 1-layer document, the Delete action in the context menu is disabled when targeting that layer.
#[test]
fn context_menu_delete_disabled_when_every_layer_targeted() {
let mut app = VectorcraftApp::new(Session::new(), Default::default());
app.session.execute("file.new", &json!({"width": 200, "height": 200})).unwrap();
let doc = &app.session.active().unwrap().doc;
assert_eq!(doc.layers.len(), 1);
let layer_id = doc.layers[0].id;
// Targeting every layer disables delete.
let listed: std::collections::HashSet<NodeId> = [layer_id].into_iter().collect();
let del_enabled = !doc.layers.iter().all(|l| listed.contains(&l.id));
assert!(!del_enabled, "Delete must be disabled when every layer is targeted");
// Add a second layer: delete is now enabled for one layer.
app.session.execute("layer.new", &json!({})).unwrap();
let doc = &app.session.active().unwrap().doc;
assert_eq!(doc.layers.len(), 2);
let del_enabled = !doc.layers.iter().all(|l| listed.contains(&l.id));
assert!(del_enabled, "Delete is enabled when at least one layer remains");
}
/// Others actions (Hide Others / Lock Others) retain all multiselected layers.
#[test]
fn context_menu_others_actions_preserve_multiselected_layers() {
let mut app = VectorcraftApp::new(Session::new(), Default::default());
app.session.execute("file.new", &json!({"width": 200, "height": 200})).unwrap();
app.session.execute("layer.new", &json!({})).unwrap();
app.session.execute("layer.new", &json!({})).unwrap();
let doc = &app.session.active().unwrap().doc;
assert_eq!(doc.layers.len(), 3);
let l1 = doc.layers[0].id.0;
let l2 = doc.layers[1].id.0;
// Hide Others with multiselection [l1, l2] hides only l3.
let (hide_cmd, hide_params, hide_enabled) = super::rows::multi_others_action(doc, &[l1, l2], true);
assert_eq!(hide_cmd, "layer.hideOthers");
assert!(hide_enabled);
assert_eq!(hide_params["ids"], json!([l1, l2]));
// Lock Others with multiselection [l1, l2] locks only l3.
let (lock_cmd, lock_params, lock_enabled) = super::rows::multi_others_action(doc, &[l1, l2], false);
assert_eq!(lock_cmd, "layer.lockOthers");
assert!(lock_enabled);
assert_eq!(lock_params["ids"], json!([l1, l2]));
}