diff --git a/crates/re_viewport/src/context_menu.rs b/crates/re_viewport/src/context_menu.rs index 689ba76569b6..908f107c39bf 100644 --- a/crates/re_viewport/src/context_menu.rs +++ b/crates/re_viewport/src/context_menu.rs @@ -1,14 +1,17 @@ -//TODO(ab): use list items to make those context menu nice to look at +use std::rc::Rc; -use crate::{Contents, ViewportBlueprint}; use itertools::Itertools; + use re_log_types::{EntityPath, EntityPathFilter}; use re_space_view::{DataQueryBlueprint, SpaceViewBlueprint}; use re_viewer_context::{ContainerId, Item, Selection, SpaceViewClassIdentifier, ViewerContext}; +use crate::{Contents, ViewportBlueprint}; + /// Trait for things that can populate a context menu trait ContextMenuItem { - //TODO(ab): should probably return `egui::WidgetText` instead + // 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() } @@ -34,31 +37,29 @@ trait ContextMenuItem { 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) => { - let mut items = vec![]; + // 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)]), + ); - // only show/hide and remove if it's not the root container - if Some(container_id) != viewport_blueprint.root_container { - let contents = vec![Contents::Container(container_id)]; - items.extend([ - ContentVisibilityToggle::item(viewport_blueprint, contents.clone()), - ContentRemove::item(contents), - Separator::item(), - ]); + if !items.is_empty() { + items.push(Separator::item()); } + // …plus some more that apply to single container only. items.extend([ SubMenu::item( "Add Container", - [ - AddContainer::item(container_id, egui_tiles::ContainerKind::Tabs), - AddContainer::item(container_id, egui_tiles::ContainerKind::Horizontal), - AddContainer::item(container_id, egui_tiles::ContainerKind::Vertical), - AddContainer::item(container_id, egui_tiles::ContainerKind::Grid), - ], + possible_child_container_kind(viewport_blueprint, container_id) + .map(|kind| AddContainer::item(container_id, kind)), ), SubMenu::item( "Add Space View", @@ -74,18 +75,49 @@ fn context_menu_items_for_selection_summary( SelectionSummary::ContentsItems(contents) => { // exclude the root container from the list of contents, as it cannot be shown/hidden // nor removed - let contents: Vec<_> = contents - .into_iter() - .filter(|c| Some(*c) != viewport_blueprint.root_container.map(Contents::Container)) - .collect(); + 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 { + } 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), + 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![] } } SelectionSummary::Heterogeneous | SelectionSummary::Empty => vec![], @@ -121,8 +153,12 @@ pub fn context_menu_ui_for_item( summarize_selection(ctx.selection()) }; - let actions = - context_menu_items_for_selection_summary(ctx, viewport_blueprint, selection_summary); + let actions = context_menu_items_for_selection_summary( + ctx, + viewport_blueprint, + item, + selection_summary, + ); if actions.is_empty() { ui.label( @@ -139,6 +175,33 @@ pub fn context_menu_ui_for_item( }); } +/// 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, + }) +} + // ================================================================================================ // Selection summary // ================================================================================================ @@ -252,14 +315,14 @@ impl ContextMenuItem for Separator { /// Control the visibility of a container or space view struct ContentVisibilityToggle { - contents: Vec, + contents: Rc>, set_visible: bool, } impl ContentVisibilityToggle { fn item( viewport_blueprint: &ViewportBlueprint, - contents: Vec, + contents: Rc>, ) -> Box { Box::new(Self { set_visible: !contents @@ -280,7 +343,7 @@ impl ContextMenuItem for ContentVisibilityToggle { } fn run(&self, ctx: &ViewerContext<'_>, viewport_blueprint: &ViewportBlueprint) { - for content in &self.contents { + for content in &*self.contents { viewport_blueprint.set_content_visibility(ctx, content, self.set_visible); } } @@ -288,11 +351,11 @@ impl ContextMenuItem for ContentVisibilityToggle { /// Remove a container or space view struct ContentRemove { - contents: Vec, + contents: Rc>, } impl ContentRemove { - fn item(contents: Vec) -> Box { + fn item(contents: Rc>) -> Box { Box::new(Self { contents }) } } @@ -303,7 +366,7 @@ impl ContextMenuItem for ContentRemove { } fn run(&self, ctx: &ViewerContext<'_>, viewport_blueprint: &ViewportBlueprint) { - for content in &self.contents { + for content in &*self.contents { viewport_blueprint.mark_user_interaction(ctx); viewport_blueprint.remove_contents(*content); } @@ -386,3 +449,46 @@ impl ContextMenuItem for AddSpaceView { viewport_blueprint.mark_user_interaction(ctx); } } + +// --- + +/// Move the selected contents to a newly created container of the given kind +struct MoveContentsToNewContainer { + parent_container: ContainerId, + position_in_parent: usize, + container_kind: egui_tiles::ContainerKind, + contents: Rc>, +} + +impl MoveContentsToNewContainer { + 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/viewport.rs b/crates/re_viewport/src/viewport.rs index 1b7d7ae34360..16028e7d0afc 100644 --- a/crates/re_viewport/src/viewport.rs +++ b/crates/re_viewport/src/viewport.rs @@ -115,6 +115,14 @@ pub enum TreeAction { target_position_in_container: usize, }, + /// Move one or more [`Contents`] to a newly created container + MoveContentsToNewContainer { + contents_to_move: Vec, + new_container_kind: egui_tiles::ContainerKind, + target_container: ContainerId, + target_position_in_container: usize, + }, + /// Set the container that is currently identified as the drop target of an ongoing drag. /// /// This is used for highlighting the drop target in the UI. Note that the drop target container is reset at every @@ -499,6 +507,36 @@ impl<'a, 'b> Viewport<'a, 'b> { ); self.tree_edited = true; } + TreeAction::MoveContentsToNewContainer { + contents_to_move, + new_container_kind, + target_container, + target_position_in_container, + } => { + let new_container_tile_id = self + .tree + .tiles + .insert_container(egui_tiles::Container::new(new_container_kind, vec![])); + + let target_container_tile_id = blueprint_id_to_tile_id(&target_container); + self.tree.move_tile_to_container( + new_container_tile_id, + target_container_tile_id, + target_position_in_container, + true, // reflow grid if needed + ); + + for (pos, content) in contents_to_move.into_iter().enumerate() { + self.tree.move_tile_to_container( + content.as_tile_id(), + new_container_tile_id, + pos, + true, // reflow grid if needed + ); + } + + self.tree_edited = true; + } TreeAction::SetDropTarget(container_id) => { self.state.candidate_drop_parent_container_id = Some(container_id); } diff --git a/crates/re_viewport/src/viewport_blueprint.rs b/crates/re_viewport/src/viewport_blueprint.rs index b46a9e8efd08..307a55f0e1ec 100644 --- a/crates/re_viewport/src/viewport_blueprint.rs +++ b/crates/re_viewport/src/viewport_blueprint.rs @@ -460,6 +460,22 @@ impl ViewportBlueprint { }); } + /// Move some [`Contents`] to a newly created container of the given kind. + pub fn move_contents_to_new_container( + &self, + contents: Vec, + new_container_kind: egui_tiles::ContainerKind, + target_container: ContainerId, + target_position_in_container: usize, + ) { + self.send_tree_action(TreeAction::MoveContentsToNewContainer { + contents_to_move: contents, + new_container_kind, + target_container, + target_position_in_container, + }); + } + /// Make sure the tab corresponding to this space view is focused. pub fn focus_tab(&self, space_view_id: SpaceViewId) { self.send_tree_action(TreeAction::FocusTab(space_view_id)); diff --git a/tests/python/release_checklist/check_context_menu.py b/tests/python/release_checklist/check_context_menu.py index cb3af5eaa695..2a300f03e806 100644 --- a/tests/python/release_checklist/check_context_menu.py +++ b/tests/python/release_checklist/check_context_menu.py @@ -15,6 +15,7 @@ * Right-click on any space view and check for context menu content: - Hide - Remove + - Move to new container * Check both work as expected. * Right-click on the viewport and check for context menu content: - Add Container @@ -23,6 +24,7 @@ * Right-click on the container and check for context menu content: - Hide - Remove + - Move to new container - Add Container - Add Space View @@ -41,9 +43,17 @@ * Select multiple space views, right-click, and check for context menu content: - Hide - Remove + - Move to new container * Same as above, but with only containers selected. * Same as above, but with both space views and containers selected. * Same as above, but with the viewport selected as well. The context menu should be identical, but none of its actions should apply to the viewport. + +### Invalid sub-container kind + +* Single-select a horizontal container, check that it disallow adding an 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. """