diff --git a/crates/viewer/re_selection_panel/src/view_entity_picker.rs b/crates/viewer/re_selection_panel/src/view_entity_picker.rs index 513111b3bb9c..d1a6b7ecb4fc 100644 --- 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 index e6f7b9113c18..1f5963ee7d65 100644 --- 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, default_height: Option, 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) -> 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 index 4a38ee15a48f..63f2fc83e02f 100644 --- 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, @@ -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 new file mode 100644 index 000000000000..8859f69acc17 --- /dev/null +++ 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)