Fix macOS workspace session restore when closing window via X button (#60477)
Closes https://github.com/zed-industries/zed/issues/48468 ## Solution This change addresses a macOS shutdown ordering issue where closing the last window with the native close button could clear session state before persistence tasks complete, causing next launch to open no project/untitled. - [crates/workspace/src/workspace.rs](/Users/caleul/Documents/Work/Projects/zed/crates/workspace/src/workspace.rs) - Make `save_last_workspace` dynamic on macOS based on whether closing the last window will quit the app (`on_last_window_closed == quit_app`). - Keeps `save_last_workspace` true only when the window close path is equivalent to quit, so last-session data remains valid only in the intended scenario. - [crates/workspace/src/multi_workspace.rs](/Users/caleul/Documents/Work/Projects/zed/crates/workspace/src/multi_workspace.rs) - In `close_window`, before removing the window, explicitly flush: - workspace serialization tasks - pending workspace removal tasks - multi-workspace serialization task - This prevents losing in-memory state on fast close/quit paths. - [crates/session/src/session.rs](/Users/caleul/Documents/Work/Projects/zed/crates/session/src/session.rs) - Guard `app_will_quit` to only persist window stack when non-empty, preventing empty-stack state from being written during close path that destroys the window stack first. ## Testing - Did you test these changes? - Manual repro/fix confirmed in local testing flow by the previous author. - Please confirm after merge with: 1) open 2–3 projects in workspace sidebar 2) set `on_last_window_closed: "quit_app"` and `restore_on_startup: "last_session"` 3) close with red X 4) relaunch app and verify sessions restore all active workspaces - Are there any parts that need more testing? - Multi-window mixed close scenarios and one-window with dirty non-serializable items. - Regression for non-quit close mode (`on_last_window_closed: "close") to ensure behavior remains unchanged. - How can others test this? - Follow the 4-step repro path above on macOS. - Check logs for `timed out waiting on app_will_quit` in `~/Library/Logs/Zed/Zed.log` and confirm no restore regression. - Platforms tested: - Verified for macOS (target path of bug). - Not validated on Linux/Windows in this patch path. ## Self-Review Checklist - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content adheres to Zed's UI standards (no UI/UX diff here) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Release Notes - Closing last macOS window via the red close button now correctly preserves and restores last-session workspace state when configured to quit on last-window close. --------- Co-authored-by: Kirill Bulatov <kirill@zed.dev>
This commit is contained in:
1 parent
cc090319c1
commit
35ce0efd7f
7 files changed
+296
-38
No files matched your search
@@ -79,6 +79,7 @@ impl AppSession {
|
||||
let mut current_window_stack = Vec::new();
|
||||
loop {
|
||||
if let Some(windows) = cx.update(|cx| window_stack(cx))
|
||||
&& !windows.is_empty()
|
||||
&& windows != current_window_stack
|
||||
{
|
||||
store_window_stack(db.clone(), &windows).await;
|
||||
@@ -103,7 +104,9 @@ impl AppSession {
|
||||
}
|
||||
|
||||
fn app_will_quit(&mut self, cx: &mut Context<Self>) -> Task<()> {
|
||||
if let Some(window_stack) = window_stack(cx) {
|
||||
if let Some(window_stack) = window_stack(cx)
|
||||
&& !window_stack.is_empty()
|
||||
{
|
||||
let db = KeyValueStore::global(cx);
|
||||
cx.background_spawn(async move { store_window_stack(db, &window_stack).await })
|
||||
} else {
|
||||
|
||||
@@ -1457,6 +1457,7 @@ pub mod test {
|
||||
pub save_as_count: usize,
|
||||
pub reload_count: usize,
|
||||
pub is_dirty: bool,
|
||||
pub save_error: Option<String>,
|
||||
pub buffer_kind: ItemBufferKind,
|
||||
pub has_conflict: bool,
|
||||
pub has_deleted_file: bool,
|
||||
@@ -1549,6 +1550,7 @@ pub mod test {
|
||||
save_as_count: 0,
|
||||
reload_count: 0,
|
||||
is_dirty: false,
|
||||
save_error: None,
|
||||
has_conflict: false,
|
||||
has_deleted_file: false,
|
||||
capability: Capability::ReadWrite,
|
||||
@@ -1589,6 +1591,11 @@ pub mod test {
|
||||
self
|
||||
}
|
||||
|
||||
pub fn with_save_error(mut self, message: impl Into<String>) -> Self {
|
||||
self.save_error = Some(message.into());
|
||||
self
|
||||
}
|
||||
|
||||
pub fn with_conflict(mut self, has_conflict: bool) -> Self {
|
||||
self.has_conflict = has_conflict;
|
||||
self
|
||||
@@ -1743,6 +1750,7 @@ pub mod test {
|
||||
save_as_count: self.save_as_count,
|
||||
reload_count: self.reload_count,
|
||||
is_dirty: self.is_dirty,
|
||||
save_error: self.save_error.clone(),
|
||||
buffer_kind: self.buffer_kind,
|
||||
has_conflict: self.has_conflict,
|
||||
has_deleted_file: self.has_deleted_file,
|
||||
@@ -1798,6 +1806,9 @@ pub mod test {
|
||||
_window: &mut Window,
|
||||
cx: &mut Context<Self>,
|
||||
) -> Task<anyhow::Result<()>> {
|
||||
if let Some(error) = &self.save_error {
|
||||
return Task::ready(Err(anyhow::anyhow!("{error}")));
|
||||
}
|
||||
self.save_count += 1;
|
||||
self.is_dirty = false;
|
||||
for item in &self.project_items {
|
||||
|
||||
@@ -564,17 +564,36 @@ impl MultiWorkspace {
|
||||
multi_workspace.workspaces().cloned().collect::<Vec<_>>()
|
||||
})?;
|
||||
|
||||
let mut prepared = anyhow::Ok(true);
|
||||
for workspace in workspaces {
|
||||
let should_continue = workspace
|
||||
.update_in(cx, |workspace, window, cx| {
|
||||
workspace.prepare_to_close(CloseIntent::CloseWindow, window, cx)
|
||||
})?
|
||||
.await?;
|
||||
if !should_continue {
|
||||
return anyhow::Ok(());
|
||||
prepared = match workspace.update_in(cx, |workspace, window, cx| {
|
||||
workspace.prepare_to_close(CloseIntent::CloseWindow, window, cx)
|
||||
}) {
|
||||
Ok(task) => task.await,
|
||||
Err(error) => Err(error),
|
||||
};
|
||||
if !matches!(prepared, Ok(true)) {
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
if !matches!(prepared, Ok(true)) {
|
||||
this.update(cx, |multi_workspace, cx| {
|
||||
for workspace in multi_workspace.workspaces() {
|
||||
workspace.update(cx, |workspace, _| {
|
||||
workspace.removing = false;
|
||||
});
|
||||
}
|
||||
})?;
|
||||
prepared?;
|
||||
return anyhow::Ok(());
|
||||
}
|
||||
|
||||
let flush_tasks = this.update_in(cx, |multi_workspace, window, cx| {
|
||||
multi_workspace.flush_pending_serialization(window, cx)
|
||||
})?;
|
||||
futures::future::join_all(flush_tasks).await;
|
||||
|
||||
cx.update(|window, _cx| {
|
||||
window.remove_window();
|
||||
})?;
|
||||
@@ -1483,6 +1502,22 @@ impl MultiWorkspace {
|
||||
self._serialize_task.take().unwrap_or(Task::ready(()))
|
||||
}
|
||||
|
||||
pub fn flush_pending_serialization(
|
||||
&mut self,
|
||||
window: &mut Window,
|
||||
cx: &mut Context<Self>,
|
||||
) -> Vec<Task<()>> {
|
||||
let mut tasks = Vec::new();
|
||||
for workspace in self.workspaces() {
|
||||
tasks.push(workspace.update(cx, |workspace, cx| {
|
||||
workspace.flush_serialization(window, cx)
|
||||
}));
|
||||
}
|
||||
tasks.append(&mut self.take_pending_removal_tasks());
|
||||
tasks.push(self.flush_serialization());
|
||||
tasks
|
||||
}
|
||||
|
||||
fn app_will_quit(&mut self, _cx: &mut Context<Self>) -> impl Future<Output = ()> + use<> {
|
||||
let mut tasks: Vec<Task<()>> = Vec::new();
|
||||
if let Some(task) = self._serialize_task.take() {
|
||||
|
||||
@@ -5118,6 +5118,137 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_close_window_quit_app_preserves_all_sidebar_workspaces(
|
||||
cx: &mut gpui::TestAppContext,
|
||||
) {
|
||||
crate::tests::init_test(cx);
|
||||
|
||||
cx.update_global::<settings::SettingsStore, ()>(|store, cx| {
|
||||
store.update_user_settings(cx, |settings| {
|
||||
settings.workspace.on_last_window_closed =
|
||||
Some(settings::OnLastWindowClosed::QuitApp);
|
||||
});
|
||||
});
|
||||
|
||||
let fs = fs::FakeFs::new(cx.executor());
|
||||
let dir1 = unique_test_dir(&fs, "close-quit-a").await;
|
||||
let dir2 = unique_test_dir(&fs, "close-quit-b").await;
|
||||
let project1 = Project::test(fs.clone(), [dir1.as_path()], cx).await;
|
||||
let project2 = Project::test(fs.clone(), [dir2.as_path()], cx).await;
|
||||
|
||||
let db = cx.update(|cx| WorkspaceDb::global(cx));
|
||||
let ws1_id = db.next_id().await.unwrap();
|
||||
let ws2_id = db.next_id().await.unwrap();
|
||||
|
||||
let (multi_workspace, cx) =
|
||||
cx.add_window_view(|window, cx| MultiWorkspace::test_new(project1.clone(), window, cx));
|
||||
|
||||
multi_workspace.update(cx, |mw, cx| {
|
||||
mw.open_sidebar(cx);
|
||||
});
|
||||
|
||||
let session_id = format!("close-quit-session-{}", Uuid::new_v4());
|
||||
|
||||
let workspace1 = multi_workspace.read_with(cx, |mw, _| mw.workspace().clone());
|
||||
workspace1.update(cx, |ws, _| {
|
||||
ws.set_database_id(ws1_id);
|
||||
ws.session_id = Some(session_id.clone());
|
||||
});
|
||||
|
||||
let workspace2 = multi_workspace.update_in(cx, |mw, window, cx| {
|
||||
let workspace = cx.new(|cx| crate::Workspace::test_new(project2.clone(), window, cx));
|
||||
workspace.update(cx, |ws, _| {
|
||||
ws.set_database_id(ws2_id);
|
||||
ws.session_id = Some(session_id.clone());
|
||||
});
|
||||
mw.add(workspace.clone(), window, cx);
|
||||
workspace
|
||||
});
|
||||
|
||||
multi_workspace.update_in(cx, |mw, window, cx| {
|
||||
mw.activate(workspace2.clone(), None, window, cx);
|
||||
});
|
||||
cx.run_until_parked();
|
||||
|
||||
multi_workspace.update_in(cx, |mw, window, cx| {
|
||||
mw.close_window(&crate::CloseWindow, window, cx);
|
||||
});
|
||||
cx.run_until_parked();
|
||||
|
||||
let mut restored_ids = db
|
||||
.last_session_workspace_locations(&session_id, None, fs.as_ref())
|
||||
.await
|
||||
.unwrap()
|
||||
.into_iter()
|
||||
.map(|workspace| workspace.workspace_id)
|
||||
.collect::<Vec<_>>();
|
||||
restored_ids.sort_by_key(|id| id.0);
|
||||
assert_eq!(restored_ids, vec![ws1_id, ws2_id]);
|
||||
}
|
||||
|
||||
#[cfg(target_os = "macos")]
|
||||
#[gpui::test]
|
||||
async fn test_close_window_platform_default_still_removes_from_session(
|
||||
cx: &mut gpui::TestAppContext,
|
||||
) {
|
||||
crate::tests::init_test(cx);
|
||||
|
||||
let fs = fs::FakeFs::new(cx.executor());
|
||||
let dir1 = unique_test_dir(&fs, "close-default-a").await;
|
||||
let dir2 = unique_test_dir(&fs, "close-default-b").await;
|
||||
let project1 = Project::test(fs.clone(), [dir1.as_path()], cx).await;
|
||||
let project2 = Project::test(fs.clone(), [dir2.as_path()], cx).await;
|
||||
|
||||
let db = cx.update(|cx| WorkspaceDb::global(cx));
|
||||
let ws1_id = db.next_id().await.unwrap();
|
||||
let ws2_id = db.next_id().await.unwrap();
|
||||
|
||||
let (multi_workspace, cx) =
|
||||
cx.add_window_view(|window, cx| MultiWorkspace::test_new(project1.clone(), window, cx));
|
||||
|
||||
multi_workspace.update(cx, |mw, cx| {
|
||||
mw.open_sidebar(cx);
|
||||
});
|
||||
|
||||
let session_id = format!("close-default-session-{}", Uuid::new_v4());
|
||||
|
||||
let workspace1 = multi_workspace.read_with(cx, |mw, _| mw.workspace().clone());
|
||||
workspace1.update(cx, |ws, _| {
|
||||
ws.set_database_id(ws1_id);
|
||||
ws.session_id = Some(session_id.clone());
|
||||
});
|
||||
|
||||
let workspace2 = multi_workspace.update_in(cx, |mw, window, cx| {
|
||||
let workspace = cx.new(|cx| crate::Workspace::test_new(project2.clone(), window, cx));
|
||||
workspace.update(cx, |ws, _| {
|
||||
ws.set_database_id(ws2_id);
|
||||
ws.session_id = Some(session_id.clone());
|
||||
});
|
||||
mw.add(workspace.clone(), window, cx);
|
||||
workspace
|
||||
});
|
||||
|
||||
multi_workspace.update_in(cx, |mw, window, cx| {
|
||||
mw.activate(workspace2.clone(), None, window, cx);
|
||||
});
|
||||
cx.run_until_parked();
|
||||
|
||||
multi_workspace.update_in(cx, |mw, window, cx| {
|
||||
mw.close_window(&crate::CloseWindow, window, cx);
|
||||
});
|
||||
cx.run_until_parked();
|
||||
|
||||
let restored_ids = db
|
||||
.last_session_workspace_locations(&session_id, None, fs.as_ref())
|
||||
.await
|
||||
.unwrap()
|
||||
.into_iter()
|
||||
.map(|workspace| workspace.workspace_id)
|
||||
.collect::<Vec<_>>();
|
||||
assert_eq!(restored_ids, Vec::new());
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_create_workspace_bounds_observer_uses_fresh_id(cx: &mut gpui::TestAppContext) {
|
||||
crate::tests::init_test(cx);
|
||||
|
||||
@@ -157,7 +157,7 @@ use uuid::Uuid;
|
||||
pub use workspace_settings::{
|
||||
AccessibleMode, AutosaveSetting, BottomDockLayout, EncodingDisplayOptions, FocusFollowsMouse,
|
||||
RestoreOnStartupBehavior, StatusBarSettings, TabBarSettings, WorkspaceSettings,
|
||||
observe_accessible_mode,
|
||||
closing_last_window_quits_app, observe_accessible_mode,
|
||||
};
|
||||
use zed_actions::{Spawn, feedback::FileBugReport, theme::ToggleMode};
|
||||
|
||||
@@ -3391,29 +3391,27 @@ impl Workspace {
|
||||
.count()
|
||||
})?;
|
||||
|
||||
#[cfg(target_os = "macos")]
|
||||
let save_last_workspace = false;
|
||||
|
||||
// On Linux and Windows, closing the last window should restore the last workspace.
|
||||
#[cfg(not(target_os = "macos"))]
|
||||
let save_last_workspace = {
|
||||
let remaining_workspaces = cx.update(|_window, cx| {
|
||||
let (remaining_workspaces, closing_last_window_quits) = cx.update(|window, cx| {
|
||||
let current_window = window.window_handle();
|
||||
let remaining_workspaces =
|
||||
cx.windows()
|
||||
.iter()
|
||||
.into_iter()
|
||||
.filter(|window| *window != current_window)
|
||||
.filter_map(|window| window.downcast::<MultiWorkspace>())
|
||||
.filter_map(|multi_workspace| {
|
||||
multi_workspace
|
||||
.update(cx, |multi_workspace, _, cx| {
|
||||
multi_workspace.workspace().read(cx).removing
|
||||
})
|
||||
.ok()
|
||||
multi_workspace.read(cx).ok().map(|multi_workspace| {
|
||||
multi_workspace.workspace().read(cx).removing
|
||||
})
|
||||
})
|
||||
.filter(|removing| !removing)
|
||||
.count()
|
||||
})?;
|
||||
.count();
|
||||
|
||||
close_intent != CloseIntent::ReplaceWindow && remaining_workspaces == 0
|
||||
};
|
||||
(remaining_workspaces, closing_last_window_quits_app(cx))
|
||||
})?;
|
||||
|
||||
let save_last_workspace = close_intent != CloseIntent::ReplaceWindow
|
||||
&& remaining_workspaces == 0
|
||||
&& closing_last_window_quits;
|
||||
|
||||
if let Some(active_call) = active_call
|
||||
&& workspace_count == 1
|
||||
@@ -7299,14 +7297,16 @@ impl Workspace {
|
||||
let db = WorkspaceDb::global(cx);
|
||||
let kvp = db::kvp::KeyValueStore::global(cx);
|
||||
window.spawn(cx, async move |_| {
|
||||
db.set_window_open_status(
|
||||
let open_status_write = db.set_window_open_status(
|
||||
database_id,
|
||||
window_bounds,
|
||||
display.unwrap_or_default(),
|
||||
)
|
||||
.await
|
||||
.log_err();
|
||||
db.set_session_id(database_id, None).await.log_err();
|
||||
);
|
||||
let session_id_write = db.set_session_id(database_id, None);
|
||||
let (open_status, session_id) =
|
||||
futures::join!(open_status_write, session_id_write);
|
||||
open_status.log_err();
|
||||
session_id.log_err();
|
||||
persistence::write_default_dock_state(&kvp, docks)
|
||||
.await
|
||||
.log_err();
|
||||
@@ -12337,6 +12337,78 @@ mod tests {
|
||||
assert!(task.await.unwrap());
|
||||
}
|
||||
|
||||
#[cfg(target_os = "macos")]
|
||||
#[gpui::test]
|
||||
async fn test_cancelled_close_window_resets_removing(cx: &mut TestAppContext) {
|
||||
init_test(cx);
|
||||
|
||||
cx.update(|cx| {
|
||||
register_serializable_item::<TestItem>(cx);
|
||||
});
|
||||
|
||||
let fs = FakeFs::new(cx.executor());
|
||||
let project = Project::test(fs, None, cx).await;
|
||||
let (multi_workspace, cx) =
|
||||
cx.add_window_view(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx));
|
||||
let workspace = multi_workspace.read_with(cx, |mw, _| mw.workspace().clone());
|
||||
|
||||
let item = cx.new(|cx| {
|
||||
TestItem::new(cx)
|
||||
.with_dirty(true)
|
||||
.with_serialize(|| Some(Task::ready(Ok(()))))
|
||||
});
|
||||
workspace.update_in(cx, |w, window, cx| {
|
||||
w.add_item_to_active_pane(Box::new(item.clone()), None, true, window, cx);
|
||||
});
|
||||
|
||||
multi_workspace.update_in(cx, |mw, window, cx| {
|
||||
mw.close_window(&CloseWindow, window, cx);
|
||||
});
|
||||
cx.executor().run_until_parked();
|
||||
|
||||
assert!(cx.has_pending_prompt());
|
||||
cx.simulate_prompt_answer("Cancel");
|
||||
cx.executor().run_until_parked();
|
||||
|
||||
assert!(!workspace.read_with(cx, |workspace, _| workspace.removing));
|
||||
let remaining_windows = cx.update(|_window, cx| cx.windows().len());
|
||||
assert_eq!(remaining_windows, 1);
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_failed_close_window_resets_removing(cx: &mut TestAppContext) {
|
||||
init_test(cx);
|
||||
|
||||
let fs = FakeFs::new(cx.executor());
|
||||
let project = Project::test(fs, None, cx).await;
|
||||
let (multi_workspace, cx) =
|
||||
cx.add_window_view(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx));
|
||||
let workspace = multi_workspace.read_with(cx, |mw, _| mw.workspace().clone());
|
||||
|
||||
let item = cx.new(|cx| {
|
||||
TestItem::new(cx)
|
||||
.with_dirty(true)
|
||||
.with_save_error("simulated save failure")
|
||||
.with_project_items(&[TestProjectItem::new(1, "1.txt", cx)])
|
||||
});
|
||||
workspace.update_in(cx, |w, window, cx| {
|
||||
w.add_item_to_active_pane(Box::new(item.clone()), None, true, window, cx);
|
||||
});
|
||||
|
||||
multi_workspace.update_in(cx, |mw, window, cx| {
|
||||
mw.close_window(&CloseWindow, window, cx);
|
||||
});
|
||||
cx.executor().run_until_parked();
|
||||
|
||||
assert!(cx.has_pending_prompt());
|
||||
cx.simulate_prompt_answer("Save");
|
||||
cx.executor().run_until_parked();
|
||||
|
||||
assert!(!workspace.read_with(cx, |workspace, _| workspace.removing));
|
||||
let remaining_windows = cx.update(|_window, cx| cx.windows().len());
|
||||
assert_eq!(remaining_windows, 1);
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_quit_without_worktrees_hot_exits(cx: &mut TestAppContext) {
|
||||
init_test(cx);
|
||||
|
||||
@@ -44,6 +44,18 @@ pub struct WorkspaceSettings {
|
||||
pub focus_follows_mouse: FocusFollowsMouse,
|
||||
}
|
||||
|
||||
#[cfg(target_os = "macos")]
|
||||
pub fn closing_last_window_quits_app(cx: &App) -> bool {
|
||||
WorkspaceSettings::get_global(cx)
|
||||
.on_last_window_closed
|
||||
.is_quit_app()
|
||||
}
|
||||
|
||||
#[cfg(not(target_os = "macos"))]
|
||||
pub fn closing_last_window_quits_app(_cx: &App) -> bool {
|
||||
true
|
||||
}
|
||||
|
||||
#[derive(Copy, Clone, Deserialize)]
|
||||
pub struct FocusFollowsMouse {
|
||||
pub enabled: bool,
|
||||
|
||||
@@ -1870,13 +1870,7 @@ fn quit(_: &Quit, cx: &mut App) {
|
||||
for window in &workspace_windows {
|
||||
window
|
||||
.update(cx, |multi_workspace, window, cx| {
|
||||
for workspace in multi_workspace.workspaces() {
|
||||
flush_tasks.push(workspace.update(cx, |workspace, cx| {
|
||||
workspace.flush_serialization(window, cx)
|
||||
}));
|
||||
}
|
||||
flush_tasks.append(&mut multi_workspace.take_pending_removal_tasks());
|
||||
flush_tasks.push(multi_workspace.flush_serialization());
|
||||
flush_tasks.extend(multi_workspace.flush_pending_serialization(window, cx));
|
||||
})
|
||||
.log_err();
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user