From 9d0d2a304110b3c9612aac29433810284e625865 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler <49431240+abey79@users.noreply.github.com> Date: Tue, 5 Mar 2024 12:50:30 +0100 Subject: [PATCH] More context menu 1: refactor (#5392) ### What More like a rewrite actually. This PR completely reorganise the way context menu actions are written and executed with two goals: - Keep the complexity in check: the previous `summarize_selection` mechanism was exposed to combinatorial explosion of possible item subset. - Improve locality: every details of if/when/how an action is displayed and run is now local to the action `impl`s, not dispatched in various functions such as `summarize_selection` etc. All in all, this should ensure a O(1) effort to add new actions. @ Reviewer: with the renames and rewrite, the diff a probably useless and this PR is best reviewed by directly looking at `context_menu/*.rs`. ### 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/5392/index.html) * Using examples from latest `main` build: [app.rerun.io](https://app.rerun.io/pr/5392/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/5392/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/5392) - [Docs preview](https://rerun.io/preview/db9a05278dd4936e5224b6331143eea369ff61ad/docs) - [Examples preview](https://rerun.io/preview/db9a05278dd4936e5224b6331143eea369ff61ad/examples) - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html) --- crates/re_viewport/src/container.rs | 20 + .../re_viewport/src/context_menu/actions.rs | 311 +++++++++++++ .../container_and_space_view_actions.rs | 224 ---------- crates/re_viewport/src/context_menu/mod.rs | 417 +++++++++--------- .../re_viewport/src/context_menu/sub_menu.rs | 40 ++ crates/re_viewport/src/context_menu/utils.rs | 61 --- .../re_viewport/src/viewport_blueprint_ui.rs | 3 +- .../release_checklist/check_context_menu.py | 11 +- 8 files changed, 587 insertions(+), 500 deletions(-) create mode 100644 crates/re_viewport/src/context_menu/actions.rs delete mode 100644 crates/re_viewport/src/context_menu/container_and_space_view_actions.rs create mode 100644 crates/re_viewport/src/context_menu/sub_menu.rs delete mode 100644 crates/re_viewport/src/context_menu/utils.rs diff --git a/crates/re_viewport/src/container.rs b/crates/re_viewport/src/container.rs index 068b7195e5d1..212dedeb35c5 100644 --- a/crates/re_viewport/src/container.rs +++ b/crates/re_viewport/src/container.rs @@ -72,6 +72,26 @@ impl Contents { } } +impl TryFrom for Contents { + type Error = (); + + fn try_from(item: Item) -> Result { + (&item).try_into() + } +} + +impl TryFrom<&Item> for Contents { + type Error = (); + + fn try_from(item: &Item) -> Result { + match item { + Item::Container(id) => Ok(Self::Container(*id)), + Item::SpaceView(id) => Ok(Self::SpaceView(*id)), + _ => Err(()), + } + } +} + #[inline] pub fn blueprint_id_to_tile_id(id: &BlueprintId) -> TileId { TileId::from_u64(id.hash()) diff --git a/crates/re_viewport/src/context_menu/actions.rs b/crates/re_viewport/src/context_menu/actions.rs new file mode 100644 index 000000000000..a67819cc68cd --- /dev/null +++ b/crates/re_viewport/src/context_menu/actions.rs @@ -0,0 +1,311 @@ +use re_log_types::{EntityPath, EntityPathFilter}; +use re_space_view::{DataQueryBlueprint, SpaceViewBlueprint}; +use re_viewer_context::{ContainerId, Item, SpaceViewClassIdentifier, SpaceViewId}; + +use super::{ContextMenuAction, ContextMenuContext}; +use crate::Contents; + +pub(super) struct ShowAction; + +impl ContextMenuAction for ShowAction { + fn supports_selection(&self, ctx: &ContextMenuContext<'_>) -> bool { + ctx.selection.iter().any(|(item, _)| match item { + Item::SpaceView(space_view_id) => !ctx + .viewport_blueprint + .is_contents_visible(&Contents::SpaceView(*space_view_id)), + Item::Container(container_id) => { + !ctx.viewport_blueprint + .is_contents_visible(&Contents::Container(*container_id)) + && ctx.viewport_blueprint.root_container != Some(*container_id) + } + _ => false, + }) + } + + fn label(&self, ctx: &ContextMenuContext<'_>) -> String { + if ctx.selection.len() > 1 { + "Show All".to_owned() + } else { + "Show".to_owned() + } + } + + fn process_container(&self, ctx: &ContextMenuContext<'_>, container_id: &ContainerId) { + ctx.viewport_blueprint.set_content_visibility( + ctx.viewer_context, + &Contents::Container(*container_id), + true, + ); + } + + fn process_space_view(&self, ctx: &ContextMenuContext<'_>, space_view_id: &SpaceViewId) { + ctx.viewport_blueprint.set_content_visibility( + ctx.viewer_context, + &Contents::SpaceView(*space_view_id), + true, + ); + } +} + +// --- + +pub(super) struct HideAction; + +impl ContextMenuAction for HideAction { + fn supports_selection(&self, ctx: &ContextMenuContext<'_>) -> bool { + ctx.selection.iter().any(|(item, _)| match item { + Item::SpaceView(space_view_id) => ctx + .viewport_blueprint + .is_contents_visible(&Contents::SpaceView(*space_view_id)), + Item::Container(container_id) => { + ctx.viewport_blueprint + .is_contents_visible(&Contents::Container(*container_id)) + && ctx.viewport_blueprint.root_container != Some(*container_id) + } + _ => false, + }) + } + + fn label(&self, ctx: &ContextMenuContext<'_>) -> String { + if ctx.selection.len() > 1 { + "Hide All".to_owned() + } else { + "Hide".to_owned() + } + } + + fn process_container(&self, ctx: &ContextMenuContext<'_>, container_id: &ContainerId) { + ctx.viewport_blueprint.set_content_visibility( + ctx.viewer_context, + &Contents::Container(*container_id), + false, + ); + } + + fn process_space_view(&self, ctx: &ContextMenuContext<'_>, space_view_id: &SpaceViewId) { + ctx.viewport_blueprint.set_content_visibility( + ctx.viewer_context, + &Contents::SpaceView(*space_view_id), + false, + ); + } +} + +// --- + +/// Remove a container or space view +pub(super) struct RemoveAction; + +impl ContextMenuAction for RemoveAction { + fn supports_multi_selection(&self, _ctx: &ContextMenuContext<'_>) -> bool { + true + } + + fn supports_item(&self, ctx: &ContextMenuContext<'_>, item: &Item) -> bool { + match item { + Item::SpaceView(_) => true, + Item::Container(container_id) => { + ctx.viewport_blueprint.root_container != Some(*container_id) + } + _ => false, + } + } + + fn label(&self, _ctx: &ContextMenuContext<'_>) -> String { + "Remove".to_owned() + } + + fn process_container(&self, ctx: &ContextMenuContext<'_>, container_id: &ContainerId) { + ctx.viewport_blueprint + .mark_user_interaction(ctx.viewer_context); + ctx.viewport_blueprint + .remove_contents(Contents::Container(*container_id)); + } + + fn process_space_view(&self, ctx: &ContextMenuContext<'_>, space_view_id: &SpaceViewId) { + ctx.viewport_blueprint + .mark_user_interaction(ctx.viewer_context); + ctx.viewport_blueprint + .remove_contents(Contents::SpaceView(*space_view_id)); + } +} + +// --- + +/// Clone a single space view +pub(super) struct CloneSpaceViewAction; + +impl ContextMenuAction for CloneSpaceViewAction { + fn supports_item(&self, _ctx: &ContextMenuContext<'_>, item: &Item) -> bool { + matches!(item, Item::SpaceView(_)) + } + + fn label(&self, _ctx: &ContextMenuContext<'_>) -> String { + "Clone".to_owned() + } + + fn process_space_view(&self, ctx: &ContextMenuContext<'_>, space_view_id: &SpaceViewId) { + if let Some(new_space_view_id) = ctx + .viewport_blueprint + .duplicate_space_view(space_view_id, ctx.viewer_context) + { + ctx.viewer_context + .selection_state() + .set_selection(Item::SpaceView(new_space_view_id)); + ctx.viewport_blueprint + .mark_user_interaction(ctx.viewer_context); + } + } +} + +// --- + +/// Add a container of a specific type +pub(super) struct AddContainerAction(pub egui_tiles::ContainerKind); + +impl ContextMenuAction for AddContainerAction { + fn supports_selection(&self, ctx: &ContextMenuContext<'_>) -> bool { + if let Some(Item::Container(container_id)) = ctx.selection.single_item() { + if let Some(container) = ctx.viewport_blueprint.container(container_id) { + // same-kind linear containers cannot be nested + (container.container_kind != egui_tiles::ContainerKind::Vertical + && container.container_kind != egui_tiles::ContainerKind::Horizontal) + || container.container_kind != self.0 + } else { + // unknown container + false + } + } else { + false + } + } + + fn label(&self, _ctx: &ContextMenuContext<'_>) -> String { + format!("{:?}", self.0) + } + + fn process_container(&self, ctx: &ContextMenuContext<'_>, container_id: &ContainerId) { + ctx.viewport_blueprint + .add_container(self.0, Some(*container_id)); + ctx.viewport_blueprint + .mark_user_interaction(ctx.viewer_context); + } +} + +// --- + +/// Add a space view of the specific class +pub(super) struct AddSpaceViewAction(pub SpaceViewClassIdentifier); + +impl ContextMenuAction for AddSpaceViewAction { + fn supports_item(&self, _ctx: &ContextMenuContext<'_>, item: &Item) -> bool { + matches!(item, Item::Container(_)) + } + + fn label(&self, ctx: &ContextMenuContext<'_>) -> String { + ctx.viewer_context + .space_view_class_registry + .get_class_or_log_error(&self.0) + .display_name() + .to_owned() + } + + fn process_container(&self, ctx: &ContextMenuContext<'_>, container_id: &ContainerId) { + let space_view = SpaceViewBlueprint::new( + self.0, + &EntityPath::root(), + DataQueryBlueprint::new(self.0, EntityPathFilter::default()), + ); + + ctx.viewport_blueprint.add_space_views( + std::iter::once(space_view), + ctx.viewer_context, + Some(*container_id), + None, + ); + ctx.viewport_blueprint + .mark_user_interaction(ctx.viewer_context); + } +} + +// --- + +/// Move the selected contents to a newly created container of the given kind +pub(super) struct MoveContentsToNewContainerAction(pub egui_tiles::ContainerKind); + +impl ContextMenuAction for MoveContentsToNewContainerAction { + fn supports_selection(&self, ctx: &ContextMenuContext<'_>) -> bool { + if let Some((parent_container_id, _)) = Self::target_container_id_and_position(ctx) { + if let Some(parent_container) = ctx.viewport_blueprint.container(&parent_container_id) { + // same-kind linear containers cannot be nested + if (parent_container.container_kind == egui_tiles::ContainerKind::Vertical + || parent_container.container_kind == egui_tiles::ContainerKind::Horizontal) + && parent_container.container_kind == self.0 + { + return false; + } + } + } + + ctx.selection.iter().all(|(item, _)| match item { + Item::SpaceView(_) => true, + Item::Container(container_id) => { + ctx.viewport_blueprint.root_container != Some(*container_id) + } + _ => false, + }) + } + + fn supports_multi_selection(&self, _ctx: &ContextMenuContext<'_>) -> bool { + true + } + + fn supports_item(&self, ctx: &ContextMenuContext<'_>, item: &Item) -> bool { + match item { + Item::SpaceView(_) => true, + Item::Container(container_id) => { + ctx.viewport_blueprint.root_container != Some(*container_id) + } + _ => false, + } + } + + fn label(&self, _ctx: &ContextMenuContext<'_>) -> String { + format!("{:?}", self.0) + } + + fn process_selection(&self, ctx: &ContextMenuContext<'_>) { + if let Some(root_container_id) = ctx.viewport_blueprint.root_container { + let (target_container_id, target_position) = + Self::target_container_id_and_position(ctx).unwrap_or((root_container_id, 0)); + + let contents = ctx + .selection + .iter() + .filter_map(|(item, _)| item.try_into().ok()) + .collect(); + + ctx.viewport_blueprint.move_contents_to_new_container( + contents, + self.0, + target_container_id, + target_position, + ); + + ctx.viewport_blueprint + .mark_user_interaction(ctx.viewer_context); + } + } +} + +impl MoveContentsToNewContainerAction { + fn target_container_id_and_position( + ctx: &ContextMenuContext<'_>, + ) -> Option<(ContainerId, usize)> { + ctx.clicked_item + .clone() + .try_into() + .ok() + .and_then(|c| ctx.viewport_blueprint.find_parent_and_position_index(&c)) + } +} diff --git a/crates/re_viewport/src/context_menu/container_and_space_view_actions.rs b/crates/re_viewport/src/context_menu/container_and_space_view_actions.rs deleted file mode 100644 index f7def0dba71f..000000000000 --- a/crates/re_viewport/src/context_menu/container_and_space_view_actions.rs +++ /dev/null @@ -1,224 +0,0 @@ -use std::rc::Rc; - -use re_log_types::{EntityPath, EntityPathFilter}; -use re_space_view::{DataQueryBlueprint, SpaceViewBlueprint}; -use re_viewer_context::{ContainerId, Item, SpaceViewClassIdentifier, SpaceViewId, ViewerContext}; - -use crate::context_menu::ContextMenuItem; -use crate::{Contents, ViewportBlueprint}; - -// ================================================================================================ -// Space View/Container edit items -// ================================================================================================ - -/// Control the visibility of a container or space view -pub(super) struct ContentVisibilityToggle { - contents: Rc>, - set_visible: bool, -} - -impl ContentVisibilityToggle { - pub(super) fn item( - viewport_blueprint: &ViewportBlueprint, - contents: Rc>, - ) -> Box { - Box::new(Self { - set_visible: !contents - .iter() - .all(|item| viewport_blueprint.is_contents_visible(item)), - contents, - }) - } -} - -impl ContextMenuItem for ContentVisibilityToggle { - fn label(&self, _ctx: &ViewerContext<'_>, _viewport_blueprint: &ViewportBlueprint) -> String { - if self.set_visible { - "Show".to_owned() - } else { - "Hide".to_owned() - } - } - - fn run(&self, ctx: &ViewerContext<'_>, viewport_blueprint: &ViewportBlueprint) { - for content in &*self.contents { - viewport_blueprint.set_content_visibility(ctx, content, self.set_visible); - } - } -} - -/// Remove a container or space view -pub(super) struct ContentRemove { - contents: Rc>, -} - -impl ContentRemove { - pub(super) fn item(contents: Rc>) -> Box { - Box::new(Self { contents }) - } -} - -impl ContextMenuItem for ContentRemove { - fn label(&self, _ctx: &ViewerContext<'_>, _viewport_blueprint: &ViewportBlueprint) -> String { - "Remove".to_owned() - } - - fn run(&self, ctx: &ViewerContext<'_>, viewport_blueprint: &ViewportBlueprint) { - for content in &*self.contents { - viewport_blueprint.mark_user_interaction(ctx); - viewport_blueprint.remove_contents(*content); - } - } -} - -// ================================================================================================ -// Space view items -// ================================================================================================ - -/// Clone a space view -pub(super) struct CloneSpaceViewItem { - space_view_id: SpaceViewId, -} - -impl CloneSpaceViewItem { - pub(super) fn item(space_view_id: SpaceViewId) -> Box { - Box::new(Self { space_view_id }) - } -} - -impl ContextMenuItem for CloneSpaceViewItem { - fn label(&self, _ctx: &ViewerContext<'_>, _viewport_blueprint: &ViewportBlueprint) -> String { - "Clone".to_owned() - } - - fn run(&self, ctx: &ViewerContext<'_>, viewport_blueprint: &ViewportBlueprint) { - if let Some(new_space_view_id) = - viewport_blueprint.duplicate_space_view(&self.space_view_id, ctx) - { - ctx.selection_state() - .set_selection(Item::SpaceView(new_space_view_id)); - viewport_blueprint.mark_user_interaction(ctx); - } - } -} - -// ================================================================================================ -// Container items -// ================================================================================================ - -/// Add a container of a specific type -pub(super) struct AddContainer { - target_container: ContainerId, - container_kind: egui_tiles::ContainerKind, -} - -impl AddContainer { - pub(super) fn item( - target_container: ContainerId, - container_kind: egui_tiles::ContainerKind, - ) -> Box { - Box::new(Self { - target_container, - container_kind, - }) - } -} - -impl ContextMenuItem for AddContainer { - fn label(&self, _ctx: &ViewerContext<'_>, _viewport_blueprint: &ViewportBlueprint) -> String { - format!("{:?}", self.container_kind) - } - - fn run(&self, ctx: &ViewerContext<'_>, viewport_blueprint: &ViewportBlueprint) { - viewport_blueprint.add_container(self.container_kind, Some(self.target_container)); - viewport_blueprint.mark_user_interaction(ctx); - } -} - -// --- - -/// Add a space view of the specific class -pub(super) struct AddSpaceView { - target_container: ContainerId, - space_view_class: SpaceViewClassIdentifier, -} - -impl AddSpaceView { - pub(super) fn item( - target_container: ContainerId, - space_view_class: SpaceViewClassIdentifier, - ) -> Box { - Box::new(Self { - target_container, - space_view_class, - }) - } -} - -impl ContextMenuItem for AddSpaceView { - fn label(&self, ctx: &ViewerContext<'_>, _viewport_blueprint: &ViewportBlueprint) -> String { - ctx.space_view_class_registry - .get_class_or_log_error(&self.space_view_class) - .display_name() - .to_owned() - } - - fn run(&self, ctx: &ViewerContext<'_>, viewport_blueprint: &ViewportBlueprint) { - let space_view = SpaceViewBlueprint::new( - self.space_view_class, - &EntityPath::root(), - DataQueryBlueprint::new(self.space_view_class, EntityPathFilter::default()), - ); - - viewport_blueprint.add_space_views( - std::iter::once(space_view), - ctx, - Some(self.target_container), - None, - ); - viewport_blueprint.mark_user_interaction(ctx); - } -} - -// --- - -/// Move the selected contents to a newly created container of the given kind -pub(super) struct MoveContentsToNewContainer { - parent_container: ContainerId, - position_in_parent: usize, - container_kind: egui_tiles::ContainerKind, - contents: Rc>, -} - -impl MoveContentsToNewContainer { - pub(super) fn item( - parent_container: ContainerId, - position_in_parent: usize, - container_kind: egui_tiles::ContainerKind, - contents: Rc>, - ) -> Box { - Box::new(Self { - parent_container, - position_in_parent, - container_kind, - contents, - }) - } -} - -impl ContextMenuItem for MoveContentsToNewContainer { - fn label(&self, _ctx: &ViewerContext<'_>, _viewport_blueprint: &ViewportBlueprint) -> String { - format!("{:?}", self.container_kind) - } - - fn run(&self, ctx: &ViewerContext<'_>, viewport_blueprint: &ViewportBlueprint) { - viewport_blueprint.move_contents_to_new_container( - (*self.contents).clone(), - self.container_kind, - self.parent_container, - self.position_in_parent, - ); - - viewport_blueprint.mark_user_interaction(ctx); - } -} diff --git a/crates/re_viewport/src/context_menu/mod.rs b/crates/re_viewport/src/context_menu/mod.rs index 7a0dcbbd7880..f4eec29844a3 100644 --- a/crates/re_viewport/src/context_menu/mod.rs +++ b/crates/re_viewport/src/context_menu/mod.rs @@ -1,21 +1,19 @@ -use std::rc::Rc; - use itertools::Itertools; +use once_cell::sync::OnceCell; +use re_entity_db::InstancePath; use re_viewer_context::{ContainerId, Item, Selection, SpaceViewId, ViewerContext}; -use crate::{Contents, ViewportBlueprint}; +use crate::ViewportBlueprint; -mod container_and_space_view_actions; -//mod space_view_data; -mod utils; +mod actions; +mod sub_menu; -use container_and_space_view_actions::{ - AddContainer, AddSpaceView, CloneSpaceViewItem, ContentRemove, ContentVisibilityToggle, - MoveContentsToNewContainer, +use actions::{ + AddContainerAction, AddSpaceViewAction, CloneSpaceViewAction, HideAction, + MoveContentsToNewContainerAction, RemoveAction, ShowAction, }; -//use space_view_data::SpaceViewData; -use utils::{Separator, SubMenu}; +use sub_menu::SubMenu; /// Controls how [`context_menu_ui_for_item`] should handle the current selection state. #[derive(Debug, Clone, Copy)] @@ -44,8 +42,18 @@ pub fn context_menu_ui_for_item( return; } + let mut show_context_menu = |selection: &Selection| { + let context_menu_ctx = ContextMenuContext { + viewer_context: ctx, + viewport_blueprint, + selection, + clicked_item: item, + }; + show_context_menu_for_selection(&context_menu_ctx, ui); + }; + // handle selection - let selection_summary = match selection_update_behavior { + 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, @@ -54,12 +62,12 @@ pub fn context_menu_ui_for_item( ctx.selection_state() .set_selection(std::iter::once(item.clone())); - summarize_selection(&Selection::from(item.clone())) + show_context_menu(&Selection::from(item.clone())); } else { - summarize_selection(ctx.selection()) + show_context_menu(ctx.selection()); } } else { - summarize_selection(ctx.selection()) + show_context_menu(ctx.selection()); } } @@ -69,232 +77,219 @@ pub fn context_menu_ui_for_item( .set_selection(std::iter::once(item.clone())); } - summarize_selection(&Selection::from(item.clone())) + show_context_menu(&Selection::from(item.clone())); } - SelectionUpdateBehavior::Ignore => summarize_selection(&Selection::from(item.clone())), + SelectionUpdateBehavior::Ignore => show_context_menu(&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(), - ); - } +/// Returns the (statically-defined) list of action, grouped in sections. +/// +/// Sections are group of actions that should be displayed together, with a separator displayed +/// between sections. +fn action_list( + ctx: &ViewerContext<'_>, +) -> &'static Vec>> { + static CONTEXT_MENU_ACTIONS: OnceCell>>> = + OnceCell::new(); + + CONTEXT_MENU_ACTIONS.get_or_init(|| { + vec![ + vec![ + Box::new(ShowAction), + Box::new(HideAction), + Box::new(RemoveAction), + ], + vec![Box::new(CloneSpaceViewAction)], + vec![ + Box::new(SubMenu { + label: "Add Container".to_owned(), + actions: vec![ + Box::new(AddContainerAction(egui_tiles::ContainerKind::Tabs)), + Box::new(AddContainerAction(egui_tiles::ContainerKind::Horizontal)), + Box::new(AddContainerAction(egui_tiles::ContainerKind::Vertical)), + Box::new(AddContainerAction(egui_tiles::ContainerKind::Grid)), + ], + }), + Box::new(SubMenu { + label: "Add Space View".to_owned(), + actions: ctx + .space_view_class_registry + .iter_registry() + .sorted_by_key(|entry| entry.class.display_name()) + .map(|entry| { + Box::new(AddSpaceViewAction(entry.class.identifier())) + as Box + }) + .collect(), + }), + ], + vec![Box::new(SubMenu { + label: "Move to new container".to_owned(), + actions: vec![ + Box::new(MoveContentsToNewContainerAction( + egui_tiles::ContainerKind::Tabs, + )), + Box::new(MoveContentsToNewContainerAction( + egui_tiles::ContainerKind::Horizontal, + )), + Box::new(MoveContentsToNewContainerAction( + egui_tiles::ContainerKind::Vertical, + )), + Box::new(MoveContentsToNewContainerAction( + egui_tiles::ContainerKind::Grid, + )), + ], + })], + ] + }) +} + +/// Display every action that accepts the provided selection. +fn show_context_menu_for_selection(ctx: &ContextMenuContext<'_>, ui: &mut egui::Ui) { + let mut should_display_separator = false; + for action_section in action_list(ctx.viewer_context) { + let mut any_action_displayed = false; + + for action in action_section { + if !action.supports_selection(ctx) { + continue; + } + + any_action_displayed = true; + + if should_display_separator { + ui.separator(); + should_display_separator = false; + } - for action in actions { - let response = action.ui(ctx, viewport_blueprint, ui); + let response = action.ui(ctx, 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 - // changes to the context menu UI code to support full-span highlighting. - fn label(&self, _ctx: &ViewerContext<'_>, _viewport_blueprint: &ViewportBlueprint) -> String { - String::new() + should_display_separator |= any_action_displayed; } +} - fn run(&self, _ctx: &ViewerContext<'_>, _viewport_blueprint: &ViewportBlueprint) {} +/// Context information provided to context menu actions +struct ContextMenuContext<'a> { + viewer_context: &'a ViewerContext<'a>, + viewport_blueprint: &'a ViewportBlueprint, + selection: &'a Selection, + clicked_item: &'a Item, +} - /// run from inside of [`egui::Response.context_menu()`] - fn ui( - &self, - ctx: &ViewerContext<'_>, - viewport_blueprint: &ViewportBlueprint, - ui: &mut egui::Ui, - ) -> egui::Response { - let label = self.label(ctx, viewport_blueprint); - let response = ui.button(label); - if response.clicked() { - self.run(ctx, viewport_blueprint); +/// Context menu actions must implement this trait. +/// +/// Actions must do three things, corresponding to three core methods: +/// 1. Decide if it can operate a given [`Selection`] ([`Self::supports_selection`]). +/// 2. If so, draw some UI in the context menu ([`Self::ui`]). +/// 3. If clicked, actually process the [`Selection`] ([`Self::process_selection`]). +/// +/// For convenience, these core methods have default implementations which delegates to simpler +/// methods (see their respective docstrings). Implementor may either implement the core method for +/// complex cases, or one or more of the helper methods. +trait ContextMenuAction { + /// Check if the action is able to operate on the provided selection. + /// + /// The default implementation delegates to [`Self::supports_multi_selection`] and + /// [`Self::supports_item`]. + fn supports_selection(&self, ctx: &ContextMenuContext<'_>) -> bool { + if ctx.selection.len() > 1 && !self.supports_multi_selection(ctx) { + return false; } - response + + ctx.selection + .iter() + .all(|(item, _)| self.supports_item(ctx, item)) } -} -fn context_menu_items_for_selection_summary( - ctx: &ViewerContext<'_>, - viewport_blueprint: &ViewportBlueprint, - item: &Item, - selection_summary: SelectionSummary, -) -> Vec> { - match selection_summary { - SelectionSummary::SingleContainerItem(container_id) => { - // We want all the actions available for collections of contents… - let mut items = context_menu_items_for_selection_summary( - ctx, - viewport_blueprint, - item, - SelectionSummary::ContentsItems(vec![Contents::Container(container_id)]), - ); + /// Returns whether this action supports multi-selections. + fn supports_multi_selection(&self, _ctx: &ContextMenuContext<'_>) -> bool { + false + } - if !items.is_empty() { - items.push(Separator::item()); - } + /// Returns whether this action supports operation on a selection containing this [`Item`]. + fn supports_item(&self, _ctx: &ContextMenuContext<'_>, _item: &Item) -> bool { + false + } - // …plus some more that apply to single container only. - items.extend([ - SubMenu::item( - "Add Container", - possible_child_container_kind(viewport_blueprint, container_id) - .map(|kind| AddContainer::item(container_id, kind)), - ), - SubMenu::item( - "Add Space View", - ctx.space_view_class_registry - .iter_registry() - .sorted_by_key(|entry| entry.class.display_name()) - .map(|entry| AddSpaceView::item(container_id, entry.class.identifier())), - ), - ]); + // --- - items + /// Draw the context menu UI for this action. + /// + /// The default implementation delegates to [`Self::label`]. + /// + /// Note: this is run from inside a [`egui::Response.context_menu()`] closure and must call + /// [`Self::process_selection`] when triggered by the user. + fn ui(&self, ctx: &ContextMenuContext<'_>, ui: &mut egui::Ui) -> egui::Response { + let label = self.label(ctx); + let response = ui.button(label); + if response.clicked() { + self.process_selection(ctx); } - SelectionSummary::SingleSpaceView(space_view_id) => { - // We want all the actions available for collections of contents… - let mut items = context_menu_items_for_selection_summary( - ctx, - viewport_blueprint, - item, - SelectionSummary::ContentsItems(vec![Contents::SpaceView(space_view_id)]), - ); + response + } - items.push(CloneSpaceViewItem::item(space_view_id)); + // TODO(ab): return a `ListItem` to make those context menu nice to look at. This requires + // changes to the context menu UI code to support full-span highlighting. + /// Returns the label displayed by [`Self::ui`]'s default implementation. + fn label(&self, _ctx: &ContextMenuContext<'_>) -> String { + String::new() + } - items - } - SelectionSummary::ContentsItems(contents) => { - // exclude the root container from the list of contents, as it cannot be shown/hidden - // nor removed - let contents: Rc> = Rc::new( - contents - .into_iter() - .filter(|c| { - Some(*c) != viewport_blueprint.root_container.map(Contents::Container) - }) - .collect(), - ); - - if contents.is_empty() { - vec![] - } else if let Some(root_container_id) = viewport_blueprint.root_container { - // The new container should be created in place of the right-clicked content, so we - // look for its parent and position, and fall back to the root container. - let clicked_content = match item { - Item::Container(container_id) => Some(Contents::Container(*container_id)), - Item::SpaceView(space_view_id) => Some(Contents::SpaceView(*space_view_id)), - _ => None, - }; - let (target_container_id, target_position) = clicked_content - .and_then(|c| viewport_blueprint.find_parent_and_position_index(&c)) - .unwrap_or((root_container_id, 0)); - - vec![ - ContentVisibilityToggle::item(viewport_blueprint, contents.clone()), - ContentRemove::item(contents.clone()), - Separator::item(), - SubMenu::item( - "Move to new container", - possible_child_container_kind(viewport_blueprint, target_container_id).map( - |kind| { - MoveContentsToNewContainer::item( - target_container_id, - target_position, - kind, - contents.clone(), - ) - }, - ), - ), - ] - } else { - vec![] + // --- + + /// Process the provided [`Selection`]. + /// + /// The default implementation dispatches to [`Self::process_store_id`] and friends. + fn process_selection(&self, ctx: &ContextMenuContext<'_>) { + for (item, _) in ctx.selection.iter() { + match item { + Item::StoreId(store_id) => self.process_store_id(ctx, store_id), + Item::ComponentPath(component_path) => { + self.process_component_path(ctx, component_path); + } + Item::SpaceView(space_view_id) => self.process_space_view(ctx, space_view_id), + Item::InstancePath(instance_path) => self.process_instance_path(ctx, instance_path), + Item::DataResult(space_view_id, instance_path) => { + self.process_data_result(ctx, space_view_id, instance_path); + } + Item::Container(container_id) => self.process_container(ctx, container_id), } } - SelectionSummary::Heterogeneous | SelectionSummary::Empty => vec![], } -} -/// Helper that returns the allowable containers -fn possible_child_container_kind( - viewport_blueprint: &ViewportBlueprint, - container_id: ContainerId, -) -> impl Iterator + 'static { - let container_kind = viewport_blueprint - .container(&container_id) - .map(|c| c.container_kind); - - static ALL_CONTAINERS: &[egui_tiles::ContainerKind] = &[ - egui_tiles::ContainerKind::Tabs, - egui_tiles::ContainerKind::Horizontal, - egui_tiles::ContainerKind::Vertical, - egui_tiles::ContainerKind::Grid, - ]; - - ALL_CONTAINERS - .iter() - .copied() - .filter(move |kind| match kind { - egui_tiles::ContainerKind::Horizontal | egui_tiles::ContainerKind::Vertical => { - container_kind != Some(*kind) - } - _ => true, - }) -} + /// Process a single recording. + fn process_store_id(&self, _ctx: &ContextMenuContext<'_>, _store_id: &re_log_types::StoreId) {} -// ================================================================================================ -// Selection summary -// ================================================================================================ - -// TODO(ab): this summary is somewhat ad hoc to the context menu needs. Could it be generalised and -// moved to the Selection itself? -#[derive(Debug, Clone)] -enum SelectionSummary { - SingleContainerItem(ContainerId), - SingleSpaceView(SpaceViewId), - ContentsItems(Vec), - Heterogeneous, - Empty, -} + /// Process a single container. + fn process_container(&self, _ctx: &ContextMenuContext<'_>, _container_id: &ContainerId) {} -fn summarize_selection(selection: &Selection) -> SelectionSummary { - if selection.is_empty() { - return SelectionSummary::Empty; - } + /// Process a single space view. + fn process_space_view(&self, _ctx: &ContextMenuContext<'_>, _space_view_id: &SpaceViewId) {} - if selection.len() == 1 { - if let Some(Item::Container(container_id)) = selection.first_item() { - return SelectionSummary::SingleContainerItem(*container_id); - } else if let Some(Item::SpaceView(space_view_id)) = selection.first_item() { - return SelectionSummary::SingleSpaceView(*space_view_id); - } + /// Process a single data result. + fn process_data_result( + &self, + _ctx: &ContextMenuContext<'_>, + _space_view_id: &SpaceViewId, + _instance_path: &InstancePath, + ) { } - // check if we have only space views or containers - let only_space_view_or_container: Option> = selection - .iter() - .map(|(item, _)| match item { - Item::Container(container_id) => Some(Contents::Container(*container_id)), - Item::SpaceView(space_view_id) => Some(Contents::SpaceView(*space_view_id)), - _ => None, - }) - .collect(); - if let Some(contents) = only_space_view_or_container { - return SelectionSummary::ContentsItems(contents); - } + /// Process a single instance. + fn process_instance_path(&self, _ctx: &ContextMenuContext<'_>, _instance_path: &InstancePath) {} - SelectionSummary::Heterogeneous + /// Process a single component. + fn process_component_path( + &self, + _ctx: &ContextMenuContext<'_>, + _component_path: &re_log_types::ComponentPath, + ) { + } } diff --git a/crates/re_viewport/src/context_menu/sub_menu.rs b/crates/re_viewport/src/context_menu/sub_menu.rs new file mode 100644 index 000000000000..37d3b5c57c3b --- /dev/null +++ b/crates/re_viewport/src/context_menu/sub_menu.rs @@ -0,0 +1,40 @@ +use egui::{Response, Ui}; + +use crate::context_menu::{ContextMenuAction, ContextMenuContext}; + +/// Group items into a sub-menu +pub(super) struct SubMenu { + pub label: String, + pub actions: Vec>, +} + +impl ContextMenuAction for SubMenu { + fn supports_selection(&self, ctx: &ContextMenuContext<'_>) -> bool { + // We need at least one sub-action to support the selection to go ahead and show the sub-menu + self.actions + .iter() + .any(|action| action.supports_selection(ctx)) + } + + fn process_selection(&self, ctx: &ContextMenuContext<'_>) { + self.actions + .iter() + .for_each(|action| action.process_selection(ctx)); + } + + fn ui(&self, ctx: &ContextMenuContext<'_>, ui: &mut Ui) -> Response { + ui.menu_button(&self.label, |ui| { + for action in &self.actions { + if !action.supports_selection(ctx) { + continue; + } + + let response = action.ui(ctx, ui); + if response.clicked() { + ui.close_menu(); + } + } + }) + .response + } +} diff --git a/crates/re_viewport/src/context_menu/utils.rs b/crates/re_viewport/src/context_menu/utils.rs deleted file mode 100644 index 3e28bb67e26c..000000000000 --- a/crates/re_viewport/src/context_menu/utils.rs +++ /dev/null @@ -1,61 +0,0 @@ -use crate::context_menu::ContextMenuItem; -use crate::ViewportBlueprint; -use re_viewer_context::ViewerContext; - -/// Group items into a sub-menu -pub(super) struct SubMenu { - label: String, - actions: Vec>, -} - -impl SubMenu { - pub(super) fn item( - label: &str, - actions: impl IntoIterator>, - ) -> Box { - let actions = actions.into_iter().collect(); - Box::new(Self { - label: label.to_owned(), - actions, - }) - } -} - -impl ContextMenuItem for SubMenu { - fn ui( - &self, - ctx: &ViewerContext<'_>, - viewport_blueprint: &ViewportBlueprint, - ui: &mut egui::Ui, - ) -> egui::Response { - ui.menu_button(&self.label, |ui| { - for action in &self.actions { - let response = action.ui(ctx, viewport_blueprint, ui); - if response.clicked() { - ui.close_menu(); - } - } - }) - .response - } -} - -/// Add a separator to the context menu -pub(super) struct Separator; - -impl Separator { - pub(super) fn item() -> Box { - Box::new(Self) - } -} - -impl ContextMenuItem for Separator { - fn ui( - &self, - _ctx: &ViewerContext<'_>, - _viewport_blueprint: &ViewportBlueprint, - ui: &mut egui::Ui, - ) -> egui::Response { - ui.separator() - } -} diff --git a/crates/re_viewport/src/viewport_blueprint_ui.rs b/crates/re_viewport/src/viewport_blueprint_ui.rs index d5edf7e07d88..7066d0485759 100644 --- a/crates/re_viewport/src/viewport_blueprint_ui.rs +++ b/crates/re_viewport/src/viewport_blueprint_ui.rs @@ -12,7 +12,8 @@ use re_viewer_context::{ ContainerId, DataQueryResult, DataResultNode, HoverHighlight, Item, SpaceViewId, ViewerContext, }; -use crate::{container::Contents, context_menu_ui_for_item, SelectionUpdateBehavior, Viewport}; +use crate::context_menu::context_menu_ui_for_item; +use crate::{container::Contents, 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 { diff --git a/tests/python/release_checklist/check_context_menu.py b/tests/python/release_checklist/check_context_menu.py index de9c9cba7b17..3de0237aa3a6 100644 --- a/tests/python/release_checklist/check_context_menu.py +++ b/tests/python/release_checklist/check_context_menu.py @@ -15,8 +15,8 @@ * Right-click on any space view and check for context menu content: - Hide - Remove - - Move to new container - Clone + - Move to new container * 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. @@ -28,13 +28,18 @@ * Right-click on the container and check for context menu content: - Hide - Remove - - Move to new container - Add Container - Add Space View + - Move to new container * 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. +### Show/Hide + +* Hide a space view and check that its context menu shows "Show" instead of "Hide". +* Select multiple space views with homogeneous or heterogeneous visibility states. Check that either or both of "Show All" and "Hide All" are showed as appropriate. + ### Selection behavior * Select a space view. @@ -56,7 +61,7 @@ ### Invalid sub-container kind -* Single-select a horizontal container, check that it disallow adding an horizontal container inside it. +* Single-select a horizontal container, check that it disallow adding a horizontal container inside it. * Same for a vertical container. * Single select a space view inside a horizontal container, check that it disallow moving to a new horizontal container. * Same for a space view inside a vertical container.