Repository navigation
Number Stored as Text: green indicator, warning button and Convert to Number (#1994) #2095
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -790,6 +790,7 @@ pub fn show(app: &mut SheetApp, ui: &mut egui::Ui) { | |||||
| list_picker(app, ui, &geo, previous_popups.1 == app.grid.list_picker); | ||||||
| header_menu(app, ui, previous_popups.2 == app.grid.header_menu); | ||||||
| context_menu(app, ui); | ||||||
| error_check_tag(app, ui, &geo, sh); | ||||||
| let track_x = if rtl { (available.left(), rect.left()) } else { (rect.right(), available.right()) }; | ||||||
| vertical_scrollbar(app, ui, sh, &geo, track_x, max_y, &t); | ||||||
| if app.session.clipboard.is_some() { | ||||||
|
|
@@ -1383,6 +1384,15 @@ pub(crate) fn paint_quadrant( | |||||
| Stroke::NONE, | ||||||
| )); | ||||||
| } | ||||||
| // Background error checking: a number stored as text gets Excel's green corner (#1994). | ||||||
| if !sh.show_formulas && sh.cell(*c).and_then(gridcraft_engine::cmd::formulas::number_stored_as_text).is_some() { | ||||||
| let s = 6.0 * z.max(0.7); | ||||||
| p.add(egui::Shape::convex_polygon( | ||||||
| vec![rect.left_top(), pos2(rect.left() + s, rect.top()), pos2(rect.left(), rect.top() + s)], | ||||||
| ERROR_CHECK_GREEN, | ||||||
| Stroke::NONE, | ||||||
| )); | ||||||
| } | ||||||
| } | ||||||
| // Sparklines. | ||||||
| for sp in &sh.sparklines { | ||||||
|
|
@@ -2970,6 +2980,53 @@ fn context_menu(app: &mut SheetApp, ui: &mut egui::Ui) { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// Excel's background error-checking indicator colour. | ||||||
| const ERROR_CHECK_GREEN: Color32 = Color32::from_rgb(0x1E, 0x8E, 0x3E); | ||||||
|
|
||||||
| /// The warning button beside an active cell holding a number stored as text, as in Excel: its | ||||||
| /// menu names the problem and offers Convert to Number for the selection (#1994). | ||||||
| fn error_check_tag(app: &mut SheetApp, ui: &mut egui::Ui, geo: &Geo, sh: &Sheet) { | ||||||
| if app.editor.is_some() || app.dialog.is_some() || sh.show_formulas || app.grid.drag != Drag::None { | ||||||
| return; | ||||||
| } | ||||||
| let Some(active) = app.session.active().map(|d| d.selection.active) else { return }; | ||||||
| if sh.cell(active).and_then(gridcraft_engine::cmd::formulas::number_stored_as_text).is_none() { | ||||||
| return; | ||||||
| } | ||||||
| let cell = geo.cell_rect(sh, active); | ||||||
| let size = vec2(18.0, 18.0); | ||||||
| // Left of the cell, or right of it when that would cover the row headings. | ||||||
| let left = if cell.left() - size.x - 2.0 >= geo.cells.left() { cell.left() - size.x - 2.0 } else { cell.right() + 2.0 }; | ||||||
| let tag = geo.mirror_rect(Rect::from_min_size(pos2(left, cell.top()), size)); | ||||||
| if !geo.cells.intersects(tag) { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Check the warning button against the mirrored cell area. On a right-to-left sheet, Proposed fix- if !geo.cells.intersects(tag) {
+ if !geo.mirror_rect(geo.cells).intersects(tag) {📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| return; | ||||||
| } | ||||||
| let lang = crate::i18n::current(ui.ctx()); | ||||||
| let resp = ui.interact(tag, egui::Id::new("gridcraft.error_check_tag"), Sense::click()).on_hover_text(lang.tr("Number Stored as Text")); | ||||||
| resp.widget_info(|| egui::WidgetInfo::labeled(egui::WidgetType::Button, true, lang.tr("Error checking options"))); | ||||||
| let t = Tokens::get(ui.ctx()); | ||||||
| let p = ui.painter(); | ||||||
| p.rect_filled(tag, 3.0, if resp.hovered() { t.hover } else { t.window }); | ||||||
| p.rect_stroke(tag, 3.0, Stroke::new(1.0, t.separator), egui::StrokeKind::Inside); | ||||||
| // A warning sign: an amber diamond with an exclamation mark. | ||||||
| let c = tag.center(); | ||||||
| let r = 6.0; | ||||||
| p.add(egui::Shape::convex_polygon( | ||||||
| vec![pos2(c.x, c.y - r), pos2(c.x + r, c.y), pos2(c.x, c.y + r), pos2(c.x - r, c.y)], | ||||||
| Color32::from_rgb(0xF2, 0xB7, 0x05), | ||||||
| Stroke::NONE, | ||||||
| )); | ||||||
| p.text(c, egui::Align2::CENTER_CENTER, "!", egui::FontId::proportional(10.0), Color32::BLACK); | ||||||
| egui::Popup::menu(&resp).show(|ui| { | ||||||
| ui.label(egui::RichText::new(lang.tr("Number Stored as Text")).strong()); | ||||||
| ui.separator(); | ||||||
| if ui.button(lang.tr("Convert to Number")).clicked() { | ||||||
| app.run_or_alert("formulas.convertToNumber", json!({})); | ||||||
| ui.close(); | ||||||
| } | ||||||
| }); | ||||||
| } | ||||||
|
|
||||||
| /// Used by `EditState` users to keep the active cell visible while pointing. | ||||||
| pub fn point_move(app: &mut SheetApp, ed: &mut EditState, dr: i64, dc: i64, extend: bool) { | ||||||
| let Some(d) = app.session.active() else { return }; | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -61,6 +61,8 @@ pub(super) fn translate(text: &str) -> Option<&'static str> { | |||||||||||
| "Insert" => "Einfügen", | ||||||||||||
| "Insert Cells…" => "Zellen einfügen…", | ||||||||||||
| "Delete" => "Löschen", | ||||||||||||
| "Number Stored as Text" => "Als Text gespeicherte Zahl", | ||||||||||||
| "Convert to Number" => "In eine Zahl umwandeln", | ||||||||||||
|
Comment on lines
+64
to
+65
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Translate the warning button’s accessible name. The grid requests Proposed fix "Convert to Number" => "In eine Zahl umwandeln",
+ "Error checking options" => "Optionen für die Fehlerüberprüfung",📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||
| "Delete Cells…" => "Zellen löschen…", | ||||||||||||
| "Insert Sheet Rows" => "Blattzeilen einfügen", | ||||||||||||
| "Insert Sheet Columns" => "Blattspalten einfügen", | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| //! A number stored as text gets Excel's green corner and a warning button whose menu converts it (#1994). | ||
|
|
||
| use egui_kittest::kittest::Queryable as _; | ||
| use gridcraft_engine::Session; | ||
| use gridcraft_ui_egui::SheetApp; | ||
| use serde_json::json; | ||
|
|
||
| type Harness = egui_kittest::Harness<'static, SheetApp>; | ||
|
|
||
| fn harness(active: &str) -> Harness { | ||
| let mut s = Session::new(); | ||
| s.new_workbook(); | ||
| s.execute("cell.set", json!({"cell": "B2", "input": "'123"})).unwrap(); | ||
| s.execute("cell.set", json!({"cell": "B3", "input": "'abc"})).unwrap(); | ||
| s.execute("selection.set", json!({"cell": active})).unwrap(); | ||
| let mut h = Harness::builder().with_size(egui::vec2(1200.0, 800.0)).build_ui_state( | ||
| |ui, app: &mut SheetApp| { | ||
| let ctx = ui.ctx().clone(); | ||
| app.logic(&ctx); | ||
| app.ui(ui); | ||
| }, | ||
| SheetApp::new(s, Default::default()), | ||
| ); | ||
| h.run_steps(4); | ||
| h | ||
| } | ||
|
|
||
| #[test] | ||
| fn the_warning_button_converts_a_number_stored_as_text() { | ||
| let mut h = harness("B2"); | ||
| h.get_by_label("Error checking options").click(); | ||
| h.run_steps(3); | ||
| h.get_by_label("Convert to Number").click(); | ||
| h.run_steps(3); | ||
| let v = h.state_mut().session.execute("cell.get", json!({"cell": "B2"})).unwrap()["value"].clone(); | ||
| assert_eq!(v, json!(123.0)); | ||
| assert!(h.query_by_label("Error checking options").is_none(), "the button goes once the number is a number"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn text_that_is_not_a_number_gets_no_button() { | ||
| let h = harness("B3"); | ||
| assert!(h.query_by_label("Error checking options").is_none()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: storytold/gridcraft
Length of output: 2095
🏁 Script executed:
Repository: storytold/gridcraft
Length of output: 28256
🏁 Script executed:
Repository: storytold/gridcraft
Length of output: 42050
Avoid recording an undo step when no cells convert.
formulas.convertToNumbercallsediteven when everyfoundvector is empty.commitalways assigns a new workbookArc, soSession::executerecords an undo step and clears redo history even whenconvertedis0. Check for eligible cells before callingedit.🐛 Suggested fix
let sheet = target_sheet(s, p)?; let ranges = target_ranges(s, p)?; + let has_candidates = { + let d = s.doc()?; + ranges.iter().any(|r| { + let Some(r) = d.wb.sheet(sheet).and_then(|sh| sh.used_range()).and_then(|u| r.intersection(&u)) else { + return false; + }; + d.wb.sheet(sheet).is_some_and(|sh| sh.cells.iter_range(r).any(|(_, x)| number_stored_as_text(x).is_some())) + }) + }; + if !has_candidates { + return Ok(json!({"converted": 0})); + } edit(s, |cx| {🤖 Prompt for AI Agents