From 3f844bc953b534d4656a478585e3a47bfdd65df9 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Wed, 5 May 2021 11:34:49 -0600 Subject: [PATCH 01/15] Fix crash when closing windows --- gpui/src/app.rs | 11 +++++++++++ gpui/src/platform/mac/window.rs | 25 ++++++++++++++++++++++++- gpui/src/platform/mod.rs | 1 + gpui/src/platform/test.rs | 6 ++++++ 4 files changed, 42 insertions(+), 1 deletion(-) diff --git a/gpui/src/app.rs b/gpui/src/app.rs index ac4c4e69b1782a416f5058025db0eafb974b7e7a..7cda6107f1e122037d8abef4128c9e6e31a61961 100644 --- a/gpui/src/app.rs +++ b/gpui/src/app.rs @@ -758,6 +758,10 @@ impl MutableAppContext { (window_id, root_handle) } + pub fn remove_window(&mut self, window_id: usize) { + self.presenters_and_platform_windows.remove(&window_id); + } + fn open_platform_window(&mut self, window_id: usize) { let mut window = self.platform.open_window( window_id, @@ -814,6 +818,13 @@ impl MutableAppContext { })); } + { + let mut app = self.upgrade(); + window.on_close(Box::new(move || { + app.update(|ctx| ctx.remove_window(window_id)); + })); + } + self.presenters_and_platform_windows .insert(window_id, (presenter.clone(), window)); diff --git a/gpui/src/platform/mac/window.rs b/gpui/src/platform/mac/window.rs index 73196346e351ac823212248f0e5bb05931af81a2..92d0503aeb76d4ad767816897f86a888632c2c6e 100644 --- a/gpui/src/platform/mac/window.rs +++ b/gpui/src/platform/mac/window.rs @@ -61,6 +61,7 @@ unsafe fn build_classes() { sel!(sendEvent:), send_event as extern "C" fn(&Object, Sel, id), ); + decl.add_method(sel!(close), close_window as extern "C" fn(&Object, Sel)); decl.register() }; @@ -125,6 +126,7 @@ struct WindowState { native_window: id, event_callback: Option>, resize_callback: Option>, + close_callback: Option>, synthetic_drag_counter: usize, executor: Rc, scene_to_render: Option, @@ -184,6 +186,7 @@ impl Window { native_window, event_callback: None, resize_callback: None, + close_callback: None, synthetic_drag_counter: 0, executor, scene_to_render: Default::default(), @@ -262,6 +265,10 @@ impl platform::Window for Window { fn on_resize(&mut self, callback: Box) { self.0.as_ref().borrow_mut().resize_callback = Some(callback); } + + fn on_close(&mut self, callback: Box) { + self.0.as_ref().borrow_mut().close_callback = Some(callback); + } } impl platform::WindowContext for Window { @@ -310,7 +317,7 @@ unsafe fn get_window_state(object: &Object) -> Rc> { unsafe fn drop_window_state(object: &Object) { let raw: *mut c_void = *object.get_ivar(WINDOW_STATE_IVAR); - Rc::from_raw(raw as *mut WindowState); + Rc::from_raw(raw as *mut RefCell); } extern "C" fn yes(_: &Object, _: Sel) -> BOOL { @@ -371,6 +378,22 @@ extern "C" fn send_event(this: &Object, _: Sel, native_event: id) { } } +extern "C" fn close_window(this: &Object, _: Sel) { + unsafe { + let window_state = get_window_state(this); + let close_callback = window_state + .as_ref() + .try_borrow_mut() + .ok() + .and_then(|mut window_state| window_state.close_callback.take()); + if let Some(callback) = close_callback { + callback(); + } + + let () = msg_send![super(this, class!(NSWindow)), close]; + } +} + extern "C" fn make_backing_layer(this: &Object, _: Sel) -> id { let window_state = unsafe { get_window_state(this) }; let window_state = window_state.as_ref().borrow(); diff --git a/gpui/src/platform/mod.rs b/gpui/src/platform/mod.rs index b98d3a687bcd35a2dc22a4ca6cff600d5fea87ea..0450856950303e91540581ef4abcfe74b3b155a8 100644 --- a/gpui/src/platform/mod.rs +++ b/gpui/src/platform/mod.rs @@ -59,6 +59,7 @@ pub trait Dispatcher: Send + Sync { pub trait Window: WindowContext { fn on_event(&mut self, callback: Box); fn on_resize(&mut self, callback: Box); + fn on_close(&mut self, callback: Box); } pub trait WindowContext { diff --git a/gpui/src/platform/test.rs b/gpui/src/platform/test.rs index 878449a0216e9a0a95447d9abaef16e089f8672c..2c8876f0c90cc95cf65a638dedb5019f7630073b 100644 --- a/gpui/src/platform/test.rs +++ b/gpui/src/platform/test.rs @@ -16,6 +16,7 @@ pub struct Window { current_scene: Option, event_handlers: Vec>, resize_handlers: Vec>, + close_handlers: Vec>, } impl Platform { @@ -92,6 +93,7 @@ impl Window { size, event_handlers: Vec::new(), resize_handlers: Vec::new(), + close_handlers: Vec::new(), scale_factor: 1.0, current_scene: None, } @@ -130,6 +132,10 @@ impl super::Window for Window { fn on_resize(&mut self, callback: Box) { self.resize_handlers.push(callback); } + + fn on_close(&mut self, callback: Box) { + self.close_handlers.push(callback); + } } pub fn platform() -> impl super::Platform { From 45a01d1526d1bc846c90bd34c738ff6278066d04 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Wed, 5 May 2021 22:48:16 -0600 Subject: [PATCH 02/15] Don't store views on windows Instead, store all views in a single top-level map that's keyed by window id and view id. This ensures we'll keep a view alive even if its window is gone, so long as we're holding a handle. This can happen with things like spawned tasks. We don't want to panic during a spawned task after we close the window. --- gpui/src/app.rs | 227 +++++++++++++++--------------------------- gpui/src/presenter.rs | 2 +- 2 files changed, 81 insertions(+), 148 deletions(-) diff --git a/gpui/src/app.rs b/gpui/src/app.rs index 7cda6107f1e122037d8abef4128c9e6e31a61961..103c90c5280433f57d34eb2468726f0dd7ceda78 100644 --- a/gpui/src/app.rs +++ b/gpui/src/app.rs @@ -382,7 +382,6 @@ pub struct MutableAppContext { model_observations: HashMap>, view_observations: HashMap>, async_observations: HashMap>, - window_invalidations: HashMap, presenters_and_platform_windows: HashMap>, Box)>, debug_elements_callbacks: HashMap crate::json::Value>>, @@ -407,6 +406,7 @@ impl MutableAppContext { assets: Arc::new(AssetCache::new(asset_source)), ctx: AppContext { models: Default::default(), + views: Default::default(), windows: Default::default(), values: Default::default(), ref_counts: Arc::new(Mutex::new(RefCounts::default())), @@ -424,7 +424,6 @@ impl MutableAppContext { model_observations: HashMap::new(), view_observations: HashMap::new(), async_observations: HashMap::new(), - window_invalidations: HashMap::new(), presenters_and_platform_windows: HashMap::new(), debug_elements_callbacks: HashMap::new(), foreground, @@ -555,7 +554,7 @@ impl MutableAppContext { self.ctx.render_view(window_id, view_id) } - pub fn render_views(&self, window_id: usize) -> Result> { + pub fn render_views(&self, window_id: usize) -> HashMap { self.ctx.render_views(window_id) } @@ -612,12 +611,7 @@ impl MutableAppContext { let mut halted_dispatch = false; for view_id in path.iter().rev() { - if let Some(mut view) = self - .ctx - .windows - .get_mut(&window_id) - .and_then(|w| w.views.remove(view_id)) - { + if let Some(mut view) = self.ctx.views.remove(&(window_id, *view_id)) { let type_id = view.as_any().type_id(); if let Some((name, mut handlers)) = self @@ -638,12 +632,7 @@ impl MutableAppContext { .insert(name, handlers); } - self.ctx - .windows - .get_mut(&window_id) - .unwrap() - .views - .insert(*view_id, view); + self.ctx.views.insert((window_id, *view_id), view); if halted_dispatch { break; @@ -687,12 +676,7 @@ impl MutableAppContext { let mut context_chain = Vec::new(); let mut context = keymap::Context::default(); for view_id in &responder_chain { - if let Some(view) = self - .ctx - .windows - .get(&window_id) - .and_then(|w| w.views.get(view_id)) - { + if let Some(view) = self.ctx.views.get(&(window_id, *view_id)) { context.extend(view.keymap_context(self.as_ref())); context_chain.push(context.clone()); } else { @@ -759,6 +743,7 @@ impl MutableAppContext { } pub fn remove_window(&mut self, window_id: usize) { + self.ctx.windows.remove(&window_id); self.presenters_and_platform_windows.remove(&window_id); } @@ -855,16 +840,14 @@ impl MutableAppContext { self.pending_flushes += 1; let mut ctx = ViewContext::new(self, window_id, view_id); let handle = if let Some(view) = build_view(&mut ctx) { + self.ctx.views.insert((window_id, view_id), Box::new(view)); if let Some(window) = self.ctx.windows.get_mut(&window_id) { - window.views.insert(view_id, Box::new(view)); - } else { - panic!("Window does not exist"); + window + .invalidation + .get_or_insert_with(Default::default) + .updated + .insert(view_id); } - self.window_invalidations - .entry(window_id) - .or_default() - .updated - .insert(view_id); Some(ViewHandle::new(window_id, view_id, &self.ctx.ref_counts)) } else { None @@ -892,13 +875,13 @@ impl MutableAppContext { self.subscriptions.remove(&view_id); self.model_observations.remove(&view_id); self.async_observations.remove(&view_id); + self.ctx.views.remove(&(window_id, view_id)); if let Some(window) = self.ctx.windows.get_mut(&window_id) { - self.window_invalidations - .entry(window_id) - .or_default() + window + .invalidation + .get_or_insert_with(Default::default) .removed .push(view_id); - window.views.remove(&view_id); } } @@ -944,7 +927,11 @@ impl MutableAppContext { fn update_windows(&mut self) { let mut invalidations = HashMap::new(); - std::mem::swap(&mut invalidations, &mut self.window_invalidations); + for (window_id, window) in &mut self.ctx.windows { + if let Some(invalidation) = window.invalidation.take() { + invalidations.insert(*window_id, invalidation); + } + } for (window_id, invalidation) in invalidations { if let Some((presenter, mut window)) = @@ -980,12 +967,7 @@ impl MutableAppContext { view_id, callback, } => { - if let Some(mut view) = self - .ctx - .windows - .get_mut(&window_id) - .and_then(|window| window.views.remove(view_id)) - { + if let Some(mut view) = self.ctx.views.remove(&(*window_id, *view_id)) { callback( view.as_any_mut(), payload.as_ref(), @@ -993,12 +975,7 @@ impl MutableAppContext { *window_id, *view_id, ); - self.ctx - .windows - .get_mut(&window_id) - .unwrap() - .views - .insert(*view_id, view); + self.ctx.views.insert((*window_id, *view_id), view); true } else { false @@ -1035,12 +1012,7 @@ impl MutableAppContext { view_id, callback, } => { - if let Some(mut view) = self - .ctx - .windows - .get_mut(window_id) - .and_then(|w| w.views.remove(view_id)) - { + if let Some(mut view) = self.ctx.views.remove(&(*window_id, *view_id)) { callback( view.as_any_mut(), observed_id, @@ -1048,12 +1020,7 @@ impl MutableAppContext { *window_id, *view_id, ); - self.ctx - .windows - .get_mut(window_id) - .unwrap() - .views - .insert(*view_id, view); + self.ctx.views.insert((*window_id, *view_id), view); true } else { false @@ -1079,20 +1046,21 @@ impl MutableAppContext { } fn notify_view_observers(&mut self, window_id: usize, view_id: usize) { - self.window_invalidations - .entry(window_id) - .or_default() - .updated - .insert(view_id); + if let Some(window) = self.ctx.windows.get_mut(&window_id) { + window + .invalidation + .get_or_insert_with(Default::default) + .updated + .insert(view_id); + } if let Some(observations) = self.view_observations.remove(&view_id) { if self.ctx.models.contains_key(&view_id) { for mut observation in observations { let alive = if let Some(mut view) = self .ctx - .windows - .get_mut(&observation.window_id) - .and_then(|w| w.views.remove(&observation.view_id)) + .views + .remove(&(observation.window_id, observation.view_id)) { (observation.callback)( view.as_any_mut(), @@ -1103,11 +1071,8 @@ impl MutableAppContext { observation.view_id, ); self.ctx - .windows - .get_mut(&observation.window_id) - .unwrap() .views - .insert(observation.view_id, view); + .insert((observation.window_id, observation.view_id), view); true } else { false @@ -1143,35 +1108,22 @@ impl MutableAppContext { self.pending_flushes += 1; - if let Some((blurred_id, mut blurred)) = - self.ctx.windows.get_mut(&window_id).and_then(|w| { - let blurred_view = w.focused_view; - w.focused_view = Some(focused_id); - blurred_view.and_then(|id| w.views.remove(&id).map(|view| (id, view))) - }) - { - blurred.on_blur(self, window_id, blurred_id); - self.ctx - .windows - .get_mut(&window_id) - .unwrap() - .views - .insert(blurred_id, blurred); + let blurred_id = self.ctx.windows.get_mut(&window_id).and_then(|window| { + let blurred_id = window.focused_view; + window.focused_view = Some(focused_id); + blurred_id + }); + + if let Some(blurred_id) = blurred_id { + if let Some(mut blurred_view) = self.ctx.views.remove(&(window_id, blurred_id)) { + blurred_view.on_blur(self, window_id, blurred_id); + self.ctx.views.insert((window_id, blurred_id), blurred_view); + } } - if let Some(mut focused) = self - .ctx - .windows - .get_mut(&window_id) - .and_then(|w| w.views.remove(&focused_id)) - { - focused.on_focus(self, window_id, focused_id); - self.ctx - .windows - .get_mut(&window_id) - .unwrap() - .views - .insert(focused_id, focused); + if let Some(mut focused_view) = self.ctx.views.remove(&(window_id, focused_id)) { + focused_view.on_focus(self, window_id, focused_id); + self.ctx.views.insert((window_id, focused_id), focused_view); } self.flush_effects(); @@ -1314,14 +1266,10 @@ impl UpdateModel for MutableAppContext { impl ReadView for MutableAppContext { fn read_view(&self, handle: &ViewHandle) -> &T { - if let Some(window) = self.ctx.windows.get(&handle.window_id) { - if let Some(view) = window.views.get(&handle.view_id) { - view.as_any().downcast_ref().expect("Downcast is type safe") - } else { - panic!("Circular view reference"); - } + if let Some(view) = self.ctx.views.get(&(handle.window_id, handle.view_id)) { + view.as_any().downcast_ref().expect("downcast is type safe") } else { - panic!("Window does not exist"); + panic!("circular view reference"); } } } @@ -1333,15 +1281,11 @@ impl UpdateView for MutableAppContext { F: FnOnce(&mut T, &mut ViewContext) -> S, { self.pending_flushes += 1; - let mut view = if let Some(window) = self.ctx.windows.get_mut(&handle.window_id) { - if let Some(view) = window.views.remove(&handle.view_id) { - view - } else { - panic!("Circular view update"); - } - } else { - panic!("Window does not exist"); - }; + let mut view = self + .ctx + .views + .remove(&(handle.window_id, handle.view_id)) + .expect("circular view update"); let mut ctx = ViewContext::new(self, handle.window_id, handle.view_id); let result = update( @@ -1351,11 +1295,8 @@ impl UpdateView for MutableAppContext { &mut ctx, ); self.ctx - .windows - .get_mut(&handle.window_id) - .unwrap() .views - .insert(handle.view_id, view); + .insert((handle.window_id, handle.view_id), view); self.flush_effects(); result } @@ -1369,6 +1310,7 @@ impl AsRef for MutableAppContext { pub struct AppContext { models: HashMap>, + views: HashMap<(usize, usize), Box>, windows: HashMap, values: RwLock>>, background: Arc, @@ -1391,23 +1333,23 @@ impl AppContext { } pub fn render_view(&self, window_id: usize, view_id: usize) -> Result { - self.windows - .get(&window_id) - .and_then(|w| w.views.get(&view_id)) + self.views + .get(&(window_id, view_id)) .map(|v| v.render(self)) .ok_or(anyhow!("view not found")) } - pub fn render_views(&self, window_id: usize) -> Result> { - self.windows - .get(&window_id) - .map(|w| { - w.views - .iter() - .map(|(id, view)| (*id, view.render(self))) - .collect::>() + pub fn render_views(&self, window_id: usize) -> HashMap { + self.views + .iter() + .filter_map(|((win_id, view_id), view)| { + if *win_id == window_id { + Some((*view_id, view.render(self))) + } else { + None + } }) - .ok_or(anyhow!("window not found")) + .collect::>() } pub fn background_executor(&self) -> &Arc { @@ -1445,25 +1387,21 @@ impl ReadModel for AppContext { impl ReadView for AppContext { fn read_view(&self, handle: &ViewHandle) -> &T { - if let Some(window) = self.windows.get(&handle.window_id) { - if let Some(view) = window.views.get(&handle.view_id) { - view.as_any() - .downcast_ref() - .expect("downcast should be type safe") - } else { - panic!("circular view reference"); - } + if let Some(view) = self.views.get(&(handle.window_id, handle.view_id)) { + view.as_any() + .downcast_ref() + .expect("downcast should be type safe") } else { - panic!("window does not exist"); + panic!("circular view reference"); } } } #[derive(Default)] struct Window { - views: HashMap>, root_view: Option, focused_view: Option, + invalidation: Option, } #[derive(Default, Clone)] @@ -2437,12 +2375,7 @@ impl WeakViewHandle { pub fn upgrade(&self, ctx: impl AsRef) -> Option> { let ctx = ctx.as_ref(); - if ctx - .windows - .get(&self.window_id) - .and_then(|w| w.views.get(&self.view_id)) - .is_some() - { + if ctx.views.get(&(self.window_id, self.view_id)).is_some() { Some(ViewHandle::new( self.window_id, self.view_id, @@ -2915,7 +2848,7 @@ mod tests { let (window_id, _) = app.add_window(|ctx| View::new(None, ctx)); let handle_1 = app.add_view(window_id, |ctx| View::new(None, ctx)); let handle_2 = app.add_view(window_id, |ctx| View::new(Some(handle_1.clone()), ctx)); - assert_eq!(app.ctx.windows[&window_id].views.len(), 3); + assert_eq!(app.ctx.views.len(), 3); handle_1.update(app, |view, ctx| { view.events.push("updated".into()); @@ -2936,7 +2869,7 @@ mod tests { view.other.take(); }); - assert_eq!(app.ctx.windows[&window_id].views.len(), 2); + assert_eq!(app.ctx.views.len(), 2); assert!(app.subscriptions.is_empty()); assert!(app.model_observations.is_empty()); }) diff --git a/gpui/src/presenter.rs b/gpui/src/presenter.rs index 1a4f39607c1ce0d8bb13c223f1f20767ecd73b52..983da559d7de1f3acdb269dc784b86256ebda12b 100644 --- a/gpui/src/presenter.rs +++ b/gpui/src/presenter.rs @@ -35,7 +35,7 @@ impl Presenter { ) -> Self { Self { window_id, - rendered_views: app.render_views(window_id).unwrap(), + rendered_views: app.render_views(window_id), parents: HashMap::new(), font_cache, text_layout_cache, From 6cb656db9a0ac1db7831988490dbc282e784b271 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Wed, 5 May 2021 23:21:19 -0600 Subject: [PATCH 03/15] Require window to have a root view and focused view This is possible now that the window doesn't need to own the view. We can create the root view before we create the window. --- gpui/src/app.rs | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/gpui/src/app.rs b/gpui/src/app.rs index 103c90c5280433f57d34eb2468726f0dd7ceda78..041fbca95966e9726db3a4a261ef79051145a667 100644 --- a/gpui/src/app.rs +++ b/gpui/src/app.rs @@ -539,7 +539,7 @@ impl MutableAppContext { self.ctx .windows .get(&window_id) - .and_then(|window| window.root_view.as_ref().unwrap().clone().downcast::()) + .and_then(|window| window.root_view.clone().downcast::()) } pub fn root_view_id(&self, window_id: usize) -> Option { @@ -732,19 +732,25 @@ impl MutableAppContext { { self.pending_flushes += 1; let window_id = post_inc(&mut self.next_window_id); - self.ctx.windows.insert(window_id, Window::default()); + let root_view = self.add_view(window_id, build_root_view); + self.ctx.windows.insert( + window_id, + Window { + root_view: root_view.clone().into(), + focused_view_id: root_view.id(), + invalidation: None, + }, + ); self.open_platform_window(window_id); - let root_handle = self.add_view(window_id, build_root_view); - self.ctx.windows.get_mut(&window_id).unwrap().root_view = Some(root_handle.clone().into()); - self.focus(window_id, root_handle.id()); self.flush_effects(); - (window_id, root_handle) + (window_id, root_view) } pub fn remove_window(&mut self, window_id: usize) { self.ctx.windows.remove(&window_id); self.presenters_and_platform_windows.remove(&window_id); + self.remove_dropped_entities(); } fn open_platform_window(&mut self, window_id: usize) { @@ -1100,7 +1106,7 @@ impl MutableAppContext { .ctx .windows .get(&window_id) - .and_then(|w| w.focused_view) + .map(|w| w.focused_view_id) .map_or(false, |cur_focused| cur_focused == focused_id) { return; @@ -1108,9 +1114,9 @@ impl MutableAppContext { self.pending_flushes += 1; - let blurred_id = self.ctx.windows.get_mut(&window_id).and_then(|window| { - let blurred_id = window.focused_view; - window.focused_view = Some(focused_id); + let blurred_id = self.ctx.windows.get_mut(&window_id).map(|window| { + let blurred_id = window.focused_view_id; + window.focused_view_id = focused_id; blurred_id }); @@ -1323,13 +1329,13 @@ impl AppContext { pub fn root_view_id(&self, window_id: usize) -> Option { self.windows .get(&window_id) - .and_then(|window| window.root_view.as_ref().map(|v| v.id())) + .map(|window| window.root_view.id()) } pub fn focused_view_id(&self, window_id: usize) -> Option { self.windows .get(&window_id) - .and_then(|window| window.focused_view) + .map(|window| window.focused_view_id) } pub fn render_view(&self, window_id: usize, view_id: usize) -> Result { @@ -1397,10 +1403,9 @@ impl ReadView for AppContext { } } -#[derive(Default)] struct Window { - root_view: Option, - focused_view: Option, + root_view: AnyViewHandle, + focused_view_id: usize, invalidation: Option, } From 7717700b2cad5afee4726f4a96123fa89763d544 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Wed, 5 May 2021 23:21:39 -0600 Subject: [PATCH 04/15] Implement Drop on AnyViewHandle This was a pretty bad oversight. --- gpui/src/app.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/gpui/src/app.rs b/gpui/src/app.rs index 041fbca95966e9726db3a4a261ef79051145a667..9f3087bb1cd13aea74e29a4be471e3dfd10ddfe7 100644 --- a/gpui/src/app.rs +++ b/gpui/src/app.rs @@ -2363,6 +2363,14 @@ impl From> for AnyViewHandle { } } +impl Drop for AnyViewHandle { + fn drop(&mut self) { + if let Some(ref_counts) = self.ref_counts.upgrade() { + ref_counts.lock().dec_view(self.window_id, self.view_id); + } + } +} + pub struct WeakViewHandle { window_id: usize, view_id: usize, From 008f2b905b1b28dfc17e9ad6d3095f3f8be7cf3a Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Thu, 6 May 2021 15:13:01 -0600 Subject: [PATCH 05/15] Hold a WeakViewHandle in BufferElement Otherwise, the element never goes away because the view never goes away because the element is holding a strong reference. Co-Authored-By: Max Brunsfeld --- zed/src/editor/buffer_element.rs | 34 +++++++++++++++++++------------- zed/src/editor/buffer_view.rs | 4 ++-- 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/zed/src/editor/buffer_element.rs b/zed/src/editor/buffer_element.rs index 8a49c208f30f220129de161ce8fdc0b4cd5b4254..8ceb72c1e94a26670be8dcbdca4a8d53fb574d6d 100644 --- a/zed/src/editor/buffer_element.rs +++ b/zed/src/editor/buffer_element.rs @@ -9,7 +9,7 @@ use gpui::{ json::{self, ToJson}, text_layout::{self, TextLayoutCache}, AfterLayoutContext, AppContext, Border, Element, Event, EventContext, FontCache, LayoutContext, - PaintContext, Quad, Scene, SizeConstraint, ViewHandle, + PaintContext, Quad, Scene, SizeConstraint, WeakViewHandle, }; use json::json; use smallvec::SmallVec; @@ -20,14 +20,18 @@ use std::{ }; pub struct BufferElement { - view: ViewHandle, + view: WeakViewHandle, } impl BufferElement { - pub fn new(view: ViewHandle) -> Self { + pub fn new(view: WeakViewHandle) -> Self { Self { view } } + fn view<'a>(&self, ctx: &'a AppContext) -> &'a BufferView { + self.view.upgrade(ctx).unwrap().read(ctx) + } + fn mouse_down( &self, position: Vector2F, @@ -37,7 +41,7 @@ impl BufferElement { ctx: &mut EventContext, ) -> bool { if paint.text_bounds.contains_point(position) { - let view = self.view.read(ctx.app); + let view = self.view(ctx.app); let position = paint.point_for_position(view, layout, position, ctx.font_cache, ctx.app); ctx.dispatch_action("buffer:select", SelectAction::Begin { position, add: cmd }); @@ -48,7 +52,7 @@ impl BufferElement { } fn mouse_up(&self, _position: Vector2F, ctx: &mut EventContext) -> bool { - if self.view.read(ctx.app).is_selecting() { + if self.view(ctx.app).is_selecting() { ctx.dispatch_action("buffer:select", SelectAction::End); true } else { @@ -63,7 +67,7 @@ impl BufferElement { paint: &mut PaintState, ctx: &mut EventContext, ) -> bool { - let view = self.view.read(ctx.app); + let view = self.view(ctx.app); if view.is_selecting() { let rect = paint.text_bounds; @@ -116,7 +120,9 @@ impl BufferElement { } fn key_down(&self, chars: &str, ctx: &mut EventContext) -> bool { - if self.view.is_focused(ctx.app) { + let view = self.view.upgrade(ctx.app).unwrap(); + + if view.is_focused(ctx.app) { if chars.is_empty() { false } else { @@ -145,7 +151,7 @@ impl BufferElement { return false; } - let view = self.view.read(ctx.app); + let view = self.view(ctx.app); let font_cache = &ctx.font_cache; let layout_cache = &ctx.text_layout_cache; let max_glyph_width = view.em_width(font_cache); @@ -167,7 +173,7 @@ impl BufferElement { } fn paint_gutter(&mut self, rect: RectF, layout: &LayoutState, ctx: &mut PaintContext) { - let view = self.view.read(ctx.app); + let view = self.view(ctx.app); let line_height = view.line_height(ctx.font_cache); let scroll_top = view.scroll_position().y() * line_height; @@ -197,7 +203,7 @@ impl BufferElement { } fn paint_text(&mut self, bounds: RectF, layout: &LayoutState, ctx: &mut PaintContext) { - let view = self.view.read(ctx.app); + let view = self.view(ctx.app); let line_height = view.line_height(ctx.font_cache); let descent = view.font_descent(ctx.font_cache); let start_row = view.scroll_position().y() as u32; @@ -313,14 +319,14 @@ impl Element for BufferElement { let app = ctx.app; let mut size = constraint.max; if size.y().is_infinite() { - let view = self.view.read(app); + let view = self.view(app); size.set_y((view.max_point(app).row() + 1) as f32 * view.line_height(ctx.font_cache)); } if size.x().is_infinite() { unimplemented!("we don't yet handle an infinite width constraint on buffer elements"); } - let view = self.view.read(app); + let view = self.view(app); let font_cache = &ctx.font_cache; let layout_cache = &ctx.text_layout_cache; let line_height = view.line_height(font_cache); @@ -404,7 +410,7 @@ impl Element for BufferElement { if let Some(layout) = layout { let app = ctx.app.as_ref(); - let view = self.view.read(app); + let view = self.view(app); view.clamp_scroll_left( layout .scroll_max(view, ctx.font_cache, ctx.text_layout_cache, app) @@ -437,7 +443,7 @@ impl Element for BufferElement { layout.text_size, ); - if self.view.read(ctx.app).is_gutter_visible() { + if self.view(ctx.app).is_gutter_visible() { self.paint_gutter(gutter_bounds, layout, ctx); } self.paint_text(text_bounds, layout, ctx); diff --git a/zed/src/editor/buffer_view.rs b/zed/src/editor/buffer_view.rs index 7d80c68217ac454d22684f582f4ab6b71ec3140d..8a91e447c1ccd7260f6664807e0927a611d90893 100644 --- a/zed/src/editor/buffer_view.rs +++ b/zed/src/editor/buffer_view.rs @@ -2161,8 +2161,8 @@ impl Entity for BufferView { } impl View for BufferView { - fn render<'a>(&self, app: &AppContext) -> ElementBox { - BufferElement::new(self.handle.upgrade(app).unwrap()).boxed() + fn render<'a>(&self, _: &AppContext) -> ElementBox { + BufferElement::new(self.handle.clone()).boxed() } fn ui_name() -> &'static str { From f408521d12af1003cc52b0f9a67bc816d1d47414 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Thu, 6 May 2021 15:13:26 -0600 Subject: [PATCH 06/15] Gracefully handle a view being updated and removed in the same tick Co-Authored-By: Max Brunsfeld --- gpui/src/presenter.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/gpui/src/presenter.rs b/gpui/src/presenter.rs index 983da559d7de1f3acdb269dc784b86256ebda12b..8fdf94de005bf133e015a8947c60e8ea5fa5245a 100644 --- a/gpui/src/presenter.rs +++ b/gpui/src/presenter.rs @@ -55,15 +55,16 @@ impl Presenter { path } - pub fn invalidate(&mut self, invalidation: WindowInvalidation, app: &AppContext) { - for view_id in invalidation.updated { - self.rendered_views - .insert(view_id, app.render_view(self.window_id, view_id).unwrap()); - } + pub fn invalidate(&mut self, mut invalidation: WindowInvalidation, app: &AppContext) { for view_id in invalidation.removed { + invalidation.updated.remove(&view_id); self.rendered_views.remove(&view_id); self.parents.remove(&view_id); } + for view_id in invalidation.updated { + self.rendered_views + .insert(view_id, app.render_view(self.window_id, view_id).unwrap()); + } } pub fn build_scene( From ab10e27424f550d49b2a6b6314c2edd0e23d2a3a Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Thu, 6 May 2021 15:18:09 -0600 Subject: [PATCH 07/15] Create a pending view handle before creating a view This way, if we create and drop a handle during the creation of a view, we don't drop the view before we have a chance to increment its initial reference count. Co-Authored-By: Max Brunsfeld --- gpui/src/app.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/gpui/src/app.rs b/gpui/src/app.rs index 9f3087bb1cd13aea74e29a4be471e3dfd10ddfe7..d86f5342f88a9d230951e87316f41694afb08fbc 100644 --- a/gpui/src/app.rs +++ b/gpui/src/app.rs @@ -844,6 +844,7 @@ impl MutableAppContext { { let view_id = post_inc(&mut self.next_entity_id); self.pending_flushes += 1; + let handle = ViewHandle::new(window_id, view_id, &self.ctx.ref_counts); let mut ctx = ViewContext::new(self, window_id, view_id); let handle = if let Some(view) = build_view(&mut ctx) { self.ctx.views.insert((window_id, view_id), Box::new(view)); @@ -854,7 +855,7 @@ impl MutableAppContext { .updated .insert(view_id); } - Some(ViewHandle::new(window_id, view_id, &self.ctx.ref_counts)) + Some(handle) } else { None }; From 4eac765f1a99bac2e23cdb8503aaf5a72bd69916 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Thu, 6 May 2021 15:27:18 -0600 Subject: [PATCH 08/15] Replace zed::watch with postage::watch for settings Co-Authored-By: Max Brunsfeld --- zed/src/editor/buffer_view.rs | 29 +++++++--------- zed/src/file_finder.rs | 9 +++-- zed/src/lib.rs | 1 - zed/src/menus.rs | 5 +-- zed/src/settings.rs | 4 +-- zed/src/watch.rs | 65 ----------------------------------- zed/src/workspace.rs | 8 ++--- zed/src/workspace/pane.rs | 5 +-- 8 files changed, 27 insertions(+), 99 deletions(-) delete mode 100644 zed/src/watch.rs diff --git a/zed/src/editor/buffer_view.rs b/zed/src/editor/buffer_view.rs index 8a91e447c1ccd7260f6664807e0927a611d90893..08de7d619ddcef397e0c3e544c1fd907db73b183 100644 --- a/zed/src/editor/buffer_view.rs +++ b/zed/src/editor/buffer_view.rs @@ -2,7 +2,7 @@ use super::{ buffer, movement, Anchor, Bias, Buffer, BufferElement, DisplayMap, DisplayPoint, Point, Selection, SelectionSetId, ToOffset, ToPoint, }; -use crate::{settings::Settings, watch, workspace, worktree::FileHandle}; +use crate::{settings::Settings, workspace, worktree::FileHandle}; use anyhow::Result; use futures_core::future::LocalBoxFuture; use gpui::{ @@ -12,6 +12,7 @@ use gpui::{ }; use gpui::{geometry::vector::Vector2F, TextLayoutCache}; use parking_lot::Mutex; +use postage::watch; use serde::{Deserialize, Serialize}; use smallvec::SmallVec; use smol::Timer; @@ -288,19 +289,13 @@ impl BufferView { settings: watch::Receiver, ctx: &mut ViewContext, ) -> Self { - settings.notify_view_on_change(ctx); - if let Some(file) = file.as_ref() { file.observe_from_view(ctx, |_, _, ctx| ctx.emit(Event::FileHandleChanged)); } ctx.observe_model(&buffer, Self::on_buffer_changed); ctx.subscribe_to_model(&buffer, Self::on_buffer_event); - let display_map = DisplayMap::new( - buffer.clone(), - smol::block_on(settings.read()).tab_size, - ctx.as_ref(), - ); + let display_map = DisplayMap::new(buffer.clone(), settings.borrow().tab_size, ctx.as_ref()); let (selection_set_id, _) = buffer.update(ctx, |buffer, ctx| { buffer.add_selection_set( @@ -1924,31 +1919,31 @@ impl BufferView { } pub fn font_size(&self) -> f32 { - smol::block_on(self.settings.read()).buffer_font_size + self.settings.borrow().buffer_font_size } pub fn font_ascent(&self, font_cache: &FontCache) -> f32 { - let settings = smol::block_on(self.settings.read()); + let settings = self.settings.borrow(); let font_id = font_cache.default_font(settings.buffer_font_family); let ascent = font_cache.metric(font_id, |m| m.ascent); font_cache.scale_metric(ascent, font_id, settings.buffer_font_size) } pub fn font_descent(&self, font_cache: &FontCache) -> f32 { - let settings = smol::block_on(self.settings.read()); + let settings = self.settings.borrow(); let font_id = font_cache.default_font(settings.buffer_font_family); let ascent = font_cache.metric(font_id, |m| m.descent); font_cache.scale_metric(ascent, font_id, settings.buffer_font_size) } pub fn line_height(&self, font_cache: &FontCache) -> f32 { - let settings = smol::block_on(self.settings.read()); + let settings = self.settings.borrow(); let font_id = font_cache.default_font(settings.buffer_font_family); font_cache.line_height(font_id, settings.buffer_font_size) } pub fn em_width(&self, font_cache: &FontCache) -> f32 { - let settings = smol::block_on(self.settings.read()); + let settings = self.settings.borrow(); let font_id = font_cache.default_font(settings.buffer_font_family); font_cache.em_width(font_id, settings.buffer_font_size) } @@ -1960,7 +1955,7 @@ impl BufferView { layout_cache: &TextLayoutCache, app: &AppContext, ) -> Result { - let settings = smol::block_on(self.settings.read()); + let settings = self.settings.borrow(); let font_size = settings.buffer_font_size; let font_id = font_cache.select_font(settings.buffer_font_family, &FontProperties::new())?; @@ -1985,7 +1980,7 @@ impl BufferView { layout_cache: &TextLayoutCache, ctx: &AppContext, ) -> Result>> { - let settings = smol::block_on(self.settings.read()); + let settings = self.settings.borrow(); let font_size = settings.buffer_font_size; let font_id = font_cache.select_font(settings.buffer_font_family, &FontProperties::new())?; @@ -2029,7 +2024,7 @@ impl BufferView { return Ok(Vec::new()); } - let settings = smol::block_on(self.settings.read()); + let settings = self.settings.borrow(); let font_id = font_cache.select_font(settings.buffer_font_family, &FontProperties::new())?; let font_size = settings.buffer_font_size; @@ -2067,7 +2062,7 @@ impl BufferView { layout_cache: &TextLayoutCache, app: &AppContext, ) -> Result> { - let settings = smol::block_on(self.settings.read()); + let settings = self.settings.borrow(); let font_id = font_cache.select_font(settings.buffer_font_family, &FontProperties::new())?; diff --git a/zed/src/file_finder.rs b/zed/src/file_finder.rs index 0965cdcda5bc77ca4d35a7840855e53a2004a688..0f966cc5789d1ab7318566e197d7eb0221837a9b 100644 --- a/zed/src/file_finder.rs +++ b/zed/src/file_finder.rs @@ -1,7 +1,7 @@ use crate::{ editor::{buffer_view, BufferView}, settings::Settings, - util, watch, + util, workspace::Workspace, worktree::{match_paths, PathMatch, Worktree}, }; @@ -14,6 +14,7 @@ use gpui::{ AppContext, Axis, Border, Entity, MutableAppContext, View, ViewContext, ViewHandle, WeakViewHandle, }; +use postage::watch; use std::{ cmp, path::Path, @@ -105,7 +106,7 @@ impl View for FileFinder { impl FileFinder { fn render_matches(&self) -> ElementBox { if self.matches.is_empty() { - let settings = smol::block_on(self.settings.read()); + let settings = self.settings.borrow(); return Container::new( Label::new( "No matches".into(), @@ -148,7 +149,7 @@ impl FileFinder { ) -> Option { self.labels_for_match(path_match, app).map( |(file_name, file_name_positions, full_path, full_path_positions)| { - let settings = smol::block_on(self.settings.read()); + let settings = self.settings.borrow(); let highlight_color = ColorU::from_u32(0x304ee2ff); let bold = *Properties::new().weight(Weight::BOLD); let mut container = Container::new( @@ -292,8 +293,6 @@ impl FileFinder { let query_buffer = ctx.add_view(|ctx| BufferView::single_line(settings.clone(), ctx)); ctx.subscribe_to_view(&query_buffer, Self::on_query_buffer_event); - settings.notify_view_on_change(ctx); - Self { handle: ctx.handle().downgrade(), settings, diff --git a/zed/src/lib.rs b/zed/src/lib.rs index 7c6155f2d19e528175550a1f7f71ead1e5653fe6..7dd383f56e612e8f0db08d6affc8d649d6acd702 100644 --- a/zed/src/lib.rs +++ b/zed/src/lib.rs @@ -9,6 +9,5 @@ mod sum_tree; mod test; mod time; mod util; -pub mod watch; pub mod workspace; mod worktree; diff --git a/zed/src/menus.rs b/zed/src/menus.rs index 08afb1e990c413f2fc89fae3d820d9c1d56dc462..0feae2128b18603d83d99977dde8f5141a3c50aa 100644 --- a/zed/src/menus.rs +++ b/zed/src/menus.rs @@ -1,8 +1,9 @@ -use crate::{settings::Settings, watch::Receiver}; +use crate::settings::Settings; use gpui::{Menu, MenuItem}; +use postage::watch; #[cfg(target_os = "macos")] -pub fn menus(settings: Receiver) -> Vec> { +pub fn menus(settings: watch::Receiver) -> Vec> { vec![ Menu { name: "Zed", diff --git a/zed/src/settings.rs b/zed/src/settings.rs index 7b847400005051ef899ee1ea23cae62fa2778e0b..44b05c99faec208b334f5de8a73e589d94712102 100644 --- a/zed/src/settings.rs +++ b/zed/src/settings.rs @@ -1,6 +1,6 @@ -use crate::watch; use anyhow::Result; use gpui::font_cache::{FamilyId, FontCache}; +use postage::watch; #[derive(Clone)] pub struct Settings { @@ -26,5 +26,5 @@ impl Settings { pub fn channel( font_cache: &FontCache, ) -> Result<(watch::Sender, watch::Receiver)> { - Ok(watch::channel(Settings::new(font_cache)?)) + Ok(watch::channel_with(Settings::new(font_cache)?)) } diff --git a/zed/src/watch.rs b/zed/src/watch.rs deleted file mode 100644 index 33622edc6f69792fa5c2674cfd63750adf4f46fa..0000000000000000000000000000000000000000 --- a/zed/src/watch.rs +++ /dev/null @@ -1,65 +0,0 @@ -// TODO: This implementation is actually broken in that it will only - -use gpui::{Entity, ModelContext, View, ViewContext}; -use smol::{channel, lock::RwLock}; -use std::ops::Deref; -use std::sync::Arc; - -pub struct Sender { - value: Arc>, - updated: channel::Sender<()>, -} - -#[derive(Clone)] -pub struct Receiver { - value: Arc>, - updated: channel::Receiver<()>, -} - -impl Sender { - pub async fn update(&mut self, f: impl FnOnce(&mut T) -> R) -> R { - let result = f(&mut *self.value.write().await); - self.updated.send(()).await.unwrap(); - result - } -} - -impl Receiver { - pub async fn updated(&self) { - let _ = self.updated.recv().await; - } - - pub async fn read<'a>(&'a self) -> impl 'a + Deref { - self.value.read().await - } -} - -// TODO: These implementations are broken because they only handle a single update. -impl Receiver { - pub fn notify_model_on_change(&self, ctx: &mut ModelContext) { - let watch = self.clone(); - ctx.spawn(async move { watch.updated().await }, |_, _, ctx| { - ctx.notify() - }) - .detach(); - } - - pub fn notify_view_on_change(&self, ctx: &mut ViewContext) { - let watch = self.clone(); - ctx.spawn(async move { watch.updated().await }, |_, _, ctx| { - ctx.notify() - }) - .detach(); - } -} - -pub fn channel(value: T) -> (Sender, Receiver) { - let value = Arc::new(RwLock::new(value)); - let (s, r) = channel::unbounded(); - let sender = Sender { - value: value.clone(), - updated: s, - }; - let receiver = Receiver { value, updated: r }; - (sender, receiver) -} diff --git a/zed/src/workspace.rs b/zed/src/workspace.rs index 629d23beecce0bdf1e1c59a76cd6fbe465f92f9b..40152ddfeb763d39a5147a7c0dad4eed89f61a46 100644 --- a/zed/src/workspace.rs +++ b/zed/src/workspace.rs @@ -3,11 +3,9 @@ pub mod pane_group; pub use pane::*; pub use pane_group::*; -use crate::{ - settings::Settings, - watch::{self, Receiver}, -}; +use crate::settings::Settings; use gpui::{MutableAppContext, PathPromptOptions}; +use postage::watch; use std::path::PathBuf; pub fn init(app: &mut MutableAppContext) { app.add_global_action("workspace:open", open); @@ -44,7 +42,7 @@ pub struct OpenParams { pub settings: watch::Receiver, } -fn open(settings: &Receiver, ctx: &mut MutableAppContext) { +fn open(settings: &watch::Receiver, ctx: &mut MutableAppContext) { let settings = settings.clone(); ctx.prompt_for_paths( PathPromptOptions { diff --git a/zed/src/workspace/pane.rs b/zed/src/workspace/pane.rs index 5da80e560cb1a7af607ff3f250322ecf6dd4f93c..e7957ec9489d0db6bcb103ef3c1cb4197a75c45e 100644 --- a/zed/src/workspace/pane.rs +++ b/zed/src/workspace/pane.rs @@ -1,5 +1,5 @@ use super::{ItemViewHandle, SplitDirection}; -use crate::{settings::Settings, watch}; +use crate::settings::Settings; use gpui::{ color::ColorU, elements::*, @@ -7,6 +7,7 @@ use gpui::{ keymap::Binding, AppContext, Border, Entity, MutableAppContext, Quad, View, ViewContext, }; +use postage::watch; use std::{cmp, path::Path, sync::Arc}; pub fn init(app: &mut MutableAppContext) { @@ -185,7 +186,7 @@ impl Pane { } fn render_tabs(&self, ctx: &AppContext) -> ElementBox { - let settings = smol::block_on(self.settings.read()); + let settings = self.settings.borrow(); let border_color = ColorU::from_u32(0xdbdbdcff); let line_height = ctx.font_cache().line_height( ctx.font_cache().default_font(settings.ui_font_family), From 5e08a02ca864ef78887c883400a04eb5f0661e9c Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Thu, 6 May 2021 16:19:38 -0600 Subject: [PATCH 09/15] Don't retain entities after EntityTasks are dropped This allows us to spawn long-running tasks without leaking entities, but it also ensures that if we *do* hold on to a task, that the entity exists until the future or stream complete so that the task doesn't need to return an Option type. Co-Authored-By: Max Brunsfeld --- gpui/src/app.rs | 177 +++++++++++++++++++++++++++++++++++------------- 1 file changed, 131 insertions(+), 46 deletions(-) diff --git a/gpui/src/app.rs b/gpui/src/app.rs index d86f5342f88a9d230951e87316f41694afb08fbc..c036f629f930d7f96f9c10980affd2f1e0fc96e1 100644 --- a/gpui/src/app.rs +++ b/gpui/src/app.rs @@ -718,11 +718,12 @@ impl MutableAppContext { { self.pending_flushes += 1; let model_id = post_inc(&mut self.next_entity_id); + let handle = ModelHandle::new(model_id, &self.ctx.ref_counts); let mut ctx = ModelContext::new(self, model_id); let model = build_model(&mut ctx); self.ctx.models.insert(model_id, Box::new(model)); self.flush_effects(); - ModelHandle::new(model_id, &self.ctx.ref_counts) + handle } pub fn add_window(&mut self, build_root_view: F) -> (usize, ViewHandle) @@ -925,6 +926,11 @@ impl MutableAppContext { if self.pending_effects.is_empty() { self.flushing_effects = false; + + for (key, view) in &self.ctx.views { + log::info!("{:?} {}", key, view.ui_name()); + } + break; } } @@ -1136,7 +1142,7 @@ impl MutableAppContext { self.flush_effects(); } - fn spawn(&mut self, future: F) -> EntityTask + fn spawn(&mut self, spawner: Spawner, future: F) -> EntityTask where F: 'static + Future, T: 'static, @@ -1147,20 +1153,20 @@ impl MutableAppContext { let app = app.clone(); self.foreground.spawn(async move { let output = future.await; - *app.borrow_mut() + app.borrow_mut() .handle_future_output(task_id, Box::new(output)) - .downcast::() - .unwrap() + .map(|output| *output.downcast::().unwrap()) }) }; EntityTask::new( task_id, task, + spawner, TaskHandlerMap::Future(self.future_handlers.clone()), ) } - fn spawn_stream(&mut self, mut stream: F) -> EntityTask + fn spawn_stream(&mut self, spawner: Spawner, mut stream: F) -> EntityTask where F: 'static + Stream + Unpin, T: 'static, @@ -1182,20 +1188,24 @@ impl MutableAppContext { } } - *app.borrow_mut() + app.borrow_mut() .stream_completed(task_id) - .downcast::() - .unwrap() + .map(|output| *output.downcast::().unwrap()) }); EntityTask::new( task_id, task, + spawner, TaskHandlerMap::Stream(self.stream_handlers.clone()), ) } - fn handle_future_output(&mut self, task_id: usize, output: Box) -> Box { + fn handle_future_output( + &mut self, + task_id: usize, + output: Box, + ) -> Option> { self.pending_flushes += 1; let future_callback = self.future_handlers.borrow_mut().remove(&task_id).unwrap(); let result = future_callback(output, self); @@ -1214,7 +1224,7 @@ impl MutableAppContext { halt } - fn stream_completed(&mut self, task_id: usize) -> Box { + fn stream_completed(&mut self, task_id: usize) -> Option> { self.pending_flushes += 1; let handler = self.stream_handlers.borrow_mut().remove(&task_id).unwrap(); @@ -1239,9 +1249,9 @@ impl ReadModel for MutableAppContext { model .as_any() .downcast_ref() - .expect("Downcast is type safe") + .expect("downcast is type safe") } else { - panic!("Circular model reference"); + panic!("circular model reference"); } } } @@ -1259,14 +1269,14 @@ impl UpdateModel for MutableAppContext { model .as_any_mut() .downcast_mut() - .expect("Downcast is type safe"), + .expect("downcast is type safe"), &mut ctx, ); self.ctx.models.insert(handle.model_id, model); self.flush_effects(); result } else { - panic!("Circular model update"); + panic!("circular model update"); } } } @@ -1298,7 +1308,7 @@ impl UpdateView for MutableAppContext { let result = update( view.as_any_mut() .downcast_mut() - .expect("Downcast is type safe"), + .expect("downcast is type safe"), &mut ctx, ); self.ctx @@ -1604,13 +1614,20 @@ impl<'a, T: Entity> ModelContext<'a, T> { U: 'static, { let handle = self.handle(); - let task = self.app.spawn::(future); + let weak_handle = handle.downgrade(); + let task = self + .app + .spawn::(Spawner::Model(handle.into()), future); self.app.future_handlers.borrow_mut().insert( task.id, Box::new(move |output, ctx| { - let output = *output.downcast().unwrap(); - handle.update(ctx, |model, ctx| Box::new(callback(model, output, ctx))) + weak_handle.upgrade(ctx.as_ref()).map(|handle| { + let output = *output.downcast().unwrap(); + handle.update(ctx, |model, ctx| { + Box::new(callback(model, output, ctx)) as Box + }) + }) }), ); @@ -1630,23 +1647,31 @@ impl<'a, T: Entity> ModelContext<'a, T> { U: 'static + Any, { let handle = self.handle(); - let task = self.app.spawn_stream(stream); + let weak_handle = handle.downgrade(); + let task = self.app.spawn_stream(Spawner::Model(handle.into()), stream); self.app.stream_handlers.borrow_mut().insert( task.id, StreamHandler { item_callback: { - let handle = handle.clone(); + let weak_handle = weak_handle.clone(); Box::new(move |output, app| { - let output = *output.downcast().unwrap(); - handle.update(app, |model, ctx| { - item_callback(model, output, ctx); - ctx.halt_stream - }) + if let Some(handle) = weak_handle.upgrade(app.as_ref()) { + let output = *output.downcast().unwrap(); + handle.update(app, |model, ctx| { + item_callback(model, output, ctx); + ctx.halt_stream + }) + } else { + true + } }) }, done_callback: Box::new(move |app| { - handle.update(app, |model, ctx| Box::new(done_callback(model, ctx))) + weak_handle.upgrade(app.as_ref()).map(|handle| { + handle.update(app, |model, ctx| Box::new(done_callback(model, ctx))) + as Box + }) }), }, ); @@ -1896,13 +1921,18 @@ impl<'a, T: View> ViewContext<'a, T> { U: 'static, { let handle = self.handle(); - let task = self.app.spawn(future); + let weak_handle = handle.downgrade(); + let task = self.app.spawn(Spawner::View(handle.into()), future); self.app.future_handlers.borrow_mut().insert( task.id, Box::new(move |output, app| { - let output = *output.downcast().unwrap(); - handle.update(app, |view, ctx| Box::new(callback(view, output, ctx))) + weak_handle.upgrade(app.as_ref()).map(|handle| { + let output = *output.downcast().unwrap(); + handle.update(app, |view, ctx| { + Box::new(callback(view, output, ctx)) as Box + }) + }) }), ); @@ -1922,22 +1952,31 @@ impl<'a, T: View> ViewContext<'a, T> { U: 'static + Any, { let handle = self.handle(); - let task = self.app.spawn_stream(stream); + let weak_handle = handle.downgrade(); + let task = self.app.spawn_stream(Spawner::View(handle.into()), stream); self.app.stream_handlers.borrow_mut().insert( task.id, StreamHandler { item_callback: { - let handle = handle.clone(); - Box::new(move |output, app| { - let output = *output.downcast().unwrap(); - handle.update(app, |view, ctx| { - item_callback(view, output, ctx); - ctx.halt_stream - }) + let weak_handle = weak_handle.clone(); + Box::new(move |output, ctx| { + if let Some(handle) = weak_handle.upgrade(ctx.as_ref()) { + let output = *output.downcast().unwrap(); + handle.update(ctx, |view, ctx| { + item_callback(view, output, ctx); + ctx.halt_stream + }) + } else { + true + } }) }, - done_callback: Box::new(move |app| { - handle.update(app, |view, ctx| Box::new(done_callback(view, ctx))) + done_callback: Box::new(move |ctx| { + weak_handle.upgrade(ctx.as_ref()).map(|handle| { + handle.update(ctx, |view, ctx| { + Box::new(done_callback(view, ctx)) as Box + }) + }) }), }, ); @@ -2178,6 +2217,40 @@ impl WeakModelHandle { } } +impl Clone for WeakModelHandle { + fn clone(&self) -> Self { + Self { + model_id: self.model_id, + model_type: PhantomData, + } + } +} + +pub struct AnyModelHandle { + model_id: usize, + ref_counts: Weak>, +} + +impl From> for AnyModelHandle { + fn from(handle: ModelHandle) -> Self { + if let Some(ref_counts) = handle.ref_counts.upgrade() { + ref_counts.lock().inc_entity(handle.model_id); + } + Self { + model_id: handle.model_id, + ref_counts: handle.ref_counts.clone(), + } + } +} + +impl Drop for AnyModelHandle { + fn drop(&mut self) { + if let Some(ref_counts) = self.ref_counts.upgrade() { + ref_counts.lock().dec_model(self.model_id); + } + } +} + pub struct ViewHandle { window_id: usize, view_id: usize, @@ -2551,20 +2624,26 @@ struct ViewObservation { callback: Box, } -type FutureHandler = Box, &mut MutableAppContext) -> Box>; +type FutureHandler = Box, &mut MutableAppContext) -> Option>>; struct StreamHandler { item_callback: Box, &mut MutableAppContext) -> bool>, - done_callback: Box Box>, + done_callback: Box Option>>, } #[must_use] pub struct EntityTask { id: usize, - task: Option>, + task: Option>>, + _spawner: Spawner, // Keeps the spawning entity alive for as long as the task exists handler_map: TaskHandlerMap, } +pub enum Spawner { + Model(AnyModelHandle), + View(AnyViewHandle), +} + enum TaskHandlerMap { Detached, Future(Rc>>), @@ -2572,10 +2651,16 @@ enum TaskHandlerMap { } impl EntityTask { - fn new(id: usize, task: executor::Task, handler_map: TaskHandlerMap) -> Self { + fn new( + id: usize, + task: executor::Task>, + spawner: Spawner, + handler_map: TaskHandlerMap, + ) -> Self { Self { id, task: Some(task), + _spawner: spawner, handler_map, } } @@ -2587,7 +2672,7 @@ impl EntityTask { pub async fn cancel(mut self) -> Option { let task = self.task.take().unwrap(); - task.cancel().await + task.cancel().await.unwrap() } } @@ -2599,7 +2684,7 @@ impl Future for EntityTask { ctx: &mut std::task::Context<'_>, ) -> std::task::Poll { let task = unsafe { self.map_unchecked_mut(|task| task.task.as_mut().unwrap()) }; - task.poll(ctx) + task.poll(ctx).map(|output| output.unwrap()) } } From 35e0eaaae227383de9eff3ec52c8896bc7dd7060 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Thu, 6 May 2021 16:30:10 -0600 Subject: [PATCH 10/15] Refocus root view if focused view is removed Co-Authored-By: Max Brunsfeld --- gpui/src/app.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/gpui/src/app.rs b/gpui/src/app.rs index c036f629f930d7f96f9c10980affd2f1e0fc96e1..a5758903fc9a96e719b1683b228c27ced066a7df 100644 --- a/gpui/src/app.rs +++ b/gpui/src/app.rs @@ -884,12 +884,21 @@ impl MutableAppContext { self.model_observations.remove(&view_id); self.async_observations.remove(&view_id); self.ctx.views.remove(&(window_id, view_id)); - if let Some(window) = self.ctx.windows.get_mut(&window_id) { + let change_focus_to = self.ctx.windows.get_mut(&window_id).and_then(|window| { window .invalidation .get_or_insert_with(Default::default) .removed .push(view_id); + if window.focused_view_id == view_id { + Some(window.root_view.id()) + } else { + None + } + }); + + if let Some(view_id) = change_focus_to { + self.focus(window_id, view_id); } } From b5f1f31693af2497ceab78b8d63754c09fca5de9 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Fri, 7 May 2021 09:43:07 -0600 Subject: [PATCH 11/15] Fix focus test Call on_focus on the root view when the window is originally created. Test dropping a focused view. Simplify test to avoid relying on emitting events. Co-Authored-By: Antonio Scandurra --- gpui/src/app.rs | 52 ++++++++++++++++++++++++++----------------------- 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/gpui/src/app.rs b/gpui/src/app.rs index a5758903fc9a96e719b1683b228c27ced066a7df..f62ab3f6be5f736683b4cf7a871c135615e55d48 100644 --- a/gpui/src/app.rs +++ b/gpui/src/app.rs @@ -734,6 +734,7 @@ impl MutableAppContext { self.pending_flushes += 1; let window_id = post_inc(&mut self.next_window_id); let root_view = self.add_view(window_id, build_root_view); + self.ctx.windows.insert( window_id, Window { @@ -743,6 +744,7 @@ impl MutableAppContext { }, ); self.open_platform_window(window_id); + root_view.update(self, |view, ctx| view.on_focus(ctx)); self.flush_effects(); (window_id, root_view) @@ -3186,13 +3188,13 @@ mod tests { #[test] fn test_focus() { - #[derive(Default)] struct View { - events: Vec, + name: String, + events: Arc>>, } impl Entity for View { - type Event = String; + type Event = (); } impl super::View for View { @@ -3204,40 +3206,42 @@ mod tests { "View" } - fn on_focus(&mut self, ctx: &mut ViewContext) { - self.events.push("self focused".into()); - ctx.emit("focused".into()); + fn on_focus(&mut self, _: &mut ViewContext) { + self.events.lock().push(format!("{} focused", &self.name)); } - fn on_blur(&mut self, ctx: &mut ViewContext) { - self.events.push("self blurred".into()); - ctx.emit("blurred".into()); + fn on_blur(&mut self, _: &mut ViewContext) { + self.events.lock().push(format!("{} blurred", &self.name)); } } App::test((), |app| { - let (window_id, view_1) = app.add_window(|_| View::default()); - let view_2 = app.add_view(window_id, |_| View::default()); - - view_1.update(app, |_, ctx| { - ctx.subscribe_to_view(&view_2, |view_1, _, event, _| { - view_1.events.push(format!("view 2 {}", event)); - }); - ctx.focus(&view_2); + let events: Arc>> = Default::default(); + let (window_id, view_1) = app.add_window(|_| View { + events: events.clone(), + name: "view 1".to_string(), }); - - view_1.update(app, |_, ctx| { - ctx.focus(&view_1); + let view_2 = app.add_view(window_id, |_| View { + events: events.clone(), + name: "view 2".to_string(), }); + view_1.update(app, |_, ctx| ctx.focus(&view_2)); + view_1.update(app, |_, ctx| ctx.focus(&view_1)); + view_1.update(app, |_, ctx| ctx.focus(&view_2)); + view_1.update(app, |_, _| drop(view_2)); + assert_eq!( - view_1.read(app).events, + *events.lock(), [ - "self focused".to_string(), - "self blurred".to_string(), + "view 1 focused".to_string(), + "view 1 blurred".to_string(), "view 2 focused".to_string(), - "self focused".to_string(), "view 2 blurred".to_string(), + "view 1 focused".to_string(), + "view 1 blurred".to_string(), + "view 2 focused".to_string(), + "view 1 focused".to_string(), ], ); }) From 318e8abf2f988cf9b572a548c11037572abbc5d1 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Fri, 7 May 2021 11:47:04 -0600 Subject: [PATCH 12/15] Avoid redundant entity refcount operations Co-Authored-By: Max Brunsfeld Co-Authored-By: Antonio Scandurra --- gpui/src/app.rs | 87 +++++++++++++++++++++++++++---------------------- 1 file changed, 48 insertions(+), 39 deletions(-) diff --git a/gpui/src/app.rs b/gpui/src/app.rs index f62ab3f6be5f736683b4cf7a871c135615e55d48..33899fc77a7ea9db90f4b69a3d698345441de24f 100644 --- a/gpui/src/app.rs +++ b/gpui/src/app.rs @@ -2059,7 +2059,7 @@ pub enum EntityLocation { pub struct ModelHandle { model_id: usize, model_type: PhantomData, - ref_counts: Weak>, + ref_counts: Arc>, } impl ModelHandle { @@ -2068,7 +2068,7 @@ impl ModelHandle { Self { model_id, model_type: PhantomData, - ref_counts: Arc::downgrade(ref_counts), + ref_counts: ref_counts.clone(), } } @@ -2145,10 +2145,7 @@ impl ModelHandle { impl Clone for ModelHandle { fn clone(&self) -> Self { - if let Some(ref_counts) = self.ref_counts.upgrade() { - ref_counts.lock().inc_entity(self.model_id); - } - + self.ref_counts.lock().inc_entity(self.model_id); Self { model_id: self.model_id, model_type: PhantomData, @@ -2190,9 +2187,7 @@ unsafe impl Sync for ModelHandle {} impl Drop for ModelHandle { fn drop(&mut self) { - if let Some(ref_counts) = self.ref_counts.upgrade() { - ref_counts.lock().dec_model(self.model_id); - } + self.ref_counts.lock().dec_model(self.model_id); } } @@ -2239,14 +2234,12 @@ impl Clone for WeakModelHandle { pub struct AnyModelHandle { model_id: usize, - ref_counts: Weak>, + ref_counts: Arc>, } impl From> for AnyModelHandle { fn from(handle: ModelHandle) -> Self { - if let Some(ref_counts) = handle.ref_counts.upgrade() { - ref_counts.lock().inc_entity(handle.model_id); - } + handle.ref_counts.lock().inc_entity(handle.model_id); Self { model_id: handle.model_id, ref_counts: handle.ref_counts.clone(), @@ -2256,9 +2249,7 @@ impl From> for AnyModelHandle { impl Drop for AnyModelHandle { fn drop(&mut self) { - if let Some(ref_counts) = self.ref_counts.upgrade() { - ref_counts.lock().dec_model(self.model_id); - } + self.ref_counts.lock().dec_model(self.model_id); } } @@ -2266,7 +2257,7 @@ pub struct ViewHandle { window_id: usize, view_id: usize, view_type: PhantomData, - ref_counts: Weak>, + ref_counts: Arc>, } impl ViewHandle { @@ -2276,7 +2267,7 @@ impl ViewHandle { window_id, view_id, view_type: PhantomData, - ref_counts: Arc::downgrade(ref_counts), + ref_counts: ref_counts.clone(), } } @@ -2353,10 +2344,7 @@ impl ViewHandle { impl Clone for ViewHandle { fn clone(&self) -> Self { - if let Some(ref_counts) = self.ref_counts.upgrade() { - ref_counts.lock().inc_entity(self.view_id); - } - + self.ref_counts.lock().inc_entity(self.view_id); Self { window_id: self.window_id, view_id: self.view_id, @@ -2385,9 +2373,9 @@ impl Debug for ViewHandle { impl Drop for ViewHandle { fn drop(&mut self) { - if let Some(ref_counts) = self.ref_counts.upgrade() { - ref_counts.lock().dec_view(self.window_id, self.view_id); - } + self.ref_counts + .lock() + .dec_view(self.window_id, self.view_id); } } @@ -2406,7 +2394,7 @@ pub struct AnyViewHandle { window_id: usize, view_id: usize, view_type: TypeId, - ref_counts: Weak>, + ref_counts: Arc>, } impl AnyViewHandle { @@ -2420,19 +2408,26 @@ impl AnyViewHandle { pub fn downcast(self) -> Option> { if self.is::() { - if let Some(ref_counts) = self.ref_counts.upgrade() { - return Some(ViewHandle::new(self.window_id, self.view_id, &ref_counts)); + let result = Some(ViewHandle { + window_id: self.window_id, + view_id: self.view_id, + ref_counts: self.ref_counts.clone(), + view_type: PhantomData, + }); + unsafe { + Arc::decrement_strong_count(&self.ref_counts); } + std::mem::forget(self); + result + } else { + None } - None } } impl From<&ViewHandle> for AnyViewHandle { fn from(handle: &ViewHandle) -> Self { - if let Some(ref_counts) = handle.ref_counts.upgrade() { - ref_counts.lock().inc_entity(handle.view_id); - } + handle.ref_counts.lock().inc_entity(handle.view_id); AnyViewHandle { window_id: handle.window_id, view_id: handle.view_id, @@ -2444,15 +2439,25 @@ impl From<&ViewHandle> for AnyViewHandle { impl From> for AnyViewHandle { fn from(handle: ViewHandle) -> Self { - (&handle).into() + let any_handle = AnyViewHandle { + window_id: handle.window_id, + view_id: handle.view_id, + view_type: TypeId::of::(), + ref_counts: handle.ref_counts.clone(), + }; + unsafe { + Arc::decrement_strong_count(&handle.ref_counts); + } + std::mem::forget(handle); + any_handle } } impl Drop for AnyViewHandle { fn drop(&mut self) { - if let Some(ref_counts) = self.ref_counts.upgrade() { - ref_counts.lock().dec_view(self.window_id, self.view_id); - } + self.ref_counts + .lock() + .dec_view(self.window_id, self.view_id); } } @@ -2473,7 +2478,7 @@ impl WeakViewHandle { pub fn upgrade(&self, ctx: impl AsRef) -> Option> { let ctx = ctx.as_ref(); - if ctx.views.get(&(self.window_id, self.view_id)).is_some() { + if ctx.ref_counts.lock().is_entity_alive(self.view_id) { Some(ViewHandle::new( self.window_id, self.view_id, @@ -2552,8 +2557,8 @@ struct RefCounts { } impl RefCounts { - fn inc_entity(&mut self, model_id: usize) { - *self.entity_counts.entry(model_id).or_insert(0) += 1; + fn inc_entity(&mut self, entity_id: usize) { + *self.entity_counts.entry(entity_id).or_insert(0) += 1; } fn inc_value(&mut self, tag_type_id: TypeId, id: usize) { @@ -2588,6 +2593,10 @@ impl RefCounts { } } + fn is_entity_alive(&self, entity_id: usize) -> bool { + self.entity_counts.contains_key(&entity_id) + } + fn take_dropped( &mut self, ) -> ( From 83a844f12081bb21138fa2a0d49cc2bf646eea4e Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Fri, 7 May 2021 14:05:05 -0700 Subject: [PATCH 13/15] Fix the Clone impl for AnyViewHandle --- gpui/src/app.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/gpui/src/app.rs b/gpui/src/app.rs index 33899fc77a7ea9db90f4b69a3d698345441de24f..43c541730858d4da344ffdd6d67e71f73be7df51 100644 --- a/gpui/src/app.rs +++ b/gpui/src/app.rs @@ -2389,7 +2389,6 @@ impl Handle for ViewHandle { } } -#[derive(Clone)] pub struct AnyViewHandle { window_id: usize, view_id: usize, @@ -2425,6 +2424,18 @@ impl AnyViewHandle { } } +impl Clone for AnyViewHandle { + fn clone(&self) -> Self { + self.ref_counts.lock().inc_entity(self.view_id); + Self { + window_id: self.window_id, + view_id: self.view_id, + view_type: self.view_type, + ref_counts: self.ref_counts.clone(), + } + } +} + impl From<&ViewHandle> for AnyViewHandle { fn from(handle: &ViewHandle) -> Self { handle.ref_counts.lock().inc_entity(handle.view_id); From b292baf33495db057374911b1fdc1355faeeccbf Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Fri, 7 May 2021 14:51:55 -0700 Subject: [PATCH 14/15] Remove logging of retained views --- gpui/src/app.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/gpui/src/app.rs b/gpui/src/app.rs index bf0e049a485885c87df474d2d9bddf3396dc066f..863cb94f270d0ec5561144d5ae4c31affc452e68 100644 --- a/gpui/src/app.rs +++ b/gpui/src/app.rs @@ -960,11 +960,6 @@ impl MutableAppContext { if self.pending_effects.is_empty() { self.flushing_effects = false; - - for (key, view) in &self.ctx.views { - log::info!("{:?} {}", key, view.ui_name()); - } - break; } } From 2c74d75687598d20a7e328f0bccafe04ea75a620 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Sat, 8 May 2021 08:49:14 -0600 Subject: [PATCH 15/15] Fix double borrow error in Window::on_close callbacks when quitting app The simplest solution I could come up with was to make quitting the app asynchronous. Calling mac::Platform::quit enqueues a request to quit the app and then allows the call stack to fully return. This ensures we aren't holding a borrow when we quit and invoke all the Window::on_close callbacks. Seems like it should be fine to be async on quitting. --- gpui/src/platform/mac/platform.rs | 13 +++++++++++++ gpui/src/platform/mac/window.rs | 15 +++++++++------ 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/gpui/src/platform/mac/platform.rs b/gpui/src/platform/mac/platform.rs index 29f75ed3ac81d5fc5a4d8342d5e7b08d0b54c435..e7ccd280b4f4f81d3adfcb4cbcd6a369a11a6b31 100644 --- a/gpui/src/platform/mac/platform.rs +++ b/gpui/src/platform/mac/platform.rs @@ -347,7 +347,20 @@ impl platform::Platform for MacPlatform { } fn quit(&self) { + // Quitting the app causes us to close windows, which invokes `Window::on_close` callbacks + // synchronously before this method terminates. If we call `Platform::quit` while holding a + // borrow of the app state (which most of the time we will do), we will end up + // double-borrowing the app state in the `on_close` callbacks for our open windows. To solve + // this, we make quitting the application asynchronous so that we aren't holding borrows to + // the app state on the stack when we actually terminate the app. + + use super::dispatcher::{dispatch_async_f, dispatch_get_main_queue}; + unsafe { + dispatch_async_f(dispatch_get_main_queue(), ptr::null_mut(), Some(quit)); + } + + unsafe extern "C" fn quit(_: *mut c_void) { let app = NSApplication::sharedApplication(nil); let _: () = msg_send![app, terminate: nil]; } diff --git a/gpui/src/platform/mac/window.rs b/gpui/src/platform/mac/window.rs index 92d0503aeb76d4ad767816897f86a888632c2c6e..2475a59381d37c7de7a8c7b86916ed3a968c9eca 100644 --- a/gpui/src/platform/mac/window.rs +++ b/gpui/src/platform/mac/window.rs @@ -380,12 +380,15 @@ extern "C" fn send_event(this: &Object, _: Sel, native_event: id) { extern "C" fn close_window(this: &Object, _: Sel) { unsafe { - let window_state = get_window_state(this); - let close_callback = window_state - .as_ref() - .try_borrow_mut() - .ok() - .and_then(|mut window_state| window_state.close_callback.take()); + let close_callback = { + let window_state = get_window_state(this); + window_state + .as_ref() + .try_borrow_mut() + .ok() + .and_then(|mut window_state| window_state.close_callback.take()) + }; + if let Some(callback) = close_callback { callback(); }