Ensure that views' `on_release` callbacks are always called (#3747)

Max Brunsfeld created

* Ensure that views' on_release callbacks are always called (even if
their window is gone), by passing them a `AppContext`, not a
`WindowContext`.
* Fix leaked handles to `CollabPanel`, `NotificationPanel`, and
`ChatPanel` caused by captures in a `ListState` render callback.

This fixes two issues we were seeing with following:
* inability to rejoin a remote project after you closed it
* following not working if a window had previously been closed

Change summary

crates/collab_ui2/src/chat_panel.rs         | 10 ++++--
crates/collab_ui2/src/collab_panel.rs       |  9 ++++--
crates/collab_ui2/src/notification_panel.rs | 12 ++++----
crates/go_to_line2/src/go_to_line.rs        | 28 +++++++++++--------
crates/gpui2/src/window.rs                  |  8 ++++-
crates/vim2/src/editor_events.rs            |  6 ++--
crates/workspace2/src/workspace2.rs         | 33 +++++++++++-----------
crates/zed2/src/open_listener.rs            |  2 
8 files changed, 62 insertions(+), 46 deletions(-)

Detailed changes

crates/collab_ui2/src/chat_panel.rs 🔗

@@ -92,11 +92,15 @@ impl ChatPanel {
 
         let workspace_handle = workspace.weak_handle();
 
-        cx.build_view(|cx| {
-            let view: View<ChatPanel> = cx.view().clone();
+        cx.build_view(|cx: &mut ViewContext<Self>| {
+            let view = cx.view().downgrade();
             let message_list =
                 ListState::new(0, gpui::ListAlignment::Bottom, px(1000.), move |ix, cx| {
-                    view.update(cx, |view, cx| view.render_message(ix, cx))
+                    if let Some(view) = view.upgrade() {
+                        view.update(cx, |view, cx| view.render_message(ix, cx))
+                    } else {
+                        div().into_any()
+                    }
                 });
 
             message_list.set_scroll_handler(cx.listener(|this, event: &ListScrollEvent, cx| {

crates/collab_ui2/src/collab_panel.rs 🔗

@@ -178,8 +178,6 @@ enum ListEntry {
 impl CollabPanel {
     pub fn new(workspace: &mut Workspace, cx: &mut ViewContext<Workspace>) -> View<Self> {
         cx.build_view(|cx| {
-            let view = cx.view().clone();
-
             let filter_editor = cx.build_view(|cx| {
                 let mut editor = Editor::single_line(cx);
                 editor.set_placeholder_text("Filter...", cx);
@@ -219,9 +217,14 @@ impl CollabPanel {
             })
             .detach();
 
+            let view = cx.view().downgrade();
             let list_state =
                 ListState::new(0, gpui::ListAlignment::Top, px(1000.), move |ix, cx| {
-                    view.update(cx, |view, cx| view.render_list_entry(ix, cx))
+                    if let Some(view) = view.upgrade() {
+                        view.update(cx, |view, cx| view.render_list_entry(ix, cx))
+                    } else {
+                        div().into_any()
+                    }
                 });
 
             let mut this = Self {

crates/collab_ui2/src/notification_panel.rs 🔗

@@ -88,8 +88,6 @@ impl NotificationPanel {
         let workspace_handle = workspace.weak_handle();
 
         cx.build_view(|cx: &mut ViewContext<Self>| {
-            let view = cx.view().clone();
-
             let mut status = client.status();
             cx.spawn(|this, mut cx| async move {
                 while let Some(_) = status.next().await {
@@ -105,12 +103,14 @@ impl NotificationPanel {
             })
             .detach();
 
+            let view = cx.view().downgrade();
             let notification_list =
                 ListState::new(0, ListAlignment::Top, px(1000.), move |ix, cx| {
-                    view.update(cx, |this, cx| {
-                        this.render_notification(ix, cx)
-                            .unwrap_or_else(|| div().into_any())
-                    })
+                    view.upgrade()
+                        .and_then(|view| {
+                            view.update(cx, |this, cx| this.render_notification(ix, cx))
+                        })
+                        .unwrap_or_else(|| div().into_any())
                 });
             notification_list.set_scroll_handler(cx.listener(
                 |this, event: &ListScrollEvent, cx| {

crates/go_to_line2/src/go_to_line.rs 🔗

@@ -1,8 +1,8 @@
 use editor::{display_map::ToDisplayPoint, scroll::autoscroll::Autoscroll, Editor};
 use gpui::{
-    actions, div, prelude::*, AppContext, DismissEvent, Div, EventEmitter, FocusHandle,
-    FocusableView, Render, SharedString, Styled, Subscription, View, ViewContext, VisualContext,
-    WindowContext,
+    actions, div, prelude::*, AnyWindowHandle, AppContext, DismissEvent, Div, EventEmitter,
+    FocusHandle, FocusableView, Render, SharedString, Styled, Subscription, View, ViewContext,
+    VisualContext,
 };
 use text::{Bias, Point};
 use theme::ActiveTheme;
@@ -74,15 +74,19 @@ impl GoToLine {
         }
     }
 
-    fn release(&mut self, cx: &mut WindowContext) {
-        let scroll_position = self.prev_scroll_position.take();
-        self.active_editor.update(cx, |editor, cx| {
-            editor.highlight_rows(None);
-            if let Some(scroll_position) = scroll_position {
-                editor.set_scroll_position(scroll_position, cx);
-            }
-            cx.notify();
-        })
+    fn release(&mut self, window: AnyWindowHandle, cx: &mut AppContext) {
+        window
+            .update(cx, |_, cx| {
+                let scroll_position = self.prev_scroll_position.take();
+                self.active_editor.update(cx, |editor, cx| {
+                    editor.highlight_rows(None);
+                    if let Some(scroll_position) = scroll_position {
+                        editor.set_scroll_position(scroll_position, cx);
+                    }
+                    cx.notify();
+                })
+            })
+            .ok();
     }
 
     fn on_line_editor_event(

crates/gpui2/src/window.rs 🔗

@@ -2422,16 +2422,20 @@ impl<'a, V: 'static> ViewContext<'a, V> {
         subscription
     }
 
+    /// Register a callback to be invoked when the view is released.
+    ///
+    /// The callback receives a handle to the view's window. This handle may be
+    /// invalid, if the window was closed before the view was released.
     pub fn on_release(
         &mut self,
-        on_release: impl FnOnce(&mut V, &mut WindowContext) + 'static,
+        on_release: impl FnOnce(&mut V, AnyWindowHandle, &mut AppContext) + 'static,
     ) -> Subscription {
         let window_handle = self.window.handle;
         let (subscription, activate) = self.app.release_listeners.insert(
             self.view.model.entity_id,
             Box::new(move |this, cx| {
                 let this = this.downcast_mut().expect("invalid entity type");
-                let _ = window_handle.update(cx, |_, cx| on_release(this, cx));
+                on_release(this, window_handle, cx)
             }),
         );
         activate();

crates/vim2/src/editor_events.rs 🔗

@@ -13,7 +13,7 @@ pub fn init(cx: &mut AppContext) {
         .detach();
 
         let id = cx.view().entity_id();
-        cx.on_release(move |_, cx| released(id, cx)).detach();
+        cx.on_release(move |_, _, cx| released(id, cx)).detach();
     })
     .detach();
 }
@@ -51,8 +51,8 @@ fn blurred(editor: View<Editor>, cx: &mut WindowContext) {
     });
 }
 
-fn released(entity_id: EntityId, cx: &mut WindowContext) {
-    Vim::update(cx, |vim, _| {
+fn released(entity_id: EntityId, cx: &mut AppContext) {
+    cx.update_global(|vim: &mut Vim, _| {
         if vim
             .active_editor
             .as_ref()

crates/workspace2/src/workspace2.rs 🔗

@@ -430,7 +430,6 @@ pub enum Event {
 }
 
 pub struct Workspace {
-    window_self: WindowHandle<Self>,
     weak_self: WeakView<Self>,
     workspace_actions: Vec<Box<dyn Fn(Div, &mut ViewContext<Self>) -> Div>>,
     zoomed: Option<AnyWeakView>,
@@ -642,9 +641,10 @@ impl Workspace {
                 this.serialize_workspace(cx);
                 cx.notify();
             }),
-            cx.on_release(|this, cx| {
+            cx.on_release(|this, window, cx| {
                 this.app_state.workspace_store.update(cx, |store, _| {
-                    store.workspaces.remove(&this.window_self);
+                    let window = window.downcast::<Self>().unwrap();
+                    debug_assert!(store.workspaces.remove(&window));
                 })
             }),
         ];
@@ -665,7 +665,6 @@ impl Workspace {
             // });
         });
         Workspace {
-            window_self: window_handle,
             weak_self: weak_handle.clone(),
             zoomed: None,
             zoomed_position: None,
@@ -3708,7 +3707,7 @@ impl WorkspaceStore {
             let active_project = ActiveCall::global(cx).read(cx).location().cloned();
 
             let mut response = proto::FollowResponse::default();
-            for workspace in &this.workspaces {
+            this.workspaces.retain(|workspace| {
                 workspace
                     .update(cx, |workspace, cx| {
                         let handler_response = workspace.handle_follow(follower.project_id, cx);
@@ -3726,8 +3725,8 @@ impl WorkspaceStore {
                             }
                         }
                     })
-                    .ok();
-            }
+                    .is_ok()
+            });
 
             if let Err(ix) = this.followers.binary_search(&follower) {
                 this.followers.insert(ix, follower);
@@ -3765,15 +3764,17 @@ impl WorkspaceStore {
         let update = envelope.payload;
 
         this.update(&mut cx, |this, cx| {
-            for workspace in &this.workspaces {
-                workspace.update(cx, |workspace, cx| {
-                    let project_id = workspace.project.read(cx).remote_id();
-                    if update.project_id != project_id && update.project_id.is_some() {
-                        return;
-                    }
-                    workspace.handle_update_followers(leader_id, update.clone(), cx);
-                })?;
-            }
+            this.workspaces.retain(|workspace| {
+                workspace
+                    .update(cx, |workspace, cx| {
+                        let project_id = workspace.project.read(cx).remote_id();
+                        if update.project_id != project_id && update.project_id.is_some() {
+                            return;
+                        }
+                        workspace.handle_update_followers(leader_id, update.clone(), cx);
+                    })
+                    .is_ok()
+            });
             Ok(())
         })?
     }

crates/zed2/src/open_listener.rs 🔗

@@ -253,7 +253,7 @@ pub async fn handle_cli_connection(
                                         let (done_tx, done_rx) = oneshot::channel();
                                         let _subscription =
                                             workspace.update(&mut cx, |workspace, cx| {
-                                                cx.on_release(move |_, _| {
+                                                cx.on_release(move |_, _, _| {
                                                     let _ = done_tx.send(());
                                                 })
                                             });