Add better support for scrolling content to our modals (#8492)

### What This PR builds proper scrollability to our modals, thereby fixing: - an ugly glitch with large add/remove entity modal - add view/container modal not scrollable with very small viewer window size #### Before <img width="625" alt="image" src="https://github.com/user-attachments/assets/cfe3d0cb-ac10-4bf5-84cb-4766589c5b41" /> #### After <img width="641" alt="image" src="https://github.com/user-attachments/assets/919cb62b-4934-429b-a41c-025f7c0cd9a9" /> <img width="951" alt="image" src="https://github.com/user-attachments/assets/e8eec505-a789-4285-b2c5-0c4906660fff" /> --------- Co-authored-by: Clement Rey <[email protected]>
rerun-io · Dec 17, 2024 · d41c53f · d41c53f
1 parent 02ae9c5
commit d41c53f
Show file tree

Hide file tree

Showing 4 changed files with 171 additions and 41 deletions.
diff --git a/crates/viewer/re_selection_panel/src/view_entity_picker.rs b/crates/viewer/re_selection_panel/src/view_entity_picker.rs
@@ -1,4 +1,3 @@
-use egui::NumExt as _;
 use itertools::Itertools;
 use nohash_hasher::IntMap;
 
@@ -39,6 +38,7 @@ impl ViewEntityPicker {
                 re_ui::modal::ModalWrapper::new("Add/remove Entities")
                     .default_height(640.0)
                     .full_span_content(true)
+                    .scrollable([false, true])
             },
             |ui, open| {
                 let Some(view_id) = &self.view_id else {
@@ -51,21 +51,7 @@ impl ViewEntityPicker {
                     return;
                 };
 
-                // Make the modal size less jumpy and work around https://github.com/emilk/egui/issues/5138
-                // TODO(ab): move that boilerplate to `ModalWrapper` by adding a `scrollable` flag.
-                let max_height = 0.85 * ui.ctx().screen_rect().height();
-                let min_height = 0.3 * ui.ctx().screen_rect().height().at_most(max_height);
-
-                egui::ScrollArea::vertical()
-                    .min_scrolled_height(max_height)
-                    .max_height(max_height)
-                    .show(ui, |ui| {
-                        add_entities_ui(ctx, ui, view);
-
-                        if ui.min_rect().height() < min_height {
-                            ui.add_space(min_height - ui.min_rect().height());
-                        }
-                    });
+                add_entities_ui(ctx, ui, view);
             },
         );
     }

diff --git a/crates/viewer/re_ui/src/modal.rs b/crates/viewer/re_ui/src/modal.rs
@@ -1,3 +1,5 @@
+use eframe::emath::NumExt;
+
 use crate::{DesignTokens, UiExt as _};
 
 /// Helper object to handle a [`ModalWrapper`] window.
@@ -83,6 +85,7 @@ pub struct ModalWrapper {
     min_height: Option<f32>,
     default_height: Option<f32>,
     full_span_content: bool,
+    scrollable: egui::Vec2b,
 }
 
 impl ModalWrapper {
@@ -94,6 +97,7 @@ impl ModalWrapper {
             min_height: None,
             default_height: None,
             full_span_content: false,
+            scrollable: false.into(),
         }
     }
 
@@ -131,6 +135,13 @@ impl ModalWrapper {
         self
     }
 
+    /// Enclose the contents in a scroll area.
+    #[inline]
+    pub fn scrollable(mut self, scrollable: impl Into<egui::Vec2b>) -> Self {
+        self.scrollable = scrollable.into();
+        self
+    }
+
     /// Show the modal window.
     ///
     /// Typically called by [`ModalHandler::ui`].
@@ -148,7 +159,7 @@ impl ModalWrapper {
             area = area.default_height(default_height);
         }
 
-        let modal_response = egui::Modal::new("add_view_or_container_modal".into())
+        let modal_response = egui::Modal::new(id.with("modal"))
             .frame(egui::Frame {
                 fill: ctx.style().visuals.panel_fill,
                 ..Default::default()
@@ -168,38 +179,82 @@ impl ModalWrapper {
                     ui.set_min_height(min_height);
                 }
 
-                egui::Frame {
-                    inner_margin: egui::Margin::symmetric(DesignTokens::view_padding(), 0.0),
-                    ..Default::default()
-                }
+                //
+                // Title bar
+                //
+
+                view_padding_frame(&ViewPaddingFrameParams {
+                    left_and_right: true,
+                    top: true,
+                    bottom: false,
+                })
                 .show(ui, |ui| {
-                    ui.add_space(DesignTokens::view_padding());
                     Self::title_bar(ui, &self.title, &mut open);
                     ui.add_space(DesignTokens::view_padding());
                     ui.full_span_separator();
+                });
 
-                    if self.full_span_content {
-                        // no further spacing for the content UI
-                        content_ui(ui, &mut open)
-                    } else {
-                        // we must restore vertical spacing and add view padding at the bottom
-                        ui.add_space(item_spacing_y);
-
-                        egui::Frame {
-                            inner_margin: egui::Margin {
-                                bottom: DesignTokens::view_padding(),
-                                ..Default::default()
-                            },
-                            ..Default::default()
+                //
+                // Inner content
+                //
+
+                let wrapped_content_ui = |ui: &mut egui::Ui, open: &mut bool| -> R {
+                    // We always have side margin, but these must happen _inside_ the scroll area
+                    // (if any). Otherwise, the scroll bar is not snug with the right border and
+                    // may interfere with the action buttons of `ListItem`s.
+                    view_padding_frame(&ViewPaddingFrameParams {
+                        left_and_right: true,
+                        top: false,
+                        bottom: false,
+                    })
+                    .show(ui, |ui| {
+                        if self.full_span_content {
+                            // no further spacing for the content UI
+                            content_ui(ui, open)
+                        } else {
+                            // we must restore vertical spacing and add view padding at the bottom
+                            ui.add_space(item_spacing_y);
+
+                            view_padding_frame(&ViewPaddingFrameParams {
+                                left_and_right: false,
+                                top: false,
+                                bottom: true,
+                            })
+                            .show(ui, |ui| {
+                                ui.spacing_mut().item_spacing.y = item_spacing_y;
+                                content_ui(ui, open)
+                            })
+                            .inner
                         }
+                    })
+                    .inner
+                };
+
+                //
+                // Optional scroll area
+                //
+
+                if self.scrollable.any() {
+                    // Make the modal size less jumpy and work around https://github.com/emilk/egui/issues/5138
+                    let max_height = 0.85 * ui.ctx().screen_rect().height();
+                    let min_height = 0.3 * ui.ctx().screen_rect().height().at_most(max_height);
+
+                    egui::ScrollArea::new(self.scrollable)
+                        .min_scrolled_height(max_height)
+                        .max_height(max_height)
                         .show(ui, |ui| {
-                            ui.spacing_mut().item_spacing.y = item_spacing_y;
-                            content_ui(ui, &mut open)
+                            let res = wrapped_content_ui(ui, &mut open);
+
+                            if ui.min_rect().height() < min_height {
+                                ui.add_space(min_height - ui.min_rect().height());
+                            }
+
+                            res
                         })
                         .inner
-                    }
-                })
-                .inner
+                } else {
+                    wrapped_content_ui(ui, &mut open)
+                }
             });
 
         if modal_response.should_close() {
@@ -230,3 +285,44 @@ impl ModalWrapper {
         });
     }
 }
+
+struct ViewPaddingFrameParams {
+    left_and_right: bool,
+    top: bool,
+    bottom: bool,
+}
+
+/// Utility to produce a [`egui::Frame`] with padding on some sides.
+#[inline]
+fn view_padding_frame(params: &ViewPaddingFrameParams) -> egui::Frame {
+    let ViewPaddingFrameParams {
+        left_and_right,
+        top,
+        bottom,
+    } = *params;
+    egui::Frame {
+        inner_margin: egui::Margin {
+            left: if left_and_right {
+                DesignTokens::view_padding()
+            } else {
+                0.0
+            },
+            right: if left_and_right {
+                DesignTokens::view_padding()
+            } else {
+                0.0
+            },
+            top: if top {
+                DesignTokens::view_padding()
+            } else {
+                0.0
+            },
+            bottom: if bottom {
+                DesignTokens::view_padding()
+            } else {
+                0.0
+            },
+        },
+        ..Default::default()
+    }
+}
diff --git a/crates/viewer/re_viewport_blueprint/src/ui/add_view_or_container_modal.rs b/crates/viewer/re_viewport_blueprint/src/ui/add_view_or_container_modal.rs
@@ -1,11 +1,12 @@
 //! Modal for adding a new view of container to an existing target container.
 
-use crate::{ViewBlueprint, ViewportBlueprint};
 use re_ui::UiExt as _;
 use re_viewer_context::{
     blueprint_id_to_tile_id, icon_for_container_kind, ContainerId, RecommendedView, ViewerContext,
 };
 
+use crate::{ViewBlueprint, ViewportBlueprint};
+
 #[derive(Default)]
 pub struct AddViewOrContainerModal {
     target_container: Option<ContainerId>,
@@ -30,6 +31,7 @@ impl AddViewOrContainerModal {
                 re_ui::modal::ModalWrapper::new("Add view or container")
                     .min_width(500.0)
                     .full_span_content(true)
+                    .scrollable([false, true])
             },
             |ui, keep_open| modal_ui(ui, ctx, viewport, self.target_container, keep_open),
         );

diff --git a/tests/python/release_checklist/check_modal_scrolling.py b/tests/python/release_checklist/check_modal_scrolling.py
@@ -0,0 +1,46 @@
+from __future__ import annotations
+
+import os
+from argparse import Namespace
+from uuid import uuid4
+
+import rerun as rr
+import rerun.blueprint as rrb
+
+README = """\
+# Modal scrolling
+
+* Select the 2D view
+* Open the Entity Path Filter modal
+* Make sure it behaves properly, including scrolling
+"""
+
+
+def log_readme() -> None:
+    rr.log("readme", rr.TextDocument(README, media_type=rr.MediaType.MARKDOWN), timeless=True)
+
+
+def log_many_entities() -> None:
+    for i in range(0, 1000):
+        rr.log(f"points/{i}", rr.Points2D([(i, i)]))
+
+
+def run(args: Namespace) -> None:
+    rr.script_setup(
+        args,
+        f"{os.path.basename(__file__)}",
+        recording_id=uuid4(),
+        default_blueprint=rrb.Grid(rrb.Spatial2DView(origin="/"), rrb.TextDocumentView(origin="readme")),
+    )
+
+    log_readme()
+    log_many_entities()
+
+
+if __name__ == "__main__":
+    import argparse
+
+    parser = argparse.ArgumentParser(description="Interactive release checklist")
+    rr.script_add_args(parser)
+    args = parser.parse_args()
+    run(args)