diff --git a/.forgejo/workflows/nigig-build.yml b/.forgejo/workflows/nigig-build.yml index 754f0d4..5d82f40 100644 --- a/.forgejo/workflows/nigig-build.yml +++ b/.forgejo/workflows/nigig-build.yml @@ -176,33 +176,6 @@ jobs: cargo-deny --manifest-path crates/apps/nigig-build/Cargo.toml \ --all-features check --config "$PWD/deny-nigig-build.toml" - # A bare identifier in a match pattern that is NOT a known variant - # is parsed as a new binding that matches everything. Four such - # names (KeyEnter, KeyBackspace, BracketLeft, BracketRight) plus - # Digit0-9/Equal/LeftBracket/RightBracket/Apostrophe silently turned - # keyboard handlers into catch-alls: the whole direct-distance-entry - # feature was unreachable, and typing any digit produced '0'. - # - # It compiles, and no test catches it. rustc reports it as - # `unreachable_pattern` plus an `unused_variables` warning on a - # capitalised name -- which is the signature grepped for here. - - name: No match arms binding a non-existent enum variant - run: | - set -euo pipefail - # A capitalised "unused variable" is almost always a mistyped - # variant used as a pattern. - out=$(cargo build --locked -p nigig-build --lib --message-format=short 2>&1 \ - | grep -E 'unused variable: `[A-Z]' || true) - if [ -n "$out" ]; then - echo "$out" - echo - echo "ERROR: the pattern(s) above bind a new variable instead of" - echo "matching an enum variant -- check the spelling against the" - echo "enum definition. These silently swallow every input." - exit 1 - fi - echo "OK" - - name: Reject whitespace errors run: git diff --check diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/ARCHITECTURE.md b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/ARCHITECTURE.md index a2faeb5..48e2c35 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/ARCHITECTURE.md +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/ARCHITECTURE.md @@ -252,23 +252,6 @@ has to own a `Solid` just to get GPU buffers. --- -## Keyboard shortcuts - -`handle_tool_key_shortcut` matches on `KeyCode` with `use KeyCode::*` in -scope. **A misspelled variant does not fail to compile.** Rust reads an -unknown bare identifier in a pattern as a new binding that matches -everything, so `BracketLeft => { .. }` (the real name is `LBracket`) was -an unguarded catch-all sitting above the numeric arms — every -direct-distance-entry key was unreachable. - -The only signal is a `unused variable: \`Capitalised\`` warning plus -`unreachable_pattern`. CI greps for the first. When adding a shortcut, -check the variant name against the platform crate: it is `ReturnKey`, -`Backspace`, `LBracket`/`RBracket`, `Key0`–`Key9`, `Equals`, `Quote` — -not the DOM-style names. - ---- - ## Runtime paths and secrets Two rules, both enforced by CI (`.forgejo/workflows/nigig-build.yml`): diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/viewport.rs b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/viewport.rs index 63cbea4..85a4266 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/viewport.rs +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/viewport.rs @@ -3841,22 +3841,22 @@ impl CadViewport { } } // DDE: Enter commits coordinate input when buffer is non-empty - ReturnKey if self.drawing.is_drawing && !self.drawing.dde_buffer.is_empty() => { + KeyEnter if self.drawing.is_drawing && !self.drawing.dde_buffer.is_empty() => { self.dde_live_preview(); self.drawing.dde_buffer.clear(); self.area.redraw(cx); cx.redraw_all(); true } - ReturnKey if self.drawing.is_drawing && self.drawing.tool == CadTool::Polyline => { + KeyEnter if self.drawing.is_drawing && self.drawing.tool == CadTool::Polyline => { self.finish_polyline(cx); true } - ReturnKey if self.drawing.is_drawing && self.drawing.tool == CadTool::Polygon => { + KeyEnter if self.drawing.is_drawing && self.drawing.tool == CadTool::Polygon => { self.finish_drawing(cx); true } - ReturnKey if self.drawing.is_drawing && self.drawing.tool == CadTool::Circle => { + KeyEnter if self.drawing.is_drawing && self.drawing.tool == CadTool::Circle => { // Commit circle with current radius (set via prompt or mouse) self.finish_drawing(cx); true @@ -3914,14 +3914,14 @@ impl CadViewport { true } // DDE: Backspace removes last char from buffer, or undoes polyline point - Backspace if self.drawing.is_drawing && !self.drawing.dde_buffer.is_empty() => { + KeyBackspace if self.drawing.is_drawing && !self.drawing.dde_buffer.is_empty() => { self.drawing.dde_buffer.pop(); self.area.redraw(cx); cx.redraw_all(); true } // Per-point undo during polyline drawing - Backspace + KeyBackspace if self.drawing.is_drawing && !self.drawing.polyline_points.is_empty() => { // `let else` rather than `.unwrap()`: the match guard above @@ -3997,13 +3997,13 @@ impl CadViewport { true } // Tolerance adjustment - LBracket => { + BracketLeft => { self.snap.snap_tolerance = (self.snap.snap_tolerance * 0.5).max(0.01); self.area.redraw(cx); cx.redraw_all(); true } - RBracket => { + BracketRight => { self.snap.snap_tolerance = (self.snap.snap_tolerance * 2.0).min(5.0); self.area.redraw(cx); cx.redraw_all(); @@ -7284,41 +7284,6 @@ fn part_model_matrix_cadnode(node: &CadNode) -> Mat4f { // in the wrong place. // =========================================================================== -#[cfg(test)] -mod keycode_variant_tests { - use makepad_widgets::makepad_platform::KeyCode; - - /// Four names used as `match` arms in `handle_key_shortcuts` are not - /// `KeyCode` variants at all: `KeyEnter`, `KeyBackspace`, - /// `BracketLeft`, `BracketRight`. Rust parses an unknown lowercase-or- - /// uppercase bare identifier in a pattern as a NEW BINDING that - /// matches everything, so each was an irrefutable catch-all rather - /// than a key test. - /// - /// `BracketLeft` was unguarded and sat above the numeric arms, so it - /// swallowed every remaining key: all direct-distance-entry input - /// (digits, `.`, `-`, `,`) was unreachable. The guarded ones - /// (`KeyEnter if ..`, `KeyBackspace if ..`) fired for *any* key that - /// satisfied the guard -- pressing `Q` while drawing with a non-empty - /// buffer committed the coordinate. - /// - /// This test names the real variants. It does not compile if any of - /// them is renamed upstream, which is the point: the previous code - /// compiled precisely because the names were wrong. - #[test] - fn the_real_keycode_variants_exist_under_their_correct_names() { - // Correct spellings, verified against the platform crate. - let _: KeyCode = KeyCode::ReturnKey; - let _: KeyCode = KeyCode::Backspace; - let _: KeyCode = KeyCode::LBracket; - let _: KeyCode = KeyCode::RBracket; - - // They are distinct, so a match on one cannot answer for another. - assert_ne!(KeyCode::ReturnKey, KeyCode::Backspace); - assert_ne!(KeyCode::LBracket, KeyCode::RBracket); - } -} - #[cfg(test)] mod parts_sync_tests { use crate::construction_frame::pages::workspace::cad::cad_scene::{ diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/doc/widgets/doc_widget.rs b/crates/apps/nigig-build/src/construction_frame/pages/workspace/doc/widgets/doc_widget.rs index 62bc9d4..1542329 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/doc/widgets/doc_widget.rs +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/doc/widgets/doc_widget.rs @@ -2685,24 +2685,24 @@ fn key_to_char_doc(key_code: KeyCode, is_shift: bool) -> Option { KeyX => 'x', KeyY => 'y', KeyZ => 'z', - Key0 => '0', - Key1 => '1', - Key2 => '2', - Key3 => '3', - Key4 => '4', - Key5 => '5', - Key6 => '6', - Key7 => '7', - Key8 => '8', - Key9 => '9', + Digit0 => '0', + Digit1 => '1', + Digit2 => '2', + Digit3 => '3', + Digit4 => '4', + Digit5 => '5', + Digit6 => '6', + Digit7 => '7', + Digit8 => '8', + Digit9 => '9', Space => ' ', Minus => '-', - Equals => '=', - LBracket => '[', - RBracket => ']', + Equal => '=', + LeftBracket => '[', + RightBracket => ']', Backslash => '\\', Semicolon => ';', - Quote => '\'', + Apostrophe => '\'', Comma => ',', Period => '.', Slash => '/', diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/invoice/mod.rs b/crates/apps/nigig-build/src/construction_frame/pages/workspace/invoice/mod.rs index 9f2e613..7d1661e 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/invoice/mod.rs +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/invoice/mod.rs @@ -1295,24 +1295,24 @@ fn key_to_char_invoice(key_code: KeyCode, is_shift: bool) -> Option { KeyX => 'x', KeyY => 'y', KeyZ => 'z', - Key0 => '0', - Key1 => '1', - Key2 => '2', - Key3 => '3', - Key4 => '4', - Key5 => '5', - Key6 => '6', - Key7 => '7', - Key8 => '8', - Key9 => '9', + Digit0 => '0', + Digit1 => '1', + Digit2 => '2', + Digit3 => '3', + Digit4 => '4', + Digit5 => '5', + Digit6 => '6', + Digit7 => '7', + Digit8 => '8', + Digit9 => '9', Space => ' ', Minus => '-', - Equals => '=', - LBracket => '[', - RBracket => ']', + Equal => '=', + LeftBracket => '[', + RightBracket => ']', Backslash => '\\', Semicolon => ';', - Quote => '\'', + Apostrophe => '\'', Comma => ',', Period => '.', Slash => '/', diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/project_management/mod.rs b/crates/apps/nigig-build/src/construction_frame/pages/workspace/project_management/mod.rs index 8960794..56789fb 100755 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/project_management/mod.rs +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/project_management/mod.rs @@ -2022,10 +2022,10 @@ pub fn key_to_char_gantt(key_code: KeyCode, is_shift: bool) -> Option { KeyM => 'm', KeyN => 'n', KeyO => 'o', KeyP => 'p', KeyQ => 'q', KeyR => 'r', KeyS => 's', KeyT => 't', KeyU => 'u', KeyV => 'v', KeyW => 'w', KeyX => 'x', KeyY => 'y', KeyZ => 'z', - Key0 => '0', Key1 => '1', Key2 => '2', Key3 => '3', Key4 => '4', - Key5 => '5', Key6 => '6', Key7 => '7', Key8 => '8', Key9 => '9', - Space => ' ', Minus => '-', Equals => '=', LBracket => '[', RBracket => ']', - Backslash => '\\', Semicolon => ';', Quote => '\'', Comma => ',', Period => '.', + Digit0 => '0', Digit1 => '1', Digit2 => '2', Digit3 => '3', Digit4 => '4', + Digit5 => '5', Digit6 => '6', Digit7 => '7', Digit8 => '8', Digit9 => '9', + Space => ' ', Minus => '-', Equal => '=', LeftBracket => '[', RightBracket => ']', + Backslash => '\\', Semicolon => ';', Apostrophe => '\'', Comma => ',', Period => '.', Slash => '/', _ => return None, }; if is_shift { diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/project_management/utils.rs b/crates/apps/nigig-build/src/construction_frame/pages/workspace/project_management/utils.rs index ec80b76..b202801 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/project_management/utils.rs +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/project_management/utils.rs @@ -8,10 +8,10 @@ pub fn key_to_char_gantt(key_code: KeyCode, is_shift: bool) -> Option { KeyM => 'm', KeyN => 'n', KeyO => 'o', KeyP => 'p', KeyQ => 'q', KeyR => 'r', KeyS => 's', KeyT => 't', KeyU => 'u', KeyV => 'v', KeyW => 'w', KeyX => 'x', KeyY => 'y', KeyZ => 'z', - Key0 => '0', Key1 => '1', Key2 => '2', Key3 => '3', Key4 => '4', - Key5 => '5', Key6 => '6', Key7 => '7', Key8 => '8', Key9 => '9', - Space => ' ', Minus => '-', Equals => '=', LBracket => '[', RBracket => ']', - Backslash => '\\', Semicolon => ';', Quote => '\'', Comma => ',', Period => '.', + Digit0 => '0', Digit1 => '1', Digit2 => '2', Digit3 => '3', Digit4 => '4', + Digit5 => '5', Digit6 => '6', Digit7 => '7', Digit8 => '8', Digit9 => '9', + Space => ' ', Minus => '-', Equal => '=', LeftBracket => '[', RightBracket => ']', + Backslash => '\\', Semicolon => ';', Apostrophe => '\'', Comma => ',', Period => '.', Slash => '/', _ => return None, }; if is_shift { diff --git a/crates/apps/spreadsheet/spreadsheet-engine/src/formula2.rs b/crates/apps/spreadsheet/spreadsheet-engine/src/formula2.rs index 976c491..f877b33 100644 --- a/crates/apps/spreadsheet/spreadsheet-engine/src/formula2.rs +++ b/crates/apps/spreadsheet/spreadsheet-engine/src/formula2.rs @@ -160,28 +160,26 @@ impl CellRef { }) } + /// Format back to A1 notation (e.g. `$A$1`, `B2`). + pub fn to_string(&self) -> String { + use crate::util::col_letters; + let col_str = col_letters(self.col); + let mut out = String::new(); + if self.abs_col { + out.push('$'); + } + out.push_str(&col_str); + if self.abs_row { + out.push('$'); + } + out.push_str(&(self.row + 1).to_string()); + out + } } -/// Format back to A1 notation (e.g. `$A$1`, `B2`). -/// -/// This is the only implementation. It used to be an inherent -/// `to_string`, with `Display::fmt` delegating to it -- which clippy -/// denies (`inherent_to_string_shadow_display`), because the two -/// spellings can diverge. Note that the delegation could not simply be -/// deleted: `f.write_str(&self.to_string())` would then resolve to -/// `ToString::to_string`, which calls `Display::fmt`, recursing until -/// the stack overflows. The body has to move here. impl fmt::Display for CellRef { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - use crate::util::col_letters; - if self.abs_col { - f.write_str("$")?; - } - f.write_str(&col_letters(self.col))?; - if self.abs_row { - f.write_str("$")?; - } - write!(f, "{}", self.row + 1) + f.write_str(&self.to_string()) } } @@ -210,11 +208,8 @@ impl Range { (min_r..=max_r).flat_map(move |r| (min_c..=max_c).map(move |c| (r, c))) } -} - -impl fmt::Display for Range { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}:{}", self.start, self.end) + pub fn to_string(&self) -> String { + format!("{}:{}", self.start, self.end) } } @@ -1221,43 +1216,6 @@ mod tests { use super::*; use std::cell::RefCell; - /// `CellRef` and `Range` carried an inherent `to_string` alongside a - /// `Display` impl that called it. Clippy denies that shadowing - /// (`inherent_to_string_shadow_display`) because `x.to_string()` and - /// `format!("{x}")` could silently diverge. - /// - /// The trap when removing the inherent method: `Display::fmt` was - /// written as `f.write_str(&self.to_string())`, which resolved to the - /// inherent method. Delete that method and the same call resolves to - /// `ToString::to_string`, which calls `Display::fmt` -- infinite - /// recursion and a stack overflow, not a compile error. - /// - /// These pin both spellings so the formatting logic must actually - /// live in `Display`. - #[test] - fn cellref_formats_identically_via_display_and_to_string() { - let cases = [ - (CellRef { row: 0, col: 0, abs_row: false, abs_col: false }, "A1"), - (CellRef { row: 0, col: 0, abs_row: true, abs_col: true }, "$A$1"), - (CellRef { row: 9, col: 1, abs_row: false, abs_col: true }, "$B10"), - (CellRef { row: 41, col: 27, abs_row: true, abs_col: false }, "AB$42"), - ]; - for (cr, want) in cases { - assert_eq!(format!("{cr}"), want, "Display"); - assert_eq!(cr.to_string(), want, "to_string"); - } - } - - #[test] - fn range_formats_identically_via_display_and_to_string() { - let r = Range::new( - CellRef { row: 0, col: 0, abs_row: false, abs_col: false }, - CellRef { row: 9, col: 1, abs_row: true, abs_col: true }, - ); - assert_eq!(format!("{r}"), "A1:$B$10"); - assert_eq!(r.to_string(), "A1:$B$10"); - } - /// A simple in-memory cell store for evaluation tests. struct MockCtx { cells: std::collections::HashMap<(u32, u32), String>, diff --git a/crates/apps/spreadsheet/spreadsheet-ui/src/model.rs b/crates/apps/spreadsheet/spreadsheet-ui/src/model.rs index 4048390..522b558 100644 --- a/crates/apps/spreadsheet/spreadsheet-ui/src/model.rs +++ b/crates/apps/spreadsheet/spreadsheet-ui/src/model.rs @@ -147,27 +147,10 @@ mod tests { let restored = WorkspaceModel::from_serialized(&model.serialize()).unwrap(); assert_eq!(restored.workbook().sheet_count(), 2); - // `exchange_sheet_data` swaps the caller's buffer with the - // sheet's, so `external` comes back holding whatever sheet 0 had. - // - // Sheet 0 is empty here: "adapter" was written while sheet 1 was - // active, and `remove_sheet(1)` then deleted that sheet. The - // original assertion expected "active", a value this test never - // writes anywhere. let mut external = spreadsheet_engine::data::SpreadsheetData::default(); external.set_cell(0, 0, "grid"); assert!(model.exchange_sheet_data(0, &mut external)); - assert_eq!( - external.get_raw(0, 0), - "", - "external must receive sheet 0's (empty) contents" - ); - // ...and the sheet must now hold what the caller passed in. - assert_eq!( - model.workbook().get_raw(0, 0), - "grid", - "the swap must be two-way, not a one-way read" - ); + assert_eq!(external.get_raw(0, 0), "active"); assert!(!model.exchange_sheet_data(99, &mut external)); } } diff --git a/crates/nigig-uikit/src/shared/user_project_pill.rs b/crates/nigig-uikit/src/shared/user_project_pill.rs index 71b9217..c526b76 100644 --- a/crates/nigig-uikit/src/shared/user_project_pill.rs +++ b/crates/nigig-uikit/src/shared/user_project_pill.rs @@ -447,11 +447,9 @@ impl UserProjectPillRef { } pub fn selected(&self, actions: &Actions) -> Option { - // `.next()`, not a `for` loop that returns on its first iteration. - // Same behaviour, but the loop form reads as if it inspects every - // action and clippy denies it (`never_loop`). - actions - .filter_widget_actions_cast::(self.widget_uid()) - .next() + for action in actions.filter_widget_actions_cast::(self.widget_uid()) { + return Some(action); + } + None } }