From 24e4446cd35a7946674c8993cc14138d4b67e194 Mon Sep 17 00:00:00 2001 From: "Joseph T. Lyons" Date: Sat, 31 May 2025 19:38:32 -0400 Subject: [PATCH] Refactor item-closing actions (#31838) While working on - https://github.com/zed-industries/zed/pull/31783 - https://github.com/zed-industries/zed/pull/31786 ... I noticed some areas that could be improved through refactoring. The bug in https://github.com/zed-industries/zed/pull/31783 came from having duplicate code. The fix had been applied to one version, but not the duplicated code. This PR attempts to do some initial clean up, through some refactoring. Release Notes: - N/A --- crates/collab/src/tests/following_tests.rs | 1 - crates/editor/src/editor_tests.rs | 3 - crates/workspace/src/pane.rs | 316 +++++++++++---------- crates/workspace/src/workspace.rs | 42 ++- 4 files changed, 184 insertions(+), 178 deletions(-) diff --git a/crates/collab/src/tests/following_tests.rs b/crates/collab/src/tests/following_tests.rs index 18ad066993cafd0e134bd9dcc994c525d36aa6b7..7b44fceb530c47a995ab2d0da6acdbbe05561cbd 100644 --- a/crates/collab/src/tests/following_tests.rs +++ b/crates/collab/src/tests/following_tests.rs @@ -1010,7 +1010,6 @@ async fn test_peers_following_each_other(cx_a: &mut TestAppContext, cx_b: &mut T workspace_b.update_in(cx_b, |workspace, window, cx| { workspace.active_pane().update(cx, |pane, cx| { pane.close_inactive_items(&Default::default(), window, cx) - .unwrap() .detach(); }); }); diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index c7af25f48db123f773d3bbe59cb412267c21a2e7..ce79027bc89a80409b83d5573990e180f629ceb1 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -20016,7 +20016,6 @@ println!("5"); pane_1 .update_in(cx, |pane, window, cx| { pane.close_inactive_items(&CloseInactiveItems::default(), window, cx) - .unwrap() }) .await .unwrap(); @@ -20053,7 +20052,6 @@ println!("5"); pane_2 .update_in(cx, |pane, window, cx| { pane.close_inactive_items(&CloseInactiveItems::default(), window, cx) - .unwrap() }) .await .unwrap(); @@ -20229,7 +20227,6 @@ println!("5"); }); pane.update_in(cx, |pane, window, cx| { pane.close_all_items(&CloseAllItems::default(), window, cx) - .unwrap() }) .await .unwrap(); diff --git a/crates/workspace/src/pane.rs b/crates/workspace/src/pane.rs index a40b3699da8359288dd4a1c75b2673524f7f5eda..05d51752ddf69d1f12c85ba1c99a89957207ebf6 100644 --- a/crates/workspace/src/pane.rs +++ b/crates/workspace/src/pane.rs @@ -1035,6 +1035,10 @@ impl Pane { self.items.get(self.active_item_index).cloned() } + fn active_item_id(&self) -> EntityId { + self.items[self.active_item_index].item_id() + } + pub fn pixel_position_of_cursor(&self, cx: &App) -> Option> { self.items .get(self.active_item_index)? @@ -1244,7 +1248,7 @@ impl Pane { return None; }; - let active_item_id = self.items[self.active_item_index].item_id(); + let active_item_id = self.active_item_id(); Some(self.close_item_by_id( active_item_id, action.save_intent.unwrap_or(SaveIntent::Close), @@ -1270,19 +1274,23 @@ impl Pane { action: &CloseInactiveItems, window: &mut Window, cx: &mut Context, - ) -> Option>> { + ) -> Task> { if self.items.is_empty() { - return None; + return Task::ready(Ok(())); } - let active_item_id = self.items[self.active_item_index].item_id(); - let non_closeable_items = self.get_non_closeable_item_ids(action.close_pinned); - Some(self.close_items( + let active_item_id = self.active_item_id(); + let pinned_item_ids = self.pinned_item_ids(); + + self.close_items( window, cx, action.save_intent.unwrap_or(SaveIntent::Close), - move |item_id| item_id != active_item_id && !non_closeable_items.contains(&item_id), - )) + move |item_id| { + item_id != active_item_id + && (action.close_pinned || !pinned_item_ids.contains(&item_id)) + }, + ) } pub fn close_clean_items( @@ -1290,18 +1298,18 @@ impl Pane { action: &CloseCleanItems, window: &mut Window, cx: &mut Context, - ) -> Option>> { - let item_ids: Vec<_> = self - .items() - .filter(|item| !item.is_dirty(cx)) - .map(|item| item.item_id()) - .collect(); - let non_closeable_items = self.get_non_closeable_item_ids(action.close_pinned); - Some( - self.close_items(window, cx, SaveIntent::Close, move |item_id| { - item_ids.contains(&item_id) && !non_closeable_items.contains(&item_id) - }), - ) + ) -> Task> { + if self.items.is_empty() { + return Task::ready(Ok(())); + } + + let clean_item_ids = self.clean_item_ids(cx); + let pinned_item_ids = self.pinned_item_ids(); + + self.close_items(window, cx, SaveIntent::Close, move |item_id| { + clean_item_ids.contains(&item_id) + && (action.close_pinned || !pinned_item_ids.contains(&item_id)) + }) } pub fn close_items_to_the_left( @@ -1313,12 +1321,14 @@ impl Pane { if self.items.is_empty() { return None; } - let active_item_id = self.items[self.active_item_index].item_id(); - let non_closeable_items = self.get_non_closeable_item_ids(action.close_pinned); + + let active_item_id = self.active_item_id(); + let pinned_item_ids = self.pinned_item_ids(); + Some(self.close_items_to_the_left_by_id( active_item_id, action, - non_closeable_items, + pinned_item_ids, window, cx, )) @@ -1328,19 +1338,19 @@ impl Pane { &mut self, item_id: EntityId, action: &CloseItemsToTheLeft, - non_closeable_items: HashSet, + pinned_item_ids: HashSet, window: &mut Window, cx: &mut Context, ) -> Task> { - let item_ids: Vec<_> = self - .items() - .take_while(|item| item.item_id() != item_id) - .map(|item| item.item_id()) - .collect(); + if self.items.is_empty() { + return Task::ready(Ok(())); + } + + let to_the_left_item_ids = self.to_the_left_item_ids(item_id); + self.close_items(window, cx, SaveIntent::Close, move |item_id| { - item_ids.contains(&item_id) - && !action.close_pinned - && !non_closeable_items.contains(&item_id) + to_the_left_item_ids.contains(&item_id) + && (action.close_pinned || !pinned_item_ids.contains(&item_id)) }) } @@ -1353,12 +1363,12 @@ impl Pane { if self.items.is_empty() { return None; } - let active_item_id = self.items[self.active_item_index].item_id(); - let non_closeable_items = self.get_non_closeable_item_ids(action.close_pinned); + let active_item_id = self.active_item_id(); + let pinned_item_ids = self.pinned_item_ids(); Some(self.close_items_to_the_right_by_id( active_item_id, action, - non_closeable_items, + pinned_item_ids, window, cx, )) @@ -1368,20 +1378,19 @@ impl Pane { &mut self, item_id: EntityId, action: &CloseItemsToTheRight, - non_closeable_items: HashSet, + pinned_item_ids: HashSet, window: &mut Window, cx: &mut Context, ) -> Task> { - let item_ids: Vec<_> = self - .items() - .rev() - .take_while(|item| item.item_id() != item_id) - .map(|item| item.item_id()) - .collect(); + if self.items.is_empty() { + return Task::ready(Ok(())); + } + + let to_the_right_item_ids = self.to_the_right_item_ids(item_id); + self.close_items(window, cx, SaveIntent::Close, move |item_id| { - item_ids.contains(&item_id) - && !action.close_pinned - && !non_closeable_items.contains(&item_id) + to_the_right_item_ids.contains(&item_id) + && (action.close_pinned || !pinned_item_ids.contains(&item_id)) }) } @@ -1390,18 +1399,19 @@ impl Pane { action: &CloseAllItems, window: &mut Window, cx: &mut Context, - ) -> Option>> { + ) -> Task> { if self.items.is_empty() { - return None; + return Task::ready(Ok(())); } - let non_closeable_items = self.get_non_closeable_item_ids(action.close_pinned); - Some(self.close_items( + let pinned_item_ids = self.pinned_item_ids(); + + self.close_items( window, cx, action.save_intent.unwrap_or(SaveIntent::Close), - |item_id| !non_closeable_items.contains(&item_id), - )) + |item_id| action.close_pinned || !pinned_item_ids.contains(&item_id), + ) } pub fn close_items_over_max_tabs(&mut self, window: &mut Window, cx: &mut Context) { @@ -1498,7 +1508,7 @@ impl Pane { } pub fn close_items( - &mut self, + &self, window: &mut Window, cx: &mut Context, mut save_intent: SaveIntent, @@ -2383,14 +2393,32 @@ impl Pane { let pane = pane.clone(); let menu_context = menu_context.clone(); ContextMenu::build(window, cx, move |mut menu, window, cx| { + let close_active_item_action = CloseActiveItem { + save_intent: None, + close_pinned: true, + }; + let close_inactive_items_action = CloseInactiveItems { + save_intent: None, + close_pinned: false, + }; + let close_items_to_the_left_action = CloseItemsToTheLeft { + close_pinned: false, + }; + let close_items_to_the_right_action = CloseItemsToTheRight { + close_pinned: false, + }; + let close_clean_items_action = CloseCleanItems { + close_pinned: false, + }; + let close_all_items_action = CloseAllItems { + save_intent: None, + close_pinned: false, + }; if let Some(pane) = pane.upgrade() { menu = menu .entry( "Close", - Some(Box::new(CloseActiveItem { - save_intent: None, - close_pinned: true, - })), + Some(Box::new(close_active_item_action)), window.handler_for(&pane, move |pane, window, cx| { pane.close_item_by_id(item_id, SaveIntent::Close, window, cx) .detach_and_log_err(cx); @@ -2398,34 +2426,27 @@ impl Pane { ) .item(ContextMenuItem::Entry( ContextMenuEntry::new("Close Others") - .action(Box::new(CloseInactiveItems { - save_intent: None, - close_pinned: false, - })) + .action(Box::new(close_inactive_items_action.clone())) .disabled(total_items == 1) .handler(window.handler_for(&pane, move |pane, window, cx| { - let non_closeable_ids = - pane.get_non_closeable_item_ids(false); - pane.close_items(window, cx, SaveIntent::Close, |id| { - id != item_id && !non_closeable_ids.contains(&id) - }) + pane.close_inactive_items( + &close_inactive_items_action, + window, + cx, + ) .detach_and_log_err(cx); })), )) .separator() .item(ContextMenuItem::Entry( ContextMenuEntry::new("Close Left") - .action(Box::new(CloseItemsToTheLeft { - close_pinned: false, - })) + .action(Box::new(close_items_to_the_left_action.clone())) .disabled(!has_items_to_left) .handler(window.handler_for(&pane, move |pane, window, cx| { pane.close_items_to_the_left_by_id( item_id, - &CloseItemsToTheLeft { - close_pinned: false, - }, - pane.get_non_closeable_item_ids(false), + &close_items_to_the_left_action, + pane.pinned_item_ids(), window, cx, ) @@ -2434,17 +2455,13 @@ impl Pane { )) .item(ContextMenuItem::Entry( ContextMenuEntry::new("Close Right") - .action(Box::new(CloseItemsToTheRight { - close_pinned: false, - })) + .action(Box::new(close_items_to_the_right_action.clone())) .disabled(!has_items_to_right) .handler(window.handler_for(&pane, move |pane, window, cx| { pane.close_items_to_the_right_by_id( item_id, - &CloseItemsToTheRight { - close_pinned: false, - }, - pane.get_non_closeable_item_ids(false), + &close_items_to_the_right_action, + pane.pinned_item_ids(), window, cx, ) @@ -2454,38 +2471,18 @@ impl Pane { .separator() .entry( "Close Clean", - Some(Box::new(CloseCleanItems { - close_pinned: false, - })), + Some(Box::new(close_clean_items_action.clone())), window.handler_for(&pane, move |pane, window, cx| { - if let Some(task) = pane.close_clean_items( - &CloseCleanItems { - close_pinned: false, - }, - window, - cx, - ) { - task.detach_and_log_err(cx) - } + pane.close_clean_items(&close_clean_items_action, window, cx) + .detach_and_log_err(cx) }), ) .entry( "Close All", - Some(Box::new(CloseAllItems { - save_intent: None, - close_pinned: false, - })), - window.handler_for(&pane, |pane, window, cx| { - if let Some(task) = pane.close_all_items( - &CloseAllItems { - save_intent: None, - close_pinned: false, - }, - window, - cx, - ) { - task.detach_and_log_err(cx) - } + Some(Box::new(close_all_items_action.clone())), + window.handler_for(&pane, move |pane, window, cx| { + pane.close_all_items(&close_all_items_action, window, cx) + .detach_and_log_err(cx) }), ); @@ -3087,16 +3084,44 @@ impl Pane { self.display_nav_history_buttons = display; } - fn get_non_closeable_item_ids(&self, close_pinned: bool) -> HashSet { - if close_pinned { - return HashSet::from_iter([]); - } - + fn pinned_item_ids(&self) -> HashSet { self.items .iter() .enumerate() - .filter(|(index, _item)| self.is_tab_pinned(*index)) - .map(|(_, item)| item.item_id()) + .filter_map(|(index, item)| { + if self.is_tab_pinned(index) { + return Some(item.item_id()); + } + + None + }) + .collect() + } + + fn clean_item_ids(&self, cx: &mut Context) -> HashSet { + self.items() + .filter_map(|item| { + if !item.is_dirty(cx) { + return Some(item.item_id()); + } + + None + }) + .collect() + } + + fn to_the_left_item_ids(&self, item_id: EntityId) -> HashSet { + self.items() + .take_while(|item| item.item_id() != item_id) + .map(|item| item.item_id()) + .collect() + } + + fn to_the_right_item_ids(&self, item_id: EntityId) -> HashSet { + self.items() + .rev() + .take_while(|item| item.item_id() != item_id) + .map(|item| item.item_id()) .collect() } @@ -3305,16 +3330,14 @@ impl Render for Pane { ) .on_action( cx.listener(|pane: &mut Self, action: &CloseInactiveItems, window, cx| { - if let Some(task) = pane.close_inactive_items(action, window, cx) { - task.detach_and_log_err(cx) - } + pane.close_inactive_items(action, window, cx) + .detach_and_log_err(cx); }), ) .on_action( cx.listener(|pane: &mut Self, action: &CloseCleanItems, window, cx| { - if let Some(task) = pane.close_clean_items(action, window, cx) { - task.detach_and_log_err(cx) - } + pane.close_clean_items(action, window, cx) + .detach_and_log_err(cx) }), ) .on_action(cx.listener( @@ -3333,9 +3356,8 @@ impl Render for Pane { )) .on_action( cx.listener(|pane: &mut Self, action: &CloseAllItems, window, cx| { - if let Some(task) = pane.close_all_items(action, window, cx) { - task.detach_and_log_err(cx) - } + pane.close_all_items(action, window, cx) + .detach_and_log_err(cx) }), ) .on_action( @@ -4413,7 +4435,6 @@ mod tests { cx, ) }) - .unwrap() .await .unwrap(); assert_item_labels(&pane, ["A!", "B!", "E*"], cx); @@ -4445,7 +4466,6 @@ mod tests { cx, ) }) - .unwrap() .await .unwrap(); assert_item_labels(&pane, ["A^", "C*^"], cx); @@ -4532,7 +4552,6 @@ mod tests { cx, ) }) - .unwrap() .await .unwrap(); assert_item_labels(&pane, ["A*!"], cx); @@ -4549,7 +4568,6 @@ mod tests { cx, ) }) - .unwrap() .await .unwrap(); @@ -4569,18 +4587,16 @@ mod tests { }); assert_item_labels(&pane, ["A^", "B^", "C*^"], cx); - let save = pane - .update_in(cx, |pane, window, cx| { - pane.close_all_items( - &CloseAllItems { - save_intent: None, - close_pinned: false, - }, - window, - cx, - ) - }) - .unwrap(); + let save = pane.update_in(cx, |pane, window, cx| { + pane.close_all_items( + &CloseAllItems { + save_intent: None, + close_pinned: false, + }, + window, + cx, + ) + }); cx.executor().run_until_parked(); cx.simulate_prompt_answer("Save all"); @@ -4591,18 +4607,16 @@ mod tests { add_labeled_item(&pane, "B", true, cx); add_labeled_item(&pane, "C", true, cx); assert_item_labels(&pane, ["A^", "B^", "C*^"], cx); - let save = pane - .update_in(cx, |pane, window, cx| { - pane.close_all_items( - &CloseAllItems { - save_intent: None, - close_pinned: false, - }, - window, - cx, - ) - }) - .unwrap(); + let save = pane.update_in(cx, |pane, window, cx| { + pane.close_all_items( + &CloseAllItems { + save_intent: None, + close_pinned: false, + }, + window, + cx, + ) + }); cx.executor().run_until_parked(); cx.simulate_prompt_answer("Discard all"); @@ -4642,7 +4656,6 @@ mod tests { cx, ) }) - .unwrap() .await .unwrap(); @@ -4681,7 +4694,6 @@ mod tests { cx, ) }) - .unwrap() .await .unwrap(); assert_item_labels(&pane, [], cx); diff --git a/crates/workspace/src/workspace.rs b/crates/workspace/src/workspace.rs index e30222dab4dc8d33ff4de9e5be2772ad81bbd956..abdac04ba05fb4527cf00f52f064a9ba73a53906 100644 --- a/crates/workspace/src/workspace.rs +++ b/crates/workspace/src/workspace.rs @@ -2658,7 +2658,7 @@ impl Workspace { let mut tasks = Vec::new(); if retain_active_pane { - if let Some(current_pane_close) = current_pane.update(cx, |pane, cx| { + let current_pane_close = current_pane.update(cx, |pane, cx| { pane.close_inactive_items( &CloseInactiveItems { save_intent: None, @@ -2667,9 +2667,9 @@ impl Workspace { window, cx, ) - }) { - tasks.push(current_pane_close); - }; + }); + + tasks.push(current_pane_close); } for pane in self.panes() { @@ -2677,7 +2677,7 @@ impl Workspace { continue; } - if let Some(close_pane_items) = pane.update(cx, |pane: &mut Pane, cx| { + let close_pane_items = pane.update(cx, |pane: &mut Pane, cx| { pane.close_all_items( &CloseAllItems { save_intent: Some(save_intent), @@ -2686,9 +2686,9 @@ impl Workspace { window, cx, ) - }) { - tasks.push(close_pane_items) - } + }); + + tasks.push(close_pane_items) } if tasks.is_empty() { @@ -8082,7 +8082,7 @@ mod tests { assert!(!msg.contains("4.txt")); cx.simulate_prompt_answer("Cancel"); - close.await.unwrap(); + close.await; left_pane .update_in(cx, |left_pane, window, cx| { @@ -8114,7 +8114,7 @@ mod tests { cx.simulate_prompt_answer("Save all"); cx.executor().run_until_parked(); - close.await.unwrap(); + close.await; right_pane.read_with(cx, |pane, _| { assert_eq!(pane.items_len(), 0); }); @@ -9040,18 +9040,16 @@ mod tests { "Should select the multi buffer in the pane" ); }); - let close_all_but_multi_buffer_task = pane - .update_in(cx, |pane, window, cx| { - pane.close_inactive_items( - &CloseInactiveItems { - save_intent: Some(SaveIntent::Save), - close_pinned: true, - }, - window, - cx, - ) - }) - .expect("should have inactive files to close"); + let close_all_but_multi_buffer_task = pane.update_in(cx, |pane, window, cx| { + pane.close_inactive_items( + &CloseInactiveItems { + save_intent: Some(SaveIntent::Save), + close_pinned: true, + }, + window, + cx, + ) + }); cx.background_executor.run_until_parked(); assert!(!cx.has_pending_prompt()); close_all_but_multi_buffer_task