Polish panel experience (#2523)

Antonio Scandurra created

In this pull request we improved key bindings (as described below) and
added tooltips.

Add these release notes to the panels release notes:

- The left, right and bottom dock can be toggled and focused at the same
time respectively via `cmd-b`, `cmd-r` and `cmd-j`. Holding `shift` will
toggle them without changing the focus.

Change summary

assets/keymaps/default.json                |  25 ++
crates/project_panel/src/project_panel.rs  |   8 
crates/terminal_view/src/terminal_panel.rs |   9 
crates/welcome/src/welcome.rs              |   2 
crates/workspace/src/dock.rs               | 200 +++++++++++------------
crates/workspace/src/workspace.rs          |  73 ++++++--
crates/zed/src/menus.rs                    |  15 +
crates/zed/src/zed.rs                      |   2 
8 files changed, 194 insertions(+), 140 deletions(-)

Detailed changes

assets/keymaps/default.json 🔗

@@ -367,7 +367,30 @@
         "workspace::ActivatePane",
         8
       ],
-      "cmd-b": "workspace::ToggleLeftDock",
+      "cmd-b": [
+        "workspace::ToggleLeftDock",
+        { "focus": true }
+      ],
+      "cmd-shift-b": [
+        "workspace::ToggleLeftDock",
+        { "focus": false }
+      ],
+      "cmd-r": [
+        "workspace::ToggleRightDock",
+        { "focus": true }
+      ],
+      "cmd-shift-r": [
+        "workspace::ToggleRightDock",
+        { "focus": false }
+      ],
+      "cmd-j": [
+        "workspace::ToggleBottomDock",
+        { "focus": true }
+      ],
+      "cmd-shift-j": [
+        "workspace::ToggleBottomDock",
+        { "focus": false }
+      ],
       "cmd-shift-f": "workspace::NewSearch",
       "cmd-k cmd-t": "theme_selector::Toggle",
       "cmd-k cmd-s": "zed::OpenKeymap",

crates/project_panel/src/project_panel.rs 🔗

@@ -15,8 +15,8 @@ use gpui::{
     geometry::vector::Vector2F,
     keymap_matcher::KeymapContext,
     platform::{CursorStyle, MouseButton, PromptLevel},
-    AnyElement, AppContext, AsyncAppContext, ClipboardItem, Element, Entity, ModelHandle, Task,
-    View, ViewContext, ViewHandle, WeakViewHandle, WindowContext,
+    Action, AnyElement, AppContext, AsyncAppContext, ClipboardItem, Element, Entity, ModelHandle,
+    Task, View, ViewContext, ViewHandle, WeakViewHandle, WindowContext,
 };
 use menu::{Confirm, SelectNext, SelectPrev};
 use project::{
@@ -1507,8 +1507,8 @@ impl workspace::dock::Panel for ProjectPanel {
         "icons/folder_tree_16.svg"
     }
 
-    fn icon_tooltip(&self) -> String {
-        "Project Panel".into()
+    fn icon_tooltip(&self) -> (String, Option<Box<dyn Action>>) {
+        ("Project Panel".into(), Some(Box::new(ToggleFocus)))
     }
 
     fn should_change_position_on_event(event: &Self::Event) -> bool {

crates/terminal_view/src/terminal_panel.rs 🔗

@@ -3,7 +3,7 @@ use std::sync::Arc;
 use crate::TerminalView;
 use db::kvp::KEY_VALUE_STORE;
 use gpui::{
-    actions, anyhow::Result, elements::*, serde_json, AppContext, AsyncAppContext, Entity,
+    actions, anyhow::Result, elements::*, serde_json, Action, AppContext, AsyncAppContext, Entity,
     Subscription, Task, View, ViewContext, ViewHandle, WeakViewHandle, WindowContext,
 };
 use project::Fs;
@@ -236,7 +236,8 @@ impl TerminalPanel {
                         Box::new(cx.add_view(|cx| {
                             TerminalView::new(terminal, workspace.database_id(), cx)
                         }));
-                    Pane::add_item(workspace, &pane, terminal, true, true, None, cx);
+                    let focus = pane.read(cx).has_focus();
+                    Pane::add_item(workspace, &pane, terminal, true, focus, None, cx);
                 }
             })?;
             this.update(&mut cx, |this, cx| this.serialize(cx))?;
@@ -364,8 +365,8 @@ impl Panel for TerminalPanel {
         "icons/terminal_12.svg"
     }
 
-    fn icon_tooltip(&self) -> String {
-        "Terminal Panel".into()
+    fn icon_tooltip(&self) -> (String, Option<Box<dyn Action>>) {
+        ("Terminal Panel".into(), Some(Box::new(ToggleFocus)))
     }
 
     fn icon_label(&self, cx: &WindowContext) -> Option<String> {

crates/welcome/src/welcome.rs 🔗

@@ -32,7 +32,7 @@ pub fn init(cx: &mut AppContext) {
 
 pub fn show_welcome_experience(app_state: &Arc<AppState>, cx: &mut AppContext) {
     open_new(&app_state, cx, |workspace, cx| {
-        workspace.toggle_dock(DockPosition::Left, cx);
+        workspace.toggle_dock(DockPosition::Left, false, cx);
         let welcome_page = cx.add_view(|cx| WelcomePage::new(workspace, cx));
         workspace.add_item_to_center(Box::new(welcome_page.clone()), cx);
         cx.focus(&welcome_page);

crates/workspace/src/dock.rs 🔗

@@ -1,9 +1,8 @@
 use crate::{StatusItemView, Workspace};
 use context_menu::{ContextMenu, ContextMenuItem};
 use gpui::{
-    elements::*, impl_actions, platform::CursorStyle, platform::MouseButton, AnyViewHandle,
-    AppContext, Axis, Entity, Subscription, View, ViewContext, ViewHandle, WeakViewHandle,
-    WindowContext,
+    elements::*, platform::CursorStyle, platform::MouseButton, Action, AnyViewHandle, AppContext,
+    Axis, Entity, Subscription, View, ViewContext, ViewHandle, WeakViewHandle, WindowContext,
 };
 use serde::Deserialize;
 use std::rc::Rc;
@@ -16,7 +15,7 @@ pub trait Panel: View {
     fn size(&self, cx: &WindowContext) -> f32;
     fn set_size(&mut self, size: f32, cx: &mut ViewContext<Self>);
     fn icon_path(&self) -> &'static str;
-    fn icon_tooltip(&self) -> String;
+    fn icon_tooltip(&self) -> (String, Option<Box<dyn Action>>);
     fn icon_label(&self, _: &WindowContext) -> Option<String> {
         None
     }
@@ -43,7 +42,7 @@ pub trait PanelHandle {
     fn size(&self, cx: &WindowContext) -> f32;
     fn set_size(&self, size: f32, cx: &mut WindowContext);
     fn icon_path(&self, cx: &WindowContext) -> &'static str;
-    fn icon_tooltip(&self, cx: &WindowContext) -> String;
+    fn icon_tooltip(&self, cx: &WindowContext) -> (String, Option<Box<dyn Action>>);
     fn icon_label(&self, cx: &WindowContext) -> Option<String>;
     fn has_focus(&self, cx: &WindowContext) -> bool;
     fn as_any(&self) -> &AnyViewHandle;
@@ -93,7 +92,7 @@ where
         self.read(cx).icon_path()
     }
 
-    fn icon_tooltip(&self, cx: &WindowContext) -> String {
+    fn icon_tooltip(&self, cx: &WindowContext) -> (String, Option<Box<dyn Action>>) {
         self.read(cx).icon_tooltip()
     }
 
@@ -166,14 +165,6 @@ pub struct PanelButtons {
     workspace: WeakViewHandle<Workspace>,
 }
 
-#[derive(Clone, Debug, Deserialize, PartialEq)]
-pub struct TogglePanel {
-    pub dock_position: DockPosition,
-    pub panel_index: usize,
-}
-
-impl_actions!(workspace, [TogglePanel]);
-
 impl Dock {
     pub fn new(position: DockPosition) -> Self {
         Self {
@@ -423,6 +414,16 @@ impl View for Dock {
             Empty::new().into_any()
         }
     }
+
+    fn focus_in(&mut self, _: AnyViewHandle, cx: &mut ViewContext<Self>) {
+        if cx.is_self_focused() {
+            if let Some(active_entry) = self.active_entry() {
+                cx.focus(active_entry.panel.as_any());
+            } else {
+                cx.focus_parent();
+            }
+        }
+    }
 }
 
 impl PanelButtons {
@@ -470,98 +471,89 @@ impl View for PanelButtons {
             .map(|item| (item.panel.clone(), item.context_menu.clone()))
             .collect::<Vec<_>>();
         Flex::row()
-            .with_children(
-                panels
-                    .into_iter()
-                    .enumerate()
-                    .map(|(ix, (view, context_menu))| {
-                        let action = TogglePanel {
-                            dock_position,
-                            panel_index: ix,
-                        };
-
-                        Stack::new()
-                            .with_child(
-                                MouseEventHandler::<Self, _>::new(ix, cx, |state, cx| {
-                                    let is_active = is_open && ix == active_ix;
-                                    let style = button_style.style_for(state, is_active);
-                                    Flex::row()
-                                        .with_child(
-                                            Svg::new(view.icon_path(cx))
-                                                .with_color(style.icon_color)
-                                                .constrained()
-                                                .with_width(style.icon_size)
+            .with_children(panels.into_iter().enumerate().map(
+                |(panel_ix, (view, context_menu))| {
+                    let (tooltip, tooltip_action) = view.icon_tooltip(cx);
+                    Stack::new()
+                        .with_child(
+                            MouseEventHandler::<Self, _>::new(panel_ix, cx, |state, cx| {
+                                let is_active = is_open && panel_ix == active_ix;
+                                let style = button_style.style_for(state, is_active);
+                                Flex::row()
+                                    .with_child(
+                                        Svg::new(view.icon_path(cx))
+                                            .with_color(style.icon_color)
+                                            .constrained()
+                                            .with_width(style.icon_size)
+                                            .aligned(),
+                                    )
+                                    .with_children(if let Some(label) = view.icon_label(cx) {
+                                        Some(
+                                            Label::new(label, style.label.text.clone())
+                                                .contained()
+                                                .with_style(style.label.container)
                                                 .aligned(),
                                         )
-                                        .with_children(if let Some(label) = view.icon_label(cx) {
-                                            Some(
-                                                Label::new(label, style.label.text.clone())
-                                                    .contained()
-                                                    .with_style(style.label.container)
-                                                    .aligned(),
-                                            )
-                                        } else {
-                                            None
-                                        })
-                                        .constrained()
-                                        .with_height(style.icon_size)
-                                        .contained()
-                                        .with_style(style.container)
-                                })
-                                .with_cursor_style(CursorStyle::PointingHand)
-                                .on_click(MouseButton::Left, {
-                                    let action = action.clone();
-                                    move |_, this, cx| {
-                                        if let Some(workspace) = this.workspace.upgrade(cx) {
-                                            let action = action.clone();
-                                            cx.window_context().defer(move |cx| {
-                                                workspace.update(cx, |workspace, cx| {
-                                                    workspace.toggle_panel(&action, cx)
-                                                });
+                                    } else {
+                                        None
+                                    })
+                                    .constrained()
+                                    .with_height(style.icon_size)
+                                    .contained()
+                                    .with_style(style.container)
+                            })
+                            .with_cursor_style(CursorStyle::PointingHand)
+                            .on_click(MouseButton::Left, {
+                                move |_, this, cx| {
+                                    if let Some(workspace) = this.workspace.upgrade(cx) {
+                                        cx.window_context().defer(move |cx| {
+                                            workspace.update(cx, |workspace, cx| {
+                                                workspace.toggle_panel(dock_position, panel_ix, cx)
                                             });
-                                        }
-                                    }
-                                })
-                                .on_click(MouseButton::Right, {
-                                    let view = view.clone();
-                                    let menu = context_menu.clone();
-                                    move |_, _, cx| {
-                                        const POSITIONS: [DockPosition; 3] = [
-                                            DockPosition::Left,
-                                            DockPosition::Right,
-                                            DockPosition::Bottom,
-                                        ];
-
-                                        menu.update(cx, |menu, cx| {
-                                            let items = POSITIONS
-                                                .into_iter()
-                                                .filter(|position| {
-                                                    *position != dock_position
-                                                        && view.position_is_valid(*position, cx)
-                                                })
-                                                .map(|position| {
-                                                    let view = view.clone();
-                                                    ContextMenuItem::handler(
-                                                        format!("Dock {}", position.to_label()),
-                                                        move |cx| view.set_position(position, cx),
-                                                    )
-                                                })
-                                                .collect();
-                                            menu.show(Default::default(), menu_corner, items, cx);
-                                        })
+                                        });
                                     }
-                                })
-                                .with_tooltip::<Self>(
-                                    ix,
-                                    view.icon_tooltip(cx),
-                                    Some(Box::new(action)),
-                                    tooltip_style.clone(),
-                                    cx,
-                                ),
-                            )
-                            .with_child(ChildView::new(&context_menu, cx))
-                    }),
-            )
+                                }
+                            })
+                            .on_click(MouseButton::Right, {
+                                let view = view.clone();
+                                let menu = context_menu.clone();
+                                move |_, _, cx| {
+                                    const POSITIONS: [DockPosition; 3] = [
+                                        DockPosition::Left,
+                                        DockPosition::Right,
+                                        DockPosition::Bottom,
+                                    ];
+
+                                    menu.update(cx, |menu, cx| {
+                                        let items = POSITIONS
+                                            .into_iter()
+                                            .filter(|position| {
+                                                *position != dock_position
+                                                    && view.position_is_valid(*position, cx)
+                                            })
+                                            .map(|position| {
+                                                let view = view.clone();
+                                                ContextMenuItem::handler(
+                                                    format!("Dock {}", position.to_label()),
+                                                    move |cx| view.set_position(position, cx),
+                                                )
+                                            })
+                                            .collect();
+                                        menu.show(Default::default(), menu_corner, items, cx);
+                                    })
+                                }
+                            })
+                            .with_tooltip::<Self>(
+                                panel_ix,
+                                tooltip,
+                                tooltip_action,
+                                tooltip_style.clone(),
+                                cx,
+                            ),
+                        )
+                        .with_child(ChildView::new(&context_menu, cx))
+                },
+            ))
             .contained()
             .with_style(group_style)
             .into_any()
@@ -672,8 +664,8 @@ pub(crate) mod test {
             "icons/test_panel.svg"
         }
 
-        fn icon_tooltip(&self) -> String {
-            "Test Panel".into()
+        fn icon_tooltip(&self) -> (String, Option<Box<dyn Action>>) {
+            ("Test Panel".into(), None)
         }
 
         fn should_change_position_on_event(event: &Self::Event) -> bool {

crates/workspace/src/workspace.rs 🔗

@@ -64,7 +64,7 @@ use crate::{
         DockData, DockStructure, SerializedPane, SerializedPaneGroup, SerializedWorkspace,
     },
 };
-use dock::{Dock, DockPosition, Panel, PanelButtons, PanelHandle, TogglePanel};
+use dock::{Dock, DockPosition, Panel, PanelButtons, PanelHandle};
 use lazy_static::lazy_static;
 use notifications::{NotificationHandle, NotifyResultExt};
 pub use pane::*;
@@ -103,6 +103,21 @@ pub trait Modal: View {
 #[derive(Clone, PartialEq)]
 pub struct RemoveWorktreeFromProject(pub WorktreeId);
 
+#[derive(Copy, Clone, Default, Deserialize, PartialEq)]
+pub struct ToggleLeftDock {
+    pub focus: bool,
+}
+
+#[derive(Copy, Clone, Default, Deserialize, PartialEq)]
+pub struct ToggleBottomDock {
+    pub focus: bool,
+}
+
+#[derive(Copy, Clone, Default, Deserialize, PartialEq)]
+pub struct ToggleRightDock {
+    pub focus: bool,
+}
+
 actions!(
     workspace,
     [
@@ -118,9 +133,6 @@ actions!(
         ActivatePreviousPane,
         ActivateNextPane,
         FollowNextCollaborator,
-        ToggleLeftDock,
-        ToggleRightDock,
-        ToggleBottomDock,
         NewTerminal,
         ToggleTerminalFocus,
         NewSearch,
@@ -133,6 +145,11 @@ actions!(
 
 actions!(zed, [OpenSettings]);
 
+impl_actions!(
+    workspace,
+    [ToggleLeftDock, ToggleBottomDock, ToggleRightDock]
+);
+
 #[derive(Clone, PartialEq)]
 pub struct OpenPaths {
     pub paths: Vec<PathBuf>,
@@ -242,21 +259,20 @@ pub fn init(app_state: Arc<AppState>, cx: &mut AppContext) {
             workspace.save_active_item(true, cx).detach_and_log_err(cx);
         },
     );
-    cx.add_action(Workspace::toggle_panel);
     cx.add_action(|workspace: &mut Workspace, _: &ActivatePreviousPane, cx| {
         workspace.activate_previous_pane(cx)
     });
     cx.add_action(|workspace: &mut Workspace, _: &ActivateNextPane, cx| {
         workspace.activate_next_pane(cx)
     });
-    cx.add_action(|workspace: &mut Workspace, _: &ToggleLeftDock, cx| {
-        workspace.toggle_dock(DockPosition::Left, cx);
+    cx.add_action(|workspace: &mut Workspace, action: &ToggleLeftDock, cx| {
+        workspace.toggle_dock(DockPosition::Left, action.focus, cx);
     });
-    cx.add_action(|workspace: &mut Workspace, _: &ToggleRightDock, cx| {
-        workspace.toggle_dock(DockPosition::Right, cx);
+    cx.add_action(|workspace: &mut Workspace, action: &ToggleRightDock, cx| {
+        workspace.toggle_dock(DockPosition::Right, action.focus, cx);
     });
-    cx.add_action(|workspace: &mut Workspace, _: &ToggleBottomDock, cx| {
-        workspace.toggle_dock(DockPosition::Bottom, cx);
+    cx.add_action(|workspace: &mut Workspace, action: &ToggleBottomDock, cx| {
+        workspace.toggle_dock(DockPosition::Bottom, action.focus, cx);
     });
     cx.add_action(Workspace::activate_pane_at_index);
 
@@ -1455,36 +1471,49 @@ impl Workspace {
         }
     }
 
-    pub fn toggle_dock(&mut self, dock_side: DockPosition, cx: &mut ViewContext<Self>) {
+    pub fn toggle_dock(
+        &mut self,
+        dock_side: DockPosition,
+        focus: bool,
+        cx: &mut ViewContext<Self>,
+    ) {
         let dock = match dock_side {
-            DockPosition::Left => &mut self.left_dock,
-            DockPosition::Bottom => &mut self.bottom_dock,
-            DockPosition::Right => &mut self.right_dock,
+            DockPosition::Left => &self.left_dock,
+            DockPosition::Bottom => &self.bottom_dock,
+            DockPosition::Right => &self.right_dock,
         };
         dock.update(cx, |dock, cx| {
             let open = !dock.is_open();
             dock.set_open(open, cx);
         });
 
-        self.serialize_workspace(cx);
-
-        cx.focus_self();
+        if dock.read(cx).is_open() && focus {
+            cx.focus(dock);
+        } else {
+            cx.focus_self();
+        }
         cx.notify();
+        self.serialize_workspace(cx);
     }
 
-    pub fn toggle_panel(&mut self, action: &TogglePanel, cx: &mut ViewContext<Self>) {
-        let dock = match action.dock_position {
+    pub fn toggle_panel(
+        &mut self,
+        position: DockPosition,
+        panel_index: usize,
+        cx: &mut ViewContext<Self>,
+    ) {
+        let dock = match position {
             DockPosition::Left => &mut self.left_dock,
             DockPosition::Bottom => &mut self.bottom_dock,
             DockPosition::Right => &mut self.right_dock,
         };
         let active_item = dock.update(cx, move |dock, cx| {
-            if dock.is_open() && dock.active_panel_index() == action.panel_index {
+            if dock.is_open() && dock.active_panel_index() == panel_index {
                 dock.set_open(false, cx);
                 None
             } else {
                 dock.set_open(true, cx);
-                dock.activate_panel(action.panel_index, cx);
+                dock.activate_panel(panel_index, cx);
                 dock.active_panel().cloned()
             }
         });

crates/zed/src/menus.rs 🔗

@@ -89,9 +89,18 @@ pub fn menus() -> Vec<Menu<'static>> {
                 MenuItem::action("Zoom Out", super::DecreaseBufferFontSize),
                 MenuItem::action("Reset Zoom", super::ResetBufferFontSize),
                 MenuItem::separator(),
-                MenuItem::action("Toggle Left Dock", workspace::ToggleLeftDock),
-                MenuItem::action("Toggle Right Dock", workspace::ToggleRightDock),
-                MenuItem::action("Toggle Bottom Dock", workspace::ToggleBottomDock),
+                MenuItem::action(
+                    "Toggle Left Dock",
+                    workspace::ToggleLeftDock { focus: false },
+                ),
+                MenuItem::action(
+                    "Toggle Right Dock",
+                    workspace::ToggleRightDock { focus: false },
+                ),
+                MenuItem::action(
+                    "Toggle Bottom Dock",
+                    workspace::ToggleBottomDock { focus: false },
+                ),
                 MenuItem::submenu(Menu {
                     name: "Editor Layout",
                     items: vec![

crates/zed/src/zed.rs 🔗

@@ -354,7 +354,7 @@ pub fn initialize_workspace(
                             .map_or(false, |entry| entry.is_dir())
                     })
             {
-                workspace.toggle_dock(project_panel_position, cx);
+                workspace.toggle_dock(project_panel_position, false, cx);
             }
 
             workspace.add_panel(terminal_panel, cx)