From f1ebad22db0600f172f383dd8b238cbb435d37b3 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Mon, 11 Dec 2023 19:55:55 +0100 Subject: [PATCH 1/4] Defer drawing the scene until macOS's `display_layer` callback Co-Authored-By: Max Brunsfeld --- crates/gpui2/src/app.rs | 20 +---------------- crates/gpui2/src/platform.rs | 5 ++++- crates/gpui2/src/platform/mac/platform.rs | 11 +++++++-- crates/gpui2/src/platform/mac/window.rs | 22 ++++++++++-------- crates/gpui2/src/platform/test/platform.rs | 4 +++- crates/gpui2/src/platform/test/window.rs | 4 +--- crates/gpui2/src/window.rs | 26 +++++++++++++--------- 7 files changed, 46 insertions(+), 46 deletions(-) diff --git a/crates/gpui2/src/app.rs b/crates/gpui2/src/app.rs index 842ac0b704..422654d578 100644 --- a/crates/gpui2/src/app.rs +++ b/crates/gpui2/src/app.rs @@ -9,7 +9,6 @@ use derive_more::{Deref, DerefMut}; pub use entity_map::*; pub use model_context::*; use refineable::Refineable; -use smallvec::SmallVec; use smol::future::FutureExt; #[cfg(any(test, feature = "test-support"))] pub use test_context::*; @@ -596,23 +595,6 @@ impl AppContext { break; } } - - let dirty_window_ids = self - .windows - .iter() - .filter_map(|(_, window)| { - let window = window.as_ref()?; - if window.dirty { - Some(window.handle.clone()) - } else { - None - } - }) - .collect::>(); - - for dirty_window_handle in dirty_window_ids { - dirty_window_handle.update(self, |_, cx| cx.draw()).unwrap(); - } } /// Repeatedly called during `flush_effects` to release any entities whose @@ -731,7 +713,7 @@ impl AppContext { fn apply_refresh_effect(&mut self) { for window in self.windows.values_mut() { if let Some(window) = window.as_mut() { - window.dirty = true; + window.platform_window.invalidate(); } } } diff --git a/crates/gpui2/src/platform.rs b/crates/gpui2/src/platform.rs index 006640af4f..4c14efd632 100644 --- a/crates/gpui2/src/platform.rs +++ b/crates/gpui2/src/platform.rs @@ -46,6 +46,8 @@ pub(crate) fn current_platform() -> Rc { Rc::new(MacPlatform::new()) } +pub type DrawWindow = Box Result>; + pub(crate) trait Platform: 'static { fn background_executor(&self) -> BackgroundExecutor; fn foreground_executor(&self) -> ForegroundExecutor; @@ -66,6 +68,7 @@ pub(crate) trait Platform: 'static { &self, handle: AnyWindowHandle, options: WindowOptions, + draw: DrawWindow, ) -> Box; fn set_display_link_output_callback( @@ -163,7 +166,7 @@ pub trait PlatformWindow { fn on_close(&self, callback: Box); fn on_appearance_changed(&self, callback: Box); fn is_topmost_for_position(&self, position: Point) -> bool; - fn draw(&self, scene: Scene); + fn invalidate(&self); fn sprite_atlas(&self) -> Arc; diff --git a/crates/gpui2/src/platform/mac/platform.rs b/crates/gpui2/src/platform/mac/platform.rs index 2deea545e1..8b83ee5d1f 100644 --- a/crates/gpui2/src/platform/mac/platform.rs +++ b/crates/gpui2/src/platform/mac/platform.rs @@ -3,7 +3,8 @@ use crate::{ Action, AnyWindowHandle, BackgroundExecutor, ClipboardItem, CursorStyle, DisplayId, ForegroundExecutor, InputEvent, Keymap, MacDispatcher, MacDisplay, MacDisplayLinker, MacTextSystem, MacWindow, Menu, MenuItem, PathPromptOptions, Platform, PlatformDisplay, - PlatformTextSystem, PlatformWindow, Result, SemanticVersion, VideoTimestamp, WindowOptions, + PlatformTextSystem, PlatformWindow, Result, Scene, SemanticVersion, VideoTimestamp, + WindowOptions, }; use anyhow::anyhow; use block::ConcreteBlock; @@ -490,8 +491,14 @@ impl Platform for MacPlatform { &self, handle: AnyWindowHandle, options: WindowOptions, + draw: Box Result>, ) -> Box { - Box::new(MacWindow::open(handle, options, self.foreground_executor())) + Box::new(MacWindow::open( + handle, + options, + draw, + self.foreground_executor(), + )) } fn set_display_link_output_callback( diff --git a/crates/gpui2/src/platform/mac/window.rs b/crates/gpui2/src/platform/mac/window.rs index 03ba635327..dcdf616ffe 100644 --- a/crates/gpui2/src/platform/mac/window.rs +++ b/crates/gpui2/src/platform/mac/window.rs @@ -1,10 +1,10 @@ use super::{display_bounds_from_native, ns_string, MacDisplay, MetalRenderer, NSRange}; use crate::{ - display_bounds_to_native, point, px, size, AnyWindowHandle, Bounds, ExternalPaths, + display_bounds_to_native, point, px, size, AnyWindowHandle, Bounds, DrawWindow, ExternalPaths, FileDropEvent, ForegroundExecutor, GlobalPixels, InputEvent, KeyDownEvent, Keystroke, Modifiers, ModifiersChangedEvent, MouseButton, MouseDownEvent, MouseMoveEvent, MouseUpEvent, Pixels, PlatformAtlas, PlatformDisplay, PlatformInputHandler, PlatformWindow, Point, - PromptLevel, Scene, Size, Timer, WindowAppearance, WindowBounds, WindowKind, WindowOptions, + PromptLevel, Size, Timer, WindowAppearance, WindowBounds, WindowKind, WindowOptions, }; use block::ConcreteBlock; use cocoa::{ @@ -45,6 +45,7 @@ use std::{ sync::{Arc, Weak}, time::Duration, }; +use util::ResultExt; const WINDOW_STATE_IVAR: &str = "windowState"; @@ -318,7 +319,7 @@ struct MacWindowState { executor: ForegroundExecutor, native_window: id, renderer: MetalRenderer, - scene_to_render: Option, + draw: Option, kind: WindowKind, event_callback: Option bool>>, activate_callback: Option>, @@ -454,6 +455,7 @@ impl MacWindow { pub fn open( handle: AnyWindowHandle, options: WindowOptions, + draw: DrawWindow, executor: ForegroundExecutor, ) -> Self { unsafe { @@ -545,7 +547,7 @@ impl MacWindow { executor, native_window, renderer: MetalRenderer::new(true), - scene_to_render: None, + draw: Some(draw), kind: options.kind, event_callback: None, activate_callback: None, @@ -958,9 +960,8 @@ impl PlatformWindow for MacWindow { } } - fn draw(&self, scene: Scene) { - let mut this = self.0.lock(); - this.scene_to_render = Some(scene); + fn invalidate(&self) { + let this = self.0.lock(); unsafe { let _: () = msg_send![this.native_window.contentView(), setNeedsDisplay: YES]; } @@ -1442,8 +1443,11 @@ extern "C" fn set_frame_size(this: &Object, _: Sel, size: NSSize) { extern "C" fn display_layer(this: &Object, _: Sel, _: id) { unsafe { let window_state = get_window_state(this); - let mut window_state = window_state.as_ref().lock(); - if let Some(scene) = window_state.scene_to_render.take() { + let mut draw = window_state.lock().draw.take().unwrap(); + let scene = draw().log_err(); + let mut window_state = window_state.lock(); + window_state.draw = Some(draw); + if let Some(scene) = scene { window_state.renderer.draw(&scene); } } diff --git a/crates/gpui2/src/platform/test/platform.rs b/crates/gpui2/src/platform/test/platform.rs index 876120b626..8997eb4902 100644 --- a/crates/gpui2/src/platform/test/platform.rs +++ b/crates/gpui2/src/platform/test/platform.rs @@ -1,6 +1,7 @@ use crate::{ AnyWindowHandle, BackgroundExecutor, ClipboardItem, CursorStyle, DisplayId, ForegroundExecutor, - Keymap, Platform, PlatformDisplay, PlatformTextSystem, TestDisplay, TestWindow, WindowOptions, + Keymap, Platform, PlatformDisplay, PlatformTextSystem, Scene, TestDisplay, TestWindow, + WindowOptions, }; use anyhow::{anyhow, Result}; use collections::VecDeque; @@ -135,6 +136,7 @@ impl Platform for TestPlatform { &self, handle: AnyWindowHandle, options: WindowOptions, + _draw: Box Result>, ) -> Box { *self.active_window.lock() = Some(handle); Box::new(TestWindow::new( diff --git a/crates/gpui2/src/platform/test/window.rs b/crates/gpui2/src/platform/test/window.rs index 9b7ad72472..424b203758 100644 --- a/crates/gpui2/src/platform/test/window.rs +++ b/crates/gpui2/src/platform/test/window.rs @@ -166,9 +166,7 @@ impl PlatformWindow for TestWindow { unimplemented!() } - fn draw(&self, scene: crate::Scene) { - self.current_scene.lock().replace(scene); - } + fn invalidate(&self) {} fn sprite_atlas(&self) -> sync::Arc { self.sprite_atlas.clone() diff --git a/crates/gpui2/src/window.rs b/crates/gpui2/src/window.rs index f98d9820c2..ad583af323 100644 --- a/crates/gpui2/src/window.rs +++ b/crates/gpui2/src/window.rs @@ -7,9 +7,9 @@ use crate::{ ModelContext, Modifiers, MonochromeSprite, MouseButton, MouseMoveEvent, MouseUpEvent, Path, Pixels, PlatformAtlas, PlatformDisplay, PlatformInputHandler, PlatformWindow, Point, PolychromeSprite, PromptLevel, Quad, Render, RenderGlyphParams, RenderImageParams, - RenderSvgParams, ScaledPixels, SceneBuilder, Shadow, SharedString, Size, Style, SubscriberSet, - Subscription, Surface, TaffyLayoutEngine, Task, Underline, UnderlineStyle, View, VisualContext, - WeakView, WindowBounds, WindowOptions, SUBPIXEL_VARIANTS, + RenderSvgParams, ScaledPixels, Scene, SceneBuilder, Shadow, SharedString, Size, Style, + SubscriberSet, Subscription, Surface, TaffyLayoutEngine, Task, Underline, UnderlineStyle, View, + VisualContext, WeakView, WindowBounds, WindowOptions, SUBPIXEL_VARIANTS, }; use anyhow::{anyhow, Context as _, Result}; use collections::HashMap; @@ -224,7 +224,6 @@ pub struct Window { bounds_observers: SubscriberSet<(), AnyObserver>, active: bool, activation_observers: SubscriberSet<(), AnyObserver>, - pub(crate) dirty: bool, pub(crate) last_blur: Option>, pub(crate) focus: Option, } @@ -278,7 +277,14 @@ impl Window { options: WindowOptions, cx: &mut AppContext, ) -> Self { - let platform_window = cx.platform.open_window(handle, options); + let platform_window = cx.platform.open_window( + handle, + options, + Box::new({ + let mut cx = cx.to_async(); + move || handle.update(&mut cx, |_, cx| cx.draw()) + }), + ); let display_id = platform_window.display().id(); let sprite_atlas = platform_window.sprite_atlas(); let mouse_position = platform_window.mouse_position(); @@ -350,7 +356,6 @@ impl Window { bounds_observers: SubscriberSet::new(), active: false, activation_observers: SubscriberSet::new(), - dirty: true, last_blur: None, focus: None, } @@ -401,7 +406,7 @@ impl<'a> WindowContext<'a> { /// Mark the window as dirty, scheduling it to be redrawn on the next frame. pub fn notify(&mut self) { - self.window.dirty = true; + self.window.platform_window.invalidate(); } /// Close this window. @@ -673,7 +678,7 @@ impl<'a> WindowContext<'a> { self.window.viewport_size = self.window.platform_window.content_size(); self.window.bounds = self.window.platform_window.bounds(); self.window.display_id = self.window.platform_window.display().id(); - self.window.dirty = true; + self.notify(); self.window .bounds_observers @@ -1174,7 +1179,7 @@ impl<'a> WindowContext<'a> { } /// Draw pixels to the display for this window based on the contents of its scene. - pub(crate) fn draw(&mut self) { + fn draw(&mut self) -> Scene { self.text_system().start_frame(); self.window.platform_window.clear_input_handler(); self.window.layout_engine.as_mut().unwrap().clear(); @@ -1226,7 +1231,6 @@ impl<'a> WindowContext<'a> { mem::swap(&mut window.rendered_frame, &mut window.next_frame); let scene = self.window.rendered_frame.scene_builder.build(); - self.window.platform_window.draw(scene); let cursor_style = self .window .requested_cursor_style @@ -1234,7 +1238,7 @@ impl<'a> WindowContext<'a> { .unwrap_or(CursorStyle::Arrow); self.platform.set_cursor_style(cursor_style); - self.window.dirty = false; + scene } /// Dispatch a mouse or keyboard event on the window. From 9b94f1483a313fe2fdfad96ca45fbd04a4b50729 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Mon, 11 Dec 2023 20:10:27 +0100 Subject: [PATCH 2/4] Redraw the window at the end of `flush_effects` in tests Co-Authored-By: Max Brunsfeld --- crates/gpui2/src/app.rs | 10 +++++++++- crates/gpui2/src/platform/test/platform.rs | 3 ++- crates/gpui2/src/platform/test/window.rs | 15 +++++++++------ crates/gpui2/src/window.rs | 5 ++++- 4 files changed, 24 insertions(+), 9 deletions(-) diff --git a/crates/gpui2/src/app.rs b/crates/gpui2/src/app.rs index 422654d578..16dc5440bd 100644 --- a/crates/gpui2/src/app.rs +++ b/crates/gpui2/src/app.rs @@ -595,6 +595,14 @@ impl AppContext { break; } } + + for (_, window) in &self.windows { + if let Some(window) = window.as_ref() { + if window.dirty { + window.platform_window.invalidate(); + } + } + } } /// Repeatedly called during `flush_effects` to release any entities whose @@ -713,7 +721,7 @@ impl AppContext { fn apply_refresh_effect(&mut self) { for window in self.windows.values_mut() { if let Some(window) = window.as_mut() { - window.platform_window.invalidate(); + window.dirty = true; } } } diff --git a/crates/gpui2/src/platform/test/platform.rs b/crates/gpui2/src/platform/test/platform.rs index 8997eb4902..2a57842991 100644 --- a/crates/gpui2/src/platform/test/platform.rs +++ b/crates/gpui2/src/platform/test/platform.rs @@ -136,11 +136,12 @@ impl Platform for TestPlatform { &self, handle: AnyWindowHandle, options: WindowOptions, - _draw: Box Result>, + draw: Box Result>, ) -> Box { *self.active_window.lock() = Some(handle); Box::new(TestWindow::new( options, + draw, self.weak.clone(), self.active_display.clone(), )) diff --git a/crates/gpui2/src/platform/test/window.rs b/crates/gpui2/src/platform/test/window.rs index 424b203758..02b3982d12 100644 --- a/crates/gpui2/src/platform/test/window.rs +++ b/crates/gpui2/src/platform/test/window.rs @@ -1,7 +1,7 @@ use crate::{ - px, AtlasKey, AtlasTextureId, AtlasTile, Pixels, PlatformAtlas, PlatformDisplay, - PlatformInputHandler, PlatformWindow, Point, Scene, Size, TestPlatform, TileId, - WindowAppearance, WindowBounds, WindowOptions, + px, AtlasKey, AtlasTextureId, AtlasTile, DrawWindow, Pixels, PlatformAtlas, PlatformDisplay, + PlatformInputHandler, PlatformWindow, Point, Size, TestPlatform, TileId, WindowAppearance, + WindowBounds, WindowOptions, }; use collections::HashMap; use parking_lot::Mutex; @@ -20,7 +20,7 @@ pub(crate) struct TestWindowHandlers { pub struct TestWindow { pub(crate) bounds: WindowBounds, - current_scene: Mutex>, + draw: Mutex, display: Rc, pub(crate) window_title: Option, pub(crate) input_handler: Option>>>, @@ -32,12 +32,13 @@ pub struct TestWindow { impl TestWindow { pub fn new( options: WindowOptions, + draw: DrawWindow, platform: Weak, display: Rc, ) -> Self { Self { bounds: options.bounds, - current_scene: Default::default(), + draw: Mutex::new(draw), display, platform, input_handler: None, @@ -166,7 +167,9 @@ impl PlatformWindow for TestWindow { unimplemented!() } - fn invalidate(&self) {} + fn invalidate(&self) { + (self.draw.lock())().unwrap(); + } fn sprite_atlas(&self) -> sync::Arc { self.sprite_atlas.clone() diff --git a/crates/gpui2/src/window.rs b/crates/gpui2/src/window.rs index ad583af323..06412889a1 100644 --- a/crates/gpui2/src/window.rs +++ b/crates/gpui2/src/window.rs @@ -223,6 +223,7 @@ pub struct Window { bounds: WindowBounds, bounds_observers: SubscriberSet<(), AnyObserver>, active: bool, + pub(crate) dirty: bool, activation_observers: SubscriberSet<(), AnyObserver>, pub(crate) last_blur: Option>, pub(crate) focus: Option, @@ -355,6 +356,7 @@ impl Window { bounds, bounds_observers: SubscriberSet::new(), active: false, + dirty: false, activation_observers: SubscriberSet::new(), last_blur: None, focus: None, @@ -406,7 +408,7 @@ impl<'a> WindowContext<'a> { /// Mark the window as dirty, scheduling it to be redrawn on the next frame. pub fn notify(&mut self) { - self.window.platform_window.invalidate(); + self.window.dirty = true; } /// Close this window. @@ -1237,6 +1239,7 @@ impl<'a> WindowContext<'a> { .take() .unwrap_or(CursorStyle::Arrow); self.platform.set_cursor_style(cursor_style); + self.window.dirty = false; scene } From fbfe108317a7041a4893b3bfb65218146ced7807 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Mon, 11 Dec 2023 12:35:59 -0800 Subject: [PATCH 3/4] Avoid double borrow in tests by drawing windows explicitly in `flush_effects` Co-authored-by: Antonio --- crates/gpui2/src/app.rs | 15 ++++++++++++++- crates/gpui2/src/platform/test/window.rs | 2 +- crates/gpui2/src/window.rs | 2 +- 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/crates/gpui2/src/app.rs b/crates/gpui2/src/app.rs index 16dc5440bd..990dcd4591 100644 --- a/crates/gpui2/src/app.rs +++ b/crates/gpui2/src/app.rs @@ -596,13 +596,26 @@ impl AppContext { } } - for (_, window) in &self.windows { + for window in self.windows.values() { if let Some(window) = window.as_ref() { if window.dirty { window.platform_window.invalidate(); } } } + + #[cfg(any(test, feature = "test-support"))] + for window in self + .windows + .values() + .filter_map(|window| { + let window = window.as_ref()?; + window.dirty.then_some(window.handle) + }) + .collect::>() + { + self.update_window(window, |_, cx| cx.draw()).ok(); + } } /// Repeatedly called during `flush_effects` to release any entities whose diff --git a/crates/gpui2/src/platform/test/window.rs b/crates/gpui2/src/platform/test/window.rs index 02b3982d12..a99629b07a 100644 --- a/crates/gpui2/src/platform/test/window.rs +++ b/crates/gpui2/src/platform/test/window.rs @@ -168,7 +168,7 @@ impl PlatformWindow for TestWindow { } fn invalidate(&self) { - (self.draw.lock())().unwrap(); + // (self.draw.lock())().unwrap(); } fn sprite_atlas(&self) -> sync::Arc { diff --git a/crates/gpui2/src/window.rs b/crates/gpui2/src/window.rs index 06412889a1..c86f232544 100644 --- a/crates/gpui2/src/window.rs +++ b/crates/gpui2/src/window.rs @@ -1181,7 +1181,7 @@ impl<'a> WindowContext<'a> { } /// Draw pixels to the display for this window based on the contents of its scene. - fn draw(&mut self) -> Scene { + pub(crate) fn draw(&mut self) -> Scene { self.text_system().start_frame(); self.window.platform_window.clear_input_handler(); self.window.layout_engine.as_mut().unwrap().clear(); From fdcb413e3321960a80634db310e28520713c3cba Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Mon, 11 Dec 2023 12:56:18 -0800 Subject: [PATCH 4/4] Fix unused field warning --- crates/gpui2/src/app.rs | 2 +- crates/gpui2/src/platform/test/platform.rs | 3 +-- crates/gpui2/src/platform/test/window.rs | 5 +---- 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/crates/gpui2/src/app.rs b/crates/gpui2/src/app.rs index 990dcd4591..8d2ac9c83d 100644 --- a/crates/gpui2/src/app.rs +++ b/crates/gpui2/src/app.rs @@ -614,7 +614,7 @@ impl AppContext { }) .collect::>() { - self.update_window(window, |_, cx| cx.draw()).ok(); + self.update_window(window, |_, cx| cx.draw()).unwrap(); } } diff --git a/crates/gpui2/src/platform/test/platform.rs b/crates/gpui2/src/platform/test/platform.rs index 2a57842991..8997eb4902 100644 --- a/crates/gpui2/src/platform/test/platform.rs +++ b/crates/gpui2/src/platform/test/platform.rs @@ -136,12 +136,11 @@ impl Platform for TestPlatform { &self, handle: AnyWindowHandle, options: WindowOptions, - draw: Box Result>, + _draw: Box Result>, ) -> Box { *self.active_window.lock() = Some(handle); Box::new(TestWindow::new( options, - draw, self.weak.clone(), self.active_display.clone(), )) diff --git a/crates/gpui2/src/platform/test/window.rs b/crates/gpui2/src/platform/test/window.rs index a99629b07a..5af990514f 100644 --- a/crates/gpui2/src/platform/test/window.rs +++ b/crates/gpui2/src/platform/test/window.rs @@ -1,5 +1,5 @@ use crate::{ - px, AtlasKey, AtlasTextureId, AtlasTile, DrawWindow, Pixels, PlatformAtlas, PlatformDisplay, + px, AtlasKey, AtlasTextureId, AtlasTile, Pixels, PlatformAtlas, PlatformDisplay, PlatformInputHandler, PlatformWindow, Point, Size, TestPlatform, TileId, WindowAppearance, WindowBounds, WindowOptions, }; @@ -20,7 +20,6 @@ pub(crate) struct TestWindowHandlers { pub struct TestWindow { pub(crate) bounds: WindowBounds, - draw: Mutex, display: Rc, pub(crate) window_title: Option, pub(crate) input_handler: Option>>>, @@ -32,13 +31,11 @@ pub struct TestWindow { impl TestWindow { pub fn new( options: WindowOptions, - draw: DrawWindow, platform: Weak, display: Rc, ) -> Self { Self { bounds: options.bounds, - draw: Mutex::new(draw), display, platform, input_handler: None,