From 75948e536ffdf2598e21fd2e7c4e21b26733e326 Mon Sep 17 00:00:00 2001 From: Elliot Thomas Date: Thu, 18 Jul 2024 08:41:29 +0100 Subject: [PATCH] Fix worktree order serialization (#14676) Fixes an issue in the serialization of workspaces that lead to incorrect ordering of worktrees and refactors some other parts of the code to use the new method. Release Notes: - N/A --- crates/recent_projects/src/recent_projects.rs | 4 +- crates/workspace/src/persistence.rs | 15 ++-- crates/workspace/src/persistence/model.rs | 83 +++++++++++++++++-- crates/workspace/src/workspace.rs | 13 +-- 4 files changed, 88 insertions(+), 27 deletions(-) diff --git a/crates/recent_projects/src/recent_projects.rs b/crates/recent_projects/src/recent_projects.rs index 799a832715..513a75bd46 100644 --- a/crates/recent_projects/src/recent_projects.rs +++ b/crates/recent_projects/src/recent_projects.rs @@ -707,7 +707,7 @@ mod tests { use project::{project_settings::ProjectSettings, Project}; use serde_json::json; use settings::SettingsStore; - use workspace::{open_paths, AppState, LocalPaths}; + use workspace::{open_paths, AppState}; use super::*; @@ -782,7 +782,7 @@ mod tests { }]; delegate.set_workspaces(vec![( WorkspaceId::default(), - LocalPaths::new(vec!["/test/path/"]).into(), + SerializedWorkspaceLocation::from_local_paths(vec!["/test/path/"]), )]); }); }) diff --git a/crates/workspace/src/persistence.rs b/crates/workspace/src/persistence.rs index 8fcadcf4f5..49cd16fca7 100644 --- a/crates/workspace/src/persistence.rs +++ b/crates/workspace/src/persistence.rs @@ -1059,7 +1059,7 @@ mod tests { let mut workspace_1 = SerializedWorkspace { id: WorkspaceId(1), - location: LocalPaths::new(["/tmp", "/tmp2"]).into(), + location: SerializedWorkspaceLocation::from_local_paths(["/tmp", "/tmp2"]), center_group: Default::default(), window_bounds: Default::default(), display: Default::default(), @@ -1069,7 +1069,7 @@ mod tests { let workspace_2 = SerializedWorkspace { id: WorkspaceId(2), - location: LocalPaths::new(["/tmp"]).into(), + location: SerializedWorkspaceLocation::from_local_paths(["/tmp"]), center_group: Default::default(), window_bounds: Default::default(), display: Default::default(), @@ -1095,7 +1095,7 @@ mod tests { }) .await; - workspace_1.location = LocalPaths::new(["/tmp", "/tmp3"]).into(); + workspace_1.location = SerializedWorkspaceLocation::from_local_paths(["/tmp", "/tmp3"]); db.save_workspace(workspace_1.clone()).await; db.save_workspace(workspace_1).await; db.save_workspace(workspace_2).await; @@ -1213,7 +1213,7 @@ mod tests { let mut workspace_2 = SerializedWorkspace { id: WorkspaceId(2), - location: LocalPaths::new(["/tmp"]).into(), + location: SerializedWorkspaceLocation::from_local_paths(["/tmp"]), center_group: Default::default(), window_bounds: Default::default(), display: Default::default(), @@ -1239,7 +1239,7 @@ mod tests { assert_eq!(db.workspace_for_roots(&["/tmp3", "/tmp2", "/tmp4"]), None); // Test 'mutate' case of updating a pre-existing id - workspace_2.location = LocalPaths::new(["/tmp", "/tmp2"]).into(); + workspace_2.location = SerializedWorkspaceLocation::from_local_paths(["/tmp", "/tmp2"]); db.save_workspace(workspace_2.clone()).await; assert_eq!( @@ -1268,7 +1268,8 @@ mod tests { ); // Make sure that updating paths differently also works - workspace_3.location = LocalPaths::new(["/tmp3", "/tmp4", "/tmp2"]).into(); + workspace_3.location = + SerializedWorkspaceLocation::from_local_paths(["/tmp3", "/tmp4", "/tmp2"]); db.save_workspace(workspace_3.clone()).await; assert_eq!(db.workspace_for_roots(&["/tmp2", "tmp"]), None); assert_eq!( @@ -1287,7 +1288,7 @@ mod tests { ) -> SerializedWorkspace { SerializedWorkspace { id: WorkspaceId(4), - location: LocalPaths::new(workspace_id).into(), + location: SerializedWorkspaceLocation::from_local_paths(workspace_id), center_group: center_group.clone(), window_bounds: Default::default(), display: Default::default(), diff --git a/crates/workspace/src/persistence/model.rs b/crates/workspace/src/persistence/model.rs index 2ee5bc2240..4fb5d48749 100644 --- a/crates/workspace/src/persistence/model.rs +++ b/crates/workspace/src/persistence/model.rs @@ -47,13 +47,6 @@ impl LocalPaths { } } -impl From for SerializedWorkspaceLocation { - fn from(local_paths: LocalPaths) -> Self { - let order = LocalPathsOrder::default_for_paths(&local_paths); - Self::Local(local_paths, order) - } -} - impl StaticColumnCount for LocalPaths {} impl Bind for &LocalPaths { fn bind(&self, statement: &Statement, start_index: i32) -> Result { @@ -155,6 +148,63 @@ pub enum SerializedWorkspaceLocation { DevServer(SerializedDevServerProject), } +impl SerializedWorkspaceLocation { + /// Create a new `SerializedWorkspaceLocation` from a list of local paths. + /// + /// The paths will be sorted and the order will be stored in the `LocalPathsOrder` struct. + /// + /// # Examples + /// + /// ``` + /// use std::path::Path; + /// use zed_workspace::SerializedWorkspaceLocation; + /// + /// let location = SerializedWorkspaceLocation::from_local_paths(vec![ + /// Path::new("path/to/workspace1"), + /// Path::new("path/to/workspace2"), + /// ]); + /// assert_eq!(location, SerializedWorkspaceLocation::Local( + /// LocalPaths::new(vec![ + /// Path::new("path/to/workspace1"), + /// Path::new("path/to/workspace2"), + /// ]), + /// LocalPathsOrder::new(vec![0, 1]), + /// )); + /// ``` + /// + /// ``` + /// use std::path::Path; + /// use zed_workspace::SerializedWorkspaceLocation; + /// + /// let location = SerializedWorkspaceLocation::from_local_paths(vec![ + /// Path::new("path/to/workspace2"), + /// Path::new("path/to/workspace1"), + /// ]); + /// + /// assert_eq!(location, SerializedWorkspaceLocation::Local( + /// LocalPaths::new(vec![ + /// Path::new("path/to/workspace1"), + /// Path::new("path/to/workspace2"), + /// ]), + /// LocalPathsOrder::new(vec![1, 0]), + /// )); + /// ``` + pub fn from_local_paths>(paths: impl IntoIterator) -> Self { + let mut indexed_paths: Vec<_> = paths + .into_iter() + .map(|p| p.as_ref().to_path_buf()) + .enumerate() + .collect(); + + indexed_paths.sort_by(|(_, a), (_, b)| a.cmp(b)); + + let sorted_paths: Vec<_> = indexed_paths.iter().map(|(_, path)| path.clone()).collect(); + let order: Vec<_> = indexed_paths.iter().map(|(index, _)| *index).collect(); + + Self::Local(LocalPaths::new(sorted_paths), LocalPathsOrder::new(order)) + } +} + #[derive(Debug, PartialEq, Clone)] pub(crate) struct SerializedWorkspace { pub(crate) id: WorkspaceId, @@ -454,3 +504,22 @@ impl Column for SerializedItem { )) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_serialize_local_paths() { + let paths = vec!["b", "a", "c"]; + let serialized = SerializedWorkspaceLocation::from_local_paths(paths); + + assert_eq!( + serialized, + SerializedWorkspaceLocation::Local( + LocalPaths::new(vec!["a", "b", "c"]), + LocalPathsOrder::new(vec![1, 0, 2]) + ) + ); + } +} diff --git a/crates/workspace/src/workspace.rs b/crates/workspace/src/workspace.rs index 0aec91a622..695d5f136b 100644 --- a/crates/workspace/src/workspace.rs +++ b/crates/workspace/src/workspace.rs @@ -94,11 +94,11 @@ pub use workspace_settings::{ AutosaveSetting, RestoreOnStartupBehaviour, TabBarSettings, WorkspaceSettings, }; +use crate::notifications::NotificationId; use crate::persistence::{ model::{DockData, DockStructure, SerializedItem, SerializedPane, SerializedPaneGroup}, SerializedAxis, }; -use crate::{notifications::NotificationId, persistence::model::LocalPathsOrder}; lazy_static! { static ref ZED_WINDOW_SIZE: Option> = env::var("ZED_WINDOW_SIZE") @@ -3943,16 +3943,7 @@ impl Workspace { let location = if let Some(local_paths) = self.local_paths(cx) { if !local_paths.is_empty() { - let (order, paths): (Vec<_>, Vec<_>) = local_paths - .iter() - .enumerate() - .sorted_by(|a, b| a.1.cmp(b.1)) - .unzip(); - - Some(SerializedWorkspaceLocation::Local( - LocalPaths::new(paths), - LocalPathsOrder::new(order), - )) + Some(SerializedWorkspaceLocation::from_local_paths(local_paths)) } else { None }