Anthony Eid created
Before this PR we wouldn't always serialize workspaces when a
mutliworkspace adds/removes a workspace. This PR fixes this by adding a
test and calling serialization in remove
Before you mark this PR as ready for review, make sure that you have:
- [x] Added a solid test coverage and/or screenshots from doing manual
testing
- [x] Done a self-review taking into account security and performance
aspects
- [ ] Aligned any UI changes with the [UI
checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist)
Release Notes:
- N/Acrates/collab/tests/integration/editor_tests.rs | 2
crates/collab/tests/integration/git_tests.rs | 2
crates/collab/tests/integration/test_server.rs | 4
crates/git_ui/src/worktree_picker.rs | 2
crates/recent_projects/src/remote_connections.rs | 2
crates/recent_projects/src/remote_servers.rs | 2
crates/settings_ui/src/settings_ui.rs | 8 +-
crates/workspace/src/multi_workspace.rs | 28 ++++---
crates/workspace/src/persistence.rs | 66 ++++++++++++++++++
crates/workspace/src/workspace.rs | 10 +-
crates/zed/src/visual_test_runner.rs | 2
11 files changed, 99 insertions(+), 29 deletions(-)
@@ -107,7 +107,7 @@ async fn test_host_disconnect(
cx,
)
});
- MultiWorkspace::new(workspace, cx)
+ MultiWorkspace::new(workspace, window, cx)
});
let cx_b = &mut VisualTestContext::from_window(*window_b, cx_b);
let workspace_b = window_b
@@ -67,7 +67,7 @@ async fn test_project_diff(cx_a: &mut TestAppContext, cx_b: &mut TestAppContext)
cx,
)
});
- MultiWorkspace::new(workspace, cx)
+ MultiWorkspace::new(workspace, window, cx)
});
let cx_b = &mut VisualTestContext::from_window(*window_b, cx_b);
let workspace_b = window_b
@@ -886,7 +886,7 @@ impl TestClient {
let window = cx.add_window(|window, cx| {
window.activate_window();
let workspace = cx.new(|cx| Workspace::new(None, project, app_state, window, cx));
- MultiWorkspace::new(workspace, cx)
+ MultiWorkspace::new(workspace, window, cx)
});
let cx = VisualTestContext::from_window(*window, cx).into_mut();
cx.run_until_parked();
@@ -905,7 +905,7 @@ impl TestClient {
let window = cx.add_window(|window, cx| {
window.activate_window();
let workspace = cx.new(|cx| Workspace::new(None, project, app_state, window, cx));
- MultiWorkspace::new(workspace, cx)
+ MultiWorkspace::new(workspace, window, cx)
});
let cx = VisualTestContext::from_window(*window, cx).into_mut();
let workspace = window
@@ -518,7 +518,7 @@ async fn open_remote_worktree(
workspace.centered_layout = workspace_position.centered_layout;
workspace
});
- cx.new(|cx| MultiWorkspace::new(workspace, cx))
+ cx.new(|cx| MultiWorkspace::new(workspace, window, cx))
})?
};
@@ -233,7 +233,7 @@ pub async fn open_remote_project(
workspace.centered_layout = workspace_position.centered_layout;
workspace
});
- cx.new(|cx| MultiWorkspace::new(workspace, cx))
+ cx.new(|cx| MultiWorkspace::new(workspace, window, cx))
})?;
let workspace = window.update(cx, |multi_workspace, _, _cx| {
multi_workspace.workspace().clone()
@@ -495,7 +495,7 @@ impl ProjectPicker {
telemetry::event!("SSH Project Created");
Workspace::new(None, project.clone(), app_state.clone(), window, cx)
});
- cx.new(|cx| MultiWorkspace::new(workspace, cx))
+ cx.new(|cx| MultiWorkspace::new(workspace, window, cx))
})
.log_err()?;
@@ -4800,7 +4800,7 @@ pub mod test {
cx,
)
});
- MultiWorkspace::new(workspace, cx)
+ MultiWorkspace::new(workspace, window, cx)
});
let (_multi_workspace2, cx) = cx.add_window_view(|window, cx| {
@@ -4813,7 +4813,7 @@ pub mod test {
cx,
)
});
- MultiWorkspace::new(workspace, cx)
+ MultiWorkspace::new(workspace, window, cx)
});
let workspace2_handle = cx.window_handle().downcast::<MultiWorkspace>().unwrap();
@@ -4945,7 +4945,7 @@ pub mod test {
cx,
)
});
- MultiWorkspace::new(workspace, cx)
+ MultiWorkspace::new(workspace, window, cx)
});
let workspace1_handle = cx.window_handle().downcast::<MultiWorkspace>().unwrap();
@@ -4995,7 +4995,7 @@ pub mod test {
cx,
)
});
- MultiWorkspace::new(workspace, cx)
+ MultiWorkspace::new(workspace, window, cx)
});
cx.run_until_parked();
@@ -2,8 +2,8 @@ use anyhow::Result;
use feature_flags::{AgentV2FeatureFlag, FeatureFlagAppExt};
use gpui::{
AnyView, App, Context, DragMoveEvent, Entity, EntityId, EventEmitter, FocusHandle, Focusable,
- ManagedView, MouseButton, Pixels, Render, Subscription, Task, Tiling, Window, actions,
- deferred, px,
+ ManagedView, MouseButton, Pixels, Render, Subscription, Task, Tiling, Window, WindowId,
+ actions, deferred, px,
};
use project::Project;
use std::path::PathBuf;
@@ -93,6 +93,7 @@ impl<T: Sidebar> SidebarHandle for Entity<T> {
}
pub struct MultiWorkspace {
+ window_id: WindowId,
workspaces: Vec<Entity<Workspace>>,
active_workspace_index: usize,
sidebar: Option<Box<dyn SidebarHandle>>,
@@ -101,8 +102,9 @@ pub struct MultiWorkspace {
}
impl MultiWorkspace {
- pub fn new(workspace: Entity<Workspace>, _cx: &mut Context<Self>) -> Self {
+ pub fn new(workspace: Entity<Workspace>, window: &mut Window, _cx: &mut Context<Self>) -> Self {
Self {
+ window_id: window.window_handle().window_id(),
workspaces: vec![workspace],
active_workspace_index: 0,
sidebar: None,
@@ -154,7 +156,7 @@ impl MultiWorkspace {
if self.sidebar_open {
self.close_sidebar(window, cx);
} else {
- self.open_sidebar(window, cx);
+ self.open_sidebar(cx);
if let Some(sidebar) = &self.sidebar {
sidebar.focus(window, cx);
}
@@ -180,21 +182,21 @@ impl MultiWorkspace {
sidebar.focus(window, cx);
}
} else {
- self.open_sidebar(window, cx);
+ self.open_sidebar(cx);
if let Some(sidebar) = &self.sidebar {
sidebar.focus(window, cx);
}
}
}
- pub fn open_sidebar(&mut self, window: &mut Window, cx: &mut Context<Self>) {
+ pub fn open_sidebar(&mut self, cx: &mut Context<Self>) {
self.sidebar_open = true;
for workspace in &self.workspaces {
workspace.update(cx, |workspace, cx| {
workspace.set_workspace_sidebar_open(true, cx);
});
}
- self.serialize(window, cx);
+ self.serialize(cx);
cx.notify();
}
@@ -208,7 +210,7 @@ impl MultiWorkspace {
let pane = self.workspace().read(cx).active_pane().clone();
let pane_focus = pane.read(cx).focus_handle(cx);
window.focus(&pane_focus, cx);
- self.serialize(window, cx);
+ self.serialize(cx);
cx.notify();
}
@@ -239,6 +241,7 @@ impl MultiWorkspace {
let index = self.add_workspace(workspace, cx);
if self.active_workspace_index != index {
self.active_workspace_index = index;
+ self.serialize(cx);
cx.notify();
}
}
@@ -266,7 +269,7 @@ impl MultiWorkspace {
"workspace index out of bounds"
);
self.active_workspace_index = index;
- self.serialize(window, cx);
+ self.serialize(cx);
self.focus_active_workspace(window, cx);
cx.notify();
}
@@ -289,8 +292,8 @@ impl MultiWorkspace {
}
}
- fn serialize(&self, window: &mut Window, cx: &mut App) {
- let window_id = window.window_handle().window_id();
+ fn serialize(&self, cx: &mut App) {
+ let window_id = self.window_id;
let state = crate::persistence::model::MultiWorkspaceState {
active_workspace_id: self.workspace().read(cx).database_id(),
sidebar_open: self.sidebar_open,
@@ -404,7 +407,7 @@ impl MultiWorkspace {
#[cfg(any(test, feature = "test-support"))]
pub fn test_new(project: Entity<Project>, window: &mut Window, cx: &mut Context<Self>) -> Self {
let workspace = cx.new(|cx| Workspace::test_new(project, window, cx));
- Self::new(workspace, cx)
+ Self::new(workspace, window, cx)
}
#[cfg(any(test, feature = "test-support"))]
@@ -453,6 +456,7 @@ impl MultiWorkspace {
}
self.focus_active_workspace(window, cx);
+ self.serialize(cx);
cx.notify();
}
@@ -2355,6 +2355,72 @@ mod tests {
use serde_json::json;
use std::{thread, time::Duration};
+ #[gpui::test]
+ async fn test_multi_workspace_serializes_on_add_and_remove(cx: &mut gpui::TestAppContext) {
+ use crate::multi_workspace::MultiWorkspace;
+ use crate::persistence::read_multi_workspace_state;
+ use feature_flags::FeatureFlagAppExt;
+ use gpui::AppContext as _;
+ use project::Project;
+
+ crate::tests::init_test(cx);
+
+ cx.update(|cx| {
+ cx.set_staff(true);
+ cx.update_flags(true, vec!["agent-v2".to_string()]);
+ });
+
+ let fs = fs::FakeFs::new(cx.executor());
+ let project1 = Project::test(fs.clone(), [], cx).await;
+ let project2 = Project::test(fs.clone(), [], cx).await;
+
+ let (multi_workspace, cx) =
+ cx.add_window_view(|window, cx| MultiWorkspace::test_new(project1.clone(), window, cx));
+
+ multi_workspace.update_in(cx, |mw, _, cx| {
+ mw.set_random_database_id(cx);
+ });
+
+ let window_id =
+ multi_workspace.update_in(cx, |_, window, _cx| window.window_handle().window_id());
+
+ // --- Add a second workspace ---
+ 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, _cx| ws.set_random_database_id());
+ mw.activate(workspace.clone(), cx);
+ workspace
+ });
+
+ // Run background tasks so serialize has a chance to flush.
+ cx.run_until_parked();
+
+ // Read back the persisted state and check that the active workspace ID was written.
+ let state_after_add = read_multi_workspace_state(window_id);
+ let active_workspace2_db_id = workspace2.read_with(cx, |ws, _| ws.database_id());
+ assert_eq!(
+ state_after_add.active_workspace_id, active_workspace2_db_id,
+ "After adding a second workspace, the serialized active_workspace_id should match \
+ the newly activated workspace's database id"
+ );
+
+ // --- Remove the second workspace (index 1) ---
+ multi_workspace.update_in(cx, |mw, window, cx| {
+ mw.remove_workspace(1, window, cx);
+ });
+
+ cx.run_until_parked();
+
+ let state_after_remove = read_multi_workspace_state(window_id);
+ let remaining_db_id =
+ multi_workspace.read_with(cx, |mw, cx| mw.workspace().read(cx).database_id());
+ assert_eq!(
+ state_after_remove.active_workspace_id, remaining_db_id,
+ "After removing a workspace, the serialized active_workspace_id should match \
+ the remaining active workspace's database id"
+ );
+ }
+
#[gpui::test]
async fn test_breakpoints() {
zlog::init_test();
@@ -1876,7 +1876,7 @@ impl Workspace {
workspace
});
- cx.new(|cx| MultiWorkspace::new(workspace, cx))
+ cx.new(|cx| MultiWorkspace::new(workspace, window, cx))
}
})?;
let workspace =
@@ -7972,8 +7972,8 @@ pub async fn restore_multiworkspace(
if state.sidebar_open {
window_handle
- .update(cx, |multi_workspace, window, cx| {
- multi_workspace.open_sidebar(window, cx);
+ .update(cx, |multi_workspace, _, cx| {
+ multi_workspace.open_sidebar(cx);
})
.ok();
}
@@ -8536,7 +8536,7 @@ pub fn open_workspace_by_id(
workspace.centered_layout = centered_layout;
workspace
});
- cx.new(|cx| MultiWorkspace::new(workspace, cx))
+ cx.new(|cx| MultiWorkspace::new(workspace, window, cx))
}
})?;
@@ -9053,7 +9053,7 @@ pub fn join_in_room_project(
let workspace = cx.new(|cx| {
Workspace::new(Default::default(), project, app_state.clone(), window, cx)
});
- cx.new(|cx| MultiWorkspace::new(workspace, cx))
+ cx.new(|cx| MultiWorkspace::new(workspace, window, cx))
})
})?
};
@@ -2574,7 +2574,7 @@ fn run_multi_workspace_sidebar_visual_tests(
Workspace::new(None, project2.clone(), app_state.clone(), window, cx)
});
cx.new(|cx| {
- let mut multi_workspace = MultiWorkspace::new(workspace1, cx);
+ let mut multi_workspace = MultiWorkspace::new(workspace1, window, cx);
multi_workspace.activate(workspace2, cx);
multi_workspace
})