Make `pane::CloseAllItems` best effort (#45368)

Ben Kunkle created

Closes #ISSUE

Release Notes:

- Fixed an issue where the `pane: close all items` action would give up
if you hit "Cancel" on the prompt for what to do with a dirty buffer

Change summary

crates/workspace/src/pane.rs      | 81 ++++++++++++++++++++++++++++----
crates/workspace/src/workspace.rs | 26 ++++++----
2 files changed, 85 insertions(+), 22 deletions(-)

Detailed changes

crates/workspace/src/pane.rs 🔗

@@ -1846,6 +1846,7 @@ impl Pane {
             }
 
             for item_to_close in items_to_close {
+                let mut should_close = true;
                 let mut should_save = true;
                 if save_intent == SaveIntent::Close {
                     workspace.update(cx, |workspace, cx| {
@@ -1861,7 +1862,7 @@ impl Pane {
                     {
                         Ok(success) => {
                             if !success {
-                                break;
+                                should_close = false;
                             }
                         }
                         Err(err) => {
@@ -1880,23 +1881,25 @@ impl Pane {
                             })?;
                             match answer.await {
                                 Ok(0) => {}
-                                Ok(1..) | Err(_) => break,
+                                Ok(1..) | Err(_) => should_close = false,
                             }
                         }
                     }
                 }
 
                 // Remove the item from the pane.
-                pane.update_in(cx, |pane, window, cx| {
-                    pane.remove_item(
-                        item_to_close.item_id(),
-                        false,
-                        pane.close_pane_if_empty,
-                        window,
-                        cx,
-                    );
-                })
-                .ok();
+                if should_close {
+                    pane.update_in(cx, |pane, window, cx| {
+                        pane.remove_item(
+                            item_to_close.item_id(),
+                            false,
+                            pane.close_pane_if_empty,
+                            window,
+                            cx,
+                        );
+                    })
+                    .ok();
+                }
             }
 
             pane.update(cx, |_, cx| cx.notify()).ok();
@@ -6614,6 +6617,60 @@ mod tests {
         cx.simulate_prompt_answer("Discard all");
         save.await.unwrap();
         assert_item_labels(&pane, [], cx);
+
+        add_labeled_item(&pane, "A", true, cx).update(cx, |item, cx| {
+            item.project_items
+                .push(TestProjectItem::new_dirty(1, "A.txt", cx))
+        });
+        add_labeled_item(&pane, "B", true, cx).update(cx, |item, cx| {
+            item.project_items
+                .push(TestProjectItem::new_dirty(2, "B.txt", cx))
+        });
+        add_labeled_item(&pane, "C", true, cx).update(cx, |item, cx| {
+            item.project_items
+                .push(TestProjectItem::new_dirty(3, "C.txt", cx))
+        });
+        assert_item_labels(&pane, ["A^", "B^", "C*^"], cx);
+
+        let close_task = 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");
+        close_task.await.unwrap();
+        assert_item_labels(&pane, [], cx);
+
+        add_labeled_item(&pane, "Clean1", false, cx);
+        add_labeled_item(&pane, "Dirty", true, cx).update(cx, |item, cx| {
+            item.project_items
+                .push(TestProjectItem::new_dirty(1, "Dirty.txt", cx))
+        });
+        add_labeled_item(&pane, "Clean2", false, cx);
+        assert_item_labels(&pane, ["Clean1", "Dirty^", "Clean2*"], cx);
+
+        let close_task = 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("Cancel");
+        close_task.await.unwrap();
+        assert_item_labels(&pane, ["Dirty*^"], cx);
     }
 
     #[gpui::test]

crates/workspace/src/workspace.rs 🔗

@@ -9424,7 +9424,7 @@ mod tests {
         let right_pane = right_pane.await.unwrap();
         cx.focus(&right_pane);
 
-        let mut close = right_pane.update_in(cx, |pane, window, cx| {
+        let close = right_pane.update_in(cx, |pane, window, cx| {
             pane.close_all_items(&CloseAllItems::default(), window, cx)
                 .unwrap()
         });
@@ -9436,9 +9436,16 @@ mod tests {
         assert!(!msg.contains("3.txt"));
         assert!(!msg.contains("4.txt"));
 
+        // With best-effort close, cancelling item 1 keeps it open but items 4
+        // and (3,4) still close since their entries exist in left pane.
         cx.simulate_prompt_answer("Cancel");
         close.await;
 
+        right_pane.read_with(cx, |pane, _| {
+            assert_eq!(pane.items_len(), 1);
+        });
+
+        // Remove item 3 from left pane, making (2,3) the only item with entry 3.
         left_pane
             .update_in(cx, |left_pane, window, cx| {
                 left_pane.close_item_by_id(
@@ -9451,26 +9458,25 @@ mod tests {
             .await
             .unwrap();
 
-        close = right_pane.update_in(cx, |pane, window, cx| {
+        let close = left_pane.update_in(cx, |pane, window, cx| {
             pane.close_all_items(&CloseAllItems::default(), window, cx)
                 .unwrap()
         });
         cx.executor().run_until_parked();
 
         let details = cx.pending_prompt().unwrap().1;
-        assert!(details.contains("1.txt"));
-        assert!(!details.contains("2.txt"));
+        assert!(details.contains("0.txt"));
         assert!(details.contains("3.txt"));
-        // ideally this assertion could be made, but today we can only
-        // save whole items not project items, so the orphaned item 3 causes
-        // 4 to be saved too.
-        // assert!(!details.contains("4.txt"));
+        assert!(details.contains("4.txt"));
+        // Ideally 2.txt wouldn't appear since entry 2 still exists in item 2.
+        // But we can only save whole items, so saving (2,3) for entry 3 includes 2.
+        // assert!(!details.contains("2.txt"));
 
         cx.simulate_prompt_answer("Save all");
-
         cx.executor().run_until_parked();
         close.await;
-        right_pane.read_with(cx, |pane, _| {
+
+        left_pane.read_with(cx, |pane, _| {
             assert_eq!(pane.items_len(), 0);
         });
     }