From 56176dfa4c6d9badc48cd88a79109cc1f03a318f Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Tue, 3 Dec 2024 14:44:03 +0100 Subject: [PATCH 1/3] Remove selection history We're removing it because we're worried users will confuse it with the new Undo feature. After this PR, you can no longer go back in the selection history. But, we're planning on adding hierarchy breadcrumbs to the selections instead, which should hopefully be an even better way of understanding and navigating selections: * https://github.com/rerun-io/rerun/issues/4491 --- crates/viewer/re_selection_panel/src/lib.rs | 1 - .../src/selection_history_ui.rs | 216 ------------------ .../re_selection_panel/src/selection_panel.rs | 13 +- crates/viewer/re_ui/src/command.rs | 7 - crates/viewer/re_viewer/src/app.rs | 6 - crates/viewer/re_viewer_context/src/lib.rs | 2 - .../src/selection_history.rs | 120 ---------- .../re_viewer_context/src/selection_state.rs | 25 +- 8 files changed, 2 insertions(+), 388 deletions(-) delete mode 100644 crates/viewer/re_selection_panel/src/selection_history_ui.rs delete mode 100644 crates/viewer/re_viewer_context/src/selection_history.rs diff --git a/crates/viewer/re_selection_panel/src/lib.rs b/crates/viewer/re_selection_panel/src/lib.rs index a2d534606742..d21d7f202750 100644 --- a/crates/viewer/re_selection_panel/src/lib.rs +++ b/crates/viewer/re_selection_panel/src/lib.rs @@ -1,7 +1,6 @@ //! The UI for the selection panel. mod defaults_ui; -mod selection_history_ui; mod selection_panel; mod space_view_entity_picker; mod space_view_space_origin_ui; diff --git a/crates/viewer/re_selection_panel/src/selection_history_ui.rs b/crates/viewer/re_selection_panel/src/selection_history_ui.rs deleted file mode 100644 index 2d985e5cab73..000000000000 --- a/crates/viewer/re_selection_panel/src/selection_history_ui.rs +++ /dev/null @@ -1,216 +0,0 @@ -use egui::RichText; - -use re_ui::{UICommand, UiExt as _}; -use re_viewer_context::{Item, ItemCollection, SelectionHistory}; -use re_viewport_blueprint::ViewportBlueprint; - -// --- - -#[derive(Default, serde::Deserialize, serde::Serialize)] -#[serde(default)] -pub struct SelectionHistoryUi {} - -impl SelectionHistoryUi { - pub(crate) fn selection_ui( - &self, - ui: &mut egui::Ui, - viewport: &ViewportBlueprint, - history: &mut SelectionHistory, - ) -> Option { - let next = self.next_button_ui(ui, viewport, history); - let prev = self.prev_button_ui(ui, viewport, history); - prev.or(next) - } - - fn prev_button_ui( - &self, - ui: &mut egui::Ui, - viewport: &ViewportBlueprint, - history: &mut SelectionHistory, - ) -> Option { - // undo selection - if let Some(previous) = history.previous() { - let response = ui - .small_icon_button(&re_ui::icons::ARROW_LEFT) - .on_hover_text(format!( - "Go to previous selection{}:\n\ - {}\n\ - \n\ - Right-click for more.", - UICommand::SelectionPrevious.format_shortcut_tooltip_suffix(ui.ctx()), - selection_to_string(viewport, &previous.selection), - )); - - let mut return_current = false; - response.context_menu(|ui| { - // undo: newest on top, oldest on bottom - let cur = history.current; - for i in (0..history.current).rev() { - self.history_item_ui(viewport, ui, i, history); - } - return_current = cur != history.current; - }); - if return_current { - return history.current().map(|sel| sel.selection); - } - - // TODO(cmc): using the keyboard shortcut should highlight the associated - // button or something (but then again it, it'd make more sense to do that - // at the egui level rather than specifically here). - if response.clicked() { - return history.select_previous(); - } - } else { - ui.add_enabled_ui(false, |ui| { - ui.small_icon_button(&re_ui::icons::ARROW_LEFT) - .on_disabled_hover_text("No past selections found"); - }); - } - - None - } - - fn next_button_ui( - &self, - ui: &mut egui::Ui, - viewport: &ViewportBlueprint, - history: &mut SelectionHistory, - ) -> Option { - // redo selection - if let Some(next) = history.next() { - let response = ui - .small_icon_button(&re_ui::icons::ARROW_RIGHT) - .on_hover_text(format!( - "Go to next selection{}:\n\ - {}\n\ - \n\ - Right-click for more.", - UICommand::SelectionNext.format_shortcut_tooltip_suffix(ui.ctx()), - selection_to_string(viewport, &next.selection), - )); - - let mut return_current = false; - response.context_menu(|ui| { - // redo: oldest on top, most recent on bottom - let cur = history.current; - for i in (history.current + 1)..history.stack.len() { - self.history_item_ui(viewport, ui, i, history); - } - return_current = cur != history.current; - }); - if return_current { - return history.current().map(|sel| sel.selection); - } - - // TODO(cmc): using the keyboard shortcut should highlight the associated - // button or something (but then again it, it'd make more sense to do that - // at the egui level rather than specifically here). - if response.clicked() { - return history.select_next(); - } - } else { - ui.add_enabled_ui(false, |ui| { - ui.small_icon_button(&re_ui::icons::ARROW_RIGHT) - .on_disabled_hover_text("No future selections found"); - }); - } - - None - } - - #[allow(clippy::unused_self)] - fn history_item_ui( - &self, - viewport: &ViewportBlueprint, - ui: &mut egui::Ui, - index: usize, - history: &mut SelectionHistory, - ) { - if let Some(sel) = history.stack.get(index) { - ui.horizontal(|ui| { - { - // borrow checker workaround - let sel = selection_to_string(viewport, sel); - if ui - .selectable_value(&mut history.current, index, sel) - .clicked() - { - ui.close_menu(); - } - } - if sel.iter_items().count() == 1 { - if let Some(item) = sel.iter_items().next() { - item_kind_ui(ui, item); - } - } - }); - } - } -} - -// Different kinds of selections can share the same path in practice! We need to -// differentiate those in the UI to avoid confusion. -fn item_kind_ui(ui: &mut egui::Ui, sel: &Item) { - ui.weak(RichText::new(format!("({})", sel.kind()))); -} - -fn selection_to_string(viewport: &ViewportBlueprint, selection: &ItemCollection) -> String { - debug_assert!( - !selection.is_empty(), - "History should never contain empty selections." - ); - if selection.len() == 1 { - if let Some(item) = selection.iter_items().next() { - item_to_string(viewport, item) - } else { - // All items got removed or weren't there to begin with. - debug_assert!( - selection.iter_space_context().next().is_some(), - "History should never keep selections that have both an empty item & context list." - ); - "".to_owned() - } - } else if let Some(kind) = selection.are_all_items_same_kind() { - format!("{}x {}s", selection.len(), kind) - } else { - "".to_owned() - } -} - -fn item_to_string(viewport: &ViewportBlueprint, item: &Item) -> String { - match item { - Item::AppId(app_id) => app_id.to_string(), - Item::DataSource(data_source) => data_source.to_string(), - Item::StoreId(store_id) => store_id.to_string(), - Item::SpaceView(space_view_id) => { - // TODO(#4678): unnamed space views should have their label formatted accordingly (subdued) - if let Some(space_view) = viewport.view(space_view_id) { - space_view.display_name_or_default().as_ref().to_owned() - } else { - "".to_owned() - } - } - Item::InstancePath(instance_path) => instance_path.to_string(), - Item::DataResult(space_view_id, instance_path) => { - // TODO(#4678): unnamed space views should have their label formatted accordingly (subdued) - let space_view_display_name = if let Some(space_view) = viewport.view(space_view_id) { - space_view.display_name_or_default().as_ref().to_owned() - } else { - "".to_owned() - }; - - format!("{instance_path} in {space_view_display_name}") - } - Item::ComponentPath(path) => { - format!("{}:{}", path.entity_path, path.component_name.short_name(),) - } - Item::Container(container_id) => { - // TODO(#4678): unnamed container should have their label formatted accordingly (subdued) - if let Some(container) = viewport.container(container_id) { - container.display_name_or_default().as_ref().to_owned() - } else { - "".to_owned() - } - } - } -} diff --git a/crates/viewer/re_selection_panel/src/selection_panel.rs b/crates/viewer/re_selection_panel/src/selection_panel.rs index 1a40519f2bfa..d4f9589bcf5d 100644 --- a/crates/viewer/re_selection_panel/src/selection_panel.rs +++ b/crates/viewer/re_selection_panel/src/selection_panel.rs @@ -25,7 +25,6 @@ use re_viewport_blueprint::{ui::show_add_space_view_or_container_modal, Viewport use crate::space_view_entity_picker::SpaceViewEntityPicker; use crate::{defaults_ui::view_components_defaults_section_ui, visualizer_ui::visualizer_ui}; use crate::{ - selection_history_ui::SelectionHistoryUi, visible_time_range_ui::visible_time_range_ui_for_data_result, visible_time_range_ui::visible_time_range_ui_for_view, }; @@ -39,8 +38,6 @@ fn default_selection_panel_width(screen_width: f32) -> f32 { #[derive(Default, serde::Deserialize, serde::Serialize)] #[serde(default)] pub struct SelectionPanel { - selection_state_ui: SelectionHistoryUi, - #[serde(skip)] /// State for the "Add entity" modal. space_view_entity_modal: SpaceViewEntityPicker, @@ -74,15 +71,7 @@ impl SelectionPanel { ui.panel_content(|ui| { let hover = "The selection view contains information and options about \ the currently selected object(s)"; - ui.panel_title_bar_with_buttons("Selection", Some(hover), |ui| { - let mut history = ctx.selection_state().history.lock(); - if let Some(selection) = - self.selection_state_ui - .selection_ui(ui, viewport, &mut history) - { - ctx.selection_state().set_selection(selection); - } - }); + ui.panel_title_bar("Selection", Some(hover)); }); // move the vertical spacing between the title and the content to _inside_ the scroll diff --git a/crates/viewer/re_ui/src/command.rs b/crates/viewer/re_ui/src/command.rs index 21109f45cc7f..f42fc755dde1 100644 --- a/crates/viewer/re_ui/src/command.rs +++ b/crates/viewer/re_ui/src/command.rs @@ -59,9 +59,6 @@ pub enum UICommand { #[cfg(not(target_arch = "wasm32"))] ZoomReset, - SelectionPrevious, - SelectionNext, - ToggleCommandPalette, // Playback: @@ -199,8 +196,6 @@ impl UICommand { "Resets the UI zoom level to the operating system's default value", ), - Self::SelectionPrevious => ("Previous selection", "Go to previous selection"), - Self::SelectionNext => ("Next selection", "Go to next selection"), Self::ToggleCommandPalette => ("Command palette…", "Toggle the Command Palette"), Self::PlaybackTogglePlayPause => { @@ -346,8 +341,6 @@ impl UICommand { #[cfg(not(target_arch = "wasm32"))] Self::ZoomReset => Some(egui::gui_zoom::kb_shortcuts::ZOOM_RESET), - Self::SelectionPrevious => Some(ctrl_shift(Key::ArrowLeft)), - Self::SelectionNext => Some(ctrl_shift(Key::ArrowRight)), Self::ToggleCommandPalette => Some(cmd(Key::P)), Self::PlaybackTogglePlayPause => Some(key(Key::Space)), diff --git a/crates/viewer/re_viewer/src/app.rs b/crates/viewer/re_viewer/src/app.rs index 21e939406064..39b3e4c02b2e 100644 --- a/crates/viewer/re_viewer/src/app.rs +++ b/crates/viewer/re_viewer/src/app.rs @@ -825,12 +825,6 @@ impl App { egui_ctx.set_zoom_factor(1.0); } - UICommand::SelectionPrevious => { - self.state.selection_state.select_previous(); - } - UICommand::SelectionNext => { - self.state.selection_state.select_next(); - } UICommand::ToggleCommandPalette => { self.cmd_palette.toggle(); } diff --git a/crates/viewer/re_viewer_context/src/lib.rs b/crates/viewer/re_viewer_context/src/lib.rs index 221a532e4fd4..c0b0359c1d3b 100644 --- a/crates/viewer/re_viewer_context/src/lib.rs +++ b/crates/viewer/re_viewer_context/src/lib.rs @@ -18,7 +18,6 @@ mod item; mod maybe_mut_ref; mod query_context; mod query_range; -mod selection_history; mod selection_state; mod space_view; mod store_context; @@ -59,7 +58,6 @@ pub use self::{ DataQueryResult, DataResultHandle, DataResultNode, DataResultTree, QueryContext, }, query_range::QueryRange, - selection_history::SelectionHistory, selection_state::{ ApplicationSelectionState, HoverHighlight, InteractionHighlight, ItemCollection, ItemSpaceContext, SelectionHighlight, diff --git a/crates/viewer/re_viewer_context/src/selection_history.rs b/crates/viewer/re_viewer_context/src/selection_history.rs deleted file mode 100644 index 99b26391f7fb..000000000000 --- a/crates/viewer/re_viewer_context/src/selection_history.rs +++ /dev/null @@ -1,120 +0,0 @@ -use crate::selection_state::ItemCollection; - -use super::Item; - -/// A `Selection` and its index into the historical stack. -#[derive(Debug, Clone)] -pub struct HistoricalSelection { - pub index: usize, - pub selection: ItemCollection, -} - -impl From<(usize, ItemCollection)> for HistoricalSelection { - fn from((index, selection): (usize, ItemCollection)) -> Self { - Self { index, selection } - } -} - -const MAX_SELECTION_HISTORY_LENGTH: usize = 100; - -// --- - -/// A stack of `Selection`s, used to implement "undo/redo"-like semantics for selections. -#[derive(Clone, Default, Debug)] -pub struct SelectionHistory { - /// Index into [`Self::stack`]. - pub current: usize, - - /// Oldest first. - pub stack: Vec, -} - -impl SelectionHistory { - /// Retains all elements that fulfill a certain condition. - pub fn retain(&mut self, f: &impl Fn(&Item) -> bool) { - re_tracing::profile_function!(); - - let mut i = 0; - self.stack.retain_mut(|selection| { - selection.retain(|item, _| f(item)); - let retain = !selection.is_empty(); - if !retain && i <= self.current { - self.current = self.current.saturating_sub(1); - } - i += 1; - retain - }); - - // In case `self.current` was bad going in to this function: - self.current = self.current.min(self.stack.len().saturating_sub(1)); - } - - pub fn current(&self) -> Option { - self.stack - .get(self.current) - .cloned() - .map(|s| (self.current, s).into()) - } - - pub fn previous(&self) -> Option { - let prev_index = self.current.checked_sub(1)?; - let prev = self.stack.get(prev_index)?; - Some((prev_index, prev.clone()).into()) - } - - pub fn next(&self) -> Option { - self.stack - .get(self.current + 1) - .map(|sel| (self.current + 1, sel.clone()).into()) - } - - #[must_use] - pub fn select_previous(&mut self) -> Option { - if let Some(previous) = self.previous() { - if previous.index != self.current { - self.current = previous.index; - return self.current().map(|s| s.selection); - } - } - None - } - - #[must_use] - pub fn select_next(&mut self) -> Option { - if let Some(next) = self.next() { - if next.index != self.current { - self.current = next.index; - return self.current().map(|s| s.selection); - } - } - None - } - - pub fn update_selection(&mut self, selection: &ItemCollection) { - // Selecting nothing is irrelevant from a history standpoint. - if selection.is_empty() { - return; - } - - // Do not grow the history if the thing being selected is equal to the value that the - // current history cursor points to. - if self.current().as_ref().map(|c| &c.selection) == Some(selection) { - return; - } - - // Make sure to clear the entire redo history past this point: we are engaging in a - // diverging timeline! - self.stack.truncate(self.current + 1); - - self.stack.push(selection.clone()); - - // Keep size under a certain maximum. - if self.stack.len() > MAX_SELECTION_HISTORY_LENGTH { - self.stack - .drain((self.stack.len() - MAX_SELECTION_HISTORY_LENGTH)..self.stack.len()); - } - - // Update current index last so it points to something valid! - self.current = self.stack.len() - 1; - } -} diff --git a/crates/viewer/re_viewer_context/src/selection_state.rs b/crates/viewer/re_viewer_context/src/selection_state.rs index 51ff5d20e027..202427cac510 100644 --- a/crates/viewer/re_viewer_context/src/selection_state.rs +++ b/crates/viewer/re_viewer_context/src/selection_state.rs @@ -6,7 +6,7 @@ use re_entity_db::EntityPath; use crate::{item::resolve_mono_instance_path_item, ViewerContext}; -use super::{Item, SelectionHistory}; +use super::Item; /// Context information that a space view might attach to an item from [`ItemCollection`] and useful /// for how a selection might be displayed and interacted with. @@ -205,10 +205,6 @@ impl ItemCollection { #[derive(Default, serde::Deserialize, serde::Serialize)] #[serde(default)] pub struct ApplicationSelectionState { - /// History of selections (what was selected previously). - #[serde(skip)] - pub history: Mutex, - /// Selection of the previous frame. Read from this. selection_previous_frame: ItemCollection, @@ -235,10 +231,6 @@ impl ApplicationSelectionState { // Use a different name so we don't get a collision in puffin. re_tracing::profile_scope!("SelectionState::on_frame_start"); - // Purge history of invalid items. - let history = self.history.get_mut(); - history.retain(&item_retain_condition); - // Purge selection of invalid items. let selection_this_frame = self.selection_this_frame.get_mut(); selection_this_frame.retain(|item, _| item_retain_condition(item)); @@ -253,25 +245,10 @@ impl ApplicationSelectionState { // Selection in contrast, is sticky! if selection_this_frame != &self.selection_previous_frame { - history.update_selection(selection_this_frame); self.selection_previous_frame = selection_this_frame.clone(); } } - /// Selects the previous element in the history if any. - pub fn select_previous(&self) { - if let Some(selection) = self.history.lock().select_previous() { - *self.selection_this_frame.lock() = selection; - } - } - - /// Selections the next element in the history if any. - pub fn select_next(&self) { - if let Some(selection) = self.history.lock().select_next() { - *self.selection_this_frame.lock() = selection; - } - } - /// Clears the current selection out. pub fn clear_selection(&self) { self.set_selection(ItemCollection::default()); From 15c6e9caa36dcee6b4d00bcfef67d0a34390b194 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 5 Dec 2024 11:14:07 +0100 Subject: [PATCH 2/3] cargo fmt --- crates/viewer/re_selection_panel/src/selection_panel.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/viewer/re_selection_panel/src/selection_panel.rs b/crates/viewer/re_selection_panel/src/selection_panel.rs index d672b9b8b944..0d6145e76564 100644 --- a/crates/viewer/re_selection_panel/src/selection_panel.rs +++ b/crates/viewer/re_selection_panel/src/selection_panel.rs @@ -19,11 +19,11 @@ use re_viewer_context::{ use re_viewport_blueprint::{ui::show_add_space_view_or_container_modal, ViewportBlueprint}; use crate::{defaults_ui::view_components_defaults_section_ui, visualizer_ui::visualizer_ui}; +use crate::{space_view_entity_picker::SpaceViewEntityPicker, ItemTitle}; use crate::{ visible_time_range_ui::visible_time_range_ui_for_data_result, visible_time_range_ui::visible_time_range_ui_for_view, }; -use crate::{space_view_entity_picker::SpaceViewEntityPicker, ItemTitle}; // --- fn default_selection_panel_width(screen_width: f32) -> f32 { From 245dd6fc1e798b550feee73aef38e0f8d020a672 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 5 Dec 2024 11:34:13 +0100 Subject: [PATCH 3/3] Update hashbrown --- Cargo.lock | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 87dec2671a6b..1e04517c3353 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -59,7 +59,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f47983a1084940ba9a39c077a8c63e55c619388be5476ac04c804cfbd1e63459" dependencies = [ "accesskit", - "hashbrown 0.15.0", + "hashbrown 0.15.2", "immutable-chunkmap", ] @@ -71,7 +71,7 @@ checksum = "7329821f3bd1101e03a7d2e03bd339e3ac0dc64c70b4c9f9ae1949e3ba8dece1" dependencies = [ "accesskit", "accesskit_consumer 0.26.0", - "hashbrown 0.15.0", + "hashbrown 0.15.2", "objc2", "objc2-app-kit", "objc2-foundation", @@ -103,7 +103,7 @@ checksum = "24fcd5d23d70670992b823e735e859374d694a3d12bfd8dd32bd3bd8bedb5d81" dependencies = [ "accesskit", "accesskit_consumer 0.26.0", - "hashbrown 0.15.0", + "hashbrown 0.15.2", "paste", "static_assertions", "windows 0.58.0", @@ -2998,9 +2998,9 @@ dependencies = [ [[package]] name = "hashbrown" -version = "0.15.0" +version = "0.15.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1e087f84d4f86bf4b218b927129862374b72199ae7d8657835f1e89000eea4fb" +checksum = "bf151400ff0baff5465007dd2f3e717f3fe502074ca563069ce3a6629d07b289" dependencies = [ "allocator-api2", "equivalent", @@ -3409,7 +3409,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "707907fe3c25f5424cce2cb7e1cbcafee6bdbe735ca90ef77c29e84591e5b9da" dependencies = [ "equivalent", - "hashbrown 0.15.0", + "hashbrown 0.15.2", "serde", ] @@ -3826,7 +3826,7 @@ version = "0.12.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "234cf4f4a04dc1f57e24b96cc0cd600cf2af460d4161ac5ecdd0af8e1f3b2a38" dependencies = [ - "hashbrown 0.15.0", + "hashbrown 0.15.2", ] [[package]]