From d7c77e94cd161b97ce9d50db1d8c873871857df1 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Sat, 10 Oct 2026 12:44:57 -0400 Subject: [PATCH] Review fix-up: locale-independent defaults, lenient saved language, listed commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - UiState::default() is English: tests, the kittest harness and the snapshot example no longer depend on the host's locale. The desktop app alone starts from the system language, when ui.json names no supported language (first run, an older file, an unknown value). - An unknown or malformed saved language reads as English instead of failing the whole UiState and resetting every other preference. - app.language.set takes {"language": "ja"} (with `code` as an alias, the name the PR description used); the View combo now uses it via Language::code(). The app.language.* commands are listed by engine.commands with the other UI commands and documented in docs/control-protocol.md. - Home's text "Orientation" (方向) and Page Layout's print "Orientation" (印刷の向き) get separate keys through a `context|label` key form. - Japanese: Hide Rows → 行を表示しない, Trace Dependents → 参照先のトレース; translate the display-theme combo merged in from main. - system_theme test: the View tab now has two combo boxes; pick the theme one. Co-Authored-By: Claude Opus 5.5 --- apps/gridcraft/src/main.rs | 7 ++- crates/ui-egui/src/control.rs | 21 ++++---- crates/ui-egui/src/i18n.rs | 77 ++++++++++++++++++++++++++-- crates/ui-egui/src/lib.rs | 16 +++--- crates/ui-egui/src/ribbon.rs | 5 +- crates/ui-egui/tests/interaction.rs | 14 ++++- crates/ui-egui/tests/system_theme.rs | 3 +- docs/control-protocol.md | 6 +++ 8 files changed, 122 insertions(+), 27 deletions(-) diff --git a/apps/gridcraft/src/main.rs b/apps/gridcraft/src/main.rs index 7343932..55dc9c2 100644 --- a/apps/gridcraft/src/main.rs +++ b/apps/gridcraft/src/main.rs @@ -79,11 +79,16 @@ fn load_prefs(app: &mut SheetApp) { if !prefs_enabled() { return; } + // First run (or a ui.json without a usable language): follow the desktop's language. + app.ui.language = gridcraft_ui_egui::i18n::Language::system(); let Some(dir) = config_dir() else { return }; if let Ok(b) = std::fs::read(dir.join("ui.json")) - && let Ok(ui) = serde_json::from_slice(&b) + && let Ok(ui) = serde_json::from_slice::(&b) { + let saved = serde_json::from_slice(&b).ok().and_then(|v| gridcraft_ui_egui::i18n::saved_language(&v)); + let language = saved.unwrap_or(app.ui.language); app.ui = ui; + app.ui.language = language; } if let Ok(b) = std::fs::read(dir.join("prefs.json")) && let Ok(p) = serde_json::from_slice(&b) diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index 902ec25..b1733fd 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -155,16 +155,19 @@ pub fn handle(app: &mut SheetApp, ctx: &egui::Context, method: &str, p: &Json) - } "engine.commands" => { let mut v: Vec = app.session.commands().into_iter().map(|c| serde_json::to_value(c).unwrap_or_default()).collect(); - for (id, label) in [ - ("ui.ribbonTab", "Ribbon Tab"), - ("view.formulaBar", "Formula Bar"), - ("view.collapseRibbon", "Collapse Ribbon"), - ("view.darkMode", "Dark Mode"), - ("view.theme", "Display Theme"), - ("view.zoom100", "100%"), - ("ui.dialog", "Open Dialog"), + for (id, label, params) in [ + ("ui.ribbonTab", "Ribbon Tab", ""), + ("view.formulaBar", "Formula Bar", ""), + ("view.collapseRibbon", "Collapse Ribbon", ""), + ("view.darkMode", "Dark Mode", ""), + ("view.theme", "Display Theme", ""), + ("view.zoom100", "100%", ""), + ("ui.dialog", "Open Dialog", ""), + ("app.language.set", "Interface Language", "{language: \"en\"|\"ja\"} (alias: code)"), + ("app.language.english", "Interface Language: English", ""), + ("app.language.japanese", "Interface Language: Japanese", ""), ] { - v.push(json!({"id": id, "label": label, "enabled": true, "ui": true})); + v.push(json!({"id": id, "label": label, "params": params, "enabled": true, "ui": true})); } ok(Json::Array(v)) } diff --git a/crates/ui-egui/src/i18n.rs b/crates/ui-egui/src/i18n.rs index 48e809d..18fda08 100644 --- a/crates/ui-egui/src/i18n.rs +++ b/crates/ui-egui/src/i18n.rs @@ -28,6 +28,14 @@ impl Language { } } + /// The language's code, as saved in `ui.json` and taken by `app.language.set`. + pub fn code(self) -> &'static str { + match self { + Self::En => "en", + Self::Ja => "ja", + } + } + pub fn parse(code: &str) -> Option { match code { "en" => Some(Self::En), @@ -56,16 +64,43 @@ impl Language { } } + /// Translates an interface label; untranslated labels fall back to English. + /// + /// A label that needs two translations (Home's text "Orientation" vs Page Layout's print + /// "Orientation") is keyed `context|label`: English shows only the part after the `|`. pub fn tr(self, text: &str) -> &str { if self == Self::Ja && let Some((_, japanese)) = JAPANESE.iter().find(|(english, _)| *english == text) { return japanese; } - text + text.rsplit_once('|').map_or(text, |(_, label)| label) } } +/// Deserializes a saved language leniently: an unknown or malformed value (a language removed in a +/// later version, a hand-edited `ui.json`) reads as English instead of failing the whole `UiState`, +/// which would reset every other preference with it. +pub fn lenient<'de, D: serde::Deserializer<'de>>(d: D) -> Result { + #[derive(Deserialize)] + #[serde(untagged)] + enum Raw { + Code(String), + Other(serde::de::IgnoredAny), + } + Ok(match Raw::deserialize(d)? { + Raw::Code(code) => Language::parse(&code).unwrap_or_default(), + Raw::Other(_) => Language::default(), + }) +} + +/// The language a saved `ui.json` asks for, if it names a supported one. The desktop app uses it +/// to tell "saved English" from "nothing usable saved" (first run, an older file, an unknown +/// value), which starts from [`Language::system`] instead. +pub fn saved_language(ui_json: &serde_json::Value) -> Option { + ui_json.get("language").and_then(serde_json::Value::as_str).and_then(Language::parse) +} + /// egui context-data key holding the language of the frame being drawn. Widgets that only get a /// `Ui` (not the app) read it to translate their labels; [`set_current`] writes it once per frame. fn lang_key() -> egui::Id { @@ -121,7 +156,8 @@ const JAPANESE: &[(&str, &str)] = &[ ("Vertical Text", "縦書き"), ("Decrease Indent", "インデントを減らす"), ("Increase Indent", "インデントを増やす"), - ("Orientation", "印刷の向き"), + ("Orientation", "方向"), + ("Page Layout|Orientation", "印刷の向き"), ("Angle Counterclockwise", "左回りに回転"), ("Angle Clockwise", "右回りに回転"), ("Rotate Text Up", "文字列を上に回転"), @@ -153,7 +189,7 @@ const JAPANESE: &[(&str, &str)] = &[ ("Column Width…", "列の幅…"), ("AutoFit Column Width", "列の幅を自動調整"), ("Default Width…", "標準の幅…"), - ("Hide Rows", "行の表示/非表示"), + ("Hide Rows", "行を表示しない"), ("Unhide Rows", "再表示"), ("Hide Columns", "列の表示/非表示"), ("Unhide Columns", "再表示"), @@ -218,7 +254,7 @@ const JAPANESE: &[(&str, &str)] = &[ ("Use in Formula", "数式で使用"), ("Create from Selection", "選択範囲から作成"), ("Trace Precedents", "参照元のトレース"), - ("Trace Dependents", "依存元のトレース"), + ("Trace Dependents", "参照先のトレース"), ("Remove Arrows", "矢印の解除"), ("Show Formulas", "数式の表示"), ("Error Checking", "エラーチェック"), @@ -274,6 +310,10 @@ const JAPANESE: &[(&str, &str)] = &[ ("Freeze First Column", "最左列の固定"), ("Unfreeze Panes", "ウィンドウ枠固定の解除"), ("Dark Mode", "ダーク モード"), + ("Display theme", "表示テーマ"), + ("System", "システム"), + ("Light", "ライト"), + ("Dark", "ダーク"), ("Interface language", "表示言語"), ("Command\nPalette", "コマンド パレット"), ("Agent\nControl", "エージェント制御"), @@ -355,13 +395,32 @@ mod tests { for (i, (en, ja)) in JAPANESE.iter().enumerate() { assert!(!ja.is_empty()); assert!(JAPANESE.iter().take(i).all(|(other, _)| en != other)); - assert_eq!(Language::En.tr(en), *en); + assert_eq!(Language::En.tr(en), en.rsplit_once('|').map_or(*en, |(_, label)| label)); } assert_eq!(Language::Ja.tr("Data"), "データ"); assert_eq!(Language::Ja.tr("Sheet1!A1"), "Sheet1!A1"); + // A `context|label` key shows only its label in English and splits the translation. + assert_eq!(Language::En.tr("Page Layout|Orientation"), "Orientation"); + assert_eq!(Language::Ja.tr("Page Layout|Orientation"), "印刷の向き"); + assert_eq!(Language::Ja.tr("Orientation"), "方向"); assert_eq!(Language::parse("xx"), None); } + #[test] + fn unknown_saved_language_keeps_the_other_preferences() { + let ui: crate::UiState = serde_json::from_str(r#"{"dark": true, "language": "xx"}"#).unwrap(); + assert!(ui.dark); + assert_eq!(ui.language, Language::En); + let ui: crate::UiState = serde_json::from_str(r#"{"dark": true, "language": 7}"#).unwrap(); + assert!(ui.dark); + let ui: crate::UiState = serde_json::from_str(r#"{"language": "ja"}"#).unwrap(); + assert_eq!(ui.language, Language::Ja); + assert_eq!(crate::UiState::default().language, Language::En, "the default never reads the host locale"); + assert_eq!(saved_language(&serde_json::json!({"language": "ja"})), Some(Language::Ja)); + assert_eq!(saved_language(&serde_json::json!({"language": "xx"})), None); + assert_eq!(saved_language(&serde_json::json!({})), None); + } + #[test] fn tags_reduce_to_a_supported_language() { assert_eq!(Language::from_tag("ja-JP"), Some(Language::Ja)); @@ -378,5 +437,13 @@ mod tests { assert_eq!(restored.language, Language::Ja); app.run("app.language.english", serde_json::json!({})).unwrap(); assert_eq!(app.ui.language, Language::En); + app.run("app.language.set", serde_json::json!({"language": "ja"})).unwrap(); + assert_eq!(app.ui.language, Language::Ja); + app.run("app.language.set", serde_json::json!({"code": "en"})).unwrap(); + assert_eq!(app.ui.language, Language::En); + assert!(app.run("app.language.set", serde_json::json!({})).is_err(), "no language is an error, not a dialog"); + for l in Language::ALL { + assert_eq!(Language::parse(l.code()), Some(l)); + } } } diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index a5e4ab5..4458090 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -45,8 +45,11 @@ pub struct UiState { pub formula_bar_expanded: bool, pub status_bar: bool, pub recent: Vec, - /// Interface language. Absent in older `ui.json` → the system's language (English fallback). - #[serde(default = "i18n::Language::system")] + /// Interface language. English by default (so tests and headless renders never depend on the + /// host's locale); the desktop app starts from [`i18n::Language::system`] when `ui.json` has + /// no usable language (see [`i18n::saved_language`]). An unknown saved value reads as English + /// rather than failing the whole `UiState`. + #[serde(deserialize_with = "i18n::lenient")] pub language: i18n::Language, } @@ -61,7 +64,7 @@ impl Default for UiState { formula_bar_expanded: false, status_bar: true, recent: vec![], - language: i18n::Language::system(), + language: i18n::Language::En, } } } @@ -234,13 +237,14 @@ impl SheetApp { } "view.zoom100" => return Some(self.session.run("view.zoom", json!({"percent": 100})).inspect(|_| self.after_engine())), "app.language.set" => { - let code = p.get("language").and_then(Json::as_str).and_then(i18n::Language::parse); - match code { + // `{"language": "ja"}`; `code` is accepted as an alias. + let code = p.get("language").or_else(|| p.get("code")).and_then(Json::as_str).unwrap_or(""); + match i18n::Language::parse(code) { Some(l) => { self.ui.language = l; Ok(json!({"language": l})) } - None => Err(format!("unknown language {:?} (use \"en\" or \"ja\")", p.get("language").and_then(Json::as_str).unwrap_or(""))), + None => Err(format!("unknown language {code:?} (use \"en\" or \"ja\")")), } } "app.language.english" => { diff --git a/crates/ui-egui/src/ribbon.rs b/crates/ui-egui/src/ribbon.rs index 100bfc8..06bb02d 100644 --- a/crates/ui-egui/src/ribbon.rs +++ b/crates/ui-egui/src/ribbon.rs @@ -1246,7 +1246,7 @@ fn page_layout(app: &mut SheetApp, ui: &mut Ui) { ], ); }); - let o = big_button(ui, Icon::Orient, "Orientation", "Orientation", true); + let o = big_button(ui, Icon::Orient, "Page Layout|Orientation", "Page Layout|Orientation", true); egui::Popup::menu(&o).show(|ui| { menu_items( app, @@ -1626,8 +1626,7 @@ fn view(app: &mut SheetApp, ui: &mut Ui) { |ui| { for l in crate::i18n::Language::ALL { if ui.selectable_label(l == lang, l.name()).clicked() { - let code = if l == crate::i18n::Language::Ja { "app.language.japanese" } else { "app.language.english" }; - act(app, code, json!({})); + act(app, "app.language.set", json!({"language": l.code()})); } } }, diff --git a/crates/ui-egui/tests/interaction.rs b/crates/ui-egui/tests/interaction.rs index ab33809..5baf10b 100644 --- a/crates/ui-egui/tests/interaction.rs +++ b/crates/ui-egui/tests/interaction.rs @@ -187,8 +187,18 @@ fn renders_every_ribbon_tab_without_panicking() { fn language_switch_translates_the_ribbon_and_persists() { use gridcraft_ui_egui::i18n::Language; let mut h = harness(blank()); - // English by default on a machine whose system language is not Japanese. - assert_ne!(h.state().ui.language, Language::Ja); + // English by default whatever the host's locale: only the desktop app reads the system language. + assert_eq!(h.state().ui.language, Language::En); + // The language commands are listed with the other UI commands. + let ctx = h.ctx.clone(); + let gridcraft_ui_egui::control::Outcome::Done(listed) = gridcraft_ui_egui::control::handle(h.state_mut(), &ctx, "engine.commands", &json!({})) + else { + panic!("engine.commands returns a response"); + }; + let ids: Vec<&str> = listed["result"].as_array().into_iter().flatten().filter_map(|c| c["id"].as_str()).collect(); + for id in ["app.language.set", "app.language.english", "app.language.japanese"] { + assert!(ids.contains(&id), "engine.commands lists {id}"); + } h.state_mut().run("app.language.japanese", json!({})).unwrap(); h.run_steps(2); // the ribbon renders the Japanese tabs without panicking assert_eq!(h.state().ui.language, Language::Ja); diff --git a/crates/ui-egui/tests/system_theme.rs b/crates/ui-egui/tests/system_theme.rs index 2a5365a..0d2cdea 100644 --- a/crates/ui-egui/tests/system_theme.rs +++ b/crates/ui-egui/tests/system_theme.rs @@ -44,7 +44,8 @@ fn set_mode(h: &mut egui_kittest::Harness<'static, SheetApp>, mode: &str) { } fn select_ribbon_theme(h: &mut egui_kittest::Harness<'static, SheetApp>, label: &str) { - h.get_by_role(egui::accesskit::Role::ComboBox).click(); + // The View tab's first combo box is the display theme (the interface language follows it). + h.get_all_by_role(egui::accesskit::Role::ComboBox).next().expect("display theme combo").click(); h.run_steps(2); h.get_by_role_and_label(egui::accesskit::Role::Button, label).click(); h.run_steps(2); diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 9c6c31b..e465a07 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -45,5 +45,11 @@ System follows live OS appearance changes and falls back to Light when the platf The saved choice remains System; new users still start in Light. The legacy `view.darkMode` command and `ui.set {dark}` select a fixed Light or Dark theme. +`{command: "app.language.set", params: {language: "ja"}}` switches the interface language +(`code` is accepted as an alias for `language`); `app.language.english` and +`app.language.japanese` are shortcuts. Only interface chrome is translated, never command ids or +document text. The choice is saved in `ui.json`; with nothing saved the desktop app follows the +system language. + For a headless equivalent (no window), use `gridcraft-cli mcp` or `gridcraft-cli run`; see [`mcp.md`](mcp.md) and [`cli.md`](cli.md).