diff --git a/crates/ui-egui/src/panels/comp_settings.rs b/crates/ui-egui/src/panels/comp_settings.rs index 26d3c3a69..b778a3999 100644 --- a/crates/ui-egui/src/panels/comp_settings.rs +++ b/crates/ui-egui/src/panels/comp_settings.rs @@ -478,15 +478,17 @@ fn tab_size(ui: &mut egui::Ui, tab: Tab, d: &CompDraft, t: &Tokens, custom: &[Co vec2(child.min_rect().width(), child.cursor().top() - top) } +/// Where the width of the dialog's content, measured when it was last shown, is kept. fn content_width_id() -> egui::Id { egui::Id::unique("comp-settings-content-width") } -/// The width the dialog's content took when it was last shown: its widest row on any tab. A -/// translation longer than the English (French, #2841) is wider than the 600-point dialog, so the -/// dialog grows to it rather than hiding OK past its right edge. -pub fn content_width(ctx: &egui::Context) -> f32 { - ctx.data(|d| d.get_temp::(content_width_id())).unwrap_or(0.0) +/// The dialog's width: 600 points, or more when the widest row on any of its tabs (as last shown), +/// a scroll bar and the modal's two 18-point margins need it. A translation longer than the English (French, +/// #2841) widens the dialog rather than hiding OK past its right edge. +pub fn dialog_width(ctx: &egui::Context) -> f32 { + let content = ctx.data(|d| d.get_temp::(content_width_id())).unwrap_or(0.0); + (content + 36.0).max(600.0) } /// Draw the dialog. Returns `Some(params)` when OK is pressed, `Some(Null)` on Cancel. `custom` @@ -501,8 +503,9 @@ pub fn show( preview: Option<&mut bool>, ) -> Option { let mut out = None; - // The dialog's visible area, which Cancel and OK stay inside. - auto.add("dialog.comp.view", ui.clip_rect().intersect(ui.max_rect()), "Composition Settings"); + // The dialog's visible area, which Cancel and OK stay inside: the scroll area's viewport, + // which stays put when the content scrolls. + auto.add("dialog.comp.view", ui.clip_rect(), "Composition Settings"); let name_row = ui.horizontal(|ui| { ui.label(tr("Composition Name:")); let r = ui.add(egui::TextEdit::singleline(&mut d.name).desired_width(300.0)); @@ -529,7 +532,19 @@ pub fn show( ui.add_space((tallest - shown).max(0.0)); // Every tab takes the width of the widest one too, so the dialog doesn't change size with them. let widest = sizes.iter().fold(name_row.response.rect.width().max(tab_row.response.rect.width()), |w, s| w.max(s.x)); - ui.ctx().data_mut(|m| m.insert_temp(content_width_id(), widest.ceil())); + // Room for a vertical scroll bar too: without it, a bar shown while the dialog settles narrows + // the content into a horizontal bar, which takes the height back from it, and the two bars + // trade places every frame. + let needed = widest + ui.spacing().scroll.allocated_width(); + let ctx = ui.ctx().clone(); + let before = dialog_width(&ctx); + ctx.data_mut(|m| m.insert_temp(content_width_id(), needed.ceil())); + if dialog_width(&ctx) != before { + // This pass was laid out at the old width: lay it out again at the new one instead of + // showing it, and paint again in case egui declines the extra pass. + ctx.request_discard("Composition Settings width"); + ctx.request_repaint(); + } ui.add_space(14.0); ui.horizontal(|ui| { ui.with_layout(egui::Layout::right_to_left(egui::Align::Center), |ui| { diff --git a/crates/ui-egui/src/panels/dialogs.rs b/crates/ui-egui/src/panels/dialogs.rs index e5c8950e7..d3ee74e58 100644 --- a/crates/ui-egui/src/panels/dialogs.rs +++ b/crates/ui-egui/src/panels/dialogs.rs @@ -446,9 +446,7 @@ fn comp_settings(app: &mut EffectcraftApp, ctx: &egui::Context, t: &Tokens, exis let live = existing && super::live_preview::active(app); let place = super::live_preview::place(app); let mut preview = app.dialog_state.live.on; - // 600 points wide, or as wide as the content took last time (with the 18-point margins). - let width = (super::comp_settings::content_width(ctx) + 36.0).max(600.0); - modal_placed(ctx, "Composition Settings", vec2(width, 470.0), t, place, |ui| { + modal_placed(ctx, "Composition Settings", vec2(super::comp_settings::dialog_width(ctx), 470.0), t, place, |ui| { result = super::comp_settings::show(ui, &mut d, t, &mut app.auto, &custom, &mut cmds, live.then_some(&mut preview)); }); app.dialog_state.live.on = preview; diff --git a/crates/ui-egui/tests/it/ui_comp_presets.rs b/crates/ui-egui/tests/it/ui_comp_presets.rs index a109d5324..262d2183b 100644 --- a/crates/ui-egui/tests/it/ui_comp_presets.rs +++ b/crates/ui-egui/tests/it/ui_comp_presets.rs @@ -136,3 +136,63 @@ fn composition_settings_buttons_fit_every_language() { assert!(oks.windows(2).all(|w| w[0] == w[1]), "{language}: OK moves between tabs: {oks:?}"); } } + +fn rect_of(h: &Harness<'_, EffectcraftApp>, id: &str) -> egui::Rect { + let r = h.state().auto.find(id).unwrap_or_else(|| panic!("no {id}")).rect; + egui::Rect::from_min_size(pos2(r[0], r[1]), egui::vec2(r[2], r[3])) +} + +/// In a window too short for it, the dialog scrolls: its registered view stays the scroll +/// area's viewport (not a part of it shifted by the scroll), and OK scrolls into it. +#[test] +fn composition_settings_view_stays_put_when_it_scrolls() { + let mut s = Session::default(); + s.execute("comp.new", json!({"name": "Edit", "width": 2560, "height": 1440, "frameRate": 60, "duration": 4})).unwrap(); + let mut h = Harness::builder().with_size(egui::vec2(1600.0, 520.0)).with_step_dt(1.0 / 60.0).build_eframe(|_| EffectcraftApp::new(s)); + h.run_steps(3); + open_settings(&mut h); + let (view, ok) = (rect_of(&h, "dialog.comp.view"), rect_of(&h, "dialog.comp.ok")); + assert!(ok.bottom() > view.bottom(), "the dialog must be taller than the window for this test: OK {ok:?}, view {view:?}"); + h.input_mut().events.push(Event::PointerMoved(view.center())); + h.input_mut().events.push(Event::MouseWheel { + unit: egui::MouseWheelUnit::Point, + delta: egui::vec2(0.0, -2000.0), + modifiers: Modifiers::NONE, + phase: egui::TouchPhase::Move, + source: egui::MouseWheelSource::Wheel, + }); + h.run_steps(30); + let (scrolled, ok) = (rect_of(&h, "dialog.comp.view"), rect_of(&h, "dialog.comp.ok")); + // The scroll bar may take a few points on the right while it is shown. + assert_eq!((scrolled.y_range(), scrolled.left()), (view.y_range(), view.left()), "the view is the viewport, wherever the content is scrolled to"); + assert!(scrolled.contains_rect(ok), "OK {ok:?} scrolls into the view {scrolled:?}"); +} + +/// Changing the interface language with the dialog open refits it to the new text and then +/// settles, with OK inside it: its scroll bars don't trade places every frame while it grows. +/// Switching back to English returns it to 600 points. +#[test] +fn composition_settings_refits_when_the_language_changes() { + let mut s = Session::default(); + s.execute("comp.new", json!({"name": "Edit", "width": 2560, "height": 1440, "frameRate": 60, "duration": 4})).unwrap(); + let mut h = Harness::builder() + .with_size(egui::vec2(1600.0, 1000.0)) + .with_step_dt(1.0 / 60.0) + .with_max_steps(30) + // `run` checks accessibility after settling; that is not what this test is about. + .with_accessibility_check(false) + .build_eframe(|_| EffectcraftApp::new(s)); + h.run_steps(3); + open_settings(&mut h); + click(&mut h, "dialog.comp.tab.advanced"); + h.run_ok().expect("the open dialog settles"); + assert_eq!(rect_of(&h, "dialog.comp.view").width(), 600.0 - 36.0); + for (language, wider) in [("fr", true), ("en", false)] { + h.state_mut().session.prefs.general.language = language.into(); + // Steps while anything asks for a repaint, and fails if that never stops. + let steps = h.try_run().unwrap_or_else(|e| panic!("{language}: {e}")); + let (view, ok) = (rect_of(&h, "dialog.comp.view"), rect_of(&h, "dialog.comp.ok")); + assert!(view.contains_rect(ok), "{language}: after {steps} steps OK {ok:?} is outside the dialog {view:?}"); + assert_eq!(view.width() > 600.0 - 36.0, wider, "{language}: the dialog is {} wide", view.width()); + } +}