From b5039ca6039f39aadc8b8ddbd0888caf21f10fe8 Mon Sep 17 00:00:00 2001 From: umesh429 Date: Thu, 15 Oct 2026 07:22:03 +0100 Subject: [PATCH] fix(ui): clicking a theme card in Preferences picks that theme (#1727) Preferences > Appearance drew a Light and a Dark theme card, each with a radio row, but neither the card nor the radio responded to a click: the radio's response was discarded and the frame never sensed clicks. Only the Appearance mode combo box changed the theme. The whole card and its radio rows now run view.theme.light / view.theme.dark, the same commands as the combo box, so the combo box shows the choice. A card click is a manual choice that OS theme changes leave alone, and Use system setting afterwards follows the OS again. Co-authored-by: umesh429 <3474974+umesh429@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 --- crates/ui-egui/src/appearance.rs | 49 ++++++++++++++++++--- crates/ui-egui/tests/ui_group/theme.rs | 61 ++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 6 deletions(-) diff --git a/crates/ui-egui/src/appearance.rs b/crates/ui-egui/src/appearance.rs index e27939927..0d607cc91 100644 --- a/crates/ui-egui/src/appearance.rs +++ b/crates/ui-egui/src/appearance.rs @@ -83,9 +83,9 @@ fn preview(ui: &mut egui::Ui, kind: ThemeKind, width: f32) { } /// One theme card: its title, Active when it is what shows now, a preview of the chosen theme -/// and a radio row per theme. -fn card(ui: &mut egui::Ui, t: &Tokens, title: &str, active: bool, themes: &[(ThemeKind, &str)]) { - egui::Frame::new() +/// and a radio row per theme. Returns true when the card or one of its radio rows was clicked. +fn card(ui: &mut egui::Ui, t: &Tokens, title: &str, active: bool, themes: &[(ThemeKind, &str)]) -> bool { + let inner = egui::Frame::new() .fill(t.card) .stroke(egui::Stroke::new(1.0, if active { t.accent } else { t.border })) .corner_radius(6.0) @@ -102,11 +102,16 @@ fn card(ui: &mut egui::Ui, t: &Tokens, title: &str, active: bool, themes: &[(The let shown = themes.first().map_or(ThemeKind::Light, |(kind, _)| *kind); preview(ui, shown, ui.available_width()); ui.add_space(4.0); + let mut clicked = false; for (kind, label) in themes { // One theme per card today, so it is always the chosen one. - let _ = ui.add(widgets::radio(*kind == shown, tl!(label))); + clicked |= ui.add(widgets::radio(*kind == shown, tl!(label))).clicked(); } + clicked }); + // The whole card picks its theme, not just the radio row. + let whole = inner.response.interact(egui::Sense::click()).on_hover_cursor(egui::CursorIcon::PointingHand); + inner.inner || whole.clicked() } /// The Appearance section of Preferences. @@ -127,8 +132,12 @@ pub fn preferences_section(ui: &mut egui::Ui, app: &mut PdfCraftApp, t: &Tokens) let light_active = app.theme == ThemeKind::Light; ui.columns(2, |cols| { let [light, dark] = cols else { return }; - card(light, t, tl!("Light theme"), light_active, LIGHT_THEMES); - card(dark, t, tl!("Dark theme"), !light_active, DARK_THEMES); + if card(light, t, tl!("Light theme"), light_active, LIGHT_THEMES) { + app.execute(mode_command(ThemePreference::Light)); + } + if card(dark, t, tl!("Dark theme"), !light_active, DARK_THEMES) { + app.execute(mode_command(ThemePreference::Dark)); + } }); } @@ -179,4 +188,32 @@ mod tests { h.run(); assert_eq!(h.state().theme_preference, ThemePreference::System); } + + /// Clicking a theme card, or its radio row, picks that theme, not only the mode combo box. + #[test] + fn clicking_a_theme_card_picks_that_theme() { + let mut app = PdfCraftApp::new(); + app.set_option("language", "en").unwrap(); + let mut fonts = false; + let mut h = Harness::builder().with_size(egui::vec2(640.0, 400.0)).build_ui_state( + move |ui, app: &mut PdfCraftApp| { + if !fonts { + ui.ctx().set_fonts(theme::font_definitions()); + fonts = true; + return; + } + let t = Tokens::get(ui.ctx()); + preferences_section(ui, app, &t); + }, + app, + ); + h.run(); + h.get_by_label("Dark gray").click(); + h.run(); + assert_eq!((h.state().theme_preference, h.state().theme), (ThemePreference::Dark, ThemeKind::Dark)); + // The card's title is not a widget of its own: the click lands on the card. + h.get_by_label("Light theme").click(); + h.run(); + assert_eq!((h.state().theme_preference, h.state().theme), (ThemePreference::Light, ThemeKind::Light)); + } } diff --git a/crates/ui-egui/tests/ui_group/theme.rs b/crates/ui-egui/tests/ui_group/theme.rs index 2a7e0e265..6e86c5099 100644 --- a/crates/ui-egui/tests/ui_group/theme.rs +++ b/crates/ui-egui/tests/ui_group/theme.rs @@ -204,3 +204,64 @@ fn native_appearance_remains_inherited_for_every_saved_choice() { assert_eq!(h.ctx.options(|o| o.theme_preference), egui::ThemePreference::System); } } + +/// Preferences ▸ Appearance: a theme card is a manual choice that the mode combo box shows and +/// OS changes leave alone; Use system setting afterwards follows the OS again. +#[test] +fn preferences_theme_cards_and_use_system_setting_stay_in_sync() { + use egui::accesskit::Role; + fn mode_shown(h: &Harness<'static, PdfCraftApp>) -> String { + let combo = + h.get_by(|n| n.role() == Role::ComboBox && n.value().is_some_and(|v| ["Use system setting", "Light", "Dark"].contains(&v.as_str()))); + combo.value().unwrap_or_default() + } + let mut h = harness(); + h.get_by_label("Display theme: Light gray").click_secondary(); + h.run_steps(4); + h.get_by_label("Appearance settings…").click(); + h.run_steps(10); + assert_theme(&h, ThemePreference::Light, ThemeKind::Light); + assert_eq!(mode_shown(&h), "Light"); + + // The OS is dark: the Dark card's radio row picks Dark, and the combo box says so. + // The cards sit below the fold of the Preferences scroll area at this window size. + h.get_by_label("Dark gray").scroll_to_me(); + h.run_steps(3); + h.get_by_label("Dark gray").click(); + h.run_steps(3); + assert_theme(&h, ThemePreference::Dark, ThemeKind::Dark); + assert_eq!(mode_shown(&h), "Dark"); + // The Light card itself, not its radio row, picks Light, and a dark OS doesn't undo it. + h.get_by_label("Light theme").click(); + h.run_steps(3); + h.input_mut().system_theme = Some(egui::Theme::Light); + h.run_steps(2); + h.input_mut().system_theme = Some(egui::Theme::Dark); + h.run_steps(2); + assert_theme(&h, ThemePreference::Light, ThemeKind::Light); + assert_eq!(mode_shown(&h), "Light"); + + // Use system setting follows the OS both ways after a card choice. + h.get_by(|n| n.role() == Role::ComboBox && n.value().as_deref() == Some("Light")).scroll_to_me(); + h.run_steps(3); + h.get_by(|n| n.role() == Role::ComboBox && n.value().as_deref() == Some("Light")).click(); + h.run_steps(3); + // Motion has a "Use system setting" radio of its own; this is the combo box's item. + h.get_by_role_and_label(Role::Button, "Use system setting").click(); + h.run_steps(3); + assert_theme(&h, ThemePreference::System, ThemeKind::Dark); + assert_eq!(mode_shown(&h), "Use system setting"); + h.input_mut().system_theme = Some(egui::Theme::Light); + h.run_steps(3); + assert_theme(&h, ThemePreference::System, ThemeKind::Light); + // Only the Light card is marked Active while the OS is light. + assert_eq!(h.query_all_by_label("Active").count(), 1); + + // A card click leaves system mode again. + h.get_by_label("Dark theme").scroll_to_me(); + h.run_steps(3); + h.get_by_label("Dark theme").click(); + h.run_steps(3); + assert_theme(&h, ThemePreference::Dark, ThemeKind::Dark); + assert_eq!(mode_shown(&h), "Dark"); +}