From d1ca6db7561ecd75f068a4dd306d1904c0415aec Mon Sep 17 00:00:00 2001 From: vipex <101529155+vipexv@users.noreply.github.com> Date: Thu, 12 Jun 2025 10:21:00 +0200 Subject: [PATCH] pane: Apply `max_tabs` change immediately (#32447) Closes #32217 Follow up of https://github.com/zed-industries/zed/pull/32301, sorry about the messy rebase in the previous PR. Release Notes: - Fixed `max_tabs` setting not applying immediately when changed TODO: - [x] Fix the off-by-one bug (currently closing one more tab than the max_tabs setting) while perserving "+1 Tab Allowance" feature. - [x] Investigate Double Invocation of `settings_changed` - [x] Write test that: - Sets max_tabs to `n` - Opens `n` buffers - Changes max_tabs to `n-1` - Asserts we have exactly `n-1` buffers remaining --------- Co-authored-by: Joseph T. Lyons --- crates/workspace/src/pane.rs | 97 +++++++++++++++++++++++++++++++++--- 1 file changed, 89 insertions(+), 8 deletions(-) diff --git a/crates/workspace/src/pane.rs b/crates/workspace/src/pane.rs index 4ebc2f66d3..29d1fd542c 100644 --- a/crates/workspace/src/pane.rs +++ b/crates/workspace/src/pane.rs @@ -32,6 +32,7 @@ use settings::{Settings, SettingsStore}; use std::{ any::Any, cmp, fmt, mem, + num::NonZeroUsize, ops::ControlFlow, path::PathBuf, rc::Rc, @@ -323,6 +324,7 @@ pub struct Pane { >, render_tab_bar: Rc) -> AnyElement>, show_tab_bar_buttons: bool, + max_tabs: Option, _subscriptions: Vec, tab_bar_scroll_handle: ScrollHandle, /// Is None if navigation buttons are permanently turned off (and should not react to setting changes). @@ -425,11 +427,12 @@ impl Pane { cx.on_focus(&focus_handle, window, Pane::focus_in), cx.on_focus_in(&focus_handle, window, Pane::focus_in), cx.on_focus_out(&focus_handle, window, Pane::focus_out), - cx.observe_global::(Self::settings_changed), + cx.observe_global_in::(window, Self::settings_changed), cx.subscribe(&project, Self::project_events), ]; let handle = cx.entity().downgrade(); + Self { alternate_file_items: (None, None), focus_handle, @@ -440,6 +443,7 @@ impl Pane { zoomed: false, active_item_index: 0, preview_item_id: None, + max_tabs: WorkspaceSettings::get_global(cx).max_tabs, last_focus_handle_by_item: Default::default(), nav_history: NavHistory(Arc::new(Mutex::new(NavHistoryState { mode: NavigationMode::Normal, @@ -620,17 +624,25 @@ impl Pane { } } - fn settings_changed(&mut self, cx: &mut Context) { + fn settings_changed(&mut self, window: &mut Window, cx: &mut Context) { let tab_bar_settings = TabBarSettings::get_global(cx); + let new_max_tabs = WorkspaceSettings::get_global(cx).max_tabs; if let Some(display_nav_history_buttons) = self.display_nav_history_buttons.as_mut() { *display_nav_history_buttons = tab_bar_settings.show_nav_history_buttons; } + self.show_tab_bar_buttons = tab_bar_settings.show_tab_bar_buttons; if !PreviewTabsSettings::get_global(cx).enabled { self.preview_item_id = None; } + + if new_max_tabs != self.max_tabs { + self.max_tabs = new_max_tabs; + self.close_items_on_settings_change(window, cx); + } + self.update_diagnostics(cx); cx.notify(); } @@ -924,7 +936,7 @@ impl Pane { .any(|existing_item| existing_item.item_id() == item.item_id()); if !item_already_exists { - self.close_items_over_max_tabs(window, cx); + self.close_items_on_item_open(window, cx); } if item.is_singleton(cx) { @@ -932,6 +944,7 @@ impl Pane { let Some(project) = self.project.upgrade() else { return; }; + let project = project.read(cx); if let Some(project_path) = project.path_for_entry(entry_id, cx) { let abs_path = project.absolute_path(&project_path, cx); @@ -1409,29 +1422,59 @@ impl Pane { ) } - pub fn close_items_over_max_tabs(&mut self, window: &mut Window, cx: &mut Context) { - let Some(max_tabs) = WorkspaceSettings::get_global(cx).max_tabs.map(|i| i.get()) else { + fn close_items_on_item_open(&mut self, window: &mut Window, cx: &mut Context) { + let target = self.max_tabs.map(|m| m.get()); + let protect_active_item = false; + self.close_items_to_target_count(target, protect_active_item, window, cx); + } + + fn close_items_on_settings_change(&mut self, window: &mut Window, cx: &mut Context) { + let target = self.max_tabs.map(|m| m.get() + 1); + // The active item in this case is the settings.json file, which should be protected from being closed + let protect_active_item = true; + self.close_items_to_target_count(target, protect_active_item, window, cx); + } + + fn close_items_to_target_count( + &mut self, + target_count: Option, + protect_active_item: bool, + window: &mut Window, + cx: &mut Context, + ) { + let Some(target_count) = target_count else { return; }; - // Reduce over the activation history to get every dirty items up to max_tabs - // count. let mut index_list = Vec::new(); let mut items_len = self.items_len(); let mut indexes: HashMap = HashMap::default(); + let active_ix = self.active_item_index(); + for (index, item) in self.items.iter().enumerate() { indexes.insert(item.item_id(), index); } + + // Close least recently used items to reach target count. + // The target count is allowed to be exceeded, as we protect pinned + // items, dirty items, and sometimes, the active item. for entry in self.activation_history.iter() { - if items_len < max_tabs { + if items_len < target_count { break; } + let Some(&index) = indexes.get(&entry.entity_id) else { continue; }; + + if protect_active_item && index == active_ix { + continue; + } + if let Some(true) = self.items.get(index).map(|item| item.is_dirty(cx)) { continue; } + if self.is_tab_pinned(index) { continue; } @@ -3848,6 +3891,7 @@ mod tests { for i in 0..7 { add_labeled_item(&pane, format!("{}", i).as_str(), false, cx); } + set_max_tabs(cx, Some(5)); add_labeled_item(&pane, "7", false, cx); // Remove items to respect the max tab cap. @@ -3884,6 +3928,43 @@ mod tests { ); } + #[gpui::test] + async fn test_reduce_max_tabs_closes_existing_items(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.clone(), window, cx)); + let pane = workspace.read_with(cx, |workspace, _| workspace.active_pane().clone()); + + add_labeled_item(&pane, "A", false, cx); + add_labeled_item(&pane, "B", false, cx); + let item_c = add_labeled_item(&pane, "C", false, cx); + let item_d = add_labeled_item(&pane, "D", false, cx); + add_labeled_item(&pane, "E", false, cx); + add_labeled_item(&pane, "Settings", false, cx); + assert_item_labels(&pane, ["A", "B", "C", "D", "E", "Settings*"], cx); + + set_max_tabs(cx, Some(5)); + assert_item_labels(&pane, ["B", "C", "D", "E", "Settings*"], cx); + + set_max_tabs(cx, Some(4)); + assert_item_labels(&pane, ["C", "D", "E", "Settings*"], cx); + + pane.update_in(cx, |pane, window, cx| { + let ix = pane.index_for_item_id(item_c.item_id()).unwrap(); + pane.pin_tab_at(ix, window, cx); + + let ix = pane.index_for_item_id(item_d.item_id()).unwrap(); + pane.pin_tab_at(ix, window, cx); + }); + assert_item_labels(&pane, ["C!", "D!", "E", "Settings*"], cx); + + set_max_tabs(cx, Some(2)); + assert_item_labels(&pane, ["C!", "D!", "Settings*"], cx); + } + #[gpui::test] async fn test_allow_pinning_dirty_item_at_max_tabs(cx: &mut TestAppContext) { init_test(cx);