pane: Improve close active item to better handle pinned tabs (#23488)

Closes #22247

- [x] Do not close pinned tab on keyboard shortcuts like `ctrl+w` or
`alt+f4`
- [x] Close pinned tab on context menu action, menu bar action, or vim
bang
- [x] While closing pinned tab via shortcut (where it won't close),
instead activate any other non-pinned tab in same pane
- [x] Else, if any other pane contains non-pinned tab, activate that
- [x] Tests

Co-authored-by: uncenter <47499684+uncenter@users.noreply.github.com>

Release Notes:

- Pinned tab now stay open when using close shortcuts, auto focuses to
any other non-pinned tab instead.

assets/keymaps/default-linux.json

@@ -274,8 +274,8 @@
       "ctrl-pagedown": "pane::ActivateNextItem",
       "ctrl-shift-pageup": "pane::SwapItemLeft",
       "ctrl-shift-pagedown": "pane::SwapItemRight",
-      "ctrl-f4": "pane::CloseActiveItem",
-      "ctrl-w": "pane::CloseActiveItem",
+      "ctrl-f4": ["pane::CloseActiveItem", { "close_pinned": false }],
+      "ctrl-w": ["pane::CloseActiveItem", { "close_pinned": false }],
       "alt-ctrl-t": ["pane::CloseInactiveItems", { "close_pinned": false }],
       "alt-ctrl-shift-w": "workspace::CloseInactiveTabsAndPanes",
       "ctrl-k e": ["pane::CloseItemsToTheLeft", { "close_pinned": false }],

assets/keymaps/default-macos.json

@@ -349,7 +349,7 @@
       "cmd-}": "pane::ActivateNextItem",
       "ctrl-shift-pageup": "pane::SwapItemLeft",
       "ctrl-shift-pagedown": "pane::SwapItemRight",
-      "cmd-w": "pane::CloseActiveItem",
+      "cmd-w": ["pane::CloseActiveItem", { "close_pinned": false }],
       "alt-cmd-t": ["pane::CloseInactiveItems", { "close_pinned": false }],
       "ctrl-alt-cmd-w": "workspace::CloseInactiveTabsAndPanes",
       "cmd-k e": ["pane::CloseItemsToTheLeft", { "close_pinned": false }],

crates/file_finder/src/file_finder_tests.rs

@@ -817,7 +817,10 @@ async fn test_external_files_history(cx: &mut gpui::TestAppContext) {
                 .as_u64() as usize,
         )
     });
-    cx.dispatch_action(workspace::CloseActiveItem { save_intent: None });
+    cx.dispatch_action(workspace::CloseActiveItem {
+        save_intent: None,
+        close_pinned: false,
+    });
 
     let initial_history_items =
         open_close_queried_buffer("sec", 1, "second.rs", &workspace, cx).await;
@@ -2000,7 +2003,10 @@ async fn open_close_queried_buffer(
     )
     .await;
 
-    cx.dispatch_action(workspace::CloseActiveItem { save_intent: None });
+    cx.dispatch_action(workspace::CloseActiveItem {
+        save_intent: None,
+        close_pinned: false,
+    });
 
     history_items
 }

crates/terminal_view/src/terminal_view.rs

@@ -257,7 +257,13 @@ impl TerminalView {
                         .action("Inline Assist", Box::new(InlineAssist::default()))
                 })
                 .separator()
-                .action("Close", Box::new(CloseActiveItem { save_intent: None }))
+                .action(
+                    "Close",
+                    Box::new(CloseActiveItem {
+                        save_intent: None,
+                        close_pinned: true,
+                    }),
+                )
         });
 
         window.focus(&context_menu.focus_handle(cx));

crates/vim/src/command.rs

@@ -567,37 +567,45 @@ fn generate_commands(_: &App) -> Vec<VimCommand> {
             ("q", "uit"),
             workspace::CloseActiveItem {
                 save_intent: Some(SaveIntent::Close),
+                close_pinned: false,
             },
         )
         .bang(workspace::CloseActiveItem {
             save_intent: Some(SaveIntent::Skip),
+            close_pinned: true,
         }),
         VimCommand::new(
             ("wq", ""),
             workspace::CloseActiveItem {
                 save_intent: Some(SaveIntent::Save),
+                close_pinned: false,
             },
         )
         .bang(workspace::CloseActiveItem {
             save_intent: Some(SaveIntent::Overwrite),
+            close_pinned: true,
         }),
         VimCommand::new(
             ("x", "it"),
             workspace::CloseActiveItem {
                 save_intent: Some(SaveIntent::SaveAll),
+                close_pinned: false,
             },
         )
         .bang(workspace::CloseActiveItem {
             save_intent: Some(SaveIntent::Overwrite),
+            close_pinned: true,
         }),
         VimCommand::new(
             ("ex", "it"),
             workspace::CloseActiveItem {
                 save_intent: Some(SaveIntent::SaveAll),
+                close_pinned: false,
             },
         )
         .bang(workspace::CloseActiveItem {
             save_intent: Some(SaveIntent::Overwrite),
+            close_pinned: true,
         }),
         VimCommand::new(
             ("up", "date"),
@@ -657,10 +665,12 @@ fn generate_commands(_: &App) -> Vec<VimCommand> {
             ("bd", "elete"),
             workspace::CloseActiveItem {
                 save_intent: Some(SaveIntent::Close),
+                close_pinned: false,
             },
         )
         .bang(workspace::CloseActiveItem {
             save_intent: Some(SaveIntent::Skip),
+            close_pinned: true,
         }),
         VimCommand::new(("bn", "ext"), workspace::ActivateNextItem).count(),
         VimCommand::new(("bN", "ext"), workspace::ActivatePrevItem).count(),
@@ -679,6 +689,7 @@ fn generate_commands(_: &App) -> Vec<VimCommand> {
             ("tabc", "lose"),
             workspace::CloseActiveItem {
                 save_intent: Some(SaveIntent::Close),
+                close_pinned: false,
             },
         ),
         VimCommand::new(

crates/workspace/src/pane.rs

@@ -99,6 +99,8 @@ pub struct ActivateItem(pub usize);
 #[serde(deny_unknown_fields)]
 pub struct CloseActiveItem {
     pub save_intent: Option<SaveIntent>,
+    #[serde(default)]
+    pub close_pinned: bool,
 }
 
 #[derive(Clone, PartialEq, Debug, Deserialize, JsonSchema, Default)]
@@ -1224,6 +1226,37 @@ impl Pane {
 
             return None;
         }
+        if self.is_tab_pinned(self.active_item_index) && !action.close_pinned {
+            // Activate any non-pinned tab in same pane
+            let non_pinned_tab_index = self
+                .items()
+                .enumerate()
+                .find(|(index, _item)| !self.is_tab_pinned(*index))
+                .map(|(index, _item)| index);
+            if let Some(index) = non_pinned_tab_index {
+                self.activate_item(index, false, false, window, cx);
+                return None;
+            }
+
+            // Activate any non-pinned tab in different pane
+            let current_pane = cx.entity();
+            self.workspace
+                .update(cx, |workspace, cx| {
+                    let panes = workspace.center.panes();
+                    let pane_with_unpinned_tab = panes.iter().find(|pane| {
+                        if **pane == &current_pane {
+                            return false;
+                        }
+                        pane.read(cx).has_unpinned_tabs()
+                    });
+                    if let Some(pane) = pane_with_unpinned_tab {
+                        pane.update(cx, |pane, cx| pane.activate_unpinned_tab(window, cx));
+                    }
+                })
+                .ok();
+
+            return None;
+        };
         let active_item_id = self.items[self.active_item_index].item_id();
         Some(self.close_item_by_id(
             active_item_id,
@@ -2105,6 +2138,24 @@ impl Pane {
         self.pinned_tab_count != 0
     }
 
+    fn has_unpinned_tabs(&self) -> bool {
+        self.pinned_tab_count < self.items.len()
+    }
+
+    fn activate_unpinned_tab(&mut self, window: &mut Window, cx: &mut Context<Self>) {
+        if self.items.is_empty() {
+            return;
+        }
+        let Some(index) = self
+            .items()
+            .enumerate()
+            .find_map(|(index, _item)| (!self.is_tab_pinned(index)).then_some(index))
+        else {
+            return;
+        };
+        self.activate_item(index, true, true, window, cx);
+    }
+
     fn render_tab(
         &self,
         ix: usize,
@@ -2280,7 +2331,10 @@ impl Pane {
                             pane.unpin_tab_at(ix, window, cx);
                         }))
                 } else {
-                    end_slot_action = &CloseActiveItem { save_intent: None };
+                    end_slot_action = &CloseActiveItem {
+                        save_intent: None,
+                        close_pinned: false,
+                    };
                     end_slot_tooltip_text = "Close Tab";
                     IconButton::new("close tab", IconName::Close)
                         .when(!always_show_close_button, |button| {
@@ -2350,7 +2404,10 @@ impl Pane {
                     menu = menu
                         .entry(
                             "Close",
-                            Some(Box::new(CloseActiveItem { save_intent: None })),
+                            Some(Box::new(CloseActiveItem {
+                                save_intent: None,
+                                close_pinned: true,
+                            })),
                             window.handler_for(&pane, move |pane, window, cx| {
                                 pane.close_item_by_id(item_id, SaveIntent::Close, window, cx)
                                     .detach_and_log_err(cx);
@@ -2991,14 +3048,9 @@ impl Pane {
 
         self.items
             .iter()
-            .map(|item| item.item_id())
-            .filter(|item_id| {
-                if let Some(ix) = self.index_for_item_id(*item_id) {
-                    self.is_tab_pinned(ix)
-                } else {
-                    true
-                }
-            })
+            .enumerate()
+            .filter(|(index, _item)| self.is_tab_pinned(*index))
+            .map(|(_, item)| item.item_id())
             .collect()
     }
 
@@ -3561,7 +3613,14 @@ mod tests {
 
         pane.update_in(cx, |pane, window, cx| {
             assert!(pane
-                .close_active_item(&CloseActiveItem { save_intent: None }, window, cx)
+                .close_active_item(
+                    &CloseActiveItem {
+                        save_intent: None,
+                        close_pinned: false
+                    },
+                    window,
+                    cx
+                )
                 .is_none())
         });
     }
@@ -3902,7 +3961,14 @@ mod tests {
         assert_item_labels(&pane, ["A", "B", "1*", "C", "D"], cx);
 
         pane.update_in(cx, |pane, window, cx| {
-            pane.close_active_item(&CloseActiveItem { save_intent: None }, window, cx)
+            pane.close_active_item(
+                &CloseActiveItem {
+                    save_intent: None,
+                    close_pinned: false,
+                },
+                window,
+                cx,
+            )
         })
         .unwrap()
         .await
@@ -3915,7 +3981,14 @@ mod tests {
         assert_item_labels(&pane, ["A", "B", "C", "D*"], cx);
 
         pane.update_in(cx, |pane, window, cx| {
-            pane.close_active_item(&CloseActiveItem { save_intent: None }, window, cx)
+            pane.close_active_item(
+                &CloseActiveItem {
+                    save_intent: None,
+                    close_pinned: false,
+                },
+                window,
+                cx,
+            )
         })
         .unwrap()
         .await
@@ -3923,7 +3996,14 @@ mod tests {
         assert_item_labels(&pane, ["A", "B*", "C"], cx);
 
         pane.update_in(cx, |pane, window, cx| {
-            pane.close_active_item(&CloseActiveItem { save_intent: None }, window, cx)
+            pane.close_active_item(
+                &CloseActiveItem {
+                    save_intent: None,
+                    close_pinned: false,
+                },
+                window,
+                cx,
+            )
         })
         .unwrap()
         .await
@@ -3931,7 +4011,14 @@ mod tests {
         assert_item_labels(&pane, ["A", "C*"], cx);
 
         pane.update_in(cx, |pane, window, cx| {
-            pane.close_active_item(&CloseActiveItem { save_intent: None }, window, cx)
+            pane.close_active_item(
+                &CloseActiveItem {
+                    save_intent: None,
+                    close_pinned: false,
+                },
+                window,
+                cx,
+            )
         })
         .unwrap()
         .await
@@ -3967,7 +4054,14 @@ mod tests {
         assert_item_labels(&pane, ["A", "B", "1*", "C", "D"], cx);
 
         pane.update_in(cx, |pane, window, cx| {
-            pane.close_active_item(&CloseActiveItem { save_intent: None }, window, cx)
+            pane.close_active_item(
+                &CloseActiveItem {
+                    save_intent: None,
+                    close_pinned: false,
+                },
+                window,
+                cx,
+            )
         })
         .unwrap()
         .await
@@ -3980,7 +4074,14 @@ mod tests {
         assert_item_labels(&pane, ["A", "B", "C", "D*"], cx);
 
         pane.update_in(cx, |pane, window, cx| {
-            pane.close_active_item(&CloseActiveItem { save_intent: None }, window, cx)
+            pane.close_active_item(
+                &CloseActiveItem {
+                    save_intent: None,
+                    close_pinned: false,
+                },
+                window,
+                cx,
+            )
         })
         .unwrap()
         .await
@@ -3988,7 +4089,14 @@ mod tests {
         assert_item_labels(&pane, ["A", "B", "C*"], cx);
 
         pane.update_in(cx, |pane, window, cx| {
-            pane.close_active_item(&CloseActiveItem { save_intent: None }, window, cx)
+            pane.close_active_item(
+                &CloseActiveItem {
+                    save_intent: None,
+                    close_pinned: false,
+                },
+                window,
+                cx,
+            )
         })
         .unwrap()
         .await
@@ -3996,7 +4104,14 @@ mod tests {
         assert_item_labels(&pane, ["A", "B*"], cx);
 
         pane.update_in(cx, |pane, window, cx| {
-            pane.close_active_item(&CloseActiveItem { save_intent: None }, window, cx)
+            pane.close_active_item(
+                &CloseActiveItem {
+                    save_intent: None,
+                    close_pinned: false,
+                },
+                window,
+                cx,
+            )
         })
         .unwrap()
         .await
@@ -4032,7 +4147,14 @@ mod tests {
         assert_item_labels(&pane, ["A", "B", "1*", "C", "D"], cx);
 
         pane.update_in(cx, |pane, window, cx| {
-            pane.close_active_item(&CloseActiveItem { save_intent: None }, window, cx)
+            pane.close_active_item(
+                &CloseActiveItem {
+                    save_intent: None,
+                    close_pinned: false,
+                },
+                window,
+                cx,
+            )
         })
         .unwrap()
         .await
@@ -4045,7 +4167,14 @@ mod tests {
         assert_item_labels(&pane, ["A", "B", "C", "D*"], cx);
 
         pane.update_in(cx, |pane, window, cx| {
-            pane.close_active_item(&CloseActiveItem { save_intent: None }, window, cx)
+            pane.close_active_item(
+                &CloseActiveItem {
+                    save_intent: None,
+                    close_pinned: false,
+                },
+                window,
+                cx,
+            )
         })
         .unwrap()
         .await
@@ -4058,7 +4187,14 @@ mod tests {
         assert_item_labels(&pane, ["A*", "B", "C"], cx);
 
         pane.update_in(cx, |pane, window, cx| {
-            pane.close_active_item(&CloseActiveItem { save_intent: None }, window, cx)
+            pane.close_active_item(
+                &CloseActiveItem {
+                    save_intent: None,
+                    close_pinned: false,
+                },
+                window,
+                cx,
+            )
         })
         .unwrap()
         .await
@@ -4066,7 +4202,14 @@ mod tests {
         assert_item_labels(&pane, ["B*", "C"], cx);
 
         pane.update_in(cx, |pane, window, cx| {
-            pane.close_active_item(&CloseActiveItem { save_intent: None }, window, cx)
+            pane.close_active_item(
+                &CloseActiveItem {
+                    save_intent: None,
+                    close_pinned: false,
+                },
+                window,
+                cx,
+            )
         })
         .unwrap()
         .await
@@ -4300,7 +4443,7 @@ mod tests {
 
         let project = Project::test(fs, None, cx).await;
         let (workspace, cx) =
-            cx.add_window_view(|window, cx| Workspace::test_new(project.clone(), window, cx));
+            cx.add_window_view(|window, cx| Workspace::test_new(project, window, cx));
         let pane = workspace.update(cx, |workspace, _| workspace.active_pane().clone());
 
         let item_a = add_labeled_item(&pane, "A", false, cx);
@@ -4326,6 +4469,71 @@ mod tests {
         assert_item_labels(&pane, [], cx);
     }
 
+    #[gpui::test]
+    async fn test_close_pinned_tab_with_non_pinned_in_same_pane(cx: &mut TestAppContext) {
+        init_test(cx);
+        let fs = FakeFs::new(cx.executor());
+        let project = Project::test(fs, None, cx).await;
+        let (workspace, cx) =
+            cx.add_window_view(|window, cx| Workspace::test_new(project, window, cx));
+
+        // Non-pinned tabs in same pane
+        let pane = workspace.update(cx, |workspace, _| workspace.active_pane().clone());
+        add_labeled_item(&pane, "A", false, cx);
+        add_labeled_item(&pane, "B", false, cx);
+        add_labeled_item(&pane, "C", false, cx);
+        pane.update_in(cx, |pane, window, cx| {
+            pane.pin_tab_at(0, window, cx);
+        });
+        set_labeled_items(&pane, ["A*", "B", "C"], cx);
+        pane.update_in(cx, |pane, window, cx| {
+            pane.close_active_item(
+                &CloseActiveItem {
+                    save_intent: None,
+                    close_pinned: false,
+                },
+                window,
+                cx,
+            );
+        });
+        // Non-pinned tab should be active
+        assert_item_labels(&pane, ["A", "B*", "C"], cx);
+    }
+
+    #[gpui::test]
+    async fn test_close_pinned_tab_with_non_pinned_in_different_pane(cx: &mut TestAppContext) {
+        init_test(cx);
+        let fs = FakeFs::new(cx.executor());
+        let project = Project::test(fs, None, cx).await;
+        let (workspace, cx) =
+            cx.add_window_view(|window, cx| Workspace::test_new(project, window, cx));
+
+        // No non-pinned tabs in same pane, non-pinned tabs in another pane
+        let pane1 = workspace.update(cx, |workspace, _| workspace.active_pane().clone());
+        let pane2 = workspace.update_in(cx, |workspace, window, cx| {
+            workspace.split_pane(pane1.clone(), SplitDirection::Right, window, cx)
+        });
+        add_labeled_item(&pane1, "A", false, cx);
+        pane1.update_in(cx, |pane, window, cx| {
+            pane.pin_tab_at(0, window, cx);
+        });
+        set_labeled_items(&pane1, ["A*"], cx);
+        add_labeled_item(&pane2, "B", false, cx);
+        set_labeled_items(&pane2, ["B"], cx);
+        pane1.update_in(cx, |pane, window, cx| {
+            pane.close_active_item(
+                &CloseActiveItem {
+                    save_intent: None,
+                    close_pinned: false,
+                },
+                window,
+                cx,
+            );
+        });
+        //  Non-pinned tab of other pane should be active
+        assert_item_labels(&pane2, ["B*"], cx);
+    }
+
     fn init_test(cx: &mut TestAppContext) {
         cx.update(|cx| {
             let settings_store = SettingsStore::test(cx);

crates/workspace/src/workspace.rs

@@ -8175,6 +8175,7 @@ mod tests {
                 pane.close_active_item(
                     &CloseActiveItem {
                         save_intent: Some(SaveIntent::Close),
+                        close_pinned: false,
                     },
                     window,
                     cx,
@@ -8279,7 +8280,14 @@ mod tests {
         });
         let close_singleton_buffer_task = pane
             .update_in(cx, |pane, window, cx| {
-                pane.close_active_item(&CloseActiveItem { save_intent: None }, window, cx)
+                pane.close_active_item(
+                    &CloseActiveItem {
+                        save_intent: None,
+                        close_pinned: false,
+                    },
+                    window,
+                    cx,
+                )
             })
             .expect("should have active singleton buffer to close");
         cx.background_executor.run_until_parked();
@@ -8385,7 +8393,14 @@ mod tests {
         });
         let _close_multi_buffer_task = pane
             .update_in(cx, |pane, window, cx| {
-                pane.close_active_item(&CloseActiveItem { save_intent: None }, window, cx)
+                pane.close_active_item(
+                    &CloseActiveItem {
+                        save_intent: None,
+                        close_pinned: false,
+                    },
+                    window,
+                    cx,
+                )
             })
             .expect("should have active multi buffer to close");
         cx.background_executor.run_until_parked();
@@ -8476,7 +8491,14 @@ mod tests {
         });
         let close_multi_buffer_task = pane
             .update_in(cx, |pane, window, cx| {
-                pane.close_active_item(&CloseActiveItem { save_intent: None }, window, cx)
+                pane.close_active_item(
+                    &CloseActiveItem {
+                        save_intent: None,
+                        close_pinned: false,
+                    },
+                    window,
+                    cx,
+                )
             })
             .expect("should have active multi buffer to close");
         cx.background_executor.run_until_parked();

crates/zed/src/zed.rs

@@ -3103,7 +3103,10 @@ mod tests {
         });
         cx.dispatch_action(
             window.into(),
-            workspace::CloseActiveItem { save_intent: None },
+            workspace::CloseActiveItem {
+                save_intent: None,
+                close_pinned: false,
+            },
         );
 
         cx.background_executor.run_until_parked();
@@ -3116,7 +3119,10 @@ mod tests {
 
         cx.dispatch_action(
             window.into(),
-            workspace::CloseActiveItem { save_intent: None },
+            workspace::CloseActiveItem {
+                save_intent: None,
+                close_pinned: false,
+            },
         );
         cx.background_executor.run_until_parked();
         cx.simulate_prompt_answer(1);

crates/zed/src/zed/app_menus.rs

@@ -76,7 +76,10 @@ pub fn app_menus() -> Vec<Menu> {
                 MenuItem::action("Save All", workspace::SaveAll { save_intent: None }),
                 MenuItem::action(
                     "Close Editor",
-                    workspace::CloseActiveItem { save_intent: None },
+                    workspace::CloseActiveItem {
+                        save_intent: None,
+                        close_pinned: true,
+                    },
                 ),
                 MenuItem::action("Close Window", workspace::CloseWindow),
             ],