Fix edge cases when calling `refresh_windows`

Antonio Scandurra and Nathan Sobo created 3 years ago

This commit ensures that new views are rendered for the first time. This fixes
a panic that could be reproduced by dropping the `ThemeSelector` and opening
the file finder during the same update.

Co-Authored-By: Nathan Sobo <[e-mail]>

Change summary

crates/gpui/src/app.rs       | 80 +++++++++++++++++++++++++++++++++++--
crates/gpui/src/presenter.rs | 39 ++++++++----------
2 files changed, 91 insertions(+), 28 deletions(-)

Detailed changes

crates/gpui/src/app.rs

@@ -1413,11 +1413,10 @@ impl MutableAppContext {
                     invalidation: None,
                 },
             );
-            this.open_platform_window(window_id, window_options);
             root_view.update(this, |view, cx| {
                 view.on_focus(cx);
-                cx.notify();
             });
+            this.open_platform_window(window_id, window_options);
 
             (window_id, root_view)
         })
@@ -1475,6 +1474,11 @@ impl MutableAppContext {
             }));
         }
 
+        let scene =
+            presenter
+                .borrow_mut()
+                .build_scene(window.size(), window.scale_factor(), false, self);
+        window.present_scene(scene);
         self.presenters_and_platform_windows
             .insert(window_id, (presenter.clone(), window));
     }
@@ -1666,13 +1670,13 @@ impl MutableAppContext {
             }
         }
 
-        for (window_id, invalidation) in invalidations {
+        for (window_id, mut invalidation) in invalidations {
             if let Some((presenter, mut window)) =
                 self.presenters_and_platform_windows.remove(&window_id)
             {
                 {
                     let mut presenter = presenter.borrow_mut();
-                    presenter.invalidate(invalidation, self);
+                    presenter.invalidate(&mut invalidation, self);
                     let scene =
                         presenter.build_scene(window.size(), window.scale_factor(), false, self);
                     window.present_scene(scene);
@@ -1695,7 +1699,7 @@ impl MutableAppContext {
     fn perform_window_refresh(&mut self) {
         let mut presenters = mem::take(&mut self.presenters_and_platform_windows);
         for (window_id, (presenter, window)) in &mut presenters {
-            let invalidation = self
+            let mut invalidation = self
                 .cx
                 .windows
                 .get_mut(&window_id)
@@ -1703,7 +1707,10 @@ impl MutableAppContext {
                 .invalidation
                 .take();
             let mut presenter = presenter.borrow_mut();
-            presenter.refresh(invalidation, self);
+            presenter.refresh(
+                invalidation.as_mut().unwrap_or(&mut Default::default()),
+                self,
+            );
             let scene = presenter.build_scene(window.size(), window.scale_factor(), true, self);
             window.present_scene(scene);
         }
@@ -5363,4 +5370,65 @@ mod tests {
         cx.update(|_| drop(view));
         condition.await;
     }
+
+    #[crate::test(self)]
+    fn test_refresh_windows(cx: &mut MutableAppContext) {
+        struct View(usize);
+
+        impl super::Entity for View {
+            type Event = ();
+        }
+
+        impl super::View for View {
+            fn ui_name() -> &'static str {
+                "test view"
+            }
+
+            fn render(&mut self, _: &mut RenderContext<Self>) -> ElementBox {
+                Empty::new().named(format!("render count: {}", post_inc(&mut self.0)))
+            }
+        }
+
+        let (window_id, root_view) = cx.add_window(Default::default(), |_| View(0));
+        let presenter = cx.presenters_and_platform_windows[&window_id].0.clone();
+
+        assert_eq!(
+            presenter.borrow().rendered_views[&root_view.id()].name(),
+            Some("render count: 0")
+        );
+
+        let view = cx.add_view(window_id, |cx| {
+            cx.refresh_windows();
+            View(0)
+        });
+
+        assert_eq!(
+            presenter.borrow().rendered_views[&root_view.id()].name(),
+            Some("render count: 1")
+        );
+        assert_eq!(
+            presenter.borrow().rendered_views[&view.id()].name(),
+            Some("render count: 0")
+        );
+
+        cx.update(|cx| cx.refresh_windows());
+        assert_eq!(
+            presenter.borrow().rendered_views[&root_view.id()].name(),
+            Some("render count: 2")
+        );
+        assert_eq!(
+            presenter.borrow().rendered_views[&view.id()].name(),
+            Some("render count: 1")
+        );
+
+        cx.update(|cx| {
+            cx.refresh_windows();
+            drop(view);
+        });
+        assert_eq!(
+            presenter.borrow().rendered_views[&root_view.id()].name(),
+            Some("render count: 3")
+        );
+        assert_eq!(presenter.borrow().rendered_views.len(), 1);
+    }
 }

crates/gpui/src/presenter.rs

@@ -20,7 +20,7 @@ use std::{
 
 pub struct Presenter {
     window_id: usize,
-    rendered_views: HashMap<usize, ElementBox>,
+    pub(crate) rendered_views: HashMap<usize, ElementBox>,
     parents: HashMap<usize, usize>,
     font_cache: Arc<FontCache>,
     text_layout_cache: TextLayoutCache,
@@ -63,39 +63,34 @@ impl Presenter {
         path
     }
 
-    pub fn invalidate(&mut self, mut invalidation: WindowInvalidation, cx: &mut MutableAppContext) {
+    pub fn invalidate(
+        &mut self,
+        invalidation: &mut WindowInvalidation,
+        cx: &mut MutableAppContext,
+    ) {
         cx.start_frame();
-        for view_id in invalidation.removed {
+        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 {
+        for view_id in &invalidation.updated {
             self.rendered_views.insert(
-                view_id,
-                cx.render_view(self.window_id, view_id, self.titlebar_height, false)
+                *view_id,
+                cx.render_view(self.window_id, *view_id, self.titlebar_height, false)
                     .unwrap(),
             );
         }
     }
 
-    pub fn refresh(
-        &mut self,
-        invalidation: Option<WindowInvalidation>,
-        cx: &mut MutableAppContext,
-    ) {
-        cx.start_frame();
-        if let Some(invalidation) = invalidation {
-            for view_id in invalidation.removed {
-                self.rendered_views.remove(&view_id);
-                self.parents.remove(&view_id);
-            }
-        }
-
+    pub fn refresh(&mut self, invalidation: &mut WindowInvalidation, cx: &mut MutableAppContext) {
+        self.invalidate(invalidation, cx);
         for (view_id, view) in &mut self.rendered_views {
-            *view = cx
-                .render_view(self.window_id, *view_id, self.titlebar_height, true)
-                .unwrap();
+            if !invalidation.updated.contains(view_id) {
+                *view = cx
+                    .render_view(self.window_id, *view_id, self.titlebar_height, true)
+                    .unwrap();
+            }
         }
     }