Merge pull request #114 from nihilist-narwhal/fix-defined-name-entry

Names: R1C1-style names, Create from Selection bottom/right, LAMBDA name case

crates/engine/src/cmd/edit.rs

@@ -148,6 +148,7 @@ pub(crate) fn input_to_cell(input: &str, old: Option<&Cell>, wb: &mut gridcraft_
             parsed = gridcraft_formula::parse(&text);
         }
         let expr = parsed.map_err(|e| EngineError::Other(format!("There's a problem with this formula: {e}")))?;
+        let expr = wb.name_call_case(expr);
         let mut cell = Cell::formula(Formula::from_expr(expr));
         cell.style = style;
         // Formulas whose result is a date/time get a format from functions like TODAY().

crates/engine/src/cmd/formulas.rs

@@ -43,7 +43,7 @@ pub fn specs() -> Vec<CommandSpec> {
             "Create from Selection",
             ["Formulas", "Defined Names"],
             Some("Cmd+Shift+F3"),
-            "{range?, top?: true, left?: false, bottom?, right?}",
+            "{range?, top?: true, left?: false, bottom?: false, right?: false, replace?: false (existing names are kept and listed in `skipped`)}",
             has_doc,
             create_from_selection
         ),
@@ -218,15 +218,26 @@ fn valid_name(n: &str) -> bool {
     if !(first.is_alphabetic() || first == '_' || first == '\\') {
         return false;
     }
-    if n.len() > 255 || n.eq_ignore_ascii_case("R") || n.eq_ignore_ascii_case("C") {
-        return false;
-    }
-    if CellRef::parse(n).is_some() {
+    if n.len() > 255 || CellRef::parse(n).is_some() || is_r1c1(n) {
         return false;
     }
     n.chars().all(|c| c.is_alphanumeric() || matches!(c, '_' | '.' | '\\' | '?'))
 }
 
+/// Looks like an R1C1 reference (`R`, `C`, `RC`, `R2`, `C3`, `R1C1`), which Excel doesn't
+/// accept as a name.
+fn is_r1c1(n: &str) -> bool {
+    let digits = |s: &str| s.bytes().take_while(u8::is_ascii_digit).count();
+    let mut rest = n;
+    if let Some(r) = rest.strip_prefix(['R', 'r']) {
+        rest = r.get(digits(r)..).unwrap_or("");
+    }
+    if let Some(c) = rest.strip_prefix(['C', 'c']) {
+        rest = c.get(digits(c)..).unwrap_or("");
+    }
+    rest.is_empty() && !n.is_empty()
+}
+
 fn define_name(s: &mut Session, p: &Json) -> Result<Json> {
     let Some(name) = str_param(p, "name").map(str::to_string) else {
         s.ui_requests.push(crate::UiRequest::Dialog("defineName".into(), json!({})));
@@ -300,6 +311,9 @@ fn create_from_selection(s: &mut Session, p: &Json) -> Result<Json> {
     let r = target_range(s, p)?;
     let top = bool_param(p, "top").unwrap_or(true);
     let left = bool_param(p, "left").unwrap_or(false);
+    let bottom = bool_param(p, "bottom").unwrap_or(false);
+    let right = bool_param(p, "right").unwrap_or(false);
+    let replace = bool_param(p, "replace").unwrap_or(false);
     let d = s.doc()?;
     let sh = d.wb.active().ok_or(EngineError::NoDocument)?;
     let q = gridcraft_formula::quote_sheet(&sh.name);
@@ -314,28 +328,38 @@ fn create_from_selection(s: &mut Session, p: &Json) -> Result<Json> {
     };
     let clean = |t: String| -> String {
         let mut n: String = t.trim().chars().map(|c| if c.is_alphanumeric() || c == '.' { c } else { '_' }).collect();
-        if n.chars().next().is_some_and(|c| c.is_ascii_digit()) || CellRef::parse(&n).is_some() {
+        if !n.is_empty() && !valid_name(&n) {
             n.insert(0, '_');
         }
-        n
+        if valid_name(&n) { n } else { String::new() }
     };
+    // The labels in the chosen edge rows/columns name the cells between them.
+    let (r0, r1) = (r.start.row + top as u32, r.end.row.saturating_sub(bottom as u32));
+    let (c0, c1) = (r.start.col + left as u32, r.end.col.saturating_sub(right as u32));
     let mut made = Vec::new();
-    if top && r.height() > 1 {
-        for c in r.start.col..=r.end.col {
-            let name = clean(sh.value(CellRef::new(r.start.row, c)).display());
-            if !name.is_empty() {
-                made.push((name, abs(RangeRef::new(CellRef::new(r.start.row + 1, c), CellRef::new(r.end.row, c)))));
+    if r0 <= r1 && c0 <= c1 {
+        for (on, row) in [(top, r.start.row), (bottom, r.end.row)] {
+            for c in (c0..=c1).filter(|_| on) {
+                let name = clean(sh.value(CellRef::new(row, c)).display());
+                if !name.is_empty() {
+                    made.push((name, abs(RangeRef::new(CellRef::new(r0, c), CellRef::new(r1, c)))));
+                }
             }
         }
-    }
-    if left && r.width() > 1 {
-        for row in r.start.row + top as u32..=r.end.row {
-            let name = clean(sh.value(CellRef::new(row, r.start.col)).display());
-            if !name.is_empty() {
-                made.push((name, abs(RangeRef::new(CellRef::new(row, r.start.col + 1), CellRef::new(row, r.end.col)))));
+        for (on, col) in [(left, r.start.col), (right, r.end.col)] {
+            for row in (r0..=r1).filter(|_| on) {
+                let name = clean(sh.value(CellRef::new(row, col)).display());
+                if !name.is_empty() {
+                    made.push((name, abs(RangeRef::new(CellRef::new(row, c0), CellRef::new(row, c1)))));
+                }
             }
         }
     }
+    // Excel asks before replacing an existing name; here it's kept unless `replace` is set, and
+    // reported.
+    let exists = |n: &str| d.wb.names.iter().any(|x| x.name.eq_ignore_ascii_case(n) && x.scope.is_none());
+    let (made, skipped): (Vec<_>, Vec<_>) = made.into_iter().partition(|(n, _)| replace || !exists(n));
+    let skipped: Vec<String> = skipped.into_iter().map(|(n, _)| n).collect();
     let n = made.len();
     edit(s, |cx| {
         for (name, f) in made {
@@ -343,7 +367,7 @@ fn create_from_selection(s: &mut Session, p: &Json) -> Result<Json> {
             cx.wb.names.push(DefinedName { name, scope: None, formula: f, comment: String::new(), hidden: false });
         }
         cx.structural = true;
-        Ok(json!({"created": n}))
+        Ok(json!({"created": n, "skipped": skipped}))
     })
 }
 

crates/engine/src/tests.rs

@@ -283,6 +283,65 @@ fn tables_and_structured_refs() {
     assert_eq!(v(&s, "B4"), Value::Number(7.0));
 }
 
+#[test]
+fn create_names_from_selection() {
+    let names = |s: &Session| -> Vec<(String, String)> {
+        let mut v: Vec<_> = s.doc().unwrap().wb.names.iter().map(|n| (n.name.clone(), n.formula.clone())).collect();
+        v.sort();
+        v
+    };
+    let named = |n: &str, f: &str| (n.to_string(), f.to_string());
+    let mut s = s();
+    s.execute(
+        "range.setValues",
+        json!({"range": "A1", "values": [["", "Jan", "Feb", ""], ["North", 1, 2, "N"], ["South", 3, 4, "S"], ["", "First", "Second", ""]]}),
+    )
+    .unwrap();
+    s.execute("formulas.createFromSelection", json!({"range": "A1:D4", "top": false, "bottom": true, "right": true})).unwrap();
+    assert_eq!(
+        names(&s),
+        [named("First", "Sheet1!$B$1:$B$3"), named("N", "Sheet1!$A$2:$C$2"), named("S", "Sheet1!$A$3:$C$3"), named("Second", "Sheet1!$C$1:$C$3")]
+    );
+    // Existing names are kept unless replacing is asked for.
+    let r = s.execute("formulas.createFromSelection", json!({"range": "A1:C3", "top": true, "left": true})).unwrap();
+    assert_eq!(r["created"], 4);
+    let r = s.execute("formulas.createFromSelection", json!({"range": "B1:C3", "top": true})).unwrap();
+    assert_eq!((r["created"].clone(), r["skipped"].clone()), (json!(0), json!(["Jan", "Feb"])));
+    s.execute("cell.set", json!({"cell": "B1", "input": "N"})).unwrap();
+    let r = s.execute("formulas.createFromSelection", json!({"range": "B1:B3", "replace": true})).unwrap();
+    assert_eq!(r["created"], 1);
+    assert!(names(&s).contains(&named("N", "Sheet1!$B$2:$B$3")));
+    assert!(names(&s).contains(&named("North", "Sheet1!$B$2:$C$2")));
+}
+
+#[test]
+fn names_that_look_like_references_are_refused() {
+    let mut s = s();
+    for bad in ["R1C1", "r2", "C3", "RC", "R", "c", "rc12", "A1", "XFD1048576", "1st"] {
+        assert!(s.execute("formulas.defineName", json!({"name": bad, "refersTo": "=1"})).is_err(), "{bad}");
+    }
+    for good in ["R1C1X", "Rate", "RCx", "Rx", "_R1", "Cost"] {
+        s.execute("formulas.defineName", json!({"name": good, "refersTo": "=1"})).unwrap();
+    }
+}
+
+#[test]
+fn lambda_names_keep_their_case_in_calls() {
+    let mut s = s();
+    s.execute("formulas.defineName", json!({"name": "Double", "refersTo": "=LAMBDA(x,x*2)"})).unwrap();
+    s.execute("formulas.defineName", json!({"name": "Quad", "refersTo": "=LAMBDA(x,Double(Double(x)))"})).unwrap();
+    s.execute("cell.set", json!({"cell": "A1", "input": "=double(4)+Quad(1)+sum(1)"})).unwrap();
+    assert_eq!(s.execute("cell.get", json!({"cell": "A1"})).unwrap()["formula"], "=Double(4)+Quad(1)+SUM(1)");
+    assert_eq!(v(&s, "A1"), Value::Number(13.0));
+    let r = s.execute("file.saveBytes", json!({"format": "xlsx"})).unwrap();
+    let b64 = r["base64"].as_str().unwrap().to_string();
+    s.execute("file.open", json!({"name": "x.xlsx", "base64": b64})).unwrap();
+    assert_eq!(s.execute("cell.get", json!({"cell": "A1"})).unwrap()["formula"], "=Double(4)+Quad(1)+SUM(1)");
+    let quad = s.doc().unwrap().wb.names.iter().find(|n| n.name == "Quad").unwrap().formula.clone();
+    assert_eq!(quad, "LAMBDA(x,Double(Double(x)))");
+    assert_eq!(v(&s, "A1"), Value::Number(13.0));
+}
+
 #[test]
 fn sheets() {
     let mut s = s();

crates/model/src/workbook.rs

@@ -3,8 +3,10 @@
 use std::sync::Arc;
 
 use gridcraft_core::DateSystem;
+use gridcraft_formula::Expr;
 use serde::{Deserialize, Serialize};
 
+use crate::cell::Formula;
 use crate::sheet::Sheet;
 use crate::style::{StyleTable, Theme};
 
@@ -143,6 +145,53 @@ impl Workbook {
             .find(|n| n.scope == Some(sheet) && n.name.eq_ignore_ascii_case(name))
             .or_else(|| self.names.iter().find(|n| n.scope.is_none() && n.name.eq_ignore_ascii_case(name)))
     }
+    /// Spells calls of LAMBDA names the way the names are defined (`Double(4)`, where the
+    /// parser gives `DOUBLE(4)`: it upper-cases function names, as built-in ones are shown).
+    pub fn name_call_case(&self, e: Expr) -> Expr {
+        spell_calls(e, &self.lambda_names())
+    }
+    /// [`Workbook::name_call_case`] for every cell formula and defined name (after reading a
+    /// file).
+    pub fn apply_name_call_case(&mut self) {
+        let lambdas = self.lambda_names();
+        if lambdas.is_empty() {
+            return;
+        }
+        for sheet in self.sheets.iter_mut() {
+            let fixed: Vec<_> = sheet
+                .cells
+                .iter()
+                .filter_map(|(c, cell)| {
+                    let f = cell.formula.as_ref()?;
+                    let e = f.expr()?;
+                    let ne = spell_calls(e.clone(), &lambdas);
+                    (ne != e).then(|| (c, Formula { array: f.array, ..Formula::from_expr(ne) }))
+                })
+                .collect();
+            if fixed.is_empty() {
+                continue;
+            }
+            let sheet = Arc::make_mut(sheet);
+            for (c, f) in fixed {
+                if let Some(cell) = sheet.cells.get_mut(c) {
+                    cell.formula = Some(Arc::new(f));
+                }
+            }
+        }
+        for n in self.names.iter_mut() {
+            if let Ok(e) = gridcraft_formula::parse(&n.formula) {
+                let ne = spell_calls(e.clone(), &lambdas);
+                if ne != e {
+                    n.formula = gridcraft_formula::print(&ne);
+                }
+            }
+        }
+    }
+    /// Names of the defined names holding a LAMBDA (callable like functions).
+    fn lambda_names(&self) -> Vec<String> {
+        let is_lambda = |f: &str| f.trim_start_matches('=').trim_start().get(..7).is_some_and(|p| p.eq_ignore_ascii_case("LAMBDA("));
+        self.names.iter().filter(|n| is_lambda(&n.formula)).map(|n| n.name.clone()).collect()
+    }
     /// Table by name across sheets: (sheet index, table index).
     pub fn table(&self, name: &str) -> Option<(usize, usize)> {
         for (si, s) in self.sheets.iter().enumerate() {
@@ -172,6 +221,20 @@ impl Workbook {
     }
 }
 
+/// Calls of the names in `lambdas` spelled as the names are.
+fn spell_calls(e: Expr, lambdas: &[String]) -> Expr {
+    if lambdas.is_empty() {
+        return e;
+    }
+    e.map(&mut |x| match x {
+        Expr::Call(n, args) => match lambdas.iter().find(|l| l.eq_ignore_ascii_case(&n)) {
+            Some(l) => Expr::Call(l.clone(), args),
+            None => Expr::Call(n, args),
+        },
+        other => other,
+    })
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

crates/xlsx/src/read.rs

@@ -279,6 +279,8 @@ pub fn read_xlsx(bytes: &[u8]) -> Result<(Workbook, ReadReport), IoError> {
     if wb_rels.iter().any(|r| r.kind == "externalLink") {
         cx.warn("links to external workbooks are not supported");
     }
+    // Calls of LAMBDA names are spelled like the names (the parser upper-cases them).
+    wb.apply_name_call_case();
     wb.styles = cx.styles;
     Ok((wb, cx.report))
 }