From 5e7c74c7b6c809df7acdea14fadcd0d2bf354619 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Wed, 20 Dec 2023 16:01:52 -0800 Subject: [PATCH 1/3] Ensure that on_release callbacks are called even if view outlives its window --- 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 | 2 +- crates/zed2/src/open_listener.rs | 2 +- 5 files changed, 27 insertions(+), 19 deletions(-) diff --git a/crates/go_to_line2/src/go_to_line.rs b/crates/go_to_line2/src/go_to_line.rs index d6b464cf0ec3a690c8587b367f88552a4e629930..02fcd4771623494bd502b65ca81f900a06d34266 100644 --- a/crates/go_to_line2/src/go_to_line.rs +++ b/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( diff --git a/crates/gpui2/src/window.rs b/crates/gpui2/src/window.rs index 8546e094d41f63f2ecd6725c178ba495ae74017d..9e8347c783ca62121efc581b24b2e40c0209d228 100644 --- a/crates/gpui2/src/window.rs +++ b/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(); diff --git a/crates/vim2/src/editor_events.rs b/crates/vim2/src/editor_events.rs index 0e2a1451fe06f145fc305f04f039eb0b62d17dcc..70f486f298cf42d92b919480936d0626a6fbc54b 100644 --- a/crates/vim2/src/editor_events.rs +++ b/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, 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() diff --git a/crates/workspace2/src/workspace2.rs b/crates/workspace2/src/workspace2.rs index 797e95088d89970f5759efee28b14e5237325f3b..c8712205cb62735cc5f39b534cd34962b6666829 100644 --- a/crates/workspace2/src/workspace2.rs +++ b/crates/workspace2/src/workspace2.rs @@ -642,7 +642,7 @@ impl Workspace { this.serialize_workspace(cx); cx.notify(); }), - cx.on_release(|this, cx| { + cx.on_release(|this, _, cx| { this.app_state.workspace_store.update(cx, |store, _| { store.workspaces.remove(&this.window_self); }) diff --git a/crates/zed2/src/open_listener.rs b/crates/zed2/src/open_listener.rs index 4c961a2b317b6d0a2e5399fa0be3e7b1b219311f..b45254f7174c600f27ff552fef8ae7c2a2be7446 100644 --- a/crates/zed2/src/open_listener.rs +++ b/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(()); }) }); From 42bdc11112346aced8d596b91bb5bdc8f335da44 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Wed, 20 Dec 2023 16:08:58 -0800 Subject: [PATCH 2/3] Prune dead workspaces from WorkspaceStore on read Also, remove unnecessary window handle from Workspace. --- crates/workspace2/src/workspace2.rs | 33 +++++++++++++++-------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/crates/workspace2/src/workspace2.rs b/crates/workspace2/src/workspace2.rs index c8712205cb62735cc5f39b534cd34962b6666829..2bc0d403101841c03c0315902c919da7659184b9 100644 --- a/crates/workspace2/src/workspace2.rs +++ b/crates/workspace2/src/workspace2.rs @@ -430,7 +430,6 @@ pub enum Event { } pub struct Workspace { - window_self: WindowHandle, weak_self: WeakView, workspace_actions: Vec) -> Div>>, zoomed: Option, @@ -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::().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(()) })? } From 24970c1da9d19af78bd5dbdac9d873959770d06d Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Wed, 20 Dec 2023 16:33:10 -0800 Subject: [PATCH 3/3] Fix view handle leaks in ListState callbacks --- crates/collab_ui2/src/chat_panel.rs | 10 +++++++--- crates/collab_ui2/src/collab_panel.rs | 9 ++++++--- crates/collab_ui2/src/notification_panel.rs | 12 ++++++------ 3 files changed, 19 insertions(+), 12 deletions(-) diff --git a/crates/collab_ui2/src/chat_panel.rs b/crates/collab_ui2/src/chat_panel.rs index f3f2a37171b32c5be5c610725951b199b1a28525..27eb4ca17a919b41135c3fb381a2f405e91b7802 100644 --- a/crates/collab_ui2/src/chat_panel.rs +++ b/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 = cx.view().clone(); + cx.build_view(|cx: &mut ViewContext| { + 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| { diff --git a/crates/collab_ui2/src/collab_panel.rs b/crates/collab_ui2/src/collab_panel.rs index 35e2f8d7ed210dc3cfb2fbe59d1d399a3f2683b9..b19d0ba2ebeb500c379d4ab3df09f8dc7fb6a7e0 100644 --- a/crates/collab_ui2/src/collab_panel.rs +++ b/crates/collab_ui2/src/collab_panel.rs @@ -178,8 +178,6 @@ enum ListEntry { impl CollabPanel { pub fn new(workspace: &mut Workspace, cx: &mut ViewContext) -> View { 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 { diff --git a/crates/collab_ui2/src/notification_panel.rs b/crates/collab_ui2/src/notification_panel.rs index 35288e810d645bf7ea2c158df071cc51fde4efc1..30c3439121541b4597642c1ed5a3e932f9c558d8 100644 --- a/crates/collab_ui2/src/notification_panel.rs +++ b/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| { - 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| {