Skip to content

Commit

Permalink
workspace: Fix inconsistent paths order serialization (#19232)
Browse files Browse the repository at this point in the history 
Release Notes:

- Fixed inconsistent serialization of workspace paths order
  • Loading branch information
@eth0net @osiewicz
eth0net authored and osiewicz committed Oct 18, 2024
1 parent 2b4801e commit c4b90ab
Show file tree
Hide file tree
Showing 5 changed files with 107 additions and 33 deletions.
1 change: 1 addition & 0 deletions Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/recent_projects/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 7 additions & 4 deletions crates/recent_projects/src/recent_projects.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -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::<Vec<_>>()
.join(""),
SerializedWorkspaceLocation::DevServer(dev_server_project) => {
Expand Down Expand Up @@ -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()),
Expand Down
105 changes: 83 additions & 22 deletions crates/workspace/src/persistence.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,8 @@ impl WorkspaceDb {
&self,
worktree_roots: &[P],
) -> Option<SerializedWorkspace> {
// 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
Expand Down Expand Up @@ -833,8 +835,8 @@ impl WorkspaceDb {
}

query! {
fn session_workspaces(session_id: String) -> Result<Vec<(LocalPaths, Option<u64>, Option<u64>)>> {
SELECT local_paths, window_id, ssh_project_id
fn session_workspaces(session_id: String) -> Result<Vec<(LocalPaths, LocalPathsOrder, Option<u64>, Option<u64>)>> {
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
Expand Down Expand Up @@ -971,7 +973,7 @@ impl WorkspaceDb {
) -> Result<Vec<SerializedWorkspaceLocation>> {
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 {
Expand All @@ -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)));
}
}
Expand Down Expand Up @@ -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<P: AsRef<Path>>(
Expand Down Expand Up @@ -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(),
Expand All @@ -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]),
),
);
}

Expand Down
22 changes: 15 additions & 7 deletions crates/workspace/src/workspace.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::<Vec<_>>();
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);
Expand Down

0 comments on commit c4b90ab

Please sign in to comment.