From c4b90abd625b7931f747be0baa4516cfc8fa38d9 Mon Sep 17 00:00:00 2001 From: Elliot Thomas Date: Thu, 17 Oct 2024 16:38:28 +0100 Subject: [PATCH] workspace: Fix inconsistent paths order serialization (#19232) Release Notes: - Fixed inconsistent serialization of workspace paths order --- Cargo.lock | 1 + crates/recent_projects/Cargo.toml | 1 + crates/recent_projects/src/recent_projects.rs | 11 +- crates/workspace/src/persistence.rs | 105 ++++++++++++++---- crates/workspace/src/workspace.rs | 22 ++-- 5 files changed, 107 insertions(+), 33 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index aead615900ab7..b58716cd57439 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8974,6 +8974,7 @@ dependencies = [ "futures 0.3.30", "fuzzy", "gpui", + "itertools 0.13.0", "language", "log", "menu", diff --git a/crates/recent_projects/Cargo.toml b/crates/recent_projects/Cargo.toml index 3dadcbef37509..c954924711e32 100644 --- a/crates/recent_projects/Cargo.toml +++ b/crates/recent_projects/Cargo.toml @@ -22,6 +22,7 @@ file_finder.workspace = true futures.workspace = true fuzzy.workspace = true gpui.workspace = true +itertools.workspace = true log.workspace = true menu.workspace = true ordered-float.workspace = true diff --git a/crates/recent_projects/src/recent_projects.rs b/crates/recent_projects/src/recent_projects.rs index 5fea5d3a5c4ac..6add7a618ad84 100644 --- a/crates/recent_projects/src/recent_projects.rs +++ b/crates/recent_projects/src/recent_projects.rs @@ -13,6 +13,7 @@ use gpui::{ Action, AnyElement, AppContext, DismissEvent, EventEmitter, FocusHandle, FocusableView, Subscription, Task, View, ViewContext, WeakView, }; +use itertools::Itertools; use ordered_float::OrderedFloat; use picker::{ highlighted_match_with_paths::{HighlightedMatchWithPaths, HighlightedText}, @@ -247,8 +248,9 @@ impl PickerDelegate for RecentProjectsDelegate { SerializedWorkspaceLocation::Local(paths, order) => order .order() .iter() - .filter_map(|i| paths.paths().get(*i)) - .map(|path| path.compact().to_string_lossy().into_owned()) + .zip(paths.paths().iter()) + .sorted_by_key(|(i, _)| *i) + .map(|(_, path)| path.compact().to_string_lossy().into_owned()) .collect::>() .join(""), SerializedWorkspaceLocation::DevServer(dev_server_project) => { @@ -447,8 +449,9 @@ impl PickerDelegate for RecentProjectsDelegate { order .order() .iter() - .filter_map(|i| paths.paths().get(*i).cloned()) - .map(|path| path.compact()) + .zip(paths.paths().iter()) + .sorted_by_key(|(i, _)| **i) + .map(|(_, path)| path.compact()) .collect(), ), SerializedWorkspaceLocation::Ssh(ssh_project) => Arc::new(ssh_project.ssh_urls()), diff --git a/crates/workspace/src/persistence.rs b/crates/workspace/src/persistence.rs index 69217fcc45796..d0cb684b52740 100644 --- a/crates/workspace/src/persistence.rs +++ b/crates/workspace/src/persistence.rs @@ -380,6 +380,8 @@ impl WorkspaceDb { &self, worktree_roots: &[P], ) -> Option { + // paths are sorted before db interactions to ensure that the order of the paths + // doesn't affect the workspace selection for existing workspaces let local_paths = LocalPaths::new(worktree_roots); // Note that we re-assign the workspace_id here in case it's empty @@ -833,8 +835,8 @@ impl WorkspaceDb { } query! { - fn session_workspaces(session_id: String) -> Result, Option)>> { - SELECT local_paths, window_id, ssh_project_id + fn session_workspaces(session_id: String) -> Result, Option)>> { + SELECT local_paths, local_paths_order, window_id, ssh_project_id FROM workspaces WHERE session_id = ?1 AND dev_server_project_id IS NULL ORDER BY timestamp DESC @@ -971,7 +973,7 @@ impl WorkspaceDb { ) -> Result> { let mut workspaces = Vec::new(); - for (location, window_id, ssh_project_id) in + for (location, order, window_id, ssh_project_id) in self.session_workspaces(last_session_id.to_owned())? { if let Some(ssh_project_id) = ssh_project_id { @@ -980,8 +982,7 @@ impl WorkspaceDb { } else if location.paths().iter().all(|path| path.exists()) && location.paths().iter().any(|path| path.is_dir()) { - let location = - SerializedWorkspaceLocation::from_local_paths(location.paths().iter()); + let location = SerializedWorkspaceLocation::Local(location, order); workspaces.push((location, window_id.map(WindowId::from))); } } @@ -1603,27 +1604,56 @@ mod tests { window_id: Some(50), }; + let workspace_6 = SerializedWorkspace { + id: WorkspaceId(6), + location: SerializedWorkspaceLocation::Local( + LocalPaths::new(["/tmp6a", "/tmp6b", "/tmp6c"]), + LocalPathsOrder::new([2, 1, 0]), + ), + center_group: Default::default(), + window_bounds: Default::default(), + display: Default::default(), + docks: Default::default(), + centered_layout: false, + session_id: Some("session-id-3".to_owned()), + window_id: Some(60), + }; + db.save_workspace(workspace_1.clone()).await; db.save_workspace(workspace_2.clone()).await; db.save_workspace(workspace_3.clone()).await; db.save_workspace(workspace_4.clone()).await; db.save_workspace(workspace_5.clone()).await; + db.save_workspace(workspace_6.clone()).await; let locations = db.session_workspaces("session-id-1".to_owned()).unwrap(); assert_eq!(locations.len(), 2); assert_eq!(locations[0].0, LocalPaths::new(["/tmp1"])); - assert_eq!(locations[0].1, Some(10)); + assert_eq!(locations[0].1, LocalPathsOrder::new([0])); + assert_eq!(locations[0].2, Some(10)); assert_eq!(locations[1].0, LocalPaths::new(["/tmp2"])); - assert_eq!(locations[1].1, Some(20)); + assert_eq!(locations[1].1, LocalPathsOrder::new([0])); + assert_eq!(locations[1].2, Some(20)); let locations = db.session_workspaces("session-id-2".to_owned()).unwrap(); assert_eq!(locations.len(), 2); assert_eq!(locations[0].0, LocalPaths::new(["/tmp3"])); - assert_eq!(locations[0].1, Some(30)); + assert_eq!(locations[0].1, LocalPathsOrder::new([0])); + assert_eq!(locations[0].2, Some(30)); let empty_paths: Vec<&str> = Vec::new(); assert_eq!(locations[1].0, LocalPaths::new(empty_paths.iter())); - assert_eq!(locations[1].1, Some(50)); - assert_eq!(locations[1].2, Some(ssh_project.id.0)); + assert_eq!(locations[1].1, LocalPathsOrder::new([])); + assert_eq!(locations[1].2, Some(50)); + assert_eq!(locations[1].3, Some(ssh_project.id.0)); + + let locations = db.session_workspaces("session-id-3".to_owned()).unwrap(); + assert_eq!(locations.len(), 1); + assert_eq!( + locations[0].0, + LocalPaths::new(["/tmp6a", "/tmp6b", "/tmp6c"]), + ); + assert_eq!(locations[0].1, LocalPathsOrder::new([2, 1, 0])); + assert_eq!(locations[0].2, Some(60)); } fn default_workspace>( @@ -1654,15 +1684,30 @@ mod tests { WorkspaceDb(open_test_db("test_serializing_workspaces_last_session_workspaces").await); let workspaces = [ - (1, dir1.path().to_str().unwrap(), 9), - (2, dir2.path().to_str().unwrap(), 5), - (3, dir3.path().to_str().unwrap(), 8), - (4, dir4.path().to_str().unwrap(), 2), + (1, vec![dir1.path()], vec![0], 9), + (2, vec![dir2.path()], vec![0], 5), + (3, vec![dir3.path()], vec![0], 8), + (4, vec![dir4.path()], vec![0], 2), + ( + 5, + vec![dir1.path(), dir2.path(), dir3.path()], + vec![0, 1, 2], + 3, + ), + ( + 6, + vec![dir2.path(), dir3.path(), dir4.path()], + vec![2, 1, 0], + 4, + ), ] .into_iter() - .map(|(id, location, window_id)| SerializedWorkspace { + .map(|(id, locations, order, window_id)| SerializedWorkspace { id: WorkspaceId(id), - location: SerializedWorkspaceLocation::from_local_paths([location]), + location: SerializedWorkspaceLocation::Local( + LocalPaths::new(locations), + LocalPathsOrder::new(order), + ), center_group: Default::default(), window_bounds: Default::default(), display: Default::default(), @@ -1681,28 +1726,44 @@ mod tests { WindowId::from(2), // Top WindowId::from(8), WindowId::from(5), - WindowId::from(9), // Bottom + WindowId::from(9), + WindowId::from(3), + WindowId::from(4), // Bottom ])); let have = db .last_session_workspace_locations("one-session", stack) .unwrap(); - assert_eq!(have.len(), 4); + assert_eq!(have.len(), 6); assert_eq!( have[0], - SerializedWorkspaceLocation::from_local_paths(&[dir4.path().to_str().unwrap()]) + SerializedWorkspaceLocation::from_local_paths(&[dir4.path()]) ); assert_eq!( have[1], - SerializedWorkspaceLocation::from_local_paths([dir3.path().to_str().unwrap()]) + SerializedWorkspaceLocation::from_local_paths([dir3.path()]) ); assert_eq!( have[2], - SerializedWorkspaceLocation::from_local_paths([dir2.path().to_str().unwrap()]) + SerializedWorkspaceLocation::from_local_paths([dir2.path()]) ); assert_eq!( have[3], - SerializedWorkspaceLocation::from_local_paths([dir1.path().to_str().unwrap()]) + SerializedWorkspaceLocation::from_local_paths([dir1.path()]) + ); + assert_eq!( + have[4], + SerializedWorkspaceLocation::Local( + LocalPaths::new([dir1.path(), dir2.path(), dir3.path()]), + LocalPathsOrder::new([0, 1, 2]), + ), + ); + assert_eq!( + have[5], + SerializedWorkspaceLocation::Local( + LocalPaths::new([dir2.path(), dir3.path(), dir4.path()]), + LocalPathsOrder::new([2, 1, 0]), + ), ); } diff --git a/crates/workspace/src/workspace.rs b/crates/workspace/src/workspace.rs index c50c3b9f126a7..626d4d9c1defa 100644 --- a/crates/workspace/src/workspace.rs +++ b/crates/workspace/src/workspace.rs @@ -1103,20 +1103,28 @@ impl Workspace { let mut paths_to_open = abs_paths; - let paths_order = serialized_workspace + let workspace_location = serialized_workspace .as_ref() .map(|ws| &ws.location) .and_then(|loc| match loc { - SerializedWorkspaceLocation::Local(_, order) => Some(order.order()), + SerializedWorkspaceLocation::Local(paths, order) => { + Some((paths.paths(), order.order())) + } _ => None, }); - if let Some(paths_order) = paths_order { - paths_to_open = paths_order + if let Some((paths, order)) = workspace_location { + // todo: should probably move this logic to a method on the SerializedWorkspaceLocation + // it's only valid for Local and would be more clear there and be able to be tested + // and reused elsewhere + paths_to_open = order .iter() - .filter_map(|i| paths_to_open.get(*i).cloned()) - .collect::>(); - if paths_order.iter().enumerate().any(|(i, &j)| i != j) { + .zip(paths.iter()) + .sorted_by_key(|(i, _)| *i) + .map(|(_, path)| path.clone()) + .collect(); + + if order.iter().enumerate().any(|(i, &j)| i != j) { project_handle .update(&mut cx, |project, cx| { project.set_worktrees_reordered(true, cx);