Merge pull request #6516 from dotCooCoo/fix/menu-scroll-wrap

Menus: a scrolling list keeps each label on one line

crates/ui-egui/src/panels/appearance/tests.rs

@@ -1,5 +1,6 @@
 use super::super::tests_long_popups::{
-    PopupWindow, assert_last_row_scrolls_into_view, assert_popup_stays_put, assert_short_list_does_not_scroll, fx_menu_ends,
+    PopupWindow, assert_last_row_scrolls_into_view, assert_popup_stays_put, assert_rows_stay_on_one_line, assert_short_list_does_not_scroll,
+    fx_menu_ends,
 };
 use super::*;
 
@@ -65,6 +66,20 @@ fn a_short_swatch_picker_does_not_scroll() {
     assert_eq!(window.clickable(TILE), tiles, "a list that fits does not scroll");
 }
 
+/// The Add New Effect popup keeps each group's name on one line when its list scrolls: the scroll
+/// bar's column widens the popup instead of narrowing the rows.
+#[test]
+fn the_add_effect_popup_keeps_its_rows_on_one_line_when_it_scrolls() {
+    let mut groups: Vec<String> = vec![];
+    for effect in vectorcraft_effects::effect_catalog() {
+        let group = effect.menu.get(1).copied().unwrap_or("Other").to_string();
+        if !groups.contains(&group) {
+            groups.push(group);
+        }
+    }
+    assert_rows_stay_on_one_line(&mut app_with_a_selected_rectangle(), draw_bottom_bar, fx_button, &groups, "Distort & Transform", 120.0);
+}
+
 /// The Add New Effect button: the third of the bottom bar's 24 pt buttons.
 fn fx_button(window: &PopupWindow) -> egui::Pos2 {
     window.clickable(vec2(24.0, 24.0))[2].center()

crates/ui-egui/src/panels/image_trace/tests.rs

@@ -1,4 +1,6 @@
-use super::super::tests_long_popups::{PopupWindow, assert_last_row_scrolls_into_view, assert_short_list_does_not_scroll};
+use super::super::tests_long_popups::{
+    PopupWindow, assert_last_row_scrolls_into_view, assert_rows_stay_on_one_line, assert_short_list_does_not_scroll,
+};
 use super::*;
 use crate::panels::library_panel;
 use egui::{Event, PointerButton, Pos2, Rect, vec2};
@@ -66,6 +68,20 @@ fn a_long_tracing_preset_list_scrolls_inside_a_short_window() {
     assert_last_row_scrolls_into_view(&mut app, 200.0, draw, tracing_presets_arrow, &first, "Saved 29");
 }
 
+/// The list of presets keeps each name on one line when it scrolls: the scroll bar's column widens
+/// the list instead of narrowing the rows.
+#[test]
+fn a_long_tracing_preset_list_keeps_its_rows_on_one_line() {
+    let mut app = VectorcraftApp::new(vectorcraft_engine::Session::new(), Default::default());
+    let draw = |app: &mut VectorcraftApp, ui: &mut Ui| trace_button(app, ui, TRACE_BUTTON_W);
+    for n in 0..30 {
+        app.session.execute("imageTrace.presets.save", &json!({"name": format!("Saved {n}"), "preset": "Default"})).unwrap();
+    }
+    let presets = app.session.execute("imageTrace.presets", &json!({})).unwrap();
+    let names: Vec<String> = presets["presets"].as_array().unwrap().iter().map(|p| p["name"].as_str().unwrap().to_string()).collect();
+    assert_rows_stay_on_one_line(&mut app, draw, tracing_presets_arrow, &names, "Black and White Logo", 120.0);
+}
+
 #[test]
 fn a_large_image_traces_and_the_status_bar_says_it_is_slower() {
     let mut app = VectorcraftApp::new(vectorcraft_engine::Session::new(), Default::default());

crates/ui-egui/src/panels/tests_long_popups.rs

@@ -21,6 +21,7 @@ type Draw<'a> = Box<dyn FnMut(&mut VectorcraftApp, &mut Ui) + 'a>;
 pub(super) struct PopupWindow<'a> {
     ctx: egui::Context,
     time: f64,
+    width: f32,
     height: f32,
     draw: Draw<'a>,
     texts: Vec<(String, Rect)>,
@@ -30,7 +31,7 @@ impl<'a> PopupWindow<'a> {
     pub(super) fn new(app: &mut VectorcraftApp, height: f32, draw: impl FnMut(&mut VectorcraftApp, &mut Ui) + 'a) -> Self {
         let ctx = egui::Context::default();
         crate::theme::install_fonts(&ctx);
-        let mut window = Self { ctx, time: 0.0, height, draw: Box::new(draw), texts: vec![] };
+        let mut window = Self { ctx, time: 0.0, width: WIDTH, height, draw: Box::new(draw), texts: vec![] };
         window.frames(app, 2);
         window
     }
@@ -38,7 +39,7 @@ impl<'a> PopupWindow<'a> {
     /// One frame with `events`.
     fn frame(&mut self, app: &mut VectorcraftApp, events: Vec<Event>) {
         self.time += 1.0 / 60.0;
-        let screen_rect = Some(Rect::from_min_size(Pos2::ZERO, vec2(WIDTH, self.height)));
+        let screen_rect = Some(Rect::from_min_size(Pos2::ZERO, vec2(self.width, self.height)));
         let input = egui::RawInput { time: Some(self.time), screen_rect, events, ..Default::default() };
         let draw = &mut self.draw;
         let mut out = self.ctx.run_ui(input, |ui| draw(app, ui));
@@ -46,6 +47,12 @@ impl<'a> PopupWindow<'a> {
         self.texts = crate::tests_removeanchors::shapes_text(&out.shapes.iter().map(|c| c.shape.clone()).collect::<Vec<_>>());
     }
 
+    /// Makes the window `width` pt wide and draws the frames that takes.
+    pub(super) fn resize(&mut self, app: &mut VectorcraftApp, width: f32) {
+        self.width = width;
+        self.frames(app, OPEN_FRAMES);
+    }
+
     fn frames(&mut self, app: &mut VectorcraftApp, count: usize) {
         for _ in 0..count {
             self.frame(app, vec![]);
@@ -58,6 +65,14 @@ impl<'a> PopupWindow<'a> {
         self.texts.iter().rfind(|(text, _)| text.trim() == wanted).map(|(_, rect)| *rect)
     }
 
+    /// The rect of the popup drawn on top: the last shown area in the foreground.
+    pub(super) fn popup(&self) -> Option<Rect> {
+        self.ctx.memory(|m| {
+            let top = m.layer_ids().filter(|layer| layer.order == egui::Order::Foreground && m.areas().is_visible(layer)).last()?;
+            m.area_rect(top.id)
+        })
+    }
+
     /// The rects of the last frame's clickable widgets `size` across, in the order they were drawn.
     pub(super) fn clickable(&self, size: egui::Vec2) -> Vec<Rect> {
         self.ctx.viewport(|vp| {
@@ -81,9 +96,14 @@ impl<'a> PopupWindow<'a> {
 
     /// Turns the wheel far down with the pointer at `over`, and lets the scrolling finish.
     pub(super) fn wheel_down(&mut self, app: &mut VectorcraftApp, over: Pos2) {
+        self.wheel(app, over, 5000.0);
+    }
+
+    /// Turns the wheel `down` pt down with the pointer at `over`, and lets the scrolling finish.
+    pub(super) fn wheel(&mut self, app: &mut VectorcraftApp, over: Pos2, down: f32) {
         let wheel = Event::MouseWheel {
             unit: egui::MouseWheelUnit::Point,
-            delta: vec2(0.0, -5000.0),
+            delta: vec2(0.0, -down),
             phase: egui::TouchPhase::Move,
             modifiers: Default::default(),
             source: egui::MouseWheelSource::Unknown,
@@ -189,6 +209,80 @@ pub(super) fn assert_last_row_scrolls_into_view(
     assert!(row.bottom() <= height, "{last} at {row:?} is inside the {height} pt window");
 }
 
+/// In windows 200, 400 and 900 pt tall, the popup of the button `pick` finds draws each of its rows
+/// `rows` that are in view on one line, as tall as the row with the shortest name, and left of the
+/// scroll bar's column, whether or not its list scrolls. The row `widest` is drawn the widest; the
+/// wheel brings it into view when the popup opens without it. The popup and that row keep their
+/// place and size while the popup stays open. After the 200 pt window becomes `narrow` pt wide,
+/// the popup and its rows in view stay inside the window.
+pub(super) fn assert_rows_stay_on_one_line(
+    app: &mut VectorcraftApp,
+    mut draw: impl FnMut(&mut VectorcraftApp, &mut Ui),
+    pick: impl Fn(&PopupWindow) -> Pos2,
+    rows: &[String],
+    widest: &str,
+    narrow: f32,
+) {
+    let bar = crate::widgets::list_scroll_style().allocated_width();
+    let in_view =
+        |window: &PopupWindow| -> Vec<(String, Rect)> { rows.iter().filter_map(|row| window.text(row).map(|rect| (row.clone(), rect))).collect() };
+    let assert_inside = |window: &PopupWindow, drawn: &[(String, Rect)], name: &str| {
+        let popup = window.popup().unwrap_or_else(|| panic!("the popup is open in the {name} window"));
+        for (row, rect) in drawn {
+            assert!(
+                rect.left() >= popup.left() && rect.right() <= popup.right() - bar,
+                "{row} at {rect:?} is drawn left of the scroll bar's column in the popup at {popup:?} in the {name} window"
+            );
+        }
+        popup
+    };
+    for height in [200.0, 400.0, TALL] {
+        let mut window = PopupWindow::new(app, height, |app, ui| draw(app, ui));
+        let button = pick(&window);
+        window.click(app, button);
+        for _ in 0..rows.len() {
+            if window.text(widest).is_some() {
+                break;
+            }
+            let drawn = in_view(&window);
+            let Some((_, middle)) = drawn.get(drawn.len() / 2) else { break };
+            window.wheel(app, middle.center(), 20.0);
+        }
+        let opened = window.text(widest).unwrap_or_else(|| panic!("the popup's row {widest} is in view in the {height} pt window"));
+        let drawn = in_view(&window);
+        assert!(drawn.len() >= 3, "the popup's rows are in view in the {height} pt window: {drawn:?}");
+        let Some((shortest, line)) = drawn.iter().min_by_key(|(row, _)| row.chars().count()) else { panic!("no rows in view") };
+        let line = line.height();
+        for (row, rect) in &drawn {
+            assert!(
+                rect.height() <= line + 0.5,
+                "{row} at {rect:?} is drawn on one line, as tall as {shortest} ({line} pt), in the {height} pt window"
+            );
+        }
+        for (row, rect) in &drawn {
+            assert!(rect.width() <= opened.width() + 0.5, "{widest} is drawn as wide as {row} at {rect:?} or wider in the {height} pt window");
+        }
+        let popup = assert_inside(&window, &drawn, &format!("{height} pt"));
+        // The popup's size settles within a frame of opening; after that it stays the same.
+        window.frames(app, 30);
+        let settled = window.popup();
+        assert!(
+            settled.is_some_and(|settled| (settled.width() - popup.width()).abs() <= 0.5 && settled.min == popup.min),
+            "the popup at {settled:?} stays where it opened, at {popup:?}, in the {height} pt window"
+        );
+        window.frames(app, 300);
+        assert_eq!(window.text(widest), Some(opened), "{widest} keeps its place and size while the popup stays open in the {height} pt window");
+        assert_eq!(window.popup(), settled, "the popup keeps its place and size while it stays open in the {height} pt window");
+        if height == 200.0 {
+            window.resize(app, narrow);
+            let drawn = in_view(&window);
+            assert!(!drawn.is_empty(), "the popup's rows are in view in the {narrow} pt wide window");
+            let popup = assert_inside(&window, &drawn, &format!("{narrow} pt wide"));
+            assert!(popup.left() >= 0.0, "the popup at {popup:?} starts inside the {narrow} pt wide window");
+        }
+    }
+}
+
 /// In a tall window the popup of the button `pick` finds shows its rows `first` to `last` without
 /// scrolling, and the wheel moves neither.
 pub(super) fn assert_short_list_does_not_scroll(

crates/ui-egui/src/widgets/dropdowns.rs

@@ -379,29 +379,51 @@ fn popup_anchor(ui: &Ui) -> Option<(Rect, bool)> {
 /// The body of a menu or popup list: as tall as its items up to the bottom bars of the window it
 /// opens in, and scrolling only past that, so long menus (Window, Effect, a panel's menu) stay
 /// reachable on small windows and in a floating panel's short OS window. Its scroll bar has a column
-/// of its own, so it never covers the shortcuts or the submenu arrows (#2086, #5347).
+/// of its own, so it never covers the shortcuts or the submenu arrows (#2086, #5347). When the list
+/// scrolls, that column widens the popup instead of narrowing the items, so their labels stay on one
+/// line.
 ///
 /// The list's height comes from the room below and above the button, row or point the popup opens
 /// from. A popup with an id of its own measures from its own top instead.
 pub fn menu_scroll<R>(ui: &mut Ui, add_contents: impl FnOnce(&mut Ui) -> R) -> R {
-    let ctx = ui.ctx();
-    let tallest = (ctx.content_rect().height() - egui::Frame::popup(ui.style()).total_margin().sum().y).max(0.0);
+    let ctx = ui.ctx().clone();
+    let margins = egui::Frame::popup(ui.style()).total_margin().sum();
+    let tallest = (ctx.content_rect().height() - margins.y).max(0.0);
     let top = ui.next_widget_position().y;
     // The rows above the list (the popup's margin, a search field) move with the popup.
     let popup_top = ui.stack().iter().last().map_or(top, |root| root.max_rect.top());
     let ahead = top - popup_top;
-    let (floor, window_top) = (menu_floor(ctx), ctx.content_rect().top());
+    let (floor, window_top) = (menu_floor(&ctx), ctx.content_rect().top());
     let room = if let Some((anchor, submenu)) = popup_anchor(ui) {
         let (down_from, up_to) = if submenu { (anchor.top(), anchor.bottom()) } else { (anchor.bottom(), anchor.top()) };
         menu_room_either(floor - down_from - ahead, up_to - window_top - ahead, tallest)
     } else {
         menu_room(floor, top, tallest)
     };
+    // The items' width on the first frame of this showing, when the scroll area does not yet show
+    // its bar. Later frames lay the items out at least that wide, and the bar's column is added to
+    // the popup's width. The width is kept until the popup closes.
+    let key = ui.next_auto_id().with("menu-scroll-width");
+    let frame = ctx.cumulative_frame_nr();
+    let first = ctx.data(|d| d.get_temp::<(u64, f32)>(key)).filter(|(shown, _)| shown + 1 >= frame).map(|(_, width)| width);
+    let scroll = list_scroll_style();
+    // The widest the items can be with the popup's margins and the bar's column inside the window.
+    let widest = (ctx.content_rect().width() - margins.x - scroll.allocated_width()).max(0.0);
     ui.scope(|ui| {
-        ui.spacing_mut().scroll = list_scroll_style();
+        ui.spacing_mut().scroll = scroll;
         // The minimum lets the list grow past a popup's default 400 pt size; it still shrinks to
         // its items when they need less.
-        egui::ScrollArea::vertical().max_height(room).min_scrolled_height(room).show(ui, add_contents).inner
+        let area = egui::ScrollArea::vertical().max_height(room).min_scrolled_height(room);
+        let out = area.show(ui, |ui| {
+            // Always the same child, so the items' ids stay the same on the frame the width changes.
+            let max = ui.max_rect();
+            let width = first.map_or(max.width(), |first| first.min(widest).max(max.width()));
+            let rect = egui::Rect::from_min_size(max.min, egui::vec2(width, max.height()));
+            ui.scope_builder(egui::UiBuilder::new().max_rect(rect).layout(*ui.layout()), add_contents).inner
+        });
+        let width = first.unwrap_or(out.content_size.x);
+        ctx.data_mut(|d| d.insert_temp(key, (frame, width)));
+        out.inner
     })
     .inner
 }