refactor: always open project panel on pane's reveal in project panel

dino created

Update the way `pane::RevealInProjectPanel` is handled to ensure that,
regardless of whether the file belongs to any open project, the project
panel is always activated and focused.

This refactor is a result of some internal feedback after changing its
handling so as to show a notification stating that the item that the
user was trying to reveal didn't belong to an open project.

We feel users are probably already used to relying on `cmd-shift-e` (on
macOS), in pretty much every context, in order to open the project
panel, and so having situations where it doesn't actually happen seems
like a bad user experience.

Change summary

crates/project_panel/src/project_panel_tests.rs | 128 ++++++++++--------
crates/workspace/src/pane.rs                    |  90 ++++---------
2 files changed, 98 insertions(+), 120 deletions(-)

Detailed changes

crates/project_panel/src/project_panel_tests.rs 🔗

@@ -6016,7 +6016,7 @@ async fn test_explicit_reveal(cx: &mut gpui::TestAppContext) {
 }
 
 #[gpui::test]
-async fn test_reveal_in_project_panel_notifications(cx: &mut gpui::TestAppContext) {
+async fn test_reveal_in_project_panel_fallback(cx: &mut gpui::TestAppContext) {
     init_test_with_editor(cx);
     let fs = FakeFs::new(cx.background_executor.clone());
     fs.insert_tree(
@@ -6033,37 +6033,38 @@ async fn test_reveal_in_project_panel_notifications(cx: &mut gpui::TestAppContex
         .read_with(cx, |mw, _| mw.workspace().clone())
         .unwrap();
     let cx = &mut VisualTestContext::from_window(window.into(), cx);
-    let panel = workspace.update_in(cx, ProjectPanel::new);
+    let panel = workspace.update_in(cx, |workspace, window, cx| {
+        let panel = ProjectPanel::new(workspace, window, cx);
+        workspace.add_panel(panel.clone(), window, cx);
+        panel
+    });
     cx.run_until_parked();
 
-    // Ensure that, attempting to run `pane: reveal in project panel` without
-    // any active item does nothing, i.e., does not focus the project panel but
-    // it also does not show a notification.
+    // Project panel should still be activated and focused, when using `pane:
+    // reveal in project panel` without an active item.
     cx.dispatch_action(workspace::RevealInProjectPanel::default());
     cx.run_until_parked();
 
     panel.update_in(cx, |panel, window, cx| {
+        panel
+            .workspace
+            .update(cx, |workspace, cx| {
+                assert!(
+                    workspace.active_item(cx).is_none(),
+                    "Workspace should not have an active item."
+                );
+            })
+            .unwrap();
+
         assert!(
-            !panel.focus_handle(cx).is_focused(window),
-            "Project panel should not be focused after attempting to reveal an invisible worktree entry"
+            panel.focus_handle(cx).is_focused(window),
+            "Project panel should be focused, even when there's no active item."
         );
-
-        panel.workspace.update(cx, |workspace, cx| {
-            assert!(
-                workspace.active_item(cx).is_none(),
-                "Workspace should not have an active item"
-            );
-            assert_eq!(
-                workspace.notification_ids(),
-                vec![],
-                "No notification should be shown when there's no active item"
-            );
-        }).unwrap();
     });
 
-    // Create a file in a different folder than the one in the project so we can
-    // later open it and ensure that, attempting to reveal it in the project
-    // panel shows a notification and does not focus the project panel.
+    // When working with a file that doesn't belong to an open project, we
+    // should still activate the project panel on `pane: reveal in project
+    // panel`.
     fs.insert_tree(
         "/external",
         json!({
@@ -6091,40 +6092,58 @@ async fn test_reveal_in_project_panel_notifications(cx: &mut gpui::TestAppContex
         .unwrap();
     cx.run_until_parked();
 
+    panel.update_in(cx, |panel, window, cx| {
+        assert!(
+            !panel.focus_handle(cx).is_focused(window),
+            "Project panel should not be focused after opening an external file."
+        );
+    });
+
     cx.dispatch_action(workspace::RevealInProjectPanel::default());
     cx.run_until_parked();
 
     panel.update_in(cx, |panel, window, cx| {
+        panel
+            .workspace
+            .update(cx, |workspace, cx| {
+                assert!(
+                    workspace.active_item(cx).is_some(),
+                    "Workspace should have an active item."
+                );
+            })
+            .unwrap();
+
         assert!(
-            !panel.focus_handle(cx).is_focused(window),
-            "Project panel should not be focused after attempting to reveal an invisible worktree entry"
+            panel.focus_handle(cx).is_focused(window),
+            "Project panel should be focused even for invisible worktree entry."
         );
+    });
 
-        panel.workspace.update(cx, |workspace, cx| {
-            assert!(
-                workspace.active_item(cx).is_some(),
-                "Workspace should have an active item"
-            );
+    // Focus again on the center pane so we're sure that the focus doesn't
+    // remain on the project panel, otherwise later assertions wouldn't matter.
+    panel.update_in(cx, |panel, window, cx| {
+        panel
+            .workspace
+            .update(cx, |workspace, cx| {
+                workspace.focus_center_pane(window, cx);
+            })
+            .log_err();
 
-            let notification_ids = workspace.notification_ids();
-            assert_eq!(
-                notification_ids.len(),
-                1,
-                "A notification should be shown when trying to reveal an invisible worktree entry"
-            );
+        assert!(
+            !panel.focus_handle(cx).is_focused(window),
+            "Project panel should not be focused after focusing on center pane."
+        );
+    });
 
-            workspace.dismiss_notification(&notification_ids[0], cx);
-            assert_eq!(
-                workspace.notification_ids().len(),
-                0,
-                "No notifications should be left after dismissing"
-            );
-        }).unwrap();
+    panel.update_in(cx, |panel, window, cx| {
+        assert!(
+            !panel.focus_handle(cx).is_focused(window),
+            "Project panel should not be focused after focusing the center pane."
+        );
     });
 
-    // Create an empty buffer so we can ensure that, attempting to reveal it in
-    // the project panel shows a notification and does not focus the project
-    // panel.
+    // Create an unsaved buffer and verify that pane: reveal in project panel`
+    // still activates and focuses the panel.
     let pane = workspace.update(cx, |workspace, _| workspace.active_pane().clone());
     pane.update_in(cx, |pane, window, cx| {
         let item = cx.new(|cx| TestItem::new(cx).with_label("Unsaved buffer"));
@@ -6135,27 +6154,20 @@ async fn test_reveal_in_project_panel_notifications(cx: &mut gpui::TestAppContex
     cx.run_until_parked();
 
     panel.update_in(cx, |panel, window, cx| {
-        assert!(
-            !panel.focus_handle(cx).is_focused(window),
-            "Project panel should not be focused after attempting to reveal an unsaved buffer"
-        );
-
         panel
             .workspace
             .update(cx, |workspace, cx| {
                 assert!(
                     workspace.active_item(cx).is_some(),
-                    "Workspace should have an active item"
-                );
-
-                let notification_ids = workspace.notification_ids();
-                assert_eq!(
-                    notification_ids.len(),
-                    1,
-                    "A notification should be shown when trying to reveal an unsaved buffer"
+                    "Workspace should have an active item."
                 );
             })
             .unwrap();
+
+        assert!(
+            panel.focus_handle(cx).is_focused(window),
+            "Project panel should be focused even for an unsaved buffer."
+        );
     });
 }
 

crates/workspace/src/pane.rs 🔗

@@ -10,10 +10,7 @@ use crate::{
         TabContentParams, TabTooltipContent, WeakItemHandle,
     },
     move_item,
-    notifications::{
-        NotificationId, NotifyResultExt, show_app_notification,
-        simple_message_notification::MessageNotification,
-    },
+    notifications::NotifyResultExt,
     toolbar::Toolbar,
     workspace_settings::{AutosaveSetting, FocusFollowsMouse, TabBarSettings, WorkspaceSettings},
 };
@@ -4393,68 +4390,37 @@ impl Render for Pane {
                         .detach_and_log_err(cx)
                 },
             ))
-            .on_action(
-                cx.listener(|pane: &mut Self, action: &RevealInProjectPanel, _, cx| {
-                    let Some(active_item) = pane.active_item() else {
-                        return;
-                    };
-
-                    let entry_id = action
-                        .entry_id
-                        .map(ProjectEntryId::from_proto)
-                        .or_else(|| active_item.project_entry_ids(cx).first().copied());
-
-                    let show_reveal_error_toast = |display_name: &str, cx: &mut App| {
-                        let notification_id = NotificationId::unique::<RevealInProjectPanel>();
-                        let message = SharedString::from(format!(
-                            "\"{display_name}\" is not part of any open projects."
-                        ));
-
-                        show_app_notification(notification_id, cx, move |cx| {
-                            let message = message.clone();
-                            cx.new(|cx| MessageNotification::new(message, cx))
-                        });
-                    };
-
-                    let Some(entry_id) = entry_id else {
-                        // When working with an unsaved buffer, display a toast
-                        // informing the user that the buffer is not present in
-                        // any of the open projects and stop execution, as we
-                        // don't want to open the project panel.
-                        let display_name = active_item
-                            .tab_tooltip_text(cx)
-                            .unwrap_or_else(|| active_item.tab_content_text(0, cx));
-
-                        return show_reveal_error_toast(&display_name, cx);
-                    };
-
-                    // We'll now check whether the entry belongs to a visible
-                    // worktree and, if that's not the case, it means the user
-                    // is interacting with a file that does not belong to any of
-                    // the open projects, so we'll show a toast informing them
-                    // of this and stop execution.
-                    let display_name = pane
-                        .project
-                        .read_with(cx, |project, cx| {
-                            project
-                                .worktree_for_entry(entry_id, cx)
-                                .filter(|worktree| !worktree.read(cx).is_visible())
-                                .map(|worktree| worktree.read(cx).root_name_str().to_string())
-                        })
-                        .ok()
-                        .flatten();
-
-                    if let Some(display_name) = display_name {
-                        return show_reveal_error_toast(&display_name, cx);
-                    }
+            .on_action(cx.listener(
+                |pane: &mut Self, action: &RevealInProjectPanel, _window, cx| {
+                    let active_item = pane.active_item();
+                    let entry_id = active_item.as_ref().and_then(|item| {
+                        action
+                            .entry_id
+                            .map(ProjectEntryId::from_proto)
+                            .or_else(|| item.project_entry_ids(cx).first().copied())
+                    });
 
                     pane.project
-                        .update(cx, |_, cx| {
-                            cx.emit(project::Event::RevealInProjectPanel(entry_id))
+                        .update(cx, |project, cx| {
+                            if let Some(entry_id) = entry_id
+                                && project
+                                    .worktree_for_entry(entry_id, cx)
+                                    .is_some_and(|worktree| worktree.read(cx).is_visible())
+                            {
+                                return cx.emit(project::Event::RevealInProjectPanel(entry_id));
+                            }
+
+                            // When no entry is found, which is the case when
+                            // working with an unsaved buffer, or the worktree
+                            // is not visible, for example, a file that doesn't
+                            // belong to an open project, we can't reveal the
+                            // entry but we still want to activate the project
+                            // panel.
+                            cx.emit(project::Event::ActivateProjectPanel);
                         })
                         .log_err();
-                }),
-            )
+                },
+            ))
             .on_action(cx.listener(|_, _: &menu::Cancel, window, cx| {
                 if cx.stop_active_drag(window) {
                 } else {