Add support for context menu for viewport tab title and selected cont…

…ainer's children list (#5321) ### What As the title says ☝🏻 Fixes #5300 To meaningfully accomodate with these additional contexts, the `context_menu_ui_for_item` function now support various modes of interaction with the selection: - blueprint tree: consider the multi-selection if the clicked item is part of it, otherwise set selection to clicked item - tab title: always set selection to clicked item - container selection panel children list: ignore the selection <img width="286" alt="image" src="https://github.com/rerun-io/rerun/assets/49431240/bb711e87-cec2-4d01-b06c-9b35f8e621aa"> <img width="343" alt="image" src="https://github.com/rerun-io/rerun/assets/49431240/517de1b9-952c-4e04-97f1-890b4c02076f"> ### Checklist * [x] I have read and agree to [Contributor Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and the [Code of Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md) * [x] I've included a screenshot or gif (if applicable) * [x] I have tested the web demo (if applicable): * Using newly built examples: [app.rerun.io](https://app.rerun.io/pr/5321/index.html) * Using examples from latest `main` build: [app.rerun.io](https://app.rerun.io/pr/5321/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json) * Using full set of examples from `nightly` build: [app.rerun.io](https://app.rerun.io/pr/5321/index.html?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json) * [x] The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG * [x] If applicable, add a new check to the [release checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)! - [PR Build Summary](https://build.rerun.io/pr/5321) - [Docs preview](https://rerun.io/preview/64784da9acec90763d8727d3463ad90980d550db/docs)  - [Examples preview](https://rerun.io/preview/64784da9acec90763d8727d3463ad90980d550db/examples)  - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
rerun-io · Feb 28, 2024 · 38ca677 · 38ca677
1 parent 155a996
commit 38ca677
Show file tree

Hide file tree

Showing 6 changed files with 155 additions and 81 deletions.
diff --git a/crates/re_viewer/src/ui/selection_panel.rs b/crates/re_viewer/src/ui/selection_panel.rs
@@ -19,8 +19,9 @@ use re_viewer_context::{
     SystemCommand, SystemCommandSender as _, UiVerbosity, ViewerContext,
 };
 use re_viewport::{
-    external::re_space_view::blueprint::components::QueryExpressions, icon_for_container_kind,
-    space_view_name_style, Contents, Viewport, ViewportBlueprint,
+    context_menu_ui_for_item, external::re_space_view::blueprint::components::QueryExpressions,
+    icon_for_container_kind, space_view_name_style, Contents, SelectionUpdateBehavior, Viewport,
+    ViewportBlueprint,
 };
 
 use crate::ui::override_ui::override_ui;
@@ -787,6 +788,13 @@ fn show_list_item_for_container_child(
 
     let response = list_item.show(ui);
 
+    context_menu_ui_for_item(
+        ctx,
+        viewport.blueprint,
+        &item,
+        &response,
+        SelectionUpdateBehavior::Ignore,
+    );
     ctx.select_hovered_on_click(&response, std::iter::once(item));
 
     if remove_contents {

diff --git a/crates/re_viewport/src/context_menu/mod.rs b/crates/re_viewport/src/context_menu/mod.rs
@@ -17,6 +17,88 @@ use container_and_space_view_actions::{
 //use space_view_data::SpaceViewData;
 use utils::{Separator, SubMenu};
 
+/// Controls how [`context_menu_ui_for_item`] should handle the current selection state.
+#[derive(Debug, Clone, Copy)]
+pub enum SelectionUpdateBehavior {
+    /// If part of the current selection, use it. Otherwise, set selection to clicked item.
+    UseSelection,
+
+    /// Discard the current selection state and set the selection to the clicked item.
+    OverrideSelection,
+
+    /// Ignore the current selection and consider only the clicked item.
+    Ignore,
+}
+
+/// Display a context menu for the provided [`Item`]
+pub fn context_menu_ui_for_item(
+    ctx: &ViewerContext<'_>,
+    viewport_blueprint: &ViewportBlueprint,
+    item: &Item,
+    item_response: &egui::Response,
+    selection_update_behavior: SelectionUpdateBehavior,
+) {
+    item_response.context_menu(|ui| {
+        if ui.input(|i| i.key_pressed(egui::Key::Escape)) {
+            ui.close_menu();
+            return;
+        }
+
+        // handle selection
+        let selection_summary = match selection_update_behavior {
+            SelectionUpdateBehavior::UseSelection => {
+                if !ctx.selection().contains_item(item) {
+                    // When the context menu is triggered open, we check if we're part of the selection,
+                    // and, if not, we update the selection to include only the item that was clicked.
+                    if item_response.hovered() && item_response.secondary_clicked() {
+                        ctx.selection_state()
+                            .set_selection(std::iter::once(item.clone()));
+
+                        summarize_selection(&Selection::from(item.clone()))
+                    } else {
+                        summarize_selection(ctx.selection())
+                    }
+                } else {
+                    summarize_selection(ctx.selection())
+                }
+            }
+
+            SelectionUpdateBehavior::OverrideSelection => {
+                if item_response.secondary_clicked() {
+                    ctx.selection_state()
+                        .set_selection(std::iter::once(item.clone()));
+                }
+
+                summarize_selection(&Selection::from(item.clone()))
+            }
+
+            SelectionUpdateBehavior::Ignore => summarize_selection(&Selection::from(item.clone())),
+        };
+
+        let actions = context_menu_items_for_selection_summary(
+            ctx,
+            viewport_blueprint,
+            item,
+            selection_summary,
+        );
+
+        if actions.is_empty() {
+            ui.label(
+                egui::RichText::from("No action available for the current selection").italics(),
+            );
+        }
+
+        for action in actions {
+            let response = action.ui(ctx, viewport_blueprint, ui);
+            if response.clicked() {
+                ui.close_menu();
+            }
+        }
+    });
+}
+
+// ---
+
 /// Trait for things that can populate a context menu
 trait ContextMenuItem {
     // TODO(ab): return a `ListItem` to make those context menu nice to look at. This requires
@@ -146,57 +228,6 @@ fn context_menu_items_for_selection_summary(
     }
 }
 
-/// Display a context menu for the provided [`Item`]
-pub fn context_menu_ui_for_item(
-    ctx: &ViewerContext<'_>,
-    viewport_blueprint: &ViewportBlueprint,
-    item: &Item,
-    item_response: &egui::Response,
-) {
-    item_response.context_menu(|ui| {
-        if ui.input(|i| i.key_pressed(egui::Key::Escape)) {
-            ui.close_menu();
-            return;
-        }
-
-        // handle selection
-        let selection_summary = if !ctx.selection().contains_item(item) {
-            // When the context menu is triggered open, we check if we're part of the selection,
-            // and, if not, we update the selection to include only the item that was clicked.
-            if item_response.hovered() && item_response.secondary_clicked() {
-                ctx.selection_state()
-                    .set_selection(std::iter::once(item.clone()));
-
-                summarize_selection(&Selection::from(item.clone()))
-            } else {
-                summarize_selection(ctx.selection())
-            }
-        } else {
-            summarize_selection(ctx.selection())
-        };
-
-        let actions = context_menu_items_for_selection_summary(
-            ctx,
-            viewport_blueprint,
-            item,
-            selection_summary,
-        );
-
-        if actions.is_empty() {
-            ui.label(
-                egui::RichText::from("No action available for the current selection").italics(),
-            );
-        }
-
-        for action in actions {
-            let response = action.ui(ctx, viewport_blueprint, ui);
-            if response.clicked() {
-                ui.close_menu();
-            }
-        }
-    });
-}
-
 /// Helper that returns the allowable containers
 fn possible_child_container_kind(
     viewport_blueprint: &ViewportBlueprint,

diff --git a/crates/re_viewport/src/lib.rs b/crates/re_viewport/src/lib.rs
@@ -25,7 +25,7 @@ mod viewport_blueprint_ui;
 pub mod blueprint;
 
 pub use container::{ContainerBlueprint, Contents};
-pub use context_menu::context_menu_ui_for_item;
+pub use context_menu::{context_menu_ui_for_item, SelectionUpdateBehavior};
 pub use viewport::{Viewport, ViewportState};
 pub use viewport_blueprint::ViewportBlueprint;
 pub use viewport_blueprint_ui::space_view_name_style;

diff --git a/crates/re_viewport/src/viewport.rs b/crates/re_viewport/src/viewport.rs
@@ -2,8 +2,6 @@
 //!
 //! Contains all space views.
 
-use std::collections::BTreeMap;
-
 use ahash::HashMap;
 use egui_tiles::{Behavior as _, EditAction};
 use once_cell::sync::Lazy;
@@ -21,8 +19,10 @@ use crate::container::blueprint_id_to_tile_id;
 use crate::screenshot::handle_pending_space_view_screenshots;
 use crate::{
     add_space_view_or_container_modal::AddSpaceViewOrContainerModal, container::Contents,
-    icon_for_container_kind, space_view_entity_picker::SpaceViewEntityPicker,
-    system_execution::execute_systems_for_all_space_views, ViewportBlueprint,
+    context_menu_ui_for_item, icon_for_container_kind,
+    space_view_entity_picker::SpaceViewEntityPicker,
+    system_execution::execute_systems_for_all_space_views, SelectionUpdateBehavior,
+    ViewportBlueprint,
 };
 
 // State for each `SpaceView` including both the auto properties and
@@ -273,7 +273,7 @@ impl<'a, 'b> Viewport<'a, 'b> {
             let mut tab_viewer = TabViewer {
                 viewport_state: state,
                 ctx,
-                space_views: &blueprint.space_views,
+                viewport_blueprint: blueprint,
                 maximized: &mut maximized,
                 edited: false,
                 executed_systems_per_space_view,
@@ -614,7 +614,7 @@ impl<'a, 'b> Viewport<'a, 'b> {
 struct TabViewer<'a, 'b> {
     viewport_state: &'a mut ViewportState,
     ctx: &'a ViewerContext<'b>,
-    space_views: &'a BTreeMap<SpaceViewId, SpaceViewBlueprint>,
+    viewport_blueprint: &'a ViewportBlueprint,
     maximized: &'a mut Option<SpaceViewId>,
     root_container_id: Option<ContainerId>,
     tree_action_sender: std::sync::mpsc::Sender<TreeAction>,
@@ -638,7 +638,8 @@ impl<'a, 'b> egui_tiles::Behavior<SpaceViewId> for TabViewer<'a, 'b> {
     ) -> egui_tiles::UiResponse {
         re_tracing::profile_function!();
 
-        let Some(space_view_blueprint) = self.space_views.get(space_view_id) else {
+        let Some(space_view_blueprint) = self.viewport_blueprint.space_views.get(space_view_id)
+        else {
             return Default::default();
         };
 
@@ -700,7 +701,7 @@ impl<'a, 'b> egui_tiles::Behavior<SpaceViewId> for TabViewer<'a, 'b> {
     }
 
     fn tab_title_for_pane(&mut self, space_view_id: &SpaceViewId) -> egui::WidgetText {
-        if let Some(space_view) = self.space_views.get(space_view_id) {
+        if let Some(space_view) = self.viewport_blueprint.space_views.get(space_view_id) {
             // Note: the formatting for unnamed space views is handled by `TabWidget::new()`
             space_view.display_name_or_default().as_ref().into()
         } else {
@@ -729,22 +730,29 @@ impl<'a, 'b> egui_tiles::Behavior<SpaceViewId> for TabViewer<'a, 'b> {
             tab_widget.paint(ui);
         }
 
-        match tiles.get(tile_id) {
-            Some(egui_tiles::Tile::Pane(space_view_id)) => {
-                self.ctx
-                    .select_hovered_on_click(&response, Item::SpaceView(*space_view_id));
-            }
+        let item = tiles.get(tile_id).and_then(|tile| match tile {
+            egui_tiles::Tile::Pane(space_view_id) => Some(Item::SpaceView(*space_view_id)),
 
-            Some(egui_tiles::Tile::Container(_)) => {
+            egui_tiles::Tile::Container(_) => {
                 if let Some(Contents::Container(container_id)) =
                     self.contents_per_tile_id.get(&tile_id)
                 {
-                    self.ctx
-                        .select_hovered_on_click(&response, Item::Container(*container_id));
+                    Some(Item::Container(*container_id))
+                } else {
+                    None
                 }
             }
+        });
 
-            None => {}
+        if let Some(item) = item {
+            context_menu_ui_for_item(
+                self.ctx,
+                self.viewport_blueprint,
+                &item,
+                &response,
+                SelectionUpdateBehavior::OverrideSelection,
+            );
+            self.ctx.select_hovered_on_click(&response, item);
         }
 
         response
@@ -773,7 +781,9 @@ impl<'a, 'b> egui_tiles::Behavior<SpaceViewId> for TabViewer<'a, 'b> {
     }
 
     fn retain_pane(&mut self, space_view_id: &SpaceViewId) -> bool {
-        self.space_views.contains_key(space_view_id)
+        self.viewport_blueprint
+            .space_views
+            .contains_key(space_view_id)
     }
 
     fn top_bar_right_ui(
@@ -792,7 +802,7 @@ impl<'a, 'b> egui_tiles::Behavior<SpaceViewId> for TabViewer<'a, 'b> {
         };
         let space_view_id = *space_view_id;
 
-        let Some(space_view) = self.space_views.get(&space_view_id) else {
+        let Some(space_view) = self.viewport_blueprint.space_views.get(&space_view_id) else {
             return;
         };
         let num_space_views = tiles.tiles().filter(|tile| tile.is_pane()).count();
@@ -945,7 +955,9 @@ impl TabWidget {
 
         let tab_desc = match tiles.get(tile_id) {
             Some(egui_tiles::Tile::Pane(space_view_id)) => {
-                if let Some(space_view) = tab_viewer.space_views.get(space_view_id) {
+                if let Some(space_view) =
+                    tab_viewer.viewport_blueprint.space_views.get(space_view_id)
+                {
                     TabDesc {
                         label: tab_viewer.tab_title_for_pane(space_view_id),
                         user_named: space_view.display_name.is_some(),

diff --git a/crates/re_viewport/src/viewport_blueprint_ui.rs b/crates/re_viewport/src/viewport_blueprint_ui.rs
@@ -9,7 +9,7 @@ use re_viewer_context::{
     ContainerId, DataQueryResult, DataResultNode, HoverHighlight, Item, SpaceViewId, ViewerContext,
 };
 
-use crate::{container::Contents, context_menu_ui_for_item, Viewport};
+use crate::{container::Contents, context_menu_ui_for_item, SelectionUpdateBehavior, Viewport};
 
 /// The style to use for displaying this space view name in the UI.
 pub fn space_view_name_style(name: &SpaceViewName) -> re_ui::LabelStyle {
@@ -105,7 +105,13 @@ impl Viewport<'_, '_> {
             self.contents_ui(ctx, ui, child, true);
         }
 
-        context_menu_ui_for_item(ctx, self.blueprint, &item, &item_response);
+        context_menu_ui_for_item(
+            ctx,
+            self.blueprint,
+            &item,
+            &item_response,
+            SelectionUpdateBehavior::UseSelection,
+        );
         ctx.select_hovered_on_click(&item_response, item);
 
         self.handle_root_container_drag_and_drop_interaction(
@@ -167,7 +173,13 @@ impl Viewport<'_, '_> {
             }
         });
 
-        context_menu_ui_for_item(ctx, self.blueprint, &item, &response);
+        context_menu_ui_for_item(
+            ctx,
+            self.blueprint,
+            &item,
+            &response,
+            SelectionUpdateBehavior::UseSelection,
+        );
         ctx.select_hovered_on_click(&response, item);
 
         self.blueprint
@@ -262,7 +274,13 @@ impl Viewport<'_, '_> {
             self.blueprint.focus_tab(space_view.id);
         }
 
-        context_menu_ui_for_item(ctx, self.blueprint, &item, &response);
+        context_menu_ui_for_item(
+            ctx,
+            self.blueprint,
+            &item,
+            &response,
+            SelectionUpdateBehavior::UseSelection,
+        );
         ctx.select_hovered_on_click(&response, item);
 
         let content = Contents::SpaceView(*space_view_id);

diff --git a/tests/python/release_checklist/check_context_menu.py b/tests/python/release_checklist/check_context_menu.py
@@ -17,8 +17,11 @@
     - Remove
     - Move to new container
     - Clone
-* Check both work as expected.
-* Right-click on the viewport and check for context menu content:
+* Check all work as expected.
+* Right-click on a space view's tab title widget and check:
+    - It should show the same actions are above.
+    - It should set the selection to that space view, regardless of the previous selection state.
+* Right-click on the top-level container ("Viewport") and check for context menu content:
     - Add Container
     - Add Space View
 * Add a container via the context menu, check the container appears at then end of the list.
@@ -28,7 +31,9 @@
     - Move to new container
     - Add Container
     - Add Space View
-
+* Select a container and, in the Selection Panel, right click on either space view or container children:
+    - The context menu action should be the same as before.
+    - The selection state is not affected by the right-click.
 
 ### Selection behavior