diff --git a/crates/workspace/src/pane.rs b/crates/workspace/src/pane.rs index 7852ad6868..d8e1ea8e2f 100644 --- a/crates/workspace/src/pane.rs +++ b/crates/workspace/src/pane.rs @@ -449,13 +449,13 @@ impl Pane { // Even if the item exists, we re-add it to reorder it after the active item. // We may revisit this behavior after adding an "activation history" for pane items. let item = existing_item.unwrap_or_else(|| pane.update(cx, |_, cx| build_item(cx))); - Pane::add_item(workspace, pane, item.clone(), true, focus_item, cx); + Pane::add_item(workspace, &pane, item.clone(), true, focus_item, None, cx); item } - pub fn add_item_at( + pub fn add_item( workspace: &mut Workspace, - pane: ViewHandle, + pane: &ViewHandle, item: Box, activate_pane: bool, focus_item: bool, @@ -463,69 +463,74 @@ impl Pane { cx: &mut ViewContext, ) { // If no destination index is specified, add or move the item after the active item. - let mut destination_index = if let Some(destination_index) = destination_index { - destination_index - } else { + let mut insertion_index = { let pane = pane.read(cx); - cmp::min(pane.active_item_index + 1, pane.items.len()) + cmp::min( + if let Some(destination_index) = destination_index { + destination_index + } else { + pane.active_item_index + 1 + }, + pane.items.len(), + ) }; // Does the item already exist? - if let Some(existing_item_index) = pane.read(cx).items.iter().position(|i| { - i.id() == item.id() || i.project_entry_ids(cx) == item.project_entry_ids(cx) + if let Some(existing_item_index) = pane.read(cx).items.iter().position(|existing_item| { + let existing_item_entry_ids = existing_item.project_entry_ids(cx); + let added_item_entry_ids = item.project_entry_ids(cx); + let entries_match = !existing_item_entry_ids.is_empty() + && existing_item_entry_ids == added_item_entry_ids; + + existing_item.id() == item.id() || entries_match }) { // If the item already exists, move it to the desired destination and activate it pane.update(cx, |pane, cx| { - if existing_item_index != destination_index { + if existing_item_index != insertion_index { cx.reparent(&item); let existing_item_is_active = existing_item_index == pane.active_item_index; - pane.items.remove(existing_item_index); - if existing_item_index < pane.active_item_index { - pane.active_item_index -= 1; - } - destination_index = destination_index.min(pane.items.len()); + // If the caller didn't specify a destination and the added item is already + // the active one, don't move it + if existing_item_is_active && destination_index.is_none() { + insertion_index = existing_item_index; + } else { + pane.items.remove(existing_item_index); + if existing_item_index < pane.active_item_index { + pane.active_item_index -= 1; + } + insertion_index = insertion_index.min(pane.items.len()); - pane.items.insert(destination_index, item.clone()); + pane.items.insert(insertion_index, item.clone()); - if existing_item_is_active { - pane.active_item_index = destination_index; - } else if destination_index <= pane.active_item_index { - pane.active_item_index += 1; + if existing_item_is_active { + pane.active_item_index = insertion_index; + } else if insertion_index <= pane.active_item_index { + pane.active_item_index += 1; + } } cx.notify(); } - pane.activate_item(destination_index, activate_pane, focus_item, cx); + pane.activate_item(insertion_index, activate_pane, focus_item, cx); }); } else { // If the item doesn't already exist, add it and activate it item.added_to_pane(workspace, pane.clone(), cx); pane.update(cx, |pane, cx| { cx.reparent(&item); - pane.items.insert(destination_index, item); - if destination_index <= pane.active_item_index { + pane.items.insert(insertion_index, item); + if insertion_index <= pane.active_item_index { pane.active_item_index += 1; } - pane.activate_item(destination_index, activate_pane, focus_item, cx); + pane.activate_item(insertion_index, activate_pane, focus_item, cx); cx.notify(); }); } } - pub(crate) fn add_item( - workspace: &mut Workspace, - pane: ViewHandle, - item: Box, - activate_pane: bool, - focus_item: bool, - cx: &mut ViewContext, - ) { - Self::add_item_at(workspace, pane, item, activate_pane, focus_item, None, cx) - } - pub fn items(&self) -> impl Iterator> { self.items.iter() } @@ -913,9 +918,9 @@ impl Pane { .expect("Tried to move item handle which was not in from pane"); // This automatically removes duplicate items in the pane - Pane::add_item_at( + Pane::add_item( workspace, - to.clone(), + &to, item_handle.clone(), true, true, @@ -1527,3 +1532,252 @@ impl NavHistory { } } } + +#[cfg(test)] +mod tests { + use gpui::TestAppContext; + use project::FakeFs; + + use crate::tests::TestItem; + + use super::*; + + #[gpui::test] + async fn test_add_item_with_new_item(cx: &mut TestAppContext) { + cx.foreground().forbid_parking(); + Settings::test_async(cx); + let fs = FakeFs::new(cx.background()); + + let project = Project::test(fs, None, cx).await; + let (_, workspace) = cx.add_window(|cx| Workspace::new(project, cx)); + let pane = workspace.read_with(cx, |workspace, _| workspace.active_pane().clone()); + + // 1. Add with a destination index + // a. Add before the active item + set_labeled_items(&workspace, &pane, ["A", "B*", "C"], cx); + workspace.update(cx, |workspace, cx| { + Pane::add_item( + workspace, + &pane, + Box::new(cx.add_view(|_| TestItem::new().with_label("D"))), + false, + false, + Some(0), + cx, + ); + }); + assert_item_labels(&pane, ["D*", "A", "B", "C"], cx); + + // b. Add after the active item + set_labeled_items(&workspace, &pane, ["A", "B*", "C"], cx); + workspace.update(cx, |workspace, cx| { + Pane::add_item( + workspace, + &pane, + Box::new(cx.add_view(|_| TestItem::new().with_label("D"))), + false, + false, + Some(2), + cx, + ); + }); + assert_item_labels(&pane, ["A", "B", "D*", "C"], cx); + + // c. Add at the end of the item list (including off the length) + set_labeled_items(&workspace, &pane, ["A", "B*", "C"], cx); + workspace.update(cx, |workspace, cx| { + Pane::add_item( + workspace, + &pane, + Box::new(cx.add_view(|_| TestItem::new().with_label("D"))), + false, + false, + Some(5), + cx, + ); + }); + assert_item_labels(&pane, ["A", "B", "C", "D*"], cx); + + // 2. Add without a destination index + // a. Add with active item at the start of the item list + set_labeled_items(&workspace, &pane, ["A*", "B", "C"], cx); + workspace.update(cx, |workspace, cx| { + Pane::add_item( + workspace, + &pane, + Box::new(cx.add_view(|_| TestItem::new().with_label("D"))), + false, + false, + None, + cx, + ); + }); + set_labeled_items(&workspace, &pane, ["A", "D*", "B", "C"], cx); + + // b. Add with active item at the end of the item list + set_labeled_items(&workspace, &pane, ["A", "B", "C*"], cx); + workspace.update(cx, |workspace, cx| { + Pane::add_item( + workspace, + &pane, + Box::new(cx.add_view(|_| TestItem::new().with_label("D"))), + false, + false, + None, + cx, + ); + }); + assert_item_labels(&pane, ["A", "B", "C", "D*"], cx); + } + + #[gpui::test] + async fn test_add_item_with_existing_item(cx: &mut TestAppContext) { + cx.foreground().forbid_parking(); + Settings::test_async(cx); + let fs = FakeFs::new(cx.background()); + + let project = Project::test(fs, None, cx).await; + let (_, workspace) = cx.add_window(|cx| Workspace::new(project, cx)); + let pane = workspace.read_with(cx, |workspace, _| workspace.active_pane().clone()); + + // 1. Add with a destination index + // 1a. Add before the active item + let [_, _, _, d] = set_labeled_items(&workspace, &pane, ["A", "B*", "C", "D"], cx); + workspace.update(cx, |workspace, cx| { + Pane::add_item(workspace, &pane, d, false, false, Some(0), cx); + }); + assert_item_labels(&pane, ["D*", "A", "B", "C"], cx); + + // 1b. Add after the active item + let [_, _, _, d] = set_labeled_items(&workspace, &pane, ["A", "B*", "C", "D"], cx); + workspace.update(cx, |workspace, cx| { + Pane::add_item(workspace, &pane, d, false, false, Some(2), cx); + }); + assert_item_labels(&pane, ["A", "B", "D*", "C"], cx); + + // 1c. Add at the end of the item list (including off the length) + let [a, _, _, _] = set_labeled_items(&workspace, &pane, ["A", "B*", "C", "D"], cx); + workspace.update(cx, |workspace, cx| { + Pane::add_item(workspace, &pane, a, false, false, Some(5), cx); + }); + assert_item_labels(&pane, ["B", "C", "D", "A*"], cx); + + // 1d. Add same item to active index + let [_, b, _] = set_labeled_items(&workspace, &pane, ["A", "B*", "C"], cx); + workspace.update(cx, |workspace, cx| { + Pane::add_item(workspace, &pane, b, false, false, Some(1), cx); + }); + assert_item_labels(&pane, ["A", "B*", "C"], cx); + + // 1e. Add item to index after same item in last position + let [_, _, c] = set_labeled_items(&workspace, &pane, ["A", "B*", "C"], cx); + workspace.update(cx, |workspace, cx| { + Pane::add_item(workspace, &pane, c, false, false, Some(2), cx); + }); + assert_item_labels(&pane, ["A", "B", "C*"], cx); + + // 2. Add without a destination index + // 2a. Add with active item at the start of the item list + let [_, _, _, d] = set_labeled_items(&workspace, &pane, ["A*", "B", "C", "D"], cx); + workspace.update(cx, |workspace, cx| { + Pane::add_item(workspace, &pane, d, false, false, None, cx); + }); + assert_item_labels(&pane, ["A", "D*", "B", "C"], cx); + + // 2b. Add with active item at the end of the item list + let [a, _, _, _] = set_labeled_items(&workspace, &pane, ["A", "B", "C", "D*"], cx); + workspace.update(cx, |workspace, cx| { + Pane::add_item(workspace, &pane, a, false, false, None, cx); + }); + assert_item_labels(&pane, ["B", "C", "D", "A*"], cx); + + // 2c. Add active item to active item at end of list + let [_, _, c] = set_labeled_items(&workspace, &pane, ["A", "B", "C*"], cx); + workspace.update(cx, |workspace, cx| { + Pane::add_item(workspace, &pane, c, false, false, None, cx); + }); + assert_item_labels(&pane, ["A", "B", "C*"], cx); + + // 2d. Add active item to active item at start of list + let [a, _, _] = set_labeled_items(&workspace, &pane, ["A*", "B", "C"], cx); + workspace.update(cx, |workspace, cx| { + Pane::add_item(workspace, &pane, a, false, false, None, cx); + }); + assert_item_labels(&pane, ["A*", "B", "C"], cx); + } + + fn set_labeled_items( + workspace: &ViewHandle, + pane: &ViewHandle, + labels: [&str; COUNT], + cx: &mut TestAppContext, + ) -> [Box>; COUNT] { + pane.update(cx, |pane, _| { + pane.items.clear(); + }); + + workspace.update(cx, |workspace, cx| { + let mut active_item_index = 0; + + let mut index = 0; + let items = labels.map(|mut label| { + if label.ends_with("*") { + label = label.trim_end_matches("*"); + active_item_index = index; + } + + let labeled_item = Box::new(cx.add_view(|_| TestItem::new().with_label(label))); + Pane::add_item( + workspace, + pane, + labeled_item.clone(), + false, + false, + None, + cx, + ); + index += 1; + labeled_item + }); + + pane.update(cx, |pane, cx| { + pane.activate_item(active_item_index, false, false, cx) + }); + + items + }) + } + + // Assert the item label, with the active item label suffixed with a '*' + fn assert_item_labels( + pane: &ViewHandle, + expected_states: [&str; COUNT], + cx: &mut TestAppContext, + ) { + pane.read_with(cx, |pane, cx| { + let actual_states = pane + .items + .iter() + .enumerate() + .map(|(ix, item)| { + let mut state = item + .to_any() + .downcast::() + .unwrap() + .read(cx) + .label + .clone(); + if ix == pane.active_item_index { + state.push('*'); + } + state + }) + .collect::>(); + + assert_eq!( + actual_states, expected_states, + "pane items do not match expectation" + ); + }) + } +} diff --git a/crates/workspace/src/workspace.rs b/crates/workspace/src/workspace.rs index 2d2782eaa4..1261a89f95 100644 --- a/crates/workspace/src/workspace.rs +++ b/crates/workspace/src/workspace.rs @@ -1448,8 +1448,8 @@ impl Workspace { } pub fn add_item(&mut self, item: Box, cx: &mut ViewContext) { - let pane = self.active_pane().clone(); - Pane::add_item(self, pane, item, true, true, cx); + let active_pane = self.active_pane().clone(); + Pane::add_item(self, &active_pane, item, true, true, None, cx); } pub fn open_path( @@ -1649,7 +1649,7 @@ impl Workspace { pane.read(cx).active_item().map(|item| { let new_pane = self.add_pane(cx); if let Some(clone) = item.clone_on_split(cx.as_mut()) { - Pane::add_item(self, new_pane.clone(), clone, true, true, cx); + Pane::add_item(self, &new_pane, clone, true, true, None, cx); } self.center.split(&pane, &new_pane, direction).unwrap(); cx.notify(); @@ -2392,7 +2392,7 @@ impl Workspace { } for (pane, item) in items_to_add { - Pane::add_item(self, pane.clone(), item.boxed_clone(), false, false, cx); + Pane::add_item(self, &pane, item.boxed_clone(), false, false, None, cx); if pane == self.active_pane { pane.update(cx, |pane, cx| pane.focus_active_item(cx)); } @@ -3330,8 +3330,9 @@ mod tests { }); } - struct TestItem { + pub struct TestItem { state: String, + pub label: String, save_count: usize, save_as_count: usize, reload_count: usize, @@ -3345,7 +3346,7 @@ mod tests { tab_detail: Cell>, } - enum TestItemEvent { + pub enum TestItemEvent { Edit, } @@ -3353,6 +3354,7 @@ mod tests { fn clone(&self) -> Self { Self { state: self.state.clone(), + label: self.label.clone(), save_count: self.save_count, save_as_count: self.save_as_count, reload_count: self.reload_count, @@ -3369,9 +3371,10 @@ mod tests { } impl TestItem { - fn new() -> Self { + pub fn new() -> Self { Self { state: String::new(), + label: String::new(), save_count: 0, save_as_count: 0, reload_count: 0, @@ -3386,6 +3389,11 @@ mod tests { } } + pub fn with_label(mut self, state: &str) -> Self { + self.label = state.to_string(); + self + } + fn set_state(&mut self, state: String, cx: &mut ViewContext) { self.push_to_nav_history(cx); self.state = state;