Mikayla Maki and eric created
Remove old multiworkspace commands and fix a bug where multiworkspaces
wouldn't be deserialized from the DB
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
- [x] Aligned any UI changes with the [UI
checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist)
Release Notes:
- N/A
---------
Co-authored-by: eric <eric@zed.dev>crates/sidebar/Cargo.toml | 1
crates/sidebar/src/sidebar.rs | 10
crates/workspace/src/multi_workspace.rs | 138 ++++++--------------------
crates/workspace/src/persistence.rs | 88 +++++-----------
crates/workspace/src/workspace.rs | 8
5 files changed, 69 insertions(+), 176 deletions(-)
@@ -45,6 +45,7 @@ editor.workspace = true
language_model = { workspace = true, features = ["test-support"] }
pretty_assertions.workspace = true
prompt_store.workspace = true
+recent_projects = { workspace = true, features = ["test-support"] }
serde_json.workspace = true
feature_flags.workspace = true
fs = { workspace = true, features = ["test-support"] }
@@ -2904,7 +2904,7 @@ mod tests {
// Add a second workspace
multi_workspace.update_in(cx, |mw, window, cx| {
- mw.create_workspace(window, cx);
+ mw.create_test_workspace(window, cx).detach();
});
cx.run_until_parked();
@@ -3974,7 +3974,7 @@ mod tests {
// Add a second workspace.
multi_workspace.update_in(cx, |mw, window, cx| {
- mw.create_workspace(window, cx);
+ mw.create_test_workspace(window, cx).detach();
});
cx.run_until_parked();
@@ -4066,7 +4066,7 @@ mod tests {
// Add a second workspace.
multi_workspace.update_in(cx, |mw, window, cx| {
- mw.create_workspace(window, cx);
+ mw.create_test_workspace(window, cx).detach();
});
cx.run_until_parked();
@@ -4316,7 +4316,7 @@ mod tests {
let sidebar = setup_sidebar(&multi_workspace, cx);
multi_workspace.update_in(cx, |mw, window, cx| {
- mw.create_workspace(window, cx);
+ mw.create_test_workspace(window, cx).detach();
});
cx.run_until_parked();
@@ -4628,7 +4628,7 @@ mod tests {
});
multi_workspace.update_in(cx, |mw, window, cx| {
- mw.activate_next_workspace(window, cx);
+ mw.activate_index(0, window, cx);
});
cx.run_until_parked();
@@ -5,7 +5,9 @@ use gpui::{
ManagedView, MouseButton, Pixels, Render, Subscription, Task, Tiling, Window, WindowId,
actions, deferred, px,
};
-use project::{DisableAiSettings, Project};
+use project::DisableAiSettings;
+#[cfg(any(test, feature = "test-support"))]
+use project::Project;
use settings::Settings;
use std::future::Future;
use std::path::PathBuf;
@@ -15,19 +17,13 @@ use util::ResultExt;
const SIDEBAR_RESIZE_HANDLE_SIZE: Pixels = px(6.0);
use crate::{
- CloseIntent, CloseWindow, DockPosition, Event as WorkspaceEvent, Item, ModalView, Panel, Toast,
- Workspace, WorkspaceId, client_side_decorations, notifications::NotificationId,
+ CloseIntent, CloseWindow, DockPosition, Event as WorkspaceEvent, Item, ModalView, Panel,
+ Workspace, WorkspaceId, client_side_decorations,
};
actions!(
multi_workspace,
[
- /// Creates a new workspace within the current window.
- NewWorkspaceInWindow,
- /// Switches to the next workspace within the current window.
- NextWorkspaceInWindow,
- /// Switches to the previous workspace within the current window.
- PreviousWorkspaceInWindow,
/// Toggles the workspace switcher sidebar.
ToggleWorkspaceSidebar,
/// Moves focus to or from the workspace sidebar without closing it.
@@ -126,7 +122,6 @@ pub struct MultiWorkspace {
sidebar_open: bool,
pending_removal_tasks: Vec<Task<()>>,
_serialize_task: Option<Task<()>>,
- _create_task: Option<Task<()>>,
_subscriptions: Vec<Subscription>,
}
@@ -138,9 +133,6 @@ impl MultiWorkspace {
if let Some(task) = this._serialize_task.take() {
task.detach();
}
- if let Some(task) = this._create_task.take() {
- task.detach();
- }
for task in std::mem::take(&mut this.pending_removal_tasks) {
task.detach();
}
@@ -161,7 +153,6 @@ impl MultiWorkspace {
sidebar_open: false,
pending_removal_tasks: Vec::new(),
_serialize_task: None,
- _create_task: None,
_subscriptions: vec![
release_subscription,
quit_subscription,
@@ -385,24 +376,6 @@ impl MultiWorkspace {
cx.notify();
}
- pub fn activate_next_workspace(&mut self, window: &mut Window, cx: &mut Context<Self>) {
- if self.workspaces.len() > 1 {
- let next_index = (self.active_workspace_index + 1) % self.workspaces.len();
- self.activate_index(next_index, window, cx);
- }
- }
-
- pub fn activate_previous_workspace(&mut self, window: &mut Window, cx: &mut Context<Self>) {
- if self.workspaces.len() > 1 {
- let prev_index = if self.active_workspace_index == 0 {
- self.workspaces.len() - 1
- } else {
- self.active_workspace_index - 1
- };
- self.activate_index(prev_index, window, cx);
- }
- }
-
fn serialize(&mut self, cx: &mut App) {
let window_id = self.window_id;
let state = crate::persistence::model::MultiWorkspaceState {
@@ -426,9 +399,6 @@ impl MultiWorkspace {
if let Some(task) = self._serialize_task.take() {
tasks.push(task);
}
- if let Some(task) = self._create_task.take() {
- tasks.push(task);
- }
tasks.extend(std::mem::take(&mut self.pending_removal_tasks));
async move {
@@ -531,15 +501,10 @@ impl MultiWorkspace {
}
pub fn take_pending_removal_tasks(&mut self) -> Vec<Task<()>> {
- let mut tasks: Vec<Task<()>> = std::mem::take(&mut self.pending_removal_tasks)
+ let tasks: Vec<Task<()>> = std::mem::take(&mut self.pending_removal_tasks)
.into_iter()
.filter(|task| !task.is_ready())
.collect();
- if let Some(task) = self._create_task.take() {
- if !task.is_ready() {
- tasks.push(task);
- }
- }
tasks
}
@@ -568,10 +533,12 @@ impl MultiWorkspace {
workspace
}
- pub fn create_workspace(&mut self, window: &mut Window, cx: &mut Context<Self>) {
- if !self.multi_workspace_enabled(cx) {
- return;
- }
+ #[cfg(any(test, feature = "test-support"))]
+ pub fn create_test_workspace(
+ &mut self,
+ window: &mut Window,
+ cx: &mut Context<Self>,
+ ) -> Task<()> {
let app_state = self.workspace().read(cx).app_state().clone();
let project = Project::local(
app_state.client.clone(),
@@ -588,53 +555,27 @@ impl MultiWorkspace {
self.focus_active_workspace(window, cx);
let weak_workspace = new_workspace.downgrade();
- self._create_task = Some(cx.spawn_in(window, async move |this, cx| {
- let result = crate::persistence::DB.next_id().await;
- this.update_in(cx, |this, window, cx| match result {
- Ok(workspace_id) => {
- if let Some(workspace) = weak_workspace.upgrade() {
- let session_id = workspace.read(cx).session_id();
- let window_id = window.window_handle().window_id().as_u64();
- workspace.update(cx, |workspace, _cx| {
- workspace.set_database_id(workspace_id);
- });
- cx.background_spawn(async move {
- crate::persistence::DB
- .set_session_binding(workspace_id, session_id, Some(window_id))
- .await
- .log_err();
- })
- .detach();
- } else {
- cx.background_spawn(async move {
- crate::persistence::DB
- .delete_workspace_by_id(workspace_id)
- .await
- .log_err();
- })
- .detach();
- }
- this.serialize(cx);
- }
- Err(error) => {
- log::error!("Failed to create workspace: {error:#}");
- if let Some(index) = weak_workspace
- .upgrade()
- .and_then(|w| this.workspaces.iter().position(|ws| *ws == w))
- {
- this.remove_workspace(index, window, cx);
- }
- this.workspace().update(cx, |workspace, cx| {
- let id = NotificationId::unique::<MultiWorkspace>();
- workspace.show_toast(
- Toast::new(id, format!("Failed to create workspace: {error}")),
- cx,
- );
+ cx.spawn_in(window, async move |this, cx| {
+ let workspace_id = crate::persistence::DB.next_id().await.unwrap();
+ let workspace = weak_workspace.upgrade().unwrap();
+ let task: Task<()> = this
+ .update_in(cx, |this, window, cx| {
+ let session_id = workspace.read(cx).session_id();
+ let window_id = window.window_handle().window_id().as_u64();
+ workspace.update(cx, |workspace, _cx| {
+ workspace.set_database_id(workspace_id);
});
- }
- })
- .log_err();
- }));
+ this.serialize(cx);
+ cx.background_spawn(async move {
+ crate::persistence::DB
+ .set_session_binding(workspace_id, session_id, Some(window_id))
+ .await
+ .log_err();
+ })
+ })
+ .unwrap();
+ task.await
+ })
}
pub fn remove_workspace(&mut self, index: usize, window: &mut Window, cx: &mut Context<Self>) {
@@ -772,21 +713,6 @@ impl Render for MultiWorkspace {
.font(ui_font)
.text_color(text_color)
.on_action(cx.listener(Self::close_window))
- .on_action(
- cx.listener(|this: &mut Self, _: &NewWorkspaceInWindow, window, cx| {
- this.create_workspace(window, cx);
- }),
- )
- .on_action(
- cx.listener(|this: &mut Self, _: &NextWorkspaceInWindow, window, cx| {
- this.activate_next_workspace(window, cx);
- }),
- )
- .on_action(cx.listener(
- |this: &mut Self, _: &PreviousWorkspaceInWindow, window, cx| {
- this.activate_previous_workspace(window, cx);
- },
- ))
.when(self.multi_workspace_enabled(cx), |this| {
this.on_action(cx.listener(
|this: &mut Self, _: &ToggleWorkspaceSidebar, window, cx| {
@@ -2401,6 +2401,14 @@ mod tests {
use serde_json::json;
use std::{thread, time::Duration};
+ /// Creates a unique directory in a FakeFs, returning the path.
+ /// Uses a UUID suffix to avoid collisions with other tests sharing the global DB.
+ async fn unique_test_dir(fs: &fs::FakeFs, prefix: &str) -> PathBuf {
+ let dir = PathBuf::from(format!("/test-dirs/{}-{}", prefix, uuid::Uuid::new_v4()));
+ fs.insert_tree(&dir, json!({})).await;
+ dir
+ }
+
#[gpui::test]
async fn test_multi_workspace_serializes_on_add_and_remove(cx: &mut gpui::TestAppContext) {
use crate::multi_workspace::MultiWorkspace;
@@ -4000,9 +4008,7 @@ mod tests {
}
#[gpui::test]
- async fn test_create_workspace_serializes_active_workspace_id_after_db_id_assigned(
- cx: &mut gpui::TestAppContext,
- ) {
+ async fn test_create_workspace_serialization(cx: &mut gpui::TestAppContext) {
use crate::multi_workspace::MultiWorkspace;
use crate::persistence::read_multi_workspace_state;
use feature_flags::FeatureFlagAppExt;
@@ -4032,72 +4038,30 @@ mod tests {
// Create a new workspace via the MultiWorkspace API (triggers next_id()).
multi_workspace.update_in(cx, |mw, window, cx| {
- mw.create_workspace(window, cx);
+ mw.create_test_workspace(window, cx).detach();
});
// Let the async next_id() and re-serialization tasks complete.
cx.run_until_parked();
- // Read back the multi-workspace state.
- let state = read_multi_workspace_state(window_id);
-
- // The new workspace should now have a database_id, and the multi-workspace
- // state should record it as the active workspace.
+ // The new workspace should now have a database_id.
let new_workspace_db_id =
multi_workspace.read_with(cx, |mw, cx| mw.workspace().read(cx).database_id());
assert!(
new_workspace_db_id.is_some(),
"New workspace should have a database_id after run_until_parked"
);
+
+ // The multi-workspace state should record it as the active workspace.
+ let state = read_multi_workspace_state(window_id);
assert_eq!(
state.active_workspace_id, new_workspace_db_id,
"Serialized active_workspace_id should match the new workspace's database_id"
);
- }
-
- #[gpui::test]
- async fn test_create_workspace_individual_serialization(cx: &mut gpui::TestAppContext) {
- use crate::multi_workspace::MultiWorkspace;
- use feature_flags::FeatureFlagAppExt;
-
- 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 project = Project::test(fs.clone(), [], cx).await;
-
- let (multi_workspace, cx) =
- cx.add_window_view(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx));
-
- multi_workspace.update_in(cx, |mw, _, cx| {
- mw.set_random_database_id(cx);
- });
-
- // Create a new workspace.
- multi_workspace.update_in(cx, |mw, window, cx| {
- mw.create_workspace(window, cx);
- });
-
- cx.run_until_parked();
-
- // Get the new workspace's database_id.
- let new_db_id =
- multi_workspace.read_with(cx, |mw, cx| mw.workspace().read(cx).database_id());
- assert!(
- new_db_id.is_some(),
- "New workspace should have a database_id"
- );
-
- let workspace_id = new_db_id.unwrap();
- // The workspace should have been serialized to the DB with real data
+ // The individual workspace row should exist with real data
// (not just the bare DEFAULT VALUES row from next_id).
+ let workspace_id = new_workspace_db_id.unwrap();
let serialized = DB.workspace_for_id(workspace_id);
assert!(
serialized.is_some(),
@@ -4120,6 +4084,7 @@ mod tests {
});
let fs = fs::FakeFs::new(cx.executor());
+ let dir = unique_test_dir(&fs, "remove").await;
let project1 = Project::test(fs.clone(), [], cx).await;
let project2 = Project::test(fs.clone(), [], cx).await;
@@ -4142,16 +4107,17 @@ mod tests {
});
// Save a full workspace row to the DB directly.
+ let session_id = format!("remove-test-session-{}", Uuid::new_v4());
DB.save_workspace(SerializedWorkspace {
id: workspace2_db_id,
- paths: PathList::new(&["/tmp/remove_test"]),
+ paths: PathList::new(&[&dir]),
location: SerializedWorkspaceLocation::Local,
center_group: Default::default(),
window_bounds: Default::default(),
display: Default::default(),
docks: Default::default(),
centered_layout: false,
- session_id: Some("remove-test-session".to_owned()),
+ session_id: Some(session_id.clone()),
breakpoints: Default::default(),
window_id: Some(99),
user_toolchains: Default::default(),
@@ -4311,6 +4277,7 @@ mod tests {
});
let fs = fs::FakeFs::new(cx.executor());
+ let dir = unique_test_dir(&fs, "pending-removal").await;
let project1 = Project::test(fs.clone(), [], cx).await;
let project2 = Project::test(fs.clone(), [], cx).await;
@@ -4333,16 +4300,17 @@ mod tests {
});
// Save a full workspace row to the DB directly and let it settle.
+ let session_id = format!("pending-removal-session-{}", Uuid::new_v4());
DB.save_workspace(SerializedWorkspace {
id: workspace2_db_id,
- paths: PathList::new(&["/tmp/pending_removal_test"]),
+ paths: PathList::new(&[&dir]),
location: SerializedWorkspaceLocation::Local,
center_group: Default::default(),
window_bounds: Default::default(),
display: Default::default(),
docks: Default::default(),
centered_layout: false,
- session_id: Some("pending-removal-session".to_owned()),
+ session_id: Some(session_id.clone()),
breakpoints: Default::default(),
window_id: Some(88),
user_toolchains: Default::default(),
@@ -4420,11 +4388,9 @@ mod tests {
mw.set_random_database_id(cx);
});
- multi_workspace.update_in(cx, |mw, window, cx| {
- mw.create_workspace(window, cx);
- });
-
- cx.run_until_parked();
+ let task =
+ multi_workspace.update_in(cx, |mw, window, cx| mw.create_test_workspace(window, cx));
+ task.await;
let new_workspace_db_id =
multi_workspace.read_with(cx, |mw, cx| mw.workspace().read(cx).database_id());
@@ -27,9 +27,8 @@ mod workspace_settings;
pub use crate::notifications::NotificationFrame;
pub use dock::Panel;
pub use multi_workspace::{
- DraggedSidebar, FocusWorkspaceSidebar, MultiWorkspace, MultiWorkspaceEvent,
- NewWorkspaceInWindow, NextWorkspaceInWindow, PreviousWorkspaceInWindow, Sidebar, SidebarHandle,
- ToggleWorkspaceSidebar,
+ DraggedSidebar, FocusWorkspaceSidebar, MultiWorkspace, MultiWorkspaceEvent, Sidebar,
+ SidebarHandle, ToggleWorkspaceSidebar,
};
pub use path_list::{PathList, SerializedPathList};
pub use toast_layer::{ToastAction, ToastLayer, ToastView};
@@ -5979,6 +5978,7 @@ impl Workspace {
self.database_id
}
+ #[cfg(any(test, feature = "test-support"))]
pub(crate) fn set_database_id(&mut self, id: WorkspaceId) {
self.database_id = Some(id);
}
@@ -8260,7 +8260,7 @@ pub async fn restore_multiworkspace(
Some(window_handle),
None,
None,
- true,
+ false,
cx,
)
})