Joseph T. Lyons created
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/Acrates/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(-)
@@ -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();
});
});
@@ -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();
@@ -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<Point<Pixels>> {
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<Self>,
- ) -> Option<Task<Result<()>>> {
+ ) -> Task<Result<()>> {
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<Self>,
- ) -> Option<Task<Result<()>>> {
- 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<Result<()>> {
+ 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<EntityId>,
+ pinned_item_ids: HashSet<EntityId>,
window: &mut Window,
cx: &mut Context<Self>,
) -> Task<Result<()>> {
- 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<EntityId>,
+ pinned_item_ids: HashSet<EntityId>,
window: &mut Window,
cx: &mut Context<Self>,
) -> Task<Result<()>> {
- 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<Self>,
- ) -> Option<Task<Result<()>>> {
+ ) -> Task<Result<()>> {
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<Self>) {
@@ -1498,7 +1508,7 @@ impl Pane {
}
pub fn close_items(
- &mut self,
+ &self,
window: &mut Window,
cx: &mut Context<Pane>,
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<EntityId> {
- if close_pinned {
- return HashSet::from_iter([]);
- }
-
+ fn pinned_item_ids(&self) -> HashSet<EntityId> {
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<Pane>) -> HashSet<EntityId> {
+ 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<EntityId> {
+ 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<EntityId> {
+ 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);
@@ -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