From f4c5015b35a8f2e233fef9806ff3735a6b82a1a7 Mon Sep 17 00:00:00 2001 From: tylerjenningsw Date: Sun, 11 Oct 2026 11:48:07 -0400 Subject: [PATCH] Settings: keep action buttons visible at high UI scales At 200 % in a 1600x1000 window, Previous / Next / Cancel / OK were cut off. Settings sized its pages from the window height less a fixed 170, and the shared modal put everything, buttons included, in one more scroll area, so a footer that didn't fit scrolled out of the dialog. The modal helper gets a modal_fit variant without that scroll area: it hands its body the height left under the title. Settings lays out its buttons first, from the bottom up, and gives the page list and the page what they leave; both scroll on their own and the buttons stay put. The pages are a clipped child, so a wide page can't widen the dialog under the buttons. Other dialogs keep modal as it was. Tests: settings_buttons_stay_in_view_at_200_percent opens Settings at 200 % in 1600x1000 and checks that each button is inside the window and painted unclipped (fails before this change: the scroll area clipped them at the bottom). It scrolls the page to UI Scale without moving them, then types 150 and clicks Cancel (200 % again, nothing saved) and OK (150 % saved). The UI Scale test no longer needs a larger window and clicks Cancel at 200 %. --- crates/ui-egui/src/panels/dialogs.rs | 21 +++- crates/ui-egui/src/panels/settings.rs | 66 ++++++------ crates/ui-egui/tests/ui_panels.rs | 144 ++++++++++++++++++++------ 3 files changed, 168 insertions(+), 63 deletions(-) diff --git a/crates/ui-egui/src/panels/dialogs.rs b/crates/ui-egui/src/panels/dialogs.rs index 81fa6bab..c385beec 100644 --- a/crates/ui-egui/src/panels/dialogs.rs +++ b/crates/ui-egui/src/panels/dialogs.rs @@ -149,9 +149,23 @@ pub fn route_layer_settings(app: &mut EffectcraftApp, id: &str, params: &Value) } } +/// The room a modal dialog may take: the window less a margin. +fn modal_space(ctx: &egui::Context) -> egui::Vec2 { + (ctx.content_rect().size() - vec2(48.0, 48.0)).max(vec2(120.0, 80.0)) +} + pub(crate) fn modal(ctx: &egui::Context, title: &str, size: egui::Vec2, t: &Tokens, body: impl FnOnce(&mut egui::Ui)) { - let available = (ctx.content_rect().size() - vec2(48.0, 48.0)).max(vec2(120.0, 80.0)); - let size = size.min(available); + let max_height = (modal_space(ctx).y - 84.0).max(40.0); + modal_fit(ctx, title, size, t, |ui, _| { + egui::ScrollArea::both().max_height(max_height).auto_shrink([false, true]).show(ui, body); + }); +} + +/// [`modal`] without its scroll area, for a dialog that keeps its buttons in view and scrolls +/// only its content: `body` gets the height left under the title, with `size` kept within the +/// window. +pub(crate) fn modal_fit(ctx: &egui::Context, title: &str, size: egui::Vec2, t: &Tokens, body: impl FnOnce(&mut egui::Ui, f32)) { + let size = size.min(modal_space(ctx)); egui::Modal::new(egui::Id::new(("modal", title))) .backdrop_color(Color32::from_black_alpha(120)) .frame(egui::Frame::window(&ctx.style_of(ctx.theme())).fill(t.panel_bg).inner_margin(egui::Margin::same(18))) @@ -161,7 +175,8 @@ pub(crate) fn modal(ctx: &egui::Context, title: &str, size: egui::Vec2, t: &Toke // The id stays keyed on the untranslated source, so switching language cannot move it. ui.label(egui::RichText::new(tr(title)).font(Tokens::semibold(15.0)).color(t.tab_text_active)); ui.add_space(10.0); - egui::ScrollArea::both().max_height((available.y - 84.0).max(40.0)).auto_shrink([false, true]).show(ui, body); + let title_h = ui.cursor().top() - ui.min_rect().top(); + body(ui, (size.y - 36.0 - title_h).max(40.0)); }); } diff --git a/crates/ui-egui/src/panels/settings.rs b/crates/ui-egui/src/panels/settings.rs index 05e27748..ae881a37 100644 --- a/crates/ui-egui/src/panels/settings.rs +++ b/crates/ui-egui/src/panels/settings.rs @@ -10,7 +10,7 @@ use crate::i18n::{tr, tr_args}; use effectcraft_engine::prefs::{APPEARANCE_MODES, DARK_THEMES, Item, Kind, LIGHT_THEMES, Page, pages}; use effectcraft_engine::segment::Task; -use egui::{Color32, RichText, vec2}; +use egui::{Color32, Rect, RichText, vec2}; use serde_json::{Value, json}; use crate::theme::{ThemeKind, Tokens}; @@ -604,10 +604,40 @@ pub fn show(app: &mut EffectcraftApp, ctx: &egui::Context, t: &Tokens) { let cur = serde_json::to_value(&app.session.prefs).unwrap_or_default(); let mut acts: Vec = vec![]; let (mut ok, mut cancel) = (false, false); - super::dialogs::modal(ctx, "Settings", vec2(820.0, 600.0), t, |ui| { - let body_h = (ctx.content_rect().height() - 170.0).clamp(120.0, 470.0); - ui.allocate_ui_with_layout(vec2(ui.available_width(), body_h), egui::Layout::left_to_right(egui::Align::Min), |ui| { - ui.set_height(body_h); + super::dialogs::modal_fit(ctx, "Settings", vec2(820.0, 600.0), t, |ui, height| { + // The buttons go first, from the bottom up, and the pages get the height they leave: at + // a high UI Scale the pages scroll and the buttons stay in view. + let area = ui.allocate_space(vec2(ui.available_width(), height)).1; + let mut foot = ui.new_child(egui::UiBuilder::new().max_rect(area).layout(egui::Layout::bottom_up(egui::Align::Min))); + foot.horizontal(|ui| { + let r = ui.add_enabled(idx > 0, egui::Button::new(tr("Previous"))); + app.auto.add("settings.previous", r.rect, "Previous"); + if r.clicked() { + page = all[idx - 1].id.to_string(); + } + let r = ui.add_enabled(idx + 1 < all.len(), egui::Button::new(tr("Next"))); + app.auto.add("settings.next", r.rect, "Next"); + if r.clicked() { + page = all[idx + 1].id.to_string(); + } + ui.with_layout(egui::Layout::right_to_left(egui::Align::Center), |ui| { + let r = ui.add(egui::Button::new(RichText::new(tr(" OK ")).color(Color32::WHITE)).fill(t.accent)); + app.auto.add("settings.ok", r.rect, "OK"); + ok |= r.clicked(); + let r = ui.button(tr("Cancel")); + app.auto.add("settings.cancel", r.rect, "Cancel"); + cancel |= r.clicked(); + }); + }); + foot.separator(); + foot.add_space(8.0); + let body_h = (foot.min_rect().top() - area.top()).max(0.0); + // (A child, clipped to its share, so a wide page can't widen the dialog under the buttons.) + let pages = Rect::from_min_size(area.min, vec2(area.width(), body_h)); + let mut body = ui.new_child(egui::UiBuilder::new().max_rect(pages).layout(egui::Layout::left_to_right(egui::Align::Min))); + body.set_clip_rect(pages.intersect(body.clip_rect())); + { + let ui = &mut body; // Page list (left, like After Effects). ui.vertical(|ui| { ui.set_width(180.0); @@ -642,33 +672,11 @@ pub fn show(app: &mut EffectcraftApp, ctx: &egui::Context, t: &Tokens) { let p = &all[idx]; ui.label(RichText::new(tr(p.title)).font(Tokens::semibold(14.0)).color(t.tab_text_active)); ui.add_space(6.0); - egui::ScrollArea::vertical().max_height(body_h - 30.0).auto_shrink([false, false]).show(ui, |ui| { + egui::ScrollArea::vertical().max_height((body_h - 30.0).max(0.0)).auto_shrink([false, false]).show(ui, |ui| { page_ui(app, ui, p, &cur, t, &mut acts); }); }); - }); - ui.add_space(8.0); - ui.separator(); - ui.horizontal(|ui| { - let r = ui.add_enabled(idx > 0, egui::Button::new(tr("Previous"))); - app.auto.add("settings.previous", r.rect, "Previous"); - if r.clicked() { - page = all[idx - 1].id.to_string(); - } - let r = ui.add_enabled(idx + 1 < all.len(), egui::Button::new(tr("Next"))); - app.auto.add("settings.next", r.rect, "Next"); - if r.clicked() { - page = all[idx + 1].id.to_string(); - } - ui.with_layout(egui::Layout::right_to_left(egui::Align::Center), |ui| { - let r = ui.add(egui::Button::new(RichText::new(tr(" OK ")).color(Color32::WHITE)).fill(t.accent)); - app.auto.add("settings.ok", r.rect, "OK"); - ok |= r.clicked(); - let r = ui.button(tr("Cancel")); - app.auto.add("settings.cancel", r.rect, "Cancel"); - cancel |= r.clicked(); - }); - }); + } }); app.dialog_state.settings_page = page; for a in acts { diff --git a/crates/ui-egui/tests/ui_panels.rs b/crates/ui-egui/tests/ui_panels.rs index a600c2e2..bbd15a5a 100644 --- a/crates/ui-egui/tests/ui_panels.rs +++ b/crates/ui-egui/tests/ui_panels.rs @@ -2,7 +2,9 @@ //! registers its automation ids; the Footage panel's buttons edit into the comp; the Progress //! panel lists and cancels a job; Lumetri Scopes switch scope through their dropdown. -use effectcraft_engine::Session; +use std::sync::Arc; + +use effectcraft_engine::{Session, config::MemoryConfig}; use effectcraft_ui_egui::EffectcraftApp; use effectcraft_ui_egui::dock::PanelKind; use egui::{Event, Pos2, pos2}; @@ -237,41 +239,47 @@ fn leaving_the_learn_workspace_closes_its_tutorials() { } } +fn open_settings(h: &mut Harness<'_, EffectcraftApp>) { + let ctx = h.ctx.clone(); + effectcraft_ui_egui::menus::invoke(h.state_mut(), &ctx, "app.settings", json!({"page": "appearance"})).unwrap(); + h.run_steps(3); +} + +fn key(h: &mut Harness<'_, EffectcraftApp>, key: egui::Key, modifiers: egui::Modifiers) { + for pressed in [true, false] { + h.input_mut().events.push(Event::Key { key, physical_key: None, pressed, repeat: false, modifiers }); + h.step(); + } +} + +/// The UI Scale setting and the zoom it gives. +fn scale(h: &Harness<'_, EffectcraftApp>) -> (u32, f32) { + (h.state().session.prefs.appearance.ui_scale, h.ctx.zoom_factor()) +} + +/// Types `text` into Settings' UI Scale a key at a time: nothing applies (and the field keeps +/// the keyboard) until Enter. +fn type_scale(h: &mut Harness<'_, EffectcraftApp>, text: &str) { + let before = scale(h); + click(h, "settings.appearance.uiScale"); + for c in text.chars() { + h.input_mut().events.push(Event::Text(c.to_string())); + h.run_steps(2); + assert_eq!(scale(h), before, "{text}: not while typing"); + assert!(h.ctx.memory(|m| m.focused().is_some()), "{text}: the field keeps the keyboard"); + } + key(h, egui::Key::Enter, egui::Modifiers::NONE); + // (Settings re-centres a frame after the zoom changes.) + h.run_steps(4); +} + /// #284, #682: Settings ▸ Appearance ▸ UI Scale takes any typed percentage from 75 to 200 and /// sizes the whole interface once it is entered, Cancel puts it back, OK keeps it and later /// sessions start with it; Ctrl+0 (Window ▸ Project) doesn't reset it. #[test] fn ui_scale_setting_sizes_the_interface() { let mut h = harness(); - // Room for the whole Settings dialog at 200 %. - h.set_size(egui::vec2(2400.0, 1600.0)); let ctx = h.ctx.clone(); - let open_settings = |h: &mut Harness<'_, EffectcraftApp>| { - let ctx = h.ctx.clone(); - effectcraft_ui_egui::menus::invoke(h.state_mut(), &ctx, "app.settings", json!({"page": "appearance"})).unwrap(); - h.run_steps(3); - }; - let key = |h: &mut Harness<'_, EffectcraftApp>, key: egui::Key, modifiers: egui::Modifiers| { - for pressed in [true, false] { - h.input_mut().events.push(Event::Key { key, physical_key: None, pressed, repeat: false, modifiers }); - h.step(); - } - }; - // Types `text` into the field a key at a time: nothing applies (and the field keeps the - // keyboard) until Enter. - let type_scale = |h: &mut Harness<'_, EffectcraftApp>, text: &str| { - let before = (h.state().session.prefs.appearance.ui_scale, h.ctx.zoom_factor()); - click(h, "settings.appearance.uiScale"); - for c in text.chars() { - h.input_mut().events.push(Event::Text(c.to_string())); - h.run_steps(2); - assert_eq!((h.state().session.prefs.appearance.ui_scale, h.ctx.zoom_factor()), before, "{text}: not while typing"); - assert!(h.ctx.memory(|m| m.focused().is_some()), "{text}: the field keeps the keyboard"); - } - key(h, egui::Key::Enter, egui::Modifiers::NONE); - h.run_steps(2); - }; - let scale = |h: &Harness<'_, EffectcraftApp>| (h.state().session.prefs.appearance.ui_scale, h.ctx.zoom_factor()); assert_eq!(ctx.zoom_factor(), 1.0); open_settings(&mut h); type_scale(&mut h, "85"); @@ -280,10 +288,14 @@ fn ui_scale_setting_sizes_the_interface() { h.run_steps(2); assert_eq!(scale(&h), (100, 1.0), "Cancel put it back"); open_settings(&mut h); - type_scale(&mut h, "50"); - assert_eq!(scale(&h), (75, 0.75), "at least 75"); type_scale(&mut h, "300"); assert_eq!(scale(&h), (200, 2.0), "at most 200"); + click(&mut h, "settings.cancel"); + h.run_steps(2); + assert_eq!(scale(&h), (100, 1.0), "Cancel put it back from 200 %"); + open_settings(&mut h); + type_scale(&mut h, "50"); + assert_eq!(scale(&h), (75, 0.75), "at least 75"); type_scale(&mut h, "90"); click(&mut h, "settings.ok"); h.run_steps(2); @@ -300,6 +312,76 @@ fn ui_scale_setting_sizes_the_interface() { assert_eq!(h2.ctx.zoom_factor(), 0.9); } +/// Settings' Previous / Next / Cancel / OK stay whole and in view at 200 % in a 1600×1000 +/// window (they scrolled out of the dialog with its pages): the page scrolls under them, Cancel +/// restores the settings and OK saves them. +#[test] +fn settings_buttons_stay_in_view_at_200_percent() { + let store = Arc::new(MemoryConfig::default()); + let saved = || { + let mut s = Session { config: Some(store.clone()), ..Default::default() }; + s.prefs.disk.disk_cache_enabled = false; + s.load_settings(); + s.prefs.appearance.ui_scale + }; + let mut s = Session { config: Some(store.clone()), ..Default::default() }; + s.prefs.disk.disk_cache_enabled = false; + s.prefs.set("appearance.uiScale", json!(200)).unwrap(); + s.save_prefs(); + let mut h = Harness::builder().with_size(egui::vec2(1600.0, 1000.0)).build_eframe(|_| EffectcraftApp::new(s)); + h.run_steps(3); + assert_eq!(scale(&h), (200, 2.0)); + let rect = |h: &Harness<'_, EffectcraftApp>, id: &str| { + let e = h.state().auto.find(id).unwrap_or_else(|| panic!("no {id}")); + egui::Rect::from_min_size(pos2(e.rect[0], e.rect[1]), egui::vec2(e.rect[2], e.rect[3])) + }; + // Whole: inside the window, and painted with none of them clipped away. + let buttons = |h: &Harness<'_, EffectcraftApp>| { + ["previous", "next", "cancel", "ok"].map(|b| { + let r = rect(h, &format!("settings.{b}")); + let clip = h + .output() + .shapes + .iter() + .find(|c| matches!(&c.shape, egui::Shape::Rect(s) if (s.rect.center() - r.center()).length() < 0.5)) + .map(|c| c.clip_rect); + assert!(h.ctx.content_rect().contains_rect(r) && clip.is_some_and(|c| c.contains_rect(r)), "{b} is cut off: {r:?} in {clip:?}"); + r + }) + }; + let scroll_page = |h: &mut Harness<'_, EffectcraftApp>| { + let at = rect(h, "settings.appearance.appearanceMode").center(); + h.input_mut().events.push(Event::PointerMoved(at)); + h.input_mut().events.push(Event::MouseWheel { + unit: egui::MouseWheelUnit::Point, + delta: egui::vec2(0.0, -1000.0), + modifiers: Default::default(), + phase: egui::TouchPhase::Move, + }); + h.run_steps(5); + }; + open_settings(&mut h); + let footer = buttons(&h); + let field = rect(&h, "settings.appearance.uiScale"); + scroll_page(&mut h); + assert_eq!(buttons(&h), footer, "the buttons don't scroll with the page"); + let scrolled = rect(&h, "settings.appearance.uiScale"); + assert!(scrolled.top() < field.top() && scrolled.bottom() < footer[2].top(), "the page scrolled to UI Scale: {field:?} -> {scrolled:?}"); + for (button, kept) in [("settings.cancel", 200), ("settings.ok", 150)] { + if h.state().dialog.is_none() { + open_settings(&mut h); + scroll_page(&mut h); + } + type_scale(&mut h, "150"); + assert_eq!(scale(&h), (150, 1.5)); + buttons(&h); + click(&mut h, button); + h.run_steps(2); + assert_eq!(h.state().dialog, None, "{button} closed Settings"); + assert_eq!((scale(&h), saved()), ((kept, kept as f32 / 100.0), kept), "{button}"); + } +} + /// Headless look at the panels (wgpu offscreen; needs a GPU adapter). Run with /// `PANELS_SNAPSHOT=/abs/dir cargo test -p effectcraft-ui-egui --test ui_panels -- --ignored`. #[test]