From 45a01d1526d1bc846c90bd34c738ff6278066d04 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Wed, 5 May 2021 22:48:16 -0600 Subject: [PATCH] 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,