Fix worktree order serialization (#14676)

Elliot Thomas created

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

Change summary

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(-)

Detailed changes

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/"]),
                     )]);
                 });
             })

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(),

crates/workspace/src/persistence/model.rs 🔗

@@ -47,13 +47,6 @@ impl LocalPaths {
     }
 }
 
-impl From<LocalPaths> 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<i32> {
@@ -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<P: AsRef<Path>>(paths: impl IntoIterator<Item = P>) -> 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])
+            )
+        );
+    }
+}

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<Size<Pixels>> = 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
             }