Merge pull request #2436 from zed-industries/close-window-end-call

Antonio Scandurra created

Move methods querying window state into `AsyncAppContext`

Change summary

crates/collab_ui/src/project_shared_notification.rs |  4 
crates/collab_ui/src/sharing_status_indicator.rs    |  4 
crates/copilot/src/sign_in.rs                       | 15 +-
crates/gpui/src/app.rs                              | 67 ++++-----
crates/gpui/src/app/test_app_context.rs             |  6 
crates/gpui/src/test.rs                             |  2 
crates/gpui_macros/src/gpui_macros.rs               |  4 
crates/workspace/src/workspace.rs                   | 94 +++++++-------
crates/zed/src/zed.rs                               | 33 ++--
9 files changed, 116 insertions(+), 113 deletions(-)

Detailed changes

crates/collab_ui/src/project_shared_notification.rs 🔗

@@ -58,14 +58,14 @@ pub fn init(app_state: &Arc<AppState>, cx: &mut AppContext) {
         room::Event::RemoteProjectUnshared { project_id } => {
             if let Some(window_ids) = notification_windows.remove(&project_id) {
                 for window_id in window_ids {
-                    cx.remove_window(window_id);
+                    cx.update_window(window_id, |cx| cx.remove_window());
                 }
             }
         }
         room::Event::Left => {
             for (_, window_ids) in notification_windows.drain() {
                 for window_id in window_ids {
-                    cx.remove_window(window_id);
+                    cx.update_window(window_id, |cx| cx.remove_window());
                 }
             }
         }

crates/collab_ui/src/sharing_status_indicator.rs 🔗

@@ -19,10 +19,10 @@ pub fn init(cx: &mut AppContext) {
                     status_indicator = Some(cx.add_status_bar_item(|_| SharingStatusIndicator));
                 }
             } else if let Some((window_id, _)) = status_indicator.take() {
-                cx.remove_status_bar_item(window_id);
+                cx.update_window(window_id, |cx| cx.remove_window());
             }
         } else if let Some((window_id, _)) = status_indicator.take() {
-            cx.remove_status_bar_item(window_id);
+            cx.update_window(window_id, |cx| cx.remove_window());
         }
     })
     .detach();

crates/copilot/src/sign_in.rs 🔗

@@ -27,14 +27,13 @@ pub fn init(cx: &mut AppContext) {
                 crate::Status::SigningIn { prompt } => {
                     if let Some(code_verification_handle) = code_verification.as_mut() {
                         let window_id = code_verification_handle.window_id();
-                        if cx.has_window(window_id) {
-                            cx.update_window(window_id, |cx| {
-                                code_verification_handle.update(cx, |code_verification, cx| {
-                                    code_verification.set_status(status, cx)
-                                });
-                                cx.activate_window();
+                        let updated = cx.update_window(window_id, |cx| {
+                            code_verification_handle.update(cx, |code_verification, cx| {
+                                code_verification.set_status(status.clone(), cx)
                             });
-                        } else {
+                            cx.activate_window();
+                        });
+                        if updated.is_none() {
                             code_verification = Some(create_copilot_auth_window(cx, &status));
                         }
                     } else if let Some(_prompt) = prompt {
@@ -56,7 +55,7 @@ pub fn init(cx: &mut AppContext) {
                 }
                 _ => {
                     if let Some(code_verification) = code_verification.take() {
-                        cx.remove_window(code_verification.window_id());
+                        cx.update_window(code_verification.window_id(), |cx| cx.remove_window());
                     }
                 }
             }

crates/gpui/src/app.rs 🔗

@@ -302,6 +302,14 @@ impl AsyncAppContext {
         self.0.borrow_mut().update(callback)
     }
 
+    pub fn read_window<T, F: FnOnce(&WindowContext) -> T>(
+        &self,
+        window_id: usize,
+        callback: F,
+    ) -> Option<T> {
+        self.0.borrow_mut().read_window(window_id, callback)
+    }
+
     pub fn update_window<T, F: FnOnce(&mut WindowContext) -> T>(
         &mut self,
         window_id: usize,
@@ -332,6 +340,22 @@ impl AsyncAppContext {
             .ok_or_else(|| anyhow!("window not found"))
     }
 
+    pub fn has_window(&self, window_id: usize) -> bool {
+        self.read(|cx| cx.windows.contains_key(&window_id))
+    }
+
+    pub fn window_is_active(&self, window_id: usize) -> bool {
+        self.read(|cx| cx.windows.get(&window_id).map_or(false, |w| w.is_active))
+    }
+
+    pub fn root_view(&self, window_id: usize) -> Option<AnyViewHandle> {
+        self.read(|cx| cx.windows.get(&window_id).map(|w| w.root_view().clone()))
+    }
+
+    pub fn window_ids(&self) -> Vec<usize> {
+        self.read(|cx| cx.windows.keys().copied().collect())
+    }
+
     pub fn add_model<T, F>(&mut self, build_model: F) -> ModelHandle<T>
     where
         T: Entity,
@@ -353,7 +377,7 @@ impl AsyncAppContext {
     }
 
     pub fn remove_window(&mut self, window_id: usize) {
-        self.update(|cx| cx.remove_window(window_id))
+        self.update_window(window_id, |cx| cx.remove_window());
     }
 
     pub fn activate_window(&mut self, window_id: usize) {
@@ -552,7 +576,7 @@ impl AppContext {
         App(self.weak_self.as_ref().unwrap().upgrade().unwrap())
     }
 
-    pub fn quit(&mut self) {
+    fn quit(&mut self) {
         let mut futures = Vec::new();
 
         self.update(|cx| {
@@ -569,7 +593,8 @@ impl AppContext {
             }
         });
 
-        self.remove_all_windows();
+        self.windows.clear();
+        self.flush_effects();
 
         let futures = futures::future::join_all(futures);
         if self
@@ -581,11 +606,6 @@ impl AppContext {
         }
     }
 
-    pub fn remove_all_windows(&mut self) {
-        self.windows.clear();
-        self.flush_effects();
-    }
-
     pub fn foreground(&self) -> &Rc<executor::Foreground> {
         &self.foreground
     }
@@ -702,24 +722,6 @@ impl AppContext {
         }
     }
 
-    pub fn has_window(&self, window_id: usize) -> bool {
-        self.window_ids()
-            .find(|window| window == &window_id)
-            .is_some()
-    }
-
-    pub fn window_is_active(&self, window_id: usize) -> bool {
-        self.windows.get(&window_id).map_or(false, |w| w.is_active)
-    }
-
-    pub fn root_view(&self, window_id: usize) -> Option<&AnyViewHandle> {
-        self.windows.get(&window_id).map(|w| w.root_view())
-    }
-
-    pub fn window_ids(&self) -> impl Iterator<Item = usize> + '_ {
-        self.windows.keys().copied()
-    }
-
     pub fn view_ui_name(&self, window_id: usize, view_id: usize) -> Option<&'static str> {
         Some(self.views.get(&(window_id, view_id))?.ui_name())
     }
@@ -1285,15 +1287,6 @@ impl AppContext {
         })
     }
 
-    pub fn remove_status_bar_item(&mut self, id: usize) {
-        self.remove_window(id);
-    }
-
-    pub fn remove_window(&mut self, window_id: usize) {
-        self.windows.remove(&window_id);
-        self.flush_effects();
-    }
-
     pub fn build_window<V, F>(
         &mut self,
         window_id: usize,
@@ -1352,7 +1345,7 @@ impl AppContext {
         {
             let mut app = self.upgrade();
             platform_window.on_close(Box::new(move || {
-                app.update(|cx| cx.remove_window(window_id));
+                app.update(|cx| cx.update_window(window_id, |cx| cx.remove_window()));
             }));
         }
 
@@ -4663,7 +4656,7 @@ mod tests {
         assert!(model_release_observed.get());
 
         drop(view);
-        cx.remove_window(window_id);
+        cx.update_window(window_id, |cx| cx.remove_window());
         assert!(view_released.get());
         assert!(view_release_observed.get());
     }

crates/gpui/src/app/test_app_context.rs 🔗

@@ -182,7 +182,11 @@ impl TestAppContext {
     }
 
     pub fn window_ids(&self) -> Vec<usize> {
-        self.cx.borrow().window_ids().collect()
+        self.cx.borrow().windows.keys().copied().collect()
+    }
+
+    pub fn remove_all_windows(&mut self) {
+        self.update(|cx| cx.windows.clear());
     }
 
     pub fn read<T, F: FnOnce(&AppContext) -> T>(&self, callback: F) -> T {

crates/gpui/src/test.rs 🔗

@@ -100,7 +100,7 @@ pub fn run_test(
                     test_fn(cx, foreground_platform.clone(), deterministic.clone(), seed);
                 });
 
-                cx.update(|cx| cx.remove_all_windows());
+                cx.remove_all_windows();
                 deterministic.run_until_parked();
                 cx.update(|cx| cx.clear_globals());
 

crates/gpui_macros/src/gpui_macros.rs 🔗

@@ -137,7 +137,7 @@ pub fn test(args: TokenStream, function: TokenStream) -> TokenStream {
                                 );
                             ));
                             cx_teardowns.extend(quote!(
-                                #cx_varname.update(|cx| cx.remove_all_windows());
+                                #cx_varname.remove_all_windows();
                                 deterministic.run_until_parked();
                                 #cx_varname.update(|cx| cx.clear_globals());
                             ));
@@ -212,7 +212,7 @@ pub fn test(args: TokenStream, function: TokenStream) -> TokenStream {
                                     );
                                 ));
                                 cx_teardowns.extend(quote!(
-                                    #cx_varname.update(|cx| cx.remove_all_windows());
+                                    #cx_varname.remove_all_windows();
                                     deterministic.run_until_parked();
                                     #cx_varname.update(|cx| cx.clear_globals());
                                 ));

crates/workspace/src/workspace.rs 🔗

@@ -918,12 +918,18 @@ impl Workspace {
     }
 
     pub fn close_global(_: &CloseWindow, cx: &mut AppContext) {
-        let id = cx.window_ids().find(|&id| cx.window_is_active(id));
-        if let Some(id) = id {
-            //This can only get called when the window's project connection has been lost
-            //so we don't need to prompt the user for anything and instead just close the window
-            cx.remove_window(id);
-        }
+        cx.spawn(|mut cx| async move {
+            let id = cx
+                .window_ids()
+                .into_iter()
+                .find(|&id| cx.window_is_active(id));
+            if let Some(id) = id {
+                //This can only get called when the window's project connection has been lost
+                //so we don't need to prompt the user for anything and instead just close the window
+                cx.remove_window(id);
+            }
+        })
+        .detach();
     }
 
     pub fn close(
@@ -948,19 +954,14 @@ impl Workspace {
     ) -> Task<Result<bool>> {
         let active_call = self.active_call().cloned();
         let window_id = cx.window_id();
-        let workspace_count = cx
-            .window_ids()
-            .collect::<Vec<_>>()
-            .into_iter()
-            .filter_map(|window_id| {
-                cx.app_context()
-                    .root_view(window_id)?
-                    .clone()
-                    .downcast::<Workspace>()
-            })
-            .count();
 
         cx.spawn(|this, mut cx| async move {
+            let workspace_count = cx
+                .window_ids()
+                .into_iter()
+                .filter_map(|window_id| cx.root_view(window_id)?.clone().downcast::<Workspace>())
+                .count();
+
             if let Some(active_call) = active_call {
                 if !quitting
                     && workspace_count == 1
@@ -2849,10 +2850,10 @@ impl std::fmt::Debug for OpenPaths {
 pub struct WorkspaceCreated(WeakViewHandle<Workspace>);
 
 pub fn activate_workspace_for_project(
-    cx: &mut AppContext,
+    cx: &mut AsyncAppContext,
     predicate: impl Fn(&mut Project, &mut ModelContext<Project>) -> bool,
 ) -> Option<WeakViewHandle<Workspace>> {
-    for window_id in cx.window_ids().collect::<Vec<_>>() {
+    for window_id in cx.window_ids() {
         let handle = cx
             .update_window(window_id, |cx| {
                 if let Some(workspace_handle) = cx.root_view().clone().downcast::<Workspace>() {
@@ -2891,13 +2892,14 @@ pub fn open_paths(
 > {
     log::info!("open paths {:?}", abs_paths);
 
-    // Open paths in existing workspace if possible
-    let existing =
-        activate_workspace_for_project(cx, |project, cx| project.contains_paths(abs_paths, cx));
-
     let app_state = app_state.clone();
     let abs_paths = abs_paths.to_vec();
     cx.spawn(|mut cx| async move {
+        // Open paths in existing workspace if possible
+        let existing = activate_workspace_for_project(&mut cx, |project, cx| {
+            project.contains_paths(&abs_paths, cx)
+        });
+
         if let Some(existing) = existing {
             Ok((
                 existing.clone(),
@@ -2960,13 +2962,16 @@ pub fn join_remote_project(
     cx: &mut AppContext,
 ) -> Task<Result<()>> {
     cx.spawn(|mut cx| async move {
-        let existing_workspace = cx.update(|cx| {
-            cx.window_ids()
-                .filter_map(|window_id| cx.root_view(window_id)?.clone().downcast::<Workspace>())
-                .find(|workspace| {
+        let existing_workspace = cx
+            .window_ids()
+            .into_iter()
+            .filter_map(|window_id| cx.root_view(window_id)?.clone().downcast::<Workspace>())
+            .find(|workspace| {
+                cx.read_window(workspace.window_id(), |cx| {
                     workspace.read(cx).project().read(cx).remote_id() == Some(project_id)
                 })
-        });
+                .unwrap_or(false)
+            });
 
         let workspace = if let Some(existing_workspace) = existing_workspace {
             existing_workspace.downgrade()
@@ -3035,24 +3040,25 @@ pub fn join_remote_project(
 }
 
 pub fn restart(_: &Restart, cx: &mut AppContext) {
-    let mut workspaces = cx
-        .window_ids()
-        .filter_map(|window_id| {
-            Some(
-                cx.root_view(window_id)?
-                    .clone()
-                    .downcast::<Workspace>()?
-                    .downgrade(),
-            )
-        })
-        .collect::<Vec<_>>();
-
-    // If multiple windows have unsaved changes, and need a save prompt,
-    // prompt in the active window before switching to a different window.
-    workspaces.sort_by_key(|workspace| !cx.window_is_active(workspace.window_id()));
-
     let should_confirm = cx.global::<Settings>().confirm_quit;
     cx.spawn(|mut cx| async move {
+        let mut workspaces = cx
+            .window_ids()
+            .into_iter()
+            .filter_map(|window_id| {
+                Some(
+                    cx.root_view(window_id)?
+                        .clone()
+                        .downcast::<Workspace>()?
+                        .downgrade(),
+                )
+            })
+            .collect::<Vec<_>>();
+
+        // If multiple windows have unsaved changes, and need a save prompt,
+        // prompt in the active window before switching to a different window.
+        workspaces.sort_by_key(|workspace| !cx.window_is_active(workspace.window_id()));
+
         if let (true, Some(workspace)) = (should_confirm, workspaces.first()) {
             let answer = cx.prompt(
                 workspace.window_id(),

crates/zed/src/zed.rs 🔗

@@ -392,24 +392,25 @@ pub fn build_window_options(
 }
 
 fn quit(_: &Quit, cx: &mut gpui::AppContext) {
-    let mut workspaces = cx
-        .window_ids()
-        .filter_map(|window_id| {
-            Some(
-                cx.root_view(window_id)?
-                    .clone()
-                    .downcast::<Workspace>()?
-                    .downgrade(),
-            )
-        })
-        .collect::<Vec<_>>();
-
-    // If multiple windows have unsaved changes, and need a save prompt,
-    // prompt in the active window before switching to a different window.
-    workspaces.sort_by_key(|workspace| !cx.window_is_active(workspace.window_id()));
-
     let should_confirm = cx.global::<Settings>().confirm_quit;
     cx.spawn(|mut cx| async move {
+        let mut workspaces = cx
+            .window_ids()
+            .into_iter()
+            .filter_map(|window_id| {
+                Some(
+                    cx.root_view(window_id)?
+                        .clone()
+                        .downcast::<Workspace>()?
+                        .downgrade(),
+                )
+            })
+            .collect::<Vec<_>>();
+
+        // If multiple windows have unsaved changes, and need a save prompt,
+        // prompt in the active window before switching to a different window.
+        workspaces.sort_by_key(|workspace| !cx.window_is_active(workspace.window_id()));
+
         if let (true, Some(workspace)) = (should_confirm, workspaces.first()) {
             let answer = cx.prompt(
                 workspace.window_id(),