Merge pull request #40 from zed-industries/close-window

Correctly handle closing windows and removing entities

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];
         }

gpui/src/platform/mac/window.rs

@@ -62,6 +62,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()
     };
 
@@ -126,6 +127,7 @@ struct WindowState {
     native_window: id,
     event_callback: Option<Box<dyn FnMut(Event)>>,
     resize_callback: Option<Box<dyn FnMut(&mut dyn platform::WindowContext)>>,
+    close_callback: Option<Box<dyn FnOnce()>>,
     synthetic_drag_counter: usize,
     executor: Rc<executor::Foreground>,
     scene_to_render: Option<Scene>,
@@ -186,6 +188,7 @@ impl Window {
                 native_window,
                 event_callback: None,
                 resize_callback: None,
+                close_callback: None,
                 synthetic_drag_counter: 0,
                 executor,
                 scene_to_render: Default::default(),
@@ -265,6 +268,10 @@ impl platform::Window for Window {
     fn on_resize(&mut self, callback: Box<dyn FnMut(&mut dyn platform::WindowContext)>) {
         self.0.as_ref().borrow_mut().resize_callback = Some(callback);
     }
+
+    fn on_close(&mut self, callback: Box<dyn FnOnce()>) {
+        self.0.as_ref().borrow_mut().close_callback = Some(callback);
+    }
 }
 
 impl platform::WindowContext for Window {
@@ -313,7 +320,7 @@ unsafe fn get_window_state(object: &Object) -> Rc<RefCell<WindowState>> {
 
 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<WindowState>);
 }
 
 extern "C" fn yes(_: &Object, _: Sel) -> BOOL {
@@ -392,6 +399,25 @@ extern "C" fn send_event(this: &Object, _: Sel, native_event: id) {
     }
 }
 
+extern "C" fn close_window(this: &Object, _: Sel) {
+    unsafe {
+        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();
+        }
+
+        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();

gpui/src/platform/mod.rs

@@ -70,6 +70,7 @@ pub trait Dispatcher: Send + Sync {
 pub trait Window: WindowContext {
     fn on_event(&mut self, callback: Box<dyn FnMut(Event)>);
     fn on_resize(&mut self, callback: Box<dyn FnMut(&mut dyn WindowContext)>);
+    fn on_close(&mut self, callback: Box<dyn FnOnce()>);
 }
 
 pub trait WindowContext {

gpui/src/platform/test.rs

@@ -23,6 +23,7 @@ pub struct Window {
     current_scene: Option<crate::Scene>,
     event_handlers: Vec<Box<dyn FnMut(super::Event)>>,
     resize_handlers: Vec<Box<dyn FnMut(&mut dyn super::WindowContext)>>,
+    close_handlers: Vec<Box<dyn FnOnce()>>,
 }
 
 impl Platform {
@@ -119,6 +120,7 @@ impl Window {
             size,
             event_handlers: Vec::new(),
             resize_handlers: Vec::new(),
+            close_handlers: Vec::new(),
             scale_factor: 1.0,
             current_scene: None,
         }
@@ -157,6 +159,10 @@ impl super::Window for Window {
     fn on_resize(&mut self, callback: Box<dyn FnMut(&mut dyn super::WindowContext)>) {
         self.resize_handlers.push(callback);
     }
+
+    fn on_close(&mut self, callback: Box<dyn FnOnce()>) {
+        self.close_handlers.push(callback);
+    }
 }
 
 pub(crate) fn platform() -> Platform {

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,
@@ -55,15 +55,16 @@ impl Presenter {
         path
     }
 
-    pub fn invalidate(&mut self, invalidation: WindowInvalidation, app: &AppContext) {
+    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());
         }
-        for view_id in invalidation.removed {
-            self.rendered_views.remove(&view_id);
-            self.parents.remove(&view_id);
-        }
     }
 
     pub fn build_scene(

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<BufferView>,
+    view: WeakViewHandle<BufferView>,
 }
 
 impl BufferElement {
-    pub fn new(view: ViewHandle<BufferView>) -> Self {
+    pub fn new(view: WeakViewHandle<BufferView>) -> 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);

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::{
@@ -11,6 +11,7 @@ use gpui::{
     MutableAppContext, TextLayoutCache, View, ViewContext, WeakViewHandle,
 };
 use parking_lot::Mutex;
+use postage::watch;
 use serde::{Deserialize, Serialize};
 use smallvec::SmallVec;
 use smol::Timer;
@@ -300,15 +301,9 @@ impl BufferView {
         settings: watch::Receiver<Settings>,
         ctx: &mut ViewContext<Self>,
     ) -> Self {
-        settings.notify_view_on_change(ctx);
-
         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(
@@ -1989,31 +1984,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)
     }
@@ -2025,7 +2020,7 @@ impl BufferView {
         layout_cache: &TextLayoutCache,
         app: &AppContext,
     ) -> Result<f32> {
-        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())?;
@@ -2050,7 +2045,7 @@ impl BufferView {
         layout_cache: &TextLayoutCache,
         ctx: &AppContext,
     ) -> Result<Vec<Arc<text_layout::Line>>> {
-        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())?;
@@ -2094,7 +2089,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;
@@ -2132,7 +2127,7 @@ impl BufferView {
         layout_cache: &TextLayoutCache,
         app: &AppContext,
     ) -> Result<Arc<text_layout::Line>> {
-        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())?;
 
@@ -2226,8 +2221,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 {

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<ElementBox> {
         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,

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;

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<Settings>) -> Vec<Menu<'static>> {
+pub fn menus(settings: watch::Receiver<Settings>) -> Vec<Menu<'static>> {
     vec![
         Menu {
             name: "Zed",

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<Settings>, watch::Receiver<Settings>)> {
-    Ok(watch::channel(Settings::new(font_cache)?))
+    Ok(watch::channel_with(Settings::new(font_cache)?))
 }

zed/src/watch.rs

@@ -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<T> {
-    value: Arc<RwLock<T>>,
-    updated: channel::Sender<()>,
-}
-
-#[derive(Clone)]
-pub struct Receiver<T> {
-    value: Arc<RwLock<T>>,
-    updated: channel::Receiver<()>,
-}
-
-impl<T> Sender<T> {
-    pub async fn update<R>(&mut self, f: impl FnOnce(&mut T) -> R) -> R {
-        let result = f(&mut *self.value.write().await);
-        self.updated.send(()).await.unwrap();
-        result
-    }
-}
-
-impl<T> Receiver<T> {
-    pub async fn updated(&self) {
-        let _ = self.updated.recv().await;
-    }
-
-    pub async fn read<'a>(&'a self) -> impl 'a + Deref<Target = T> {
-        self.value.read().await
-    }
-}
-
-// TODO: These implementations are broken because they only handle a single update.
-impl<T: 'static + Clone> Receiver<T> {
-    pub fn notify_model_on_change<M: 'static + Entity>(&self, ctx: &mut ModelContext<M>) {
-        let watch = self.clone();
-        ctx.spawn(async move { watch.updated().await }, |_, _, ctx| {
-            ctx.notify()
-        })
-        .detach();
-    }
-
-    pub fn notify_view_on_change<V: 'static + View>(&self, ctx: &mut ViewContext<V>) {
-        let watch = self.clone();
-        ctx.spawn(async move { watch.updated().await }, |_, _, ctx| {
-            ctx.notify()
-        })
-        .detach();
-    }
-}
-
-pub fn channel<T>(value: T) -> (Sender<T>, Receiver<T>) {
-    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)
-}

zed/src/workspace.rs

@@ -4,7 +4,6 @@ use crate::{
     editor::{Buffer, BufferView},
     settings::Settings,
     time::ReplicaId,
-    watch::{self, Receiver},
     worktree::{FileHandle, Worktree, WorktreeHandle},
 };
 use futures_core::{future::LocalBoxFuture, Future};
@@ -16,6 +15,7 @@ use gpui::{
 use log::error;
 pub use pane::*;
 pub use pane_group::*;
+use postage::watch;
 use smol::prelude::*;
 use std::{collections::HashMap, path::PathBuf};
 use std::{
@@ -43,7 +43,7 @@ pub struct OpenParams {
     pub settings: watch::Receiver<Settings>,
 }
 
-fn open(settings: &Receiver<Settings>, ctx: &mut MutableAppContext) {
+fn open(settings: &watch::Receiver<Settings>, ctx: &mut MutableAppContext) {
     let settings = settings.clone();
     ctx.prompt_for_paths(
         PathPromptOptions {
@@ -182,7 +182,7 @@ impl<T: Item> WeakItemHandle for WeakModelHandle<T> {
     fn add_view(
         &self,
         window_id: usize,
-        settings: Receiver<Settings>,
+        settings: watch::Receiver<Settings>,
         ctx: &mut MutableAppContext,
     ) -> Option<Box<dyn ItemViewHandle>> {
         if let Some(handle) = self.upgrade(ctx.as_ref()) {

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),