From 0f162921f6f8bc7ff54ad2343c60e871aa5bbd9f Mon Sep 17 00:00:00 2001
From: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
Date: Thu, 5 Dec 2024 13:07:15 +0100
Subject: [PATCH] Remove selection history (#8296)

### What
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


![image](https://github.com/user-attachments/assets/afb38bff-7e83-4dac-bb7f-cd27a8acca4d)
---
 Cargo.lock                                    |  14 +-
 crates/viewer/re_selection_panel/src/lib.rs   |   1 -
 .../src/selection_history_ui.rs               | 216 ------------------
 .../re_selection_panel/src/selection_panel.rs |  15 +-
 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 +-
 9 files changed, 10 insertions(+), 396 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/Cargo.lock b/Cargo.lock
index b071cb0aacb1..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.1",
+ "hashbrown 0.15.2",
  "immutable-chunkmap",
 ]
 
@@ -71,7 +71,7 @@ checksum = "7329821f3bd1101e03a7d2e03bd339e3ac0dc64c70b4c9f9ae1949e3ba8dece1"
 dependencies = [
  "accesskit",
  "accesskit_consumer 0.26.0",
- "hashbrown 0.15.1",
+ "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.1",
+ "hashbrown 0.15.2",
  "paste",
  "static_assertions",
  "windows 0.58.0",
@@ -2998,9 +2998,9 @@ dependencies = [
 
 [[package]]
 name = "hashbrown"
-version = "0.15.1"
+version = "0.15.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "3a9bfc1af68b1726ea47d3d5109de126281def866b33970e10fbab11b5dafab3"
+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.1",
+ "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.1",
+ "hashbrown 0.15.2",
 ]
 
 [[package]]
diff --git a/crates/viewer/re_selection_panel/src/lib.rs b/crates/viewer/re_selection_panel/src/lib.rs
index 4517a2783bb3..02b267f6ea28 100644
--- a/crates/viewer/re_selection_panel/src/lib.rs
+++ b/crates/viewer/re_selection_panel/src/lib.rs
@@ -2,7 +2,6 @@
 
 mod defaults_ui;
 mod item_title;
-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<ItemCollection> {
-        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<ItemCollection> {
-        // 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<ItemCollection> {
-        // 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."
-            );
-            "<space context>".to_owned()
-        }
-    } else if let Some(kind) = selection.are_all_items_same_kind() {
-        format!("{}x {}s", selection.len(), kind)
-    } else {
-        "<multiple selections>".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 {
-                "<removed space view>".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 {
-                "<removed space view>".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 {
-                "<removed container>".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 f5997b3db199..0d6145e76564 100644
--- a/crates/viewer/re_selection_panel/src/selection_panel.rs
+++ b/crates/viewer/re_selection_panel/src/selection_panel.rs
@@ -19,12 +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::{
-    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,
 };
-use crate::{space_view_entity_picker::SpaceViewEntityPicker, ItemTitle};
 
 // ---
 fn default_selection_panel_width(screen_width: f32) -> f32 {
@@ -35,8 +34,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,
@@ -70,15 +67,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<ItemCollection>,
-}
-
-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<HistoricalSelection> {
-        self.stack
-            .get(self.current)
-            .cloned()
-            .map(|s| (self.current, s).into())
-    }
-
-    pub fn previous(&self) -> Option<HistoricalSelection> {
-        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<HistoricalSelection> {
-        self.stack
-            .get(self.current + 1)
-            .map(|sel| (self.current + 1, sel.clone()).into())
-    }
-
-    #[must_use]
-    pub fn select_previous(&mut self) -> Option<ItemCollection> {
-        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<ItemCollection> {
-        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<SelectionHistory>,
-
     /// 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());