Add tests for Pane::add_item

K Simmons created

Change summary

crates/workspace/src/pane.rs      | 328 +++++++++++++++++++++++++++++---
crates/workspace/src/workspace.rs |  22 +
2 files changed, 306 insertions(+), 44 deletions(-)

Detailed changes

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>,
+        pane: &ViewHandle<Pane>,
         item: Box<dyn ItemHandle>,
         activate_pane: bool,
         focus_item: bool,
@@ -463,69 +463,74 @@ impl Pane {
         cx: &mut ViewContext<Workspace>,
     ) {
         // 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<Pane>,
-        item: Box<dyn ItemHandle>,
-        activate_pane: bool,
-        focus_item: bool,
-        cx: &mut ViewContext<Workspace>,
-    ) {
-        Self::add_item_at(workspace, pane, item, activate_pane, focus_item, None, cx)
-    }
-
     pub fn items(&self) -> impl Iterator<Item = &Box<dyn ItemHandle>> {
         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<const COUNT: usize>(
+        workspace: &ViewHandle<Workspace>,
+        pane: &ViewHandle<Pane>,
+        labels: [&str; COUNT],
+        cx: &mut TestAppContext,
+    ) -> [Box<ViewHandle<TestItem>>; 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<const COUNT: usize>(
+        pane: &ViewHandle<Pane>,
+        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::<TestItem>()
+                        .unwrap()
+                        .read(cx)
+                        .label
+                        .clone();
+                    if ix == pane.active_item_index {
+                        state.push('*');
+                    }
+                    state
+                })
+                .collect::<Vec<_>>();
+
+            assert_eq!(
+                actual_states, expected_states,
+                "pane items do not match expectation"
+            );
+        })
+    }
+}

crates/workspace/src/workspace.rs 🔗

@@ -1448,8 +1448,8 @@ impl Workspace {
     }
 
     pub fn add_item(&mut self, item: Box<dyn ItemHandle>, cx: &mut ViewContext<Self>) {
-        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<Option<usize>>,
     }
 
-    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>) {
             self.push_to_nav_history(cx);
             self.state = state;