gpui: Defer appearance change callback to avoid reentrant borrow (#61789)
### Objective PR #58902 made native macOS window chrome follow the selected theme, but AppKit can synchronously invoke `viewDidChangeEffectiveAppearance` while the settings observer holds an `App` borrow. `handle.update` then fails with `RefCell already borrowed`, leaving `Window::appearance()` and its observers stale. ### Steps to reproduce 1. Set Zed's theme mode to Dark. 2. Set macOS Appearance to Light. 3. Change Zed's theme mode to System. 4. Notice that the window chrome switches to light, but Zed's UI remains dark. https://github.com/user-attachments/assets/f91962cc-76ba-4d8e-b587-2a2343f365ba ### Expectation Zed should switch to its configured light theme when returning to System mode. ### Solution Defer `Window::appearance_changed` to the foreground executor so the current `App` borrow finishes before refreshing the cached appearance and notifying observers. ### Testing - Added a regression test. ### Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [ ] Unsafe blocks (if any) have justifying comments - [x] The content adheres to Zed's UI standards ([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) and [icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md) guidelines) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Release Notes: - N/A
This commit is contained in:
1 parent
f89b733a07
commit
5aca79139a
3 files changed
+59
-10
No files matched your search
@@ -33,8 +33,10 @@ pub(crate) struct TestWindowState {
|
||||
hover_status_change_callback: Option<Box<dyn FnMut(bool)>>,
|
||||
resize_callback: Option<Box<dyn FnMut(Size<Pixels>, f32)>>,
|
||||
moved_callback: Option<Box<dyn FnMut()>>,
|
||||
appearance_change_callback: Option<Box<dyn FnMut()>>,
|
||||
input_handler: Option<PlatformInputHandler>,
|
||||
is_fullscreen: bool,
|
||||
appearance: WindowAppearance,
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
@@ -85,8 +87,10 @@ impl TestWindow {
|
||||
hover_status_change_callback: None,
|
||||
resize_callback: None,
|
||||
moved_callback: None,
|
||||
appearance_change_callback: None,
|
||||
input_handler: None,
|
||||
is_fullscreen: false,
|
||||
appearance: WindowAppearance::Light,
|
||||
})))
|
||||
}
|
||||
|
||||
@@ -113,6 +117,17 @@ impl TestWindow {
|
||||
self.0.lock().active_status_change_callback = Some(callback);
|
||||
}
|
||||
|
||||
pub fn simulate_appearance_change(&self, appearance: WindowAppearance) {
|
||||
let mut lock = self.0.lock();
|
||||
lock.appearance = appearance;
|
||||
let Some(mut callback) = lock.appearance_change_callback.take() else {
|
||||
return;
|
||||
};
|
||||
drop(lock);
|
||||
callback();
|
||||
self.0.lock().appearance_change_callback = Some(callback);
|
||||
}
|
||||
|
||||
pub fn simulate_input(&mut self, event: PlatformInput) -> bool {
|
||||
let mut lock = self.0.lock();
|
||||
let Some(mut callback) = lock.input_callback.take() else {
|
||||
@@ -152,7 +167,7 @@ impl PlatformWindow for TestWindow {
|
||||
}
|
||||
|
||||
fn appearance(&self) -> WindowAppearance {
|
||||
WindowAppearance::Light
|
||||
self.0.lock().appearance
|
||||
}
|
||||
|
||||
fn display(&self) -> Option<std::rc::Rc<dyn crate::PlatformDisplay>> {
|
||||
@@ -290,7 +305,9 @@ impl PlatformWindow for TestWindow {
|
||||
self.0.lock().hit_test_window_control_callback = Some(callback);
|
||||
}
|
||||
|
||||
fn on_appearance_changed(&self, _callback: Box<dyn FnMut()>) {}
|
||||
fn on_appearance_changed(&self, callback: Box<dyn FnMut()>) {
|
||||
self.0.lock().appearance_change_callback = Some(callback);
|
||||
}
|
||||
|
||||
fn draw(&self, scene: &Scene) {
|
||||
let scale_factor = self.scale_factor();
|
||||
|
||||
@@ -1688,11 +1688,19 @@ impl Window {
|
||||
}
|
||||
}));
|
||||
platform_window.on_appearance_changed(Box::new({
|
||||
let mut cx = cx.to_async();
|
||||
let cx = cx.to_async();
|
||||
let foreground_executor = cx.foreground_executor().clone();
|
||||
move || {
|
||||
handle
|
||||
.update(&mut cx, |_, window, cx| window.appearance_changed(cx))
|
||||
.log_err();
|
||||
let mut cx = cx.clone();
|
||||
// Defer the update because changing the AppKit appearance may
|
||||
// synchronously invoke this callback while App is already borrowed.
|
||||
foreground_executor
|
||||
.spawn(async move {
|
||||
handle
|
||||
.update(&mut cx, |_, window, cx| window.appearance_changed(cx))
|
||||
.log_err();
|
||||
})
|
||||
.detach();
|
||||
}
|
||||
}));
|
||||
platform_window.on_button_layout_changed(Box::new({
|
||||
@@ -6681,8 +6689,8 @@ mod tests {
|
||||
|
||||
use crate::{
|
||||
AppContext as _, Bounds, Context, FocusHandle, InteractiveElement as _, IntoElement,
|
||||
ParentElement, Pixels, Render, Styled, TestAppContext, Window, WindowOptions, canvas, div,
|
||||
px, size,
|
||||
ParentElement, Pixels, Render, Styled, TestAppContext, Window, WindowAppearance,
|
||||
WindowOptions, canvas, div, px, size,
|
||||
};
|
||||
|
||||
struct EmptyView;
|
||||
@@ -6743,6 +6751,31 @@ mod tests {
|
||||
.unwrap();
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
fn test_appearance_change_runs_after_app_update(cx: &mut TestAppContext) {
|
||||
let window = cx.add_window(|_, _| EmptyView);
|
||||
let observed_appearance = Rc::new(Cell::new(None));
|
||||
let _subscription = window
|
||||
.update(cx, {
|
||||
let observed_appearance = observed_appearance.clone();
|
||||
move |_, window, _| {
|
||||
window.observe_window_appearance(move |window, _| {
|
||||
observed_appearance.set(Some(window.appearance()));
|
||||
})
|
||||
}
|
||||
})
|
||||
.unwrap();
|
||||
let test_window = cx.test_window(window.into());
|
||||
|
||||
cx.update(|_| {
|
||||
test_window.simulate_appearance_change(WindowAppearance::Dark);
|
||||
assert_eq!(observed_appearance.get(), None);
|
||||
});
|
||||
cx.run_until_parked();
|
||||
|
||||
assert_eq!(observed_appearance.get(), Some(WindowAppearance::Dark));
|
||||
}
|
||||
|
||||
struct RootView {
|
||||
explicit_size: bool,
|
||||
child_bounds: Rc<Cell<Bounds<Pixels>>>,
|
||||
|
||||
@@ -2849,8 +2849,7 @@ extern "C" fn view_did_change_effective_appearance(this: &Object, _: Sel) {
|
||||
}
|
||||
|
||||
// AppKit can relayout the standard traffic light buttons as part of
|
||||
// applying a new appearance. Reapply GPUI's custom position after
|
||||
// notifying appearance observers.
|
||||
// applying a new appearance, so reapply GPUI's custom position.
|
||||
state.lock().move_traffic_light();
|
||||
}
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user