From a11446f2f0df099eb06d7bbb7ca405b10d9d357c Mon Sep 17 00:00:00 2001 From: vangel76 Date: Sun, 11 Oct 2026 12:09:19 +0200 Subject: [PATCH] =?UTF-8?q?Name=20Manager:=20New=E2=80=A6=20and=20Edit?= =?UTF-8?q?=E2=80=A6=20open=20the=20name=20dialog=20and=20return=20to=20th?= =?UTF-8?q?e=20manager=20(#238)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dialogs::show put the Name Manager back into app.dialog after its body had opened the New Name dialog, so New… did nothing. A dialog opened from another one now takes over and the opener waits behind it (back), coming back when the child closes; Go To still hands over to Go To Special. The Name Manager gets Excel's New…/Edit…/Delete buttons over a selectable list. formulas.defineName takes oldName/oldScope to edit a name in place; a renamed name is renamed in cell formulas and other names' definitions. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/formulas.rs | 74 ++++++++++++++- crates/engine/src/tests.rs | 20 ++++ crates/ui-egui/src/dialogs.rs | 140 ++++++++++++++++++++-------- crates/ui-egui/tests/interaction.rs | 37 ++++++++ 4 files changed, 231 insertions(+), 40 deletions(-) diff --git a/crates/engine/src/cmd/formulas.rs b/crates/engine/src/cmd/formulas.rs index f3576e4..ae301c9 100644 --- a/crates/engine/src/cmd/formulas.rs +++ b/crates/engine/src/cmd/formulas.rs @@ -1,7 +1,7 @@ //! Formulas tab: AutoSum, Insert Function, names, auditing, calculation. use gridcraft_core::{CellRef, RangeRef}; -use gridcraft_model::{CalcMode, DefinedName}; +use gridcraft_model::{CalcMode, DefinedName, Formula}; use serde_json::{Value as Json, json}; use super::*; @@ -32,7 +32,7 @@ pub fn specs() -> Vec { "Define Name…", ["Formulas", "Defined Names"], None, - "{name, refersTo?: \"=Sheet1!$A$1:$A$10\" (default: the selection), scope?: \"Workbook\"|sheet name, comment?}", + "{name, refersTo?: \"=Sheet1!$A$1:$A$10\" (default: the selection), scope?: \"Workbook\"|sheet name, comment?, oldName?, oldScope?} (oldName: edit that name (Name Manager › Edit); a new name also renames it in formulas)", has_doc, define_name ), @@ -265,7 +265,32 @@ fn define_name(s: &mut Session, p: &Json) -> Result { return Err(EngineError::Other("There's a problem with this formula.".into())); } let comment = str_param(p, "comment").unwrap_or("").to_string(); + // Editing an existing name (Name Manager › Edit…): it must exist, and its new name must not + // collide with another name in the target scope. + let old = match str_param(p, "oldName") { + Some(old_name) => { + let old_scope = match str_param(p, "oldScope") { + None | Some("Workbook") => None, + Some(sh) => Some(d.wb.sheet_index(sh).ok_or_else(|| bad("formulas.defineName", "no such sheet"))?), + }; + if !d.wb.names.iter().any(|n| n.name.eq_ignore_ascii_case(old_name) && n.scope == old_scope) { + return Err(bad("formulas.defineName", "no such name to edit")); + } + let same = old_name.eq_ignore_ascii_case(&name) && old_scope == scope; + if !same && d.wb.names.iter().any(|n| n.name.eq_ignore_ascii_case(&name) && n.scope == scope) { + return Err(EngineError::Other("The name entered already exists. Enter a unique name.".into())); + } + Some((old_name.to_string(), old_scope)) + } + None => None, + }; edit(s, |cx| { + if let Some((old_name, old_scope)) = &old { + cx.wb.names.retain(|n| !(n.name.eq_ignore_ascii_case(old_name) && n.scope == *old_scope)); + if !old_name.eq_ignore_ascii_case(&name) { + rename_name_in_formulas(&mut cx.wb, old_name, *old_scope, &name); + } + } cx.wb.names.retain(|n| !(n.name.eq_ignore_ascii_case(&name) && n.scope == scope)); cx.wb.names.push(DefinedName { name: name.clone(), scope, formula: refers.clone(), comment: comment.clone(), hidden: false }); cx.structural = true; @@ -273,6 +298,51 @@ fn define_name(s: &mut Session, p: &Json) -> Result { }) } +/// Formulas that use a renamed name follow it: cell formulas and other names' definitions, on +/// the sheets where `old` resolves to the renamed name (its own sheet for a sheet-scoped name; +/// every sheet without a same-named sheet-level name for a workbook-level one). +fn rename_name_in_formulas(wb: &mut gridcraft_model::Workbook, old: &str, old_scope: Option, new: &str) { + let shadowed: Vec = wb.names.iter().filter(|n| n.name.eq_ignore_ascii_case(old)).filter_map(|n| n.scope).collect(); + let applies = |sheet: usize| match old_scope { + Some(s) => s == sheet, + None => !shadowed.contains(&sheet), + }; + // The renamed expression, or `None` when the name doesn't occur in it. + let rename = |e: gridcraft_formula::Expr| { + let mut hit = false; + let out = e.map(&mut |x| match x { + gridcraft_formula::Expr::Name(n) if n.eq_ignore_ascii_case(old) => { + hit = true; + gridcraft_formula::Expr::Name(new.to_string()) + } + o => o, + }); + hit.then_some(out) + }; + for i in 0..wb.sheets.len() { + if !applies(i) { + continue; + } + let Some(sh) = wb.sheet_mut(i) else { continue }; + let formulas: Vec<(CellRef, std::sync::Arc)> = sh.cells.iter().filter_map(|(c, x)| x.formula.clone().map(|f| (c, f))).collect(); + for (c, f) in formulas { + let Some(e) = f.expr().and_then(rename) else { continue }; + let mut nf = Formula::from_expr(e); + nf.array = f.array; + if let Some(cell) = sh.cells.get_mut(c) { + cell.formula = Some(std::sync::Arc::new(nf)); + } + } + } + for n in wb.names.iter_mut() { + if n.scope.is_none_or(applies) + && let Some(e) = gridcraft_formula::parse(&n.formula).ok().and_then(rename) + { + n.formula = gridcraft_formula::print(&e); + } + } +} + fn delete_name(s: &mut Session, p: &Json) -> Result { let name = str_param(p, "name").ok_or_else(|| bad("formulas.deleteName", "missing `name`"))?.to_string(); let scope = match str_param(p, "scope") { diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 6bd03d5..37ff3dc 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -52,6 +52,26 @@ fn entering_empty_cells_preserves_whole_sheet_formatting() { } } +#[test] +fn editing_a_name_renames_it_in_formulas() { + // Name Manager › Edit…: `oldName` edits the name in place; formulas that use it follow. + let mut s = s(); + s.execute("cell.set", json!({"cell": "A1", "input": "21"})).unwrap(); + s.execute("formulas.defineName", json!({"name": "Price", "refersTo": "=Sheet1!$A$1"})).unwrap(); + s.execute("formulas.defineName", json!({"name": "Other", "refersTo": "=1"})).unwrap(); + s.execute("cell.set", json!({"cell": "B1", "input": "=Price*2"})).unwrap(); + let err = s.execute("formulas.defineName", json!({"name": "Other", "refersTo": "=Sheet1!$A$1", "oldName": "Price"})); + assert!(err.is_err(), "renaming onto another existing name is refused"); + s.execute("formulas.defineName", json!({"name": "Cost", "refersTo": "=Sheet1!$A$1", "comment": "unit", "oldName": "Price"})).unwrap(); + let d = s.doc().unwrap(); + let names: Vec<&str> = d.wb.names.iter().map(|n| n.name.as_str()).collect(); + assert_eq!(names, ["Other", "Cost"]); + let b1 = d.wb.active().unwrap().cell(CellRef::parse("B1").unwrap()).and_then(|c| c.formula.clone()).unwrap(); + assert_eq!(b1.text, "Cost*2"); + assert_eq!(v(&s, "B1"), Value::number(42.0)); + assert!(s.execute("formulas.defineName", json!({"name": "X", "oldName": "Missing"})).is_err()); +} + #[test] fn entry_and_text_paste_use_inherited_formats_and_cell_overrides() { let mut s = s(); diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index 7dd3acb..e772446 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -33,6 +33,8 @@ pub struct Dialog { pub result: Option, picture_inbox: Option, picture_target: Option<(u64, usize)>, + /// The dialog this one was opened from (Name Manager › New…): it comes back when this closes. + back: Option>, } impl Dialog { @@ -53,6 +55,7 @@ impl Dialog { result: None, picture_inbox: None, picture_target: None, + back: None, } } @@ -69,6 +72,7 @@ impl Dialog { result: None, picture_inbox: None, picture_target: None, + back: None, } } @@ -353,18 +357,32 @@ impl Dialog { ], json!({"percent": app.session.active().and_then(|d| d.wb.active().map(|s| s.zoom)).unwrap_or(100)}), ), - "defineName" => Dialog::form( - "defineName", - "New Name", - "formulas.defineName", - vec![ - Text { key: "name", label: "Name:" }, - Text { key: "scope", label: "Scope (Workbook or sheet name):" }, - Text { key: "comment", label: "Comment:" }, - Text { key: "refersTo", label: "Refers to:" }, - ], - json!({"scope": "Workbook", "refersTo": app.session.active().and_then(|d| d.wb.active().map(|s| format!("={}!{}", gridcraft_engine::formula::quote_sheet(&s.name), abs(&d.selection.current()))))}), - ), + "defineName" => { + // Name Manager › Edit… passes the name being edited (`oldName`, `oldScope`) and + // its current fields; New… and Define Name start from the selection. + let editing = params.get("oldName").and_then(Json::as_str).is_some(); + let selection = app.session.active().and_then(|d| { + d.wb.active().map(|s| format!("={}!{}", gridcraft_engine::formula::quote_sheet(&s.name), abs(&d.selection.current()))) + }); + let mut defaults = json!({"scope": "Workbook", "refersTo": selection}); + for key in ["name", "scope", "comment", "refersTo", "oldName", "oldScope"] { + if let Some(v) = params.get(key).filter(|v| v.is_string()) { + defaults[key] = v.clone(); + } + } + Dialog::form( + "defineName", + if editing { "Edit Name" } else { "New Name" }, + "formulas.defineName", + vec![ + Text { key: "name", label: "Name:" }, + Text { key: "scope", label: "Scope (Workbook or sheet name):" }, + Text { key: "comment", label: "Comment:" }, + Text { key: "refersTo", label: "Refers to:" }, + ], + defaults, + ) + } "dataValidation" => Dialog::form( "dataValidation", "Data Validation", @@ -636,10 +654,12 @@ fn build_params(d: &Dialog) -> Json { } } "defineName" => { - if d.values.get("scope").and_then(Json::as_str) == Some("Workbook") - && let Some(m) = v.as_object_mut() - { - m.remove("scope"); + if let Some(m) = v.as_object_mut() { + for key in ["scope", "oldScope"] { + if m.get(key).and_then(Json::as_str) == Some("Workbook") { + m.remove(key); + } + } } } _ => {} @@ -677,7 +697,7 @@ pub fn show(app: &mut SheetApp, ctx: &egui::Context) { "find" => find(app, ui, &mut d), "goTo" => go_to(app, ui, &mut d, &mut confirm), "sort" => sort(app, ui, &mut d, &mut confirm), - "nameManager" => name_manager(app, ui), + "nameManager" => name_manager(app, ui, &mut d, &mut open), "manageRules" => manage_rules(app, ui), "commandSearch" => command_search(app, ui, &mut d, &mut confirm), "goalSeek" => goal_seek(app, ui, &mut d, &mut confirm), @@ -883,8 +903,21 @@ pub fn show(app: &mut SheetApp, ctx: &egui::Context) { } else if confirm { open = false; } + if let Some(child) = app.dialog.as_mut() { + // The body opened another dialog (Name Manager › New…). It takes over; this one waits + // behind it and comes back when it closes, as Excel returns to the Name Manager. Go To + // hands over to Go To Special for good, as in Excel. + if open && d.name != "goTo" { + child.back = Some(Box::new(d)); + } else if child.back.is_none() { + child.back = d.back.take(); + } + return; + } if open { app.dialog = Some(d); + } else if let Some(back) = d.back.take() { + app.dialog = Some(*back); } } @@ -1485,30 +1518,61 @@ fn sort(app: &mut SheetApp, ui: &mut egui::Ui, d: &mut Dialog, confirm: &mut boo } } -fn name_manager(app: &mut SheetApp, ui: &mut egui::Ui) { +fn name_manager(app: &mut SheetApp, ui: &mut egui::Ui, d: &mut Dialog, open: &mut bool) { let list = app.session.run("formulas.nameManager", json!({})).unwrap_or(Json::Null); - egui::Grid::new("names").striped(true).num_columns(4).show(ui, |ui| { - ui.strong("Name"); - ui.strong("Value"); - ui.strong("Refers To"); - ui.strong("Scope"); - ui.end_row(); - for n in list.as_array().cloned().unwrap_or_default() { - ui.label(n["name"].as_str().unwrap_or("")); - ui.label(n["value"].to_string().chars().take(30).collect::()); - ui.label(n["refersTo"].as_str().unwrap_or("")); - ui.horizontal(|ui| { - ui.label(n["scope"].as_str().unwrap_or("")); - if ui.small_button("Delete").clicked() { - app.run_or_alert("formulas.deleteName", json!({"name": n["name"], "scope": n["scope"]})); - } - }); - ui.end_row(); + let names = list.as_array().cloned().unwrap_or_default(); + let selected = names.get(d.list_index).cloned(); + let field = |n: &Json, k: &str| n.get(k).and_then(Json::as_str).unwrap_or("").to_string(); + ui.horizontal(|ui| { + if ui.button("New…").clicked() { + app.open_dialog("defineName", json!({})); + } + if ui.add_enabled(selected.is_some(), egui::Button::new("Edit…")).clicked() + && let Some(n) = &selected + { + app.open_dialog( + "defineName", + json!({ + "name": field(n, "name"), "scope": field(n, "scope"), "comment": field(n, "comment"), + "refersTo": field(n, "refersTo"), "oldName": field(n, "name"), "oldScope": field(n, "scope"), + }), + ); + } + if ui.add_enabled(selected.is_some(), egui::Button::new("Delete")).clicked() + && let Some(n) = &selected + { + app.run_or_alert("formulas.deleteName", json!({"name": field(n, "name"), "scope": field(n, "scope")})); + } + }); + ui.add_space(4.0); + egui::ScrollArea::vertical().max_height(260.0).show(ui, |ui| { + egui::Grid::new("names").striped(true).num_columns(4).show(ui, |ui| { + ui.strong("Name"); + ui.strong("Value"); + ui.strong("Refers To"); + ui.strong("Scope"); + ui.end_row(); + if names.is_empty() { + ui.label(egui::RichText::new("No names defined").italics()); + ui.end_row(); + } + for (i, n) in names.iter().enumerate() { + if ui.selectable_label(i == d.list_index, field(n, "name")).clicked() { + d.list_index = i; + } + ui.label(n.get("value").map(|v| v.to_string()).unwrap_or_default().chars().take(30).collect::()); + ui.label(field(n, "refersTo")); + ui.label(field(n, "scope")); + ui.end_row(); + } + }); + }); + ui.add_space(8.0); + ui.with_layout(egui::Layout::right_to_left(egui::Align::Center), |ui| { + if ui.button(" Close ").clicked() { + *open = false; } }); - if ui.button("New…").clicked() { - app.open_dialog("defineName", json!({})); - } } fn manage_rules(app: &mut SheetApp, ui: &mut egui::Ui) { diff --git a/crates/ui-egui/tests/interaction.rs b/crates/ui-egui/tests/interaction.rs index 84f20a6..658d016 100644 --- a/crates/ui-egui/tests/interaction.rs +++ b/crates/ui-egui/tests/interaction.rs @@ -1,6 +1,7 @@ //! UI interaction tests: real egui input through the whole app (no GPU needed). use egui::{Event, Key, Modifiers, PointerButton}; +use egui_kittest::kittest::Queryable as _; use gridcraft_engine::Session; use gridcraft_engine::core::{CellRef, Value}; use gridcraft_ui_egui::{SheetApp, grid::Geo}; @@ -213,6 +214,42 @@ fn language_switch_translates_the_ribbon_and_persists() { assert_eq!(h.state().ui.language, Language::En); } +#[test] +fn name_manager_new_and_edit_open_the_name_dialog_and_come_back() { + // #238: New… in the Name Manager did nothing (the manager put itself back over the dialog it + // had just opened). + let mut h = harness(blank()); + h.state_mut().open_dialog("nameManager", json!({})); + h.run_steps(4); + h.get_by_label("New…").click(); + h.run_steps(2); + assert_eq!(h.state().dialog.as_ref().map(|d| d.title.as_str()), Some("New Name")); + if let Some(d) = h.state_mut().dialog.as_mut() { + d.values.insert("name".into(), json!("Total")); + d.values.insert("refersTo".into(), json!("=Sheet1!$A$1")); + } + h.run_steps(1); + key(&mut h, Key::Enter, Modifiers::NONE); + h.run_steps(2); + let names = |h: &egui_kittest::Harness<'static, SheetApp>| { + h.state().session.active().unwrap().wb.names.iter().map(|n| n.name.clone()).collect::>() + }; + assert_eq!(names(&h), ["Total"]); + assert_eq!(h.state().dialog.as_ref().map(|d| d.name()), Some("nameManager"), "OK returns to the Name Manager"); + + h.get_by_label("Edit…").click(); + h.run_steps(2); + assert_eq!(h.state().dialog.as_ref().map(|d| d.title.as_str()), Some("Edit Name")); + if let Some(d) = h.state_mut().dialog.as_mut() { + d.values.insert("name".into(), json!("GrandTotal")); + } + h.run_steps(1); + key(&mut h, Key::Enter, Modifiers::NONE); + h.run_steps(2); + assert_eq!(names(&h), ["GrandTotal"], "Edit renames instead of adding a second name"); + assert_eq!(h.state().dialog.as_ref().map(|d| d.name()), Some("nameManager")); +} + #[test] fn dialogs_open_and_close() { let mut h = harness(blank());