Be consistent in how items are removed from selection

crates/re_viewer/src/app_state.rs

@@ -161,7 +161,10 @@ impl AppState {
 
         recording_config_entry(recording_configs, recording.store_id().clone(), recording)
             .selection_state
-            .on_frame_start(|item| viewport.is_item_valid(item));
+            .on_frame_start(
+                |item| viewport.is_item_valid(item),
+                re_viewer_context::Item::StoreId(store_context.recording.store_id().clone()),
+            );
 
         let rec_cfg =
             recording_config_entry(recording_configs, recording.store_id().clone(), recording);
@@ -297,19 +300,12 @@ impl AppState {
             app_blueprint.time_panel_expanded,
         );
 
-        {
-            if ctx.selection().is_empty() {
-                // Make sure something is selected before showing the selection panel.
-                ctx.selection_state()
-                    .set_selection(re_viewer_context::Item::StoreId(ctx.recording_id().clone()));
-            }
-            selection_panel.show_panel(
-                &ctx,
-                ui,
-                &mut viewport,
-                app_blueprint.selection_panel_expanded,
-            );
-        }
+        selection_panel.show_panel(
+            &ctx,
+            ui,
+            &mut viewport,
+            app_blueprint.selection_panel_expanded,
+        );
 
         let central_panel_frame = egui::Frame {
             fill: ui.style().visuals.panel_fill,

crates/re_viewer_context/src/selection_state.rs

@@ -227,18 +227,29 @@ pub struct ApplicationSelectionState {
 
 impl ApplicationSelectionState {
     /// Called at the start of each frame
-    pub fn on_frame_start(&mut self, item_retain_condition: impl Fn(&Item) -> bool) {
+    pub fn on_frame_start(
+        &mut self,
+        item_retain_condition: impl Fn(&Item) -> bool,
+        fallback_selection: Item,
+    ) {
         // 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));
+        if selection_this_frame.is_empty() {
+            *selection_this_frame = ItemCollection::from(fallback_selection);
+        }
+
         // Hovering needs to be refreshed every frame: If it wasn't hovered last frame, it's no longer hovered!
         self.hovered_previous_frame = std::mem::take(self.hovered_this_frame.get_mut());
 
         // Selection in contrast, is sticky!
-        let selection_this_frame = self.selection_this_frame.get_mut();
         if selection_this_frame != &self.selection_previous_frame {
             history.update_selection(selection_this_frame);
             self.selection_previous_frame = selection_this_frame.clone();
@@ -286,17 +297,6 @@ impl ApplicationSelectionState {
         *self.hovered_this_frame.lock() = hovered.into();
     }
 
-    /// Remove given items from the selection.
-    ///
-    /// Has no effect on items that were not selected in the first place.
-    /// Ignores `ItemSpaceContext`s in the passed collection if any.
-    pub fn remove_from_selection(&self, items: impl Into<ItemCollection>) {
-        let removed_items = items.into();
-        self.selection_this_frame
-            .lock()
-            .retain(|item, _| !removed_items.contains_item(item));
-    }
-
     /// Select passed objects unless already selected in which case they get unselected.
     /// If however an object is already selected but now gets passed a *different* selected space context, it stays selected after all
     /// but with an updated selected space context!

crates/re_viewport/src/context_menu/actions/remove.rs

@@ -32,19 +32,13 @@ impl ContextMenuAction for RemoveAction {
             .mark_user_interaction(ctx.viewer_context);
         ctx.viewport_blueprint
             .remove_contents(Contents::Container(*container_id));
-        ctx.viewer_context
-            .selection_state()
-            .remove_from_selection(Item::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));
-        ctx.viewer_context
-            .selection_state()
-            .remove_from_selection(Item::SpaceView(*space_view_id));
     }
 
     fn process_data_result(
@@ -58,10 +52,6 @@ impl ContextMenuAction for RemoveAction {
                 ctx.viewer_context,
                 EntityPathRule::including_subtree(instance_path.entity_path.clone()),
             );
-
-            ctx.viewer_context
-                .selection_state()
-                .remove_from_selection(Item::DataResult(*space_view_id, instance_path.clone()));
         }
     }
 }

crates/re_viewport/src/viewport_blueprint.rs

@@ -257,9 +257,18 @@ impl ViewportBlueprint {
     pub fn is_item_valid(&self, item: &Item) -> bool {
         match item {
             Item::StoreId(_) | Item::ComponentPath(_) | Item::InstancePath(_) => true,
-            Item::SpaceView(space_view_id) | Item::DataResult(space_view_id, _) => {
-                self.space_view(space_view_id).is_some()
+
+            Item::SpaceView(space_view_id) => self.space_view(space_view_id).is_some(),
+
+            Item::DataResult(space_view_id, instance_path) => {
+                self.space_view(space_view_id).map_or(false, |space_view| {
+                    space_view
+                        .contents
+                        .entity_path_filter
+                        .is_included(&instance_path.entity_path)
+                })
             }
+
             Item::Container(container_id) => self.container(container_id).is_some(),
         }
     }