From 84fd7e26e3dc869f527721cbb8a8b47567cd3c49 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Tue, 31 Mar 2026 16:17:49 -0700 Subject: [PATCH] Cleanups Co-authored-by: Mikayla Maki Co-authored-by: Max Brunsfeld --- crates/agent_ui/src/agent_panel.rs | 13 +- crates/agent_ui/src/conversation_view.rs | 1 - crates/recent_projects/src/recent_projects.rs | 13 +- crates/settings_ui/src/settings_ui.rs | 1 - crates/sidebar/src/project_group_builder.rs | 1 - crates/sidebar/src/sidebar.rs | 44 +++---- crates/sidebar/src/sidebar_tests.rs | 114 +++++++++--------- crates/title_bar/src/title_bar.rs | 2 - crates/workspace/src/multi_workspace.rs | 59 +++++---- crates/workspace/src/multi_workspace_tests.rs | 56 ++++++--- crates/workspace/src/persistence.rs | 25 +++- crates/workspace/src/workspace.rs | 44 ++++--- crates/zed/src/visual_test_runner.rs | 3 +- crates/zed/src/zed.rs | 57 ++++++--- 14 files changed, 242 insertions(+), 191 deletions(-) diff --git a/crates/agent_ui/src/agent_panel.rs b/crates/agent_ui/src/agent_panel.rs index 35900387b8123f8c160352bab0d98a5f31c553dd..83a5ae9e538d31fd332ca6b96711d1c34049c37c 100644 --- a/crates/agent_ui/src/agent_panel.rs +++ b/crates/agent_ui/src/agent_panel.rs @@ -5031,7 +5031,7 @@ mod tests { multi_workspace .read_with(cx, |multi_workspace, _cx| { assert_eq!( - multi_workspace.workspaces().len(), + multi_workspace.len(), 1, "LocalProject should not create a new workspace" ); @@ -5394,16 +5394,15 @@ mod tests { .read_with(cx, |multi_workspace, cx| { // There should be more than one workspace now (the original + the new worktree). assert!( - multi_workspace.workspaces().len() > 1, + multi_workspace.len() > 1, "expected a new workspace to have been created, found {}", - multi_workspace.workspaces().len(), + multi_workspace.len(), ); // Check the newest workspace's panel for the correct agent. - let workspaces = multi_workspace.workspaces(); - let new_workspace = workspaces - .iter() - .find(|ws| ws.entity_id() != workspace.entity_id()) + let new_workspace = multi_workspace + .workspaces() + .find(|ws| *ws != workspace) .expect("should find the new workspace"); let new_panel = new_workspace .read(cx) diff --git a/crates/agent_ui/src/conversation_view.rs b/crates/agent_ui/src/conversation_view.rs index c13b6d29f92c273b36b948c90f5f5c6f1b659970..c169bcbc123204edf41ab75cfdf4dc0aee569911 100644 --- a/crates/agent_ui/src/conversation_view.rs +++ b/crates/agent_ui/src/conversation_view.rs @@ -3354,7 +3354,6 @@ pub(crate) mod tests { // Verify workspace1 is no longer the active workspace multi_workspace_handle .read_with(cx, |mw, _cx| { - assert_eq!(mw.active_workspace_index(), 1); assert_ne!(mw.workspace(), &workspace1); }) .unwrap(); diff --git a/crates/recent_projects/src/recent_projects.rs b/crates/recent_projects/src/recent_projects.rs index 4dc06036ef8416fd859cc815ab090ba5896c0040..6a9efe78e448b8015bb8b0d2b8c3d47f2744634b 100644 --- a/crates/recent_projects/src/recent_projects.rs +++ b/crates/recent_projects/src/recent_projects.rs @@ -354,7 +354,6 @@ pub fn init(cx: &mut App) { .update(cx, |multi_workspace, window, cx| { let sibling_workspace_ids: HashSet = multi_workspace .workspaces() - .iter() .filter_map(|ws| ws.read(cx).database_id()) .collect(); @@ -1034,9 +1033,7 @@ impl PickerDelegate for RecentProjectsDelegate { .update(cx, |multi_workspace, window, cx| { let workspace = multi_workspace .workspaces() - .iter() - .find(|ws| ws.read(cx).database_id() == Some(workspace_id)) - .cloned(); + .find(|ws| ws.read(cx).database_id() == Some(workspace_id)); if let Some(workspace) = workspace { multi_workspace.activate(workspace, window, cx); } @@ -1816,9 +1813,7 @@ impl RecentProjectsDelegate { .update(cx, |multi_workspace, window, cx| { let workspace = multi_workspace .workspaces() - .iter() - .find(|ws| ws.read(cx).database_id() == Some(workspace_id)) - .cloned(); + .find(|ws| ws.read(cx).database_id() == Some(workspace_id)); if let Some(workspace) = workspace { multi_workspace.remove(&workspace, window, cx); } @@ -2027,7 +2022,9 @@ mod tests { ); assert!( - !multi_workspace.workspaces().contains(&dirty_workspace), + !multi_workspace + .workspaces() + .any(|workspace| workspace == dirty_workspace), "The original dirty workspace should have been replaced" ); diff --git a/crates/settings_ui/src/settings_ui.rs b/crates/settings_ui/src/settings_ui.rs index 89268b66f4c2f20411358eb63925187c6c3f382d..2fb1eca2e9685bd73e1823e1921cced067ffc6b0 100644 --- a/crates/settings_ui/src/settings_ui.rs +++ b/crates/settings_ui/src/settings_ui.rs @@ -3752,7 +3752,6 @@ fn all_projects( .flat_map(|multi_workspace| { multi_workspace .workspaces() - .iter() .map(|workspace| workspace.read(cx).project().clone()) .collect::>() }), diff --git a/crates/sidebar/src/project_group_builder.rs b/crates/sidebar/src/project_group_builder.rs index 7b5646f40c888a986aefc13d25cad7d80477616a..322dddab51c993cb342abd20f6c6866ff68afda5 100644 --- a/crates/sidebar/src/project_group_builder.rs +++ b/crates/sidebar/src/project_group_builder.rs @@ -15,7 +15,6 @@ use std::{ }; use gpui::{App, Entity}; -use ui::SharedString; use workspace::{MultiWorkspace, PathList, ProjectGroupKey, Workspace}; #[derive(Default)] diff --git a/crates/sidebar/src/sidebar.rs b/crates/sidebar/src/sidebar.rs index dc5aef6191a118105b6f40247df3361cbbff48cc..01f6ab9ea956fa0981517f6f5f219b5e44a03bfa 100644 --- a/crates/sidebar/src/sidebar.rs +++ b/crates/sidebar/src/sidebar.rs @@ -443,7 +443,7 @@ impl Sidebar { }) .detach(); - let workspaces = multi_workspace.read(cx).workspaces().to_vec(); + let workspaces = multi_workspace.read(cx).workspaces().collect::>(); cx.defer_in(window, move |this, window, cx| { for workspace in &workspaces { this.subscribe_to_workspace(workspace, window, cx); @@ -697,7 +697,7 @@ impl Sidebar { return; }; let mw = multi_workspace.read(cx); - let workspaces = mw.workspaces().to_vec(); + let workspaces = mw.workspaces().collect::>(); let active_workspace = Some(mw.active_workspace()); let agent_server_store = workspaces @@ -1582,7 +1582,7 @@ impl Sidebar { let workspace_count = multi_workspace .upgrade() - .map_or(0, |mw| mw.read(cx).workspaces().len()); + .map_or(0, |mw| mw.read(cx).workspaces().count()); let menu = if workspace_count > 1 { let workspace_for_move = workspace.clone(); let multi_workspace_for_move = multi_workspace.clone(); @@ -1967,9 +1967,7 @@ impl Sidebar { let workspace = window.read(cx).ok().and_then(|multi_workspace| { multi_workspace .workspaces() - .iter() .find(|workspace| predicate(workspace, cx)) - .cloned() })?; Some((window, workspace)) }) @@ -1984,9 +1982,7 @@ impl Sidebar { multi_workspace .read(cx) .workspaces() - .iter() .find(|workspace| predicate(workspace, cx)) - .cloned() }) } @@ -2317,7 +2313,7 @@ impl Sidebar { return; }; - let workspaces = multi_workspace.read(cx).workspaces().to_vec(); + let workspaces = multi_workspace.read(cx).workspaces().collect::>(); for workspace in workspaces { if let Some(agent_panel) = workspace.read(cx).panel::(cx) { let cancelled = @@ -2911,7 +2907,6 @@ impl Sidebar { .map(|mw| { mw.read(cx) .workspaces() - .iter() .filter_map(|ws| ws.read(cx).database_id()) .collect() }) @@ -3341,12 +3336,9 @@ impl Sidebar { } fn active_workspace(&self, cx: &App) -> Option> { - self.multi_workspace.upgrade().and_then(|w| { - w.read(cx) - .workspaces() - .get(w.read(cx).active_workspace_index()) - .cloned() - }) + self.multi_workspace + .upgrade() + .map(|w| w.read(cx).active_workspace()) } fn show_thread_import_modal(&mut self, window: &mut Window, cx: &mut Context) { @@ -3454,12 +3446,11 @@ impl Sidebar { } fn show_archive(&mut self, window: &mut Window, cx: &mut Context) { - let Some(active_workspace) = self.multi_workspace.upgrade().and_then(|w| { - w.read(cx) - .workspaces() - .get(w.read(cx).active_workspace_index()) - .cloned() - }) else { + let Some(active_workspace) = self + .multi_workspace + .upgrade() + .map(|w| w.read(cx).active_workspace()) + else { return; }; let Some(agent_panel) = active_workspace.read(cx).panel::(cx) else { @@ -3764,21 +3755,18 @@ pub fn dump_workspace_info( let multi_workspace = workspace.multi_workspace().and_then(|weak| weak.upgrade()); let workspaces: Vec> = match &multi_workspace { - Some(mw) => mw.read(cx).workspaces().to_vec(), + Some(mw) => mw.read(cx).workspaces().collect::>(), None => vec![this_entity.clone()], }; - let active_index = multi_workspace + let active_workspace = multi_workspace .as_ref() - .map(|mw| mw.read(cx).active_workspace_index()); + .map(|mw| mw.read(cx).active_workspace()); writeln!(output, "MultiWorkspace: {} workspace(s)", workspaces.len()).ok(); - if let Some(index) = active_index { - writeln!(output, "Active workspace index: {index}").ok(); - } writeln!(output).ok(); for (index, ws) in workspaces.iter().enumerate() { - let is_active = active_index == Some(index); + let is_active = active_workspace.as_ref() == Some(ws); writeln!( output, "--- Workspace {index}{} ---", diff --git a/crates/sidebar/src/sidebar_tests.rs b/crates/sidebar/src/sidebar_tests.rs index 18fa8c9375c8d9c30ca3c1369613c0385f1cc94d..213ae4b68f90eed2c11e70348d6eb01fa33119fd 100644 --- a/crates/sidebar/src/sidebar_tests.rs +++ b/crates/sidebar/src/sidebar_tests.rs @@ -476,7 +476,7 @@ async fn test_workspace_lifecycle(cx: &mut TestAppContext) { // Remove the second workspace multi_workspace.update_in(cx, |mw, window, cx| { - let workspace = mw.workspaces()[1].clone(); + let workspace = mw.workspaces().nth(1).unwrap().clone(); mw.remove(&workspace, window, cx); }); cx.run_until_parked(); @@ -1826,13 +1826,13 @@ async fn test_confirm_on_historical_thread_activates_workspace(cx: &mut TestAppC // Switch to workspace 1 so we can verify the confirm switches back. multi_workspace.update_in(cx, |mw, window, cx| { - let workspace = mw.workspaces()[1].clone(); + let workspace = mw.workspaces().nth(1).unwrap().clone(); mw.activate(workspace, window, cx); }); cx.run_until_parked(); assert_eq!( - multi_workspace.read_with(cx, |mw, _| mw.active_workspace_index()), - 1 + multi_workspace.read_with(cx, |mw, _| mw.active_workspace()), + multi_workspace.read_with(cx, |mw, _| mw.workspaces().nth(1).unwrap()) ); // Confirm on the historical (non-live) thread at index 1. @@ -1846,8 +1846,8 @@ async fn test_confirm_on_historical_thread_activates_workspace(cx: &mut TestAppC cx.run_until_parked(); assert_eq!( - multi_workspace.read_with(cx, |mw, _| mw.active_workspace_index()), - 0 + multi_workspace.read_with(cx, |mw, _| mw.active_workspace()), + multi_workspace.read_with(cx, |mw, _| mw.workspaces().nth(0).unwrap()) ); } @@ -1994,7 +1994,8 @@ async fn test_focused_thread_tracks_user_intent(cx: &mut TestAppContext) { let panel_b = add_agent_panel(&workspace_b, cx); cx.run_until_parked(); - let workspace_a = multi_workspace.read_with(cx, |mw, _cx| mw.workspaces()[0].clone()); + let workspace_a = + multi_workspace.read_with(cx, |mw, _cx| mw.workspaces().nth(0).unwrap().clone()); // ── 1. Initial state: focused thread derived from active panel ───── sidebar.read_with(cx, |sidebar, _cx| { @@ -2091,7 +2092,7 @@ async fn test_focused_thread_tracks_user_intent(cx: &mut TestAppContext) { }); multi_workspace.update_in(cx, |mw, window, cx| { - let workspace = mw.workspaces()[0].clone(); + let workspace = mw.workspaces().nth(0).unwrap().clone(); mw.activate(workspace, window, cx); }); cx.run_until_parked(); @@ -2146,8 +2147,8 @@ async fn test_focused_thread_tracks_user_intent(cx: &mut TestAppContext) { // Switching workspaces via the multi_workspace (simulates clicking // a workspace header) should clear focused_thread. multi_workspace.update_in(cx, |mw, window, cx| { - if let Some(index) = mw.workspaces().iter().position(|w| w == &workspace_b) { - let workspace = mw.workspaces()[index].clone(); + let workspace = mw.workspaces().find(|w| w == &workspace_b); + if let Some(workspace) = workspace { mw.activate(workspace, window, cx); } }); @@ -2455,7 +2456,7 @@ async fn test_cmd_n_shows_new_thread_entry_in_absorbed_worktree(cx: &mut TestApp // Switch to the worktree workspace. multi_workspace.update_in(cx, |mw, window, cx| { - let workspace = mw.workspaces()[1].clone(); + let workspace = mw.workspaces().nth(1).unwrap().clone(); mw.activate(workspace, window, cx); }); @@ -3128,7 +3129,7 @@ async fn test_absorbed_worktree_running_thread_shows_live_status(cx: &mut TestAp // Switch back to the main workspace before setting up the sidebar. multi_workspace.update_in(cx, |mw, window, cx| { - let workspace = mw.workspaces()[0].clone(); + let workspace = mw.workspaces().nth(0).unwrap().clone(); mw.activate(workspace, window, cx); }); @@ -3239,7 +3240,7 @@ async fn test_absorbed_worktree_completion_triggers_notification(cx: &mut TestAp let worktree_panel = add_agent_panel(&worktree_workspace, cx); multi_workspace.update_in(cx, |mw, window, cx| { - let workspace = mw.workspaces()[0].clone(); + let workspace = mw.workspaces().nth(0).unwrap().clone(); mw.activate(workspace, window, cx); }); @@ -3354,7 +3355,7 @@ async fn test_clicking_worktree_thread_opens_workspace_when_none_exists(cx: &mut // Only 1 workspace should exist. assert_eq!( - multi_workspace.read_with(cx, |mw, _| mw.workspaces().len()), + multi_workspace.read_with(cx, |mw, _| mw.workspaces().count()), 1, ); @@ -3371,11 +3372,11 @@ async fn test_clicking_worktree_thread_opens_workspace_when_none_exists(cx: &mut // A new workspace should have been created for the worktree path. let new_workspace = multi_workspace.read_with(cx, |mw, _| { assert_eq!( - mw.workspaces().len(), + mw.workspaces().count(), 2, "confirming a worktree thread without a workspace should open one", ); - mw.workspaces()[1].clone() + mw.workspaces().nth(1).unwrap().clone() }); let new_path_list = @@ -3599,7 +3600,7 @@ async fn test_clicking_absorbed_worktree_thread_activates_worktree_workspace( // Activate the main workspace before setting up the sidebar. multi_workspace.update_in(cx, |mw, window, cx| { - let workspace = mw.workspaces()[0].clone(); + let workspace = mw.workspaces().nth(0).unwrap().clone(); mw.activate(workspace, window, cx); }); @@ -3626,8 +3627,8 @@ async fn test_clicking_absorbed_worktree_thread_activates_worktree_workspace( .expect("should find the worktree thread entry"); assert_eq!( - multi_workspace.read_with(cx, |mw, _| mw.active_workspace_index()), - 0, + multi_workspace.read_with(cx, |mw, _| mw.active_workspace()), + multi_workspace.read_with(cx, |mw, _| mw.workspaces().nth(0).unwrap()), "main workspace should be active initially" ); @@ -3642,9 +3643,7 @@ async fn test_clicking_absorbed_worktree_thread_activates_worktree_workspace( cx.run_until_parked(); // The worktree workspace should now be active, not the main one. - let active_workspace = multi_workspace.read_with(cx, |mw, _| { - mw.workspaces()[mw.active_workspace_index()].clone() - }); + let active_workspace = multi_workspace.read_with(cx, |mw, _| mw.active_workspace().clone()); assert_eq!( active_workspace, worktree_workspace, "clicking an absorbed worktree thread should activate the worktree workspace" @@ -3684,13 +3683,13 @@ async fn test_activate_archived_thread_with_saved_paths_activates_matching_works // Ensure workspace A is active. multi_workspace.update_in(cx, |mw, window, cx| { - let workspace = mw.workspaces()[0].clone(); + let workspace = mw.workspaces().nth(0).unwrap().clone(); mw.activate(workspace, window, cx); }); cx.run_until_parked(); assert_eq!( - multi_workspace.read_with(cx, |mw, _| mw.active_workspace_index()), - 0 + multi_workspace.read_with(cx, |mw, _| mw.active_workspace()), + multi_workspace.read_with(cx, |mw, _| mw.workspaces().nth(0).unwrap()) ); // Call activate_archived_thread – should resolve saved paths and @@ -3713,8 +3712,8 @@ async fn test_activate_archived_thread_with_saved_paths_activates_matching_works cx.run_until_parked(); assert_eq!( - multi_workspace.read_with(cx, |mw, _| mw.active_workspace_index()), - 1, + multi_workspace.read_with(cx, |mw, _| mw.active_workspace()), + multi_workspace.read_with(cx, |mw, _| mw.workspaces().nth(1).unwrap()), "should have activated the workspace matching the saved path_list" ); } @@ -3747,13 +3746,13 @@ async fn test_activate_archived_thread_cwd_fallback_with_matching_workspace( // Start with workspace A active. multi_workspace.update_in(cx, |mw, window, cx| { - let workspace = mw.workspaces()[0].clone(); + let workspace = mw.workspaces().nth(0).unwrap().clone(); mw.activate(workspace, window, cx); }); cx.run_until_parked(); assert_eq!( - multi_workspace.read_with(cx, |mw, _| mw.active_workspace_index()), - 0 + multi_workspace.read_with(cx, |mw, _| mw.active_workspace()), + multi_workspace.read_with(cx, |mw, _| mw.workspaces().nth(0).unwrap()) ); // No thread saved to the store – cwd is the only path hint. @@ -3775,8 +3774,8 @@ async fn test_activate_archived_thread_cwd_fallback_with_matching_workspace( cx.run_until_parked(); assert_eq!( - multi_workspace.read_with(cx, |mw, _| mw.active_workspace_index()), - 1, + multi_workspace.read_with(cx, |mw, _| mw.active_workspace()), + multi_workspace.read_with(cx, |mw, _| mw.workspaces().nth(1).unwrap()), "should have activated the workspace matching the cwd" ); } @@ -3809,13 +3808,13 @@ async fn test_activate_archived_thread_no_paths_no_cwd_uses_active_workspace( // Activate workspace B (index 1) to make it the active one. multi_workspace.update_in(cx, |mw, window, cx| { - let workspace = mw.workspaces()[1].clone(); + let workspace = mw.workspaces().nth(1).unwrap().clone(); mw.activate(workspace, window, cx); }); cx.run_until_parked(); assert_eq!( - multi_workspace.read_with(cx, |mw, _| mw.active_workspace_index()), - 1 + multi_workspace.read_with(cx, |mw, _| mw.active_workspace()), + multi_workspace.read_with(cx, |mw, _| mw.workspaces().nth(1).unwrap()) ); // No saved thread, no cwd – should fall back to the active workspace. @@ -3837,8 +3836,8 @@ async fn test_activate_archived_thread_no_paths_no_cwd_uses_active_workspace( cx.run_until_parked(); assert_eq!( - multi_workspace.read_with(cx, |mw, _| mw.active_workspace_index()), - 1, + multi_workspace.read_with(cx, |mw, _| mw.active_workspace()), + multi_workspace.read_with(cx, |mw, _| mw.workspaces().nth(1).unwrap()), "should have stayed on the active workspace when no path info is available" ); } @@ -3868,7 +3867,7 @@ async fn test_activate_archived_thread_saved_paths_opens_new_workspace(cx: &mut let session_id = acp::SessionId::new(Arc::from("archived-new-ws")); assert_eq!( - multi_workspace.read_with(cx, |mw, _| mw.workspaces().len()), + multi_workspace.read_with(cx, |mw, _| mw.workspaces().count()), 1, "should start with one workspace" ); @@ -3891,7 +3890,7 @@ async fn test_activate_archived_thread_saved_paths_opens_new_workspace(cx: &mut cx.run_until_parked(); assert_eq!( - multi_workspace.read_with(cx, |mw, _| mw.workspaces().len()), + multi_workspace.read_with(cx, |mw, _| mw.workspaces().count()), 2, "should have opened a second workspace for the archived thread's saved paths" ); @@ -3941,14 +3940,14 @@ async fn test_activate_archived_thread_reuses_workspace_in_another_window(cx: &m assert_eq!( multi_workspace_a - .read_with(cx_a, |mw, _| mw.workspaces().len()) + .read_with(cx_a, |mw, _| mw.workspaces().count()) .unwrap(), 1, "should not add the other window's workspace into the current window" ); assert_eq!( multi_workspace_b - .read_with(cx_a, |mw, _| mw.workspaces().len()) + .read_with(cx_a, |mw, _| mw.workspaces().count()) .unwrap(), 1, "should reuse the existing workspace in the other window" @@ -4017,14 +4016,14 @@ async fn test_activate_archived_thread_reuses_workspace_in_another_window_with_t assert_eq!( multi_workspace_a - .read_with(cx_a, |mw, _| mw.workspaces().len()) + .read_with(cx_a, |mw, _| mw.workspaces().count()) .unwrap(), 1, "should not add the other window's workspace into the current window" ); assert_eq!( multi_workspace_b - .read_with(cx_a, |mw, _| mw.workspaces().len()) + .read_with(cx_a, |mw, _| mw.workspaces().count()) .unwrap(), 1, "should reuse the existing workspace in the other window" @@ -4103,14 +4102,14 @@ async fn test_activate_archived_thread_prefers_current_window_for_matching_paths }); assert_eq!( multi_workspace_a - .read_with(cx_a, |mw, _| mw.workspaces().len()) + .read_with(cx_a, |mw, _| mw.workspaces().count()) .unwrap(), 1, "current window should continue reusing its existing workspace" ); assert_eq!( multi_workspace_b - .read_with(cx_a, |mw, _| mw.workspaces().len()) + .read_with(cx_a, |mw, _| mw.workspaces().count()) .unwrap(), 1, "other windows should not be activated just because they also match the saved paths" @@ -4193,13 +4192,14 @@ async fn test_archive_thread_uses_next_threads_own_workspace(cx: &mut TestAppCon // Activate main workspace so the sidebar tracks the main panel. multi_workspace.update_in(cx, |mw, window, cx| { - let workspace = mw.workspaces()[0].clone(); + let workspace = mw.workspaces().nth(0).unwrap().clone(); mw.activate(workspace, window, cx); }); let sidebar = setup_sidebar(&multi_workspace, cx); - let main_workspace = multi_workspace.read_with(cx, |mw, _| mw.workspaces()[0].clone()); + let main_workspace = + multi_workspace.read_with(cx, |mw, _| mw.workspaces().nth(0).unwrap().clone()); let main_panel = add_agent_panel(&main_workspace, cx); let _worktree_panel = add_agent_panel(&worktree_workspace, cx); @@ -5001,8 +5001,9 @@ mod property_test { ) { match operation { Operation::SaveThread { workspace_index } => { - let workspace = - multi_workspace.read_with(cx, |mw, _| mw.workspaces()[workspace_index].clone()); + let workspace = multi_workspace.read_with(cx, |mw, _| { + mw.workspaces().nth(workspace_index).unwrap().clone() + }); let path_list = workspace .read_with(cx, |workspace, cx| PathList::new(&workspace.root_paths(cx))); save_thread_to_path(state, path_list, cx); @@ -5088,7 +5089,7 @@ mod property_test { } Operation::RemoveWorkspace { index } => { let removed = multi_workspace.update_in(cx, |mw, window, cx| { - let workspace = mw.workspaces()[index].clone(); + let workspace = mw.workspaces().nth(index).unwrap().clone(); mw.remove(&workspace, window, cx) }); if removed { @@ -5102,8 +5103,8 @@ mod property_test { } } Operation::SwitchWorkspace { index } => { - let workspace = - multi_workspace.read_with(cx, |mw, _| mw.workspaces()[index].clone()); + let workspace = multi_workspace + .read_with(cx, |mw, _| mw.workspaces().nth(index).unwrap().clone()); multi_workspace.update_in(cx, |mw, window, cx| { mw.activate(workspace, window, cx); }); @@ -5150,8 +5151,9 @@ mod property_test { .unwrap(); // Re-scan the main workspace's project so it discovers the new worktree. - let main_workspace = - multi_workspace.read_with(cx, |mw, _| mw.workspaces()[workspace_index].clone()); + let main_workspace = multi_workspace.read_with(cx, |mw, _| { + mw.workspaces().nth(workspace_index).unwrap().clone() + }); let main_project = main_workspace.read_with(cx, |ws, _| ws.project().clone()); main_project .update(cx, |p, cx| p.git_scans_complete(cx)) @@ -5198,7 +5200,7 @@ mod property_test { anyhow::bail!("sidebar should still have an associated multi-workspace"); }; - let workspaces = multi_workspace.read(cx).workspaces().to_vec(); + let workspaces = multi_workspace.read(cx).workspaces().collect::>(); // Workspaces with no root paths are not shown because the // sidebar skips empty path lists. All other workspaces should @@ -5236,7 +5238,7 @@ mod property_test { let Some(multi_workspace) = sidebar.multi_workspace.upgrade() else { anyhow::bail!("sidebar should still have an associated multi-workspace"); }; - let workspaces = multi_workspace.read(cx).workspaces().to_vec(); + let workspaces = multi_workspace.read(cx).workspaces().collect::>(); let thread_store = ThreadMetadataStore::global(cx); let sidebar_thread_ids: HashSet = sidebar diff --git a/crates/title_bar/src/title_bar.rs b/crates/title_bar/src/title_bar.rs index 42c348bacb680e2a09586d0dc0279fc8c95d1604..1ee31232bd795ffc7598780aae5191097b40202f 100644 --- a/crates/title_bar/src/title_bar.rs +++ b/crates/title_bar/src/title_bar.rs @@ -751,7 +751,6 @@ impl TitleBar { .map(|mw| { mw.read(cx) .workspaces() - .iter() .filter_map(|ws| ws.read(cx).database_id()) .collect() }) @@ -814,7 +813,6 @@ impl TitleBar { .map(|mw| { mw.read(cx) .workspaces() - .iter() .filter_map(|ws| ws.read(cx).database_id()) .collect() }) diff --git a/crates/workspace/src/multi_workspace.rs b/crates/workspace/src/multi_workspace.rs index 7268a1aebe1a03f3bb5f5bf32ce45e57fce5f580..e6e5d51c3c24bf512380d836a5ca95c1d09bb8dc 100644 --- a/crates/workspace/src/multi_workspace.rs +++ b/crates/workspace/src/multi_workspace.rs @@ -15,7 +15,7 @@ use std::future::Future; use std::path::PathBuf; use std::sync::Arc; use ui::prelude::*; -use util::{ResultExt, debug_panic, path_list::PathList}; +use util::{ResultExt, path_list::PathList}; use zed_actions::agents_sidebar::{MoveWorkspaceToNewWindow, ToggleThreadSwitcher}; use agent_settings::AgentSettings; @@ -435,11 +435,10 @@ impl MultiWorkspace { } } - pub fn workspaces(&self) -> Vec> { + pub fn workspaces(&self) -> impl Iterator> + '_ { self.project_groups .iter() .flat_map(|group| group.workspaces.iter().cloned()) - .collect() } pub fn open_sidebar(&mut self, cx: &mut Context) { @@ -470,8 +469,9 @@ impl MultiWorkspace { pub fn close_window(&mut self, _: &CloseWindow, window: &mut Window, cx: &mut Context) { cx.spawn_in(window, async move |this, cx| { - let workspaces = - this.update(cx, |multi_workspace, _cx| multi_workspace.workspaces())?; + let workspaces: Vec<_> = this.update(cx, |multi_workspace, _cx| { + multi_workspace.workspaces().collect() + })?; for workspace in workspaces { let should_continue = workspace @@ -514,16 +514,6 @@ impl MultiWorkspace { self.active_workspace.clone() } - pub fn active_workspace_index(&self) -> usize { - self.workspaces() - .iter() - .position(|workspace| workspace == &self.active_workspace) - .unwrap_or_else(|| { - debug_panic!("active workspace was not present in project groups"); - 0 - }) - } - /// Adds a workspace to this window without changing which workspace is /// active. pub fn add(&mut self, workspace: Entity, window: &Window, cx: &mut Context) { @@ -535,6 +525,13 @@ impl MultiWorkspace { self.insert_workspace(workspace, window, cx); } + /// Returns the number of workspaces in this multiworkspace + pub fn len(&self) -> usize { + self.project_groups + .iter() + .fold(0, |acc, group| acc + group.workspaces.len()) + } + /// Ensures the workspace is in the multiworkspace and makes it the active one. pub fn activate( &mut self, @@ -571,9 +568,10 @@ impl MultiWorkspace { return; } - if let Some(index) = self.workspaces().iter().position(|w| *w == workspace) { - let changed = self.active_workspace_index() != index; - self.active_workspace = self.workspaces()[index].clone(); + let exists = self.workspaces().any(|w| w == workspace); + if exists { + let changed = self.active_workspace != workspace; + self.active_workspace = workspace; if changed { cx.emit(MultiWorkspaceEvent::ActiveWorkspaceChanged); self.serialize(cx); @@ -611,7 +609,7 @@ impl MultiWorkspace { window: &Window, cx: &mut Context, ) { - if self.workspaces().iter().any(|w| *w == workspace) { + if self.workspaces().any(|w| w == workspace) { return; } @@ -668,15 +666,16 @@ impl MultiWorkspace { } fn cycle_workspace(&mut self, delta: isize, window: &mut Window, cx: &mut Context) { - let workspaces = self.workspaces(); - let count = workspaces.len() as isize; - if count <= 1 { - return; + let count = self.len(); + let current_ix = self + .workspaces() + .position(|w| w == self.active_workspace) + .unwrap_or(0); + let next_ix = ((current_ix as isize + delta).rem_euclid(count as isize)) as usize; + let next_workspace = self.workspaces().nth(next_ix); + if let Some(next_workspace) = next_workspace { + self.activate(next_workspace, window, cx); } - let current = self.active_workspace_index() as isize; - let next = ((current + delta).rem_euclid(count)) as usize; - let workspace = workspaces[next].clone(); - self.activate(workspace, window, cx); } fn next_workspace(&mut self, _: &NextWorkspace, window: &mut Window, cx: &mut Context) { @@ -909,7 +908,7 @@ impl MultiWorkspace { window: &mut Window, cx: &mut Context, ) -> bool { - let all_workspaces = self.workspaces(); + let all_workspaces: Vec<_> = self.workspaces().collect(); if all_workspaces.len() <= 1 { return false; } @@ -928,11 +927,11 @@ impl MultiWorkspace { // If we removed the active workspace, pick a new one. if self.active_workspace == *workspace { - self.active_workspace = self + let workspace = self .workspaces() - .into_iter() .next() .expect("there is always at least one workspace after the len() > 1 check"); + self.active_workspace = workspace; } self.detach_workspace(workspace, cx); diff --git a/crates/workspace/src/multi_workspace_tests.rs b/crates/workspace/src/multi_workspace_tests.rs index 50161121719ec7b2835fd11e389f24860e57d8f5..3726a3c5f57a28527d5f26159ff495ac88cc0e52 100644 --- a/crates/workspace/src/multi_workspace_tests.rs +++ b/crates/workspace/src/multi_workspace_tests.rs @@ -98,7 +98,12 @@ async fn test_replace(cx: &mut TestAppContext) { let (multi_workspace, cx) = cx.add_window_view(|window, cx| MultiWorkspace::test_new(project_a.clone(), window, cx)); - let workspace_a_id = multi_workspace.read_with(cx, |mw, _cx| mw.workspaces()[0].entity_id()); + let workspace_a_id = multi_workspace.read_with(cx, |mw, _cx| { + mw.workspaces() + .next() + .expect("workspace should exist") + .entity_id() + }); // Replace the only workspace (single-workspace case). let workspace_b = multi_workspace.update_in(cx, |mw, window, cx| { @@ -108,14 +113,20 @@ async fn test_replace(cx: &mut TestAppContext) { }); multi_workspace.read_with(cx, |mw, _cx| { - assert_eq!(mw.workspaces().len(), 1); + assert_eq!(mw.len(), 1); assert_eq!( - mw.workspaces()[0].entity_id(), + mw.workspaces() + .next() + .expect("workspace should exist") + .entity_id(), workspace_b.entity_id(), "slot should now be project_b" ); assert_ne!( - mw.workspaces()[0].entity_id(), + mw.workspaces() + .next() + .expect("workspace should exist") + .entity_id(), workspace_a_id, "project_a should be gone" ); @@ -127,8 +138,12 @@ async fn test_replace(cx: &mut TestAppContext) { }); multi_workspace.read_with(cx, |mw, _cx| { - assert_eq!(mw.workspaces().len(), 2); - assert_eq!(mw.active_workspace_index(), 1); + assert_eq!(mw.len(), 2); + assert_eq!( + mw.workspaces() + .position(|workspace| workspace == mw.active_workspace()), + Some(1) + ); }); let workspace_d = multi_workspace.update_in(cx, |mw, window, cx| { @@ -138,15 +153,25 @@ async fn test_replace(cx: &mut TestAppContext) { }); multi_workspace.read_with(cx, |mw, _cx| { - assert_eq!(mw.workspaces().len(), 2, "should still have 2 workspaces"); - assert_eq!(mw.active_workspace_index(), 1); + assert_eq!(mw.len(), 2, "should still have 2 workspaces"); assert_eq!( - mw.workspaces()[1].entity_id(), + mw.workspaces() + .position(|workspace| workspace == mw.active_workspace()), + Some(1) + ); + assert_eq!( + mw.workspaces() + .nth(1) + .expect("no workspace at index 1") + .entity_id(), workspace_d.entity_id(), "active slot should now be project_d" ); assert_ne!( - mw.workspaces()[1].entity_id(), + mw.workspaces() + .nth(1) + .expect("no workspace at index 1") + .entity_id(), workspace_c.entity_id(), "project_c should be gone" ); @@ -158,14 +183,11 @@ async fn test_replace(cx: &mut TestAppContext) { }); multi_workspace.read_with(cx, |mw, _cx| { + assert_eq!(mw.len(), 2, "no workspace should be added or removed"); assert_eq!( - mw.workspaces().len(), - 2, - "no workspace should be added or removed" - ); - assert_eq!( - mw.active_workspace_index(), - 0, + mw.workspaces() + .position(|workspace| workspace == mw.active_workspace()), + Some(0), "should have switched to workspace_b" ); }); diff --git a/crates/workspace/src/persistence.rs b/crates/workspace/src/persistence.rs index 334ad0925fd62a2ea529ed0e755d605924be266c..41edc44bc02e23db52d48e017f17cd6f600f1708 100644 --- a/crates/workspace/src/persistence.rs +++ b/crates/workspace/src/persistence.rs @@ -2541,7 +2541,11 @@ mod tests { // --- Remove the second workspace (index 1) --- multi_workspace.update_in(cx, |mw, window, cx| { - let ws = mw.workspaces()[1].clone(); + let ws = mw + .workspaces() + .nth(1) + .expect("no workspace at index 1") + .clone(); mw.remove(&ws, window, cx); }); @@ -4224,7 +4228,11 @@ mod tests { // Remove workspace at index 1 (the second workspace). multi_workspace.update_in(cx, |mw, window, cx| { - let ws = mw.workspaces()[1].clone(); + let ws = mw + .workspaces() + .nth(1) + .expect("no workspace at index 1") + .clone(); mw.remove(&ws, window, cx); }); @@ -4335,7 +4343,11 @@ mod tests { // Remove workspace2 (index 1). multi_workspace.update_in(cx, |mw, window, cx| { - let ws = mw.workspaces()[1].clone(); + let ws = mw + .workspaces() + .nth(1) + .expect("no workspace at index 1") + .clone(); mw.remove(&ws, window, cx); }); @@ -4419,7 +4431,11 @@ mod tests { // Remove workspace2 — this pushes a task to pending_removal_tasks. multi_workspace.update_in(cx, |mw, window, cx| { - let ws = mw.workspaces()[1].clone(); + let ws = mw + .workspaces() + .nth(1) + .expect("no workspace at index 1") + .clone(); mw.remove(&ws, window, cx); }); @@ -4428,7 +4444,6 @@ mod tests { let all_tasks = multi_workspace.update_in(cx, |mw, window, cx| { let mut tasks: Vec> = mw .workspaces() - .iter() .map(|workspace| { workspace.update(cx, |workspace, cx| { workspace.flush_serialization(window, cx) diff --git a/crates/workspace/src/workspace.rs b/crates/workspace/src/workspace.rs index e704898a1857b3d24f7ce52042f26e9501a4bdba..24c65f2cae1ca240a7c4a78c6640c83ee2c24ccf 100644 --- a/crates/workspace/src/workspace.rs +++ b/crates/workspace/src/workspace.rs @@ -8701,12 +8701,10 @@ pub async fn restore_multiworkspace( if let Some(target_id) = state.active_workspace_id { window_handle .update(cx, |multi_workspace, window, cx| { - let target_index = multi_workspace + let target_workspace = multi_workspace .workspaces() - .iter() - .position(|ws| ws.read(cx).database_id() == Some(target_id)); - let index = target_index.unwrap_or(0); - if let Some(workspace) = multi_workspace.workspaces().get(index).cloned() { + .find(|ws| ws.read(cx).database_id() == Some(target_id)); + if let Some(workspace) = target_workspace { multi_workspace.activate(workspace, window, cx); } }) @@ -8714,7 +8712,8 @@ pub async fn restore_multiworkspace( } else { window_handle .update(cx, |multi_workspace, window, cx| { - if let Some(workspace) = multi_workspace.workspaces().first().cloned() { + let first_workspace = multi_workspace.workspaces().next(); + if let Some(workspace) = first_workspace { multi_workspace.activate(workspace, window, cx); } }) @@ -9119,7 +9118,7 @@ pub fn workspace_windows_for_location( }; multi_workspace.read(cx).is_ok_and(|multi_workspace| { - multi_workspace.workspaces().iter().any(|workspace| { + multi_workspace.workspaces().any(|workspace| { match workspace.read(cx).workspace_location(cx) { WorkspaceLocation::Location(location, _) => { match (&location, serialized_location) { @@ -10753,7 +10752,11 @@ mod tests { // Activate workspace A multi_workspace_handle .update(cx, |mw, window, cx| { - let workspace = mw.workspaces()[0].clone(); + let workspace = mw + .workspaces() + .nth(0) + .expect("no workspace at index 0") + .clone(); mw.activate(workspace, window, cx); }) .unwrap(); @@ -10775,7 +10778,11 @@ mod tests { // Verify workspace A is active multi_workspace_handle .read_with(cx, |mw, _| { - assert_eq!(mw.active_workspace_index(), 0); + assert_eq!( + mw.workspaces() + .position(|workspace| workspace == mw.active_workspace()), + Some(0) + ); }) .unwrap(); @@ -10791,8 +10798,9 @@ mod tests { multi_workspace_handle .read_with(cx, |mw, _| { assert_eq!( - mw.active_workspace_index(), - 1, + mw.workspaces() + .position(|workspace| workspace == mw.active_workspace()), + Some(1), "workspace B should be activated when it prompts" ); }) @@ -14523,7 +14531,7 @@ mod tests { // Switch to workspace A multi_workspace_handle .update(cx, |mw, window, cx| { - let workspace = mw.workspaces()[0].clone(); + let workspace = mw.workspaces().next().expect("no workspace").clone(); mw.activate(workspace, window, cx); }) .unwrap(); @@ -14569,7 +14577,11 @@ mod tests { // Switch to workspace B multi_workspace_handle .update(cx, |mw, window, cx| { - let workspace = mw.workspaces()[1].clone(); + let workspace = mw + .workspaces() + .nth(1) + .expect("no workspace at index 1") + .clone(); mw.activate(workspace, window, cx); }) .unwrap(); @@ -14578,7 +14590,11 @@ mod tests { // Switch back to workspace A multi_workspace_handle .update(cx, |mw, window, cx| { - let workspace = mw.workspaces()[0].clone(); + let workspace = mw + .workspaces() + .nth(0) + .expect("no workspace at index 0") + .clone(); mw.activate(workspace, window, cx); }) .unwrap(); diff --git a/crates/zed/src/visual_test_runner.rs b/crates/zed/src/visual_test_runner.rs index e5713e90df397a01af850af55338897f9d437e55..c242af09cdf65c7c8ee53d98c59d4a1af4072ba2 100644 --- a/crates/zed/src/visual_test_runner.rs +++ b/crates/zed/src/visual_test_runner.rs @@ -73,7 +73,8 @@ fn main() { // Create test files in the real filesystem create_test_files(&project_path); - let test_result = std::panic::catch_unwind(|| run_visual_tests(project_path, update_baseline)); + let test_result = + std::panic::catch_unwind(|| run_visual_tests(project_path.into(), update_baseline)); // Note: We don't delete temp_path here because background worktree tasks may still // be running. The directory will be cleaned up when the process exits or by the OS. diff --git a/crates/zed/src/zed.rs b/crates/zed/src/zed.rs index 01e2354849f3a70399c680c44bd1a3cfbeb64dc4..3a0aa19d79c04a8e6c77ba515fba2e177680ac8b 100644 --- a/crates/zed/src/zed.rs +++ b/crates/zed/src/zed.rs @@ -1348,7 +1348,7 @@ fn quit(_: &Quit, cx: &mut App) { let window = *window; let workspaces = window .update(cx, |multi_workspace, _, _| { - multi_workspace.workspaces().to_vec() + multi_workspace.workspaces().collect::>() }) .log_err(); @@ -2282,7 +2282,6 @@ mod tests { .update(cx, |multi_workspace, window, cx| { let mut tasks = multi_workspace .workspaces() - .iter() .map(|workspace| { workspace.update(cx, |workspace, cx| { workspace.flush_serialization(window, cx) @@ -5355,7 +5354,7 @@ mod tests { let workspace1 = window .read_with(cx, |multi_workspace, _| { - multi_workspace.workspaces()[0].clone() + multi_workspace.workspaces().nth(0).unwrap().clone() }) .unwrap(); @@ -5365,7 +5364,8 @@ mod tests { multi_workspace.activate(workspace3.clone(), window, cx); // Switch back to workspace1 for test setup multi_workspace.activate(workspace1, window, cx); - assert_eq!(multi_workspace.active_workspace_index(), 0); + let workspaces: Vec<_> = multi_workspace.workspaces().collect(); + assert_eq!(multi_workspace.workspace(), &workspaces[0]); }) .unwrap(); @@ -5374,8 +5374,9 @@ mod tests { // Verify setup: 3 workspaces, workspace 0 active, still 1 window window .read_with(cx, |multi_workspace, _| { - assert_eq!(multi_workspace.workspaces().len(), 3); - assert_eq!(multi_workspace.active_workspace_index(), 0); + let workspaces: Vec<_> = multi_workspace.workspaces().collect(); + assert_eq!(workspaces.len(), 3); + assert_eq!(multi_workspace.workspace(), &workspaces[0]); }) .unwrap(); assert_eq!(cx.windows().len(), 1); @@ -5397,9 +5398,10 @@ mod tests { // Verify workspace 2 is active and file opened there window .read_with(cx, |multi_workspace, cx| { + let workspaces: Vec<_> = multi_workspace.workspaces().collect(); assert_eq!( - multi_workspace.active_workspace_index(), - 2, + multi_workspace.workspace(), + &workspaces[2], "Should have switched to workspace 3 which contains /dir3" ); let active_item = multi_workspace @@ -5431,9 +5433,10 @@ mod tests { // Verify workspace 1 is active and file opened there window .read_with(cx, |multi_workspace, cx| { + let workspaces: Vec<_> = multi_workspace.workspaces().collect(); assert_eq!( - multi_workspace.active_workspace_index(), - 1, + multi_workspace.workspace(), + &workspaces[1], "Should have switched to workspace 2 which contains /dir2" ); let active_item = multi_workspace @@ -5480,9 +5483,10 @@ mod tests { // Verify workspace 0 is active and file opened there window .read_with(cx, |multi_workspace, cx| { + let workspaces: Vec<_> = multi_workspace.workspaces().collect(); assert_eq!( - multi_workspace.active_workspace_index(), - 0, + multi_workspace.workspace(), + &workspaces[0], "Should have switched back to workspace 0 which contains /dir1" ); let active_item = multi_workspace @@ -5592,7 +5596,8 @@ mod tests { // Verify workspace1_1 is active window1 .read_with(cx, |multi_workspace, _| { - assert_eq!(multi_workspace.active_workspace_index(), 0); + let workspaces: Vec<_> = multi_workspace.workspaces().collect(); + assert_eq!(multi_workspace.workspace(), &workspaces[0]); }) .unwrap(); @@ -5658,7 +5663,12 @@ mod tests { // Verify workspace1_1 is still active (not workspace1_2 with dirty item) window1 .read_with(cx, |multi_workspace, _| { - assert_eq!(multi_workspace.active_workspace_index(), 0); + assert_eq!( + multi_workspace + .workspaces() + .position(|workspace| workspace == multi_workspace.active_workspace()), + Some(0) + ); }) .unwrap(); @@ -5669,8 +5679,10 @@ mod tests { window1 .read_with(cx, |multi_workspace, _| { assert_eq!( - multi_workspace.active_workspace_index(), - 1, + multi_workspace + .workspaces() + .position(|workspace| workspace == multi_workspace.active_workspace()), + Some(1), "Case 2: Non-active workspace should be activated when it has dirty item" ); }) @@ -5853,7 +5865,11 @@ mod tests { // still be active rather than whichever workspace happened to restore last. window_a .update(cx, |multi_workspace, window, cx| { - let workspace = multi_workspace.workspaces()[0].clone(); + let workspace = multi_workspace + .workspaces() + .nth(0) + .expect("no workspace at index 0") + .clone(); multi_workspace.activate(workspace, window, cx); }) .unwrap(); @@ -5950,7 +5966,9 @@ mod tests { .iter() .map(|window| { window - .read_with(cx, |multi_workspace, _| multi_workspace.workspaces().len()) + .read_with(cx, |multi_workspace, _| { + multi_workspace.workspaces().count() + }) .unwrap() }) .collect(); @@ -5973,7 +5991,6 @@ mod tests { .read_with(cx, |multi_workspace, cx| { multi_workspace .workspaces() - .iter() .map(|ws| ws.read(cx).root_paths(cx)) .collect() }) @@ -6010,7 +6027,7 @@ mod tests { let active = multi_workspace.workspace(); ( active.read(cx).root_paths(cx), - multi_workspace.workspaces().len(), + multi_workspace.workspaces().count(), ) }) .unwrap();