From 699b95288a9cc7e10fc0f8fb2b19d6d142d387f6 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Sat, 10 Oct 2026 12:14:34 -0400 Subject: [PATCH] Review fix-up: switch to Home only when the legacy keytip prefix changes ribbon_keys ran ui.ribbonTab every frame while the prefix was E/O/OC, forcing the Home tab continuously. Run it once on the prefix change and add a regression test. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/lib.rs | 6 ++++-- crates/ui-egui/tests/keytips.rs | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index f5664fbe2..a271c4837 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -519,8 +519,10 @@ impl SheetApp { // Leave pointer cancellation to the popup so clicking a menu item still works. egui::Popup::close_all(ctx); } - // The legacy Edit/Format paths lead to the same existing Home menus. - if matches!(self.keytips.prefix(), Some("E" | "O" | "OC")) { + // The legacy Edit/Format paths lead to the same existing Home menus. Switch once, when + // the prefix changes, not every frame (that would reset a tab the user picked meanwhile). + let after = self.keytips.prefix(); + if after != before.as_deref() && matches!(after, Some("E" | "O" | "OC")) { self.run_or_alert("ui.ribbonTab", json!({"tab": "Home"})); self.ui.ribbon_collapsed = false; } diff --git a/crates/ui-egui/tests/keytips.rs b/crates/ui-egui/tests/keytips.rs index 619254b19..2fe718be1 100644 --- a/crates/ui-egui/tests/keytips.rs +++ b/crates/ui-egui/tests/keytips.rs @@ -208,6 +208,20 @@ fn ribbon_and_legacy_autofit_sequences_resize_the_selected_column() { } } +#[test] +fn legacy_prefix_switches_to_home_once_not_every_frame() { + let mut h = harness(blank()); + h.state_mut().ui.ribbon_tab = "Insert".into(); + sequence(&mut h, &[Key::E], false); + assert_eq!(h.state().keytips.prefix(), Some("E")); + assert_eq!(h.state().ui.ribbon_tab, "Home"); + // While the prefix stays the same, later frames must not keep forcing the Home tab. + h.state_mut().ui.ribbon_tab = "Insert".into(); + h.run_steps(3); + assert_eq!(h.state().keytips.prefix(), Some("E")); + assert_eq!(h.state().ui.ribbon_tab, "Insert"); +} + #[test] fn escape_backs_out_without_clearing_copy_and_f10_toggles_off() { let mut h = harness(copied_row());