From 81db8c68b2eb40872f3b888ae9f0a031b2ea4d2a Mon Sep 17 00:00:00 2001 From: Ben Kunkle Date: Tue, 4 Aug 2026 20:48:46 -0600 Subject: [PATCH] workspace: Choose the replacement workspace inside MultiWorkspace::remove (#62141) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Objective Follow-up to #62028. When archiving removed a thread's workspace, the replacement was derived from whichever sidebar row sat adjacent to the archived one. Rows are a flat list with project headers as separators, so archiving the oldest thread in a project selected the next project's newest thread and moved the user out of the project they were working in — and when that neighbor was remote, connected to its host just to pick a fallback. # Solution - `MultiWorkspace::remove` now chooses the replacement itself from a `RemovalIntent` (keep the project vs close it): a live workspace in the same project, the project's own roots when kept, the nearest retained neighbor, an adjacent local project, then an empty workspace. The workspaces being removed are excluded from that search in one place instead of five. - Sidebar neighbor selection stays within the entry's project section. - The three archive paths now share one removal orchestration, and the three "open the closed workspace first" helpers collapse into one. # Testing New tests cover: the keep-project fallback, neighbor selection staying inside the project section, and archiving with a mock-remote neighboring project never connecting to it. Each was verified to fail against the previous behavior. ## Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content adheres to Zed's UI standards ([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) and [icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md) guidelines) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable --- Release Notes: - Fixed archiving a thread switching the window to a different project. - Fixed a possible crash when archiving a thread while its window was closing. --- crates/git_ui_core/src/worktree_picker.rs | 30 +- crates/git_ui_core/src/worktree_service.rs | 5 +- crates/recent_projects/src/recent_projects.rs | 2 +- crates/sidebar/src/sidebar.rs | 716 +++++------------ crates/sidebar/src/sidebar_tests.rs | 136 +++- crates/workspace/src/multi_workspace.rs | 746 ++++++++---------- crates/workspace/src/multi_workspace_tests.rs | 131 ++- crates/workspace/src/persistence.rs | 31 +- crates/workspace/src/workspace.rs | 18 +- 9 files changed, 785 insertions(+), 1030 deletions(-) diff --git a/crates/git_ui_core/src/worktree_picker.rs b/crates/git_ui_core/src/worktree_picker.rs index 67ddbe8c23..21ed8b8c07 100644 --- a/crates/git_ui_core/src/worktree_picker.rs +++ b/crates/git_ui_core/src/worktree_picker.rs @@ -20,7 +20,8 @@ use ui::{ use util::ResultExt as _; use util::paths::PathExt; use workspace::{ - ModalView, MultiWorkspace, Workspace, dock::DockPosition, notifications::DetachAndPromptErr, + ModalView, MultiWorkspace, RemovalIntent, Workspace, dock::DockPosition, + notifications::DetachAndPromptErr, }; use crate::notifications::show_error_toast; @@ -448,19 +449,17 @@ impl WorktreePickerDelegate { && let Some(workspace) = self.workspace.upgrade() { let group_key = workspace.read(cx).project_group_key(cx); - if let Some(group_workspaces) = multi_workspace + for group_workspace in multi_workspace .read(cx) .workspaces_for_project_group(&group_key, cx) { - for group_workspace in group_workspaces { - for worktree in group_workspace - .read(cx) - .project() - .read(cx) - .visible_worktrees(cx) - { - paths.insert(worktree.read(cx).abs_path().to_path_buf()); - } + for worktree in group_workspace + .read(cx) + .project() + .read(cx) + .visible_worktrees(cx) + { + paths.insert(worktree.read(cx).abs_path().to_path_buf()); } } } @@ -660,7 +659,7 @@ impl WorktreePickerDelegate { let group_key = workspace.read(cx).project_group_key(cx); multi_workspace .read(cx) - .workspaces_for_project_group(&group_key, cx)? + .workspaces_for_project_group(&group_key, cx) .into_iter() .find(|group_workspace| { *group_workspace != workspace @@ -693,7 +692,12 @@ impl WorktreePickerDelegate { cx.spawn_in(window, async move |picker, cx| { let removed = window_handle .update(cx, |multi_workspace, window, cx| { - multi_workspace.close_workspace(&workspace_to_remove, window, cx) + multi_workspace.remove( + [workspace_to_remove.clone()], + RemovalIntent::CloseProject, + window, + cx, + ) })? .await?; diff --git a/crates/git_ui_core/src/worktree_service.rs b/crates/git_ui_core/src/worktree_service.rs index fece13da04..fa5c1764dd 100644 --- a/crates/git_ui_core/src/worktree_service.rs +++ b/crates/git_ui_core/src/worktree_service.rs @@ -1194,7 +1194,7 @@ async fn open_worktree_workspace( None }; - let task = multi_workspace.find_or_create_workspace_with_source_workspace( + let task = multi_workspace.find_or_create_workspace( path_list, remote_connection_options, None, @@ -1206,7 +1206,6 @@ async fn open_worktree_workspace( cx, ) }, - &[], init, OpenMode::Add, source_for_transfer.clone(), @@ -1355,7 +1354,7 @@ async fn open_worktree_workspace( } else { // Background open: register the new workspace as a retained tab // but leave the user where they are. - multi_workspace.add_background_workspace(new_workspace.clone(), window, cx); + multi_workspace.add(new_workspace.clone(), window, cx); } if is_creating_new_worktree { diff --git a/crates/recent_projects/src/recent_projects.rs b/crates/recent_projects/src/recent_projects.rs index eb7aa8d1f4..c26d7537b5 100644 --- a/crates/recent_projects/src/recent_projects.rs +++ b/crates/recent_projects/src/recent_projects.rs @@ -1183,9 +1183,9 @@ impl PickerDelegate for RecentProjectsDelegate { cx, ) }, - &[], None, OpenMode::Activate, + None, window, cx, ) diff --git a/crates/sidebar/src/sidebar.rs b/crates/sidebar/src/sidebar.rs index d82584c7cb..a98997468f 100644 --- a/crates/sidebar/src/sidebar.rs +++ b/crates/sidebar/src/sidebar.rs @@ -64,8 +64,8 @@ use util::ResultExt as _; use util::path_list::PathList; use workspace::{ CloseWindow, FocusWorkspaceSidebar, MultiWorkspace, MultiWorkspaceEvent, NextProject, - NextThread, Open, OpenMode, PreviousProject, PreviousThread, ProjectGroupKey, SaveIntent, - Sidebar as WorkspaceSidebar, SidebarSide, Toast, ToggleWorkspaceSidebar, Workspace, + NextThread, Open, OpenMode, PreviousProject, PreviousThread, ProjectGroupKey, RemovalIntent, + SaveIntent, Sidebar as WorkspaceSidebar, SidebarSide, Toast, ToggleWorkspaceSidebar, Workspace, notifications::NotificationId, sidebar_side_context_menu, }; @@ -408,7 +408,6 @@ enum ListEntry { enum ActivatableEntry { Thread { metadata: ThreadMetadata, - workspace: ThreadEntryWorkspace, }, Terminal { metadata: TerminalThreadMetadata, @@ -421,7 +420,6 @@ impl ActivatableEntry { match entry { ListEntry::Thread(thread) => Some(Self::Thread { metadata: thread.metadata.clone(), - workspace: thread.workspace.clone(), }), ListEntry::Terminal(terminal) => Some(Self::Terminal { metadata: terminal.metadata.clone(), @@ -430,38 +428,6 @@ impl ActivatableEntry { ListEntry::ProjectHeader { .. } => None, } } - - fn project_location(&self, cx: &App) -> (PathList, ProjectGroupKey) { - match self { - Self::Thread { - workspace: ThreadEntryWorkspace::Open(workspace), - .. - } - | Self::Terminal { - workspace: ThreadEntryWorkspace::Open(workspace), - .. - } => ( - PathList::new(&workspace.read(cx).root_paths(cx)), - workspace.read(cx).project_group_key(cx), - ), - Self::Thread { - workspace: - ThreadEntryWorkspace::Closed { - folder_paths, - project_group_key, - }, - .. - } - | Self::Terminal { - workspace: - ThreadEntryWorkspace::Closed { - folder_paths, - project_group_key, - }, - .. - } => (folder_paths.clone(), project_group_key.clone()), - } - } } #[cfg(test)] @@ -487,9 +453,9 @@ impl ListEntry { ThreadEntryWorkspace::Open(workspace) => vec![workspace.clone()], ThreadEntryWorkspace::Closed { .. } => Vec::new(), }, - ListEntry::ProjectHeader { key, .. } => multi_workspace - .workspaces_for_project_group(key, cx) - .unwrap_or_default(), + ListEntry::ProjectHeader { key, .. } => { + multi_workspace.workspaces_for_project_group(key, cx) + } } } } @@ -1300,9 +1266,9 @@ impl Sidebar { host, provisional_key, |options, window, cx| connect_remote(active_workspace, options, window, cx), - &[], None, OpenMode::Activate, + None, window, cx, ) @@ -1339,9 +1305,9 @@ impl Sidebar { host, provisional_key, |options, window, cx| connect_remote(active_workspace, options, window, cx), - &[], None, OpenMode::Activate, + None, window, cx, ) @@ -2539,7 +2505,7 @@ impl Sidebar { let open_workspaces = self .multi_workspace .upgrade() - .and_then(|mw| mw.read(cx).workspaces_for_project_group(key, cx)) + .map(|mw| mw.read(cx).workspaces_for_project_group(key, cx)) .unwrap_or_default(); if open_workspaces.is_empty() { @@ -2761,7 +2727,8 @@ impl Sidebar { let Some(base) = multi_workspace .read(cx) .workspaces_for_project_group(&key, cx) - .and_then(|workspaces| workspaces.first().cloned()) + .first() + .cloned() else { continue; }; @@ -2856,9 +2823,7 @@ impl Sidebar { let open_workspaces = multi_workspace .read_with(cx, |multi_workspace, cx| { - multi_workspace - .workspaces_for_project_group(&project_group_key, cx) - .unwrap_or_default() + multi_workspace.workspaces_for_project_group(&project_group_key, cx) }) .unwrap_or_default(); @@ -3057,8 +3022,9 @@ impl Sidebar { close_multi_workspace .update(cx, |multi_workspace, cx| { multi_workspace - .close_workspace( - &close_workspace, + .remove( + [close_workspace.clone()], + RemovalIntent::CloseProject, window, cx, ) @@ -4011,9 +3977,9 @@ impl Sidebar { host, provisional_key, |options, window, cx| connect_remote(active_workspace, options, window, cx), - &[], None, OpenMode::Activate, + None, window, cx, ) @@ -4415,21 +4381,43 @@ impl Sidebar { } } - /// Find the neighbor thread in the sidebar (by display position). - /// Look below first, then above, for the nearest thread that isn't - /// the one being archived. We capture both the neighbor's metadata - /// (for activation) and its workspace paths (for the workspace - /// removal fallback). + /// Find the entry to select after the entry at `current_position` is + /// removed: the nearest activatable entry in the same project section, + /// below first, then above. Only when that section has no other + /// activatable entry, the nearest one in the whole list. fn neighboring_activatable_entry(&self, current_position: usize) -> Option { - let after = self - .contents - .entries - .get(current_position.checked_add(1)?..)?; - let before = self.contents.entries.get(..current_position)?; - after + let entries = &self.contents.entries; + let is_header = |entry: &ListEntry| matches!(entry, ListEntry::ProjectHeader { .. }); + + let section_start = entries + .get(..current_position)? .iter() - .chain(before.iter().rev()) - .find_map(ActivatableEntry::from_list_entry) + .rposition(is_header) + .map_or(0, |header| header + 1); + let section_end = entries + .get(current_position + 1..)? + .iter() + .position(is_header) + .map_or(entries.len(), |offset| current_position + 1 + offset); + + for (start, end) in [(section_start, section_end), (0, entries.len())] { + let Some(before) = entries.get(start..current_position) else { + continue; + }; + let Some(after) = entries.get(current_position + 1..end) else { + continue; + }; + + let Some(entry) = after + .iter() + .chain(before.iter().rev()) + .find_map(ActivatableEntry::from_list_entry) + else { + continue; + }; + return Some(entry); + } + None } fn activate_entry( @@ -4622,9 +4610,9 @@ impl Sidebar { host, provisional_key, |options, window, cx| connect_remote(active_workspace, options, window, cx), - &[], None, OpenMode::Activate, + None, window, cx, ) @@ -4913,15 +4901,18 @@ impl Sidebar { } } + /// Closed linked-worktree entries need an open workspace so archive root + /// planning can inspect repositories before deleting the worktree. fn open_workspace_for_archive( &mut self, folder_paths: PathList, project_group_key: ProjectGroupKey, window: &mut Window, cx: &mut Context, - ) -> Option<(Task>>, Entity)> { + then: impl FnOnce(&mut Self, Entity, &mut Window, &mut Context) + 'static, + ) { let Some(multi_workspace) = self.multi_workspace.upgrade() else { - return None; + return; }; let host = project_group_key.host(); @@ -4934,70 +4925,21 @@ impl Sidebar { host, Some(project_group_key), |options, window, cx| connect_remote(active_workspace, options, window, cx), - &[], None, OpenMode::Add, + None, window, cx, ) }); - Some((open_task, modal_workspace)) - } - - fn open_workspace_and_archive_thread( - &mut self, - session_id: acp::SessionId, - folder_paths: PathList, - project_group_key: ProjectGroupKey, - window: &mut Window, - cx: &mut Context, - ) { - let Some((open_task, modal_workspace)) = - self.open_workspace_for_archive(folder_paths, project_group_key, window, cx) - else { - return; - }; - cx.spawn_in(window, async move |this, cx| { let result = open_task.await; remote_connection::dismiss_connection_modal(&modal_workspace, cx); let workspace = result?; Self::wait_for_archive_workspace_metadata(&workspace, cx).await; - this.update_in(cx, |this, window, cx| { - this.update_entries(cx); - this.archive_thread(&session_id, window, cx); - })?; - anyhow::Ok(()) - }) - .detach_and_log_err(cx); - } - - fn open_workspace_and_close_terminal( - &mut self, - metadata: TerminalThreadMetadata, - folder_paths: PathList, - project_group_key: ProjectGroupKey, - window: &mut Window, - cx: &mut Context, - ) { - let Some((open_task, modal_workspace)) = - self.open_workspace_for_archive(folder_paths, project_group_key, window, cx) - else { - return; - }; - - cx.spawn_in(window, async move |this, cx| { - let result = open_task.await; - remote_connection::dismiss_connection_modal(&modal_workspace, cx); - let workspace = result?; - Self::wait_for_archive_workspace_metadata(&workspace, cx).await; - - this.update_in(cx, |this, window, cx| { - let workspace = ThreadEntryWorkspace::Open(workspace); - this.close_terminal(&metadata, &workspace, window, cx); - })?; + this.update_in(cx, |this, window, cx| then(this, workspace, window, cx))?; anyhow::Ok(()) }) .detach_and_log_err(cx); @@ -5023,12 +4965,20 @@ impl Sidebar { cx, ) { - self.open_workspace_and_close_terminal( - metadata.clone(), + let metadata = metadata.clone(); + self.open_workspace_for_archive( folder_paths.clone(), project_group_key.clone(), window, cx, + move |this, workspace, window, cx| { + this.close_terminal( + &metadata, + &ThreadEntryWorkspace::Open(workspace), + window, + cx, + ); + }, ); return; } @@ -5078,123 +5028,40 @@ impl Sidebar { cx, ); - if !workspaces_to_remove.is_empty() { - let multi_workspace = self.multi_workspace.upgrade().unwrap(); - let terminal_workspace_removed = matches!( - workspace, - ThreadEntryWorkspace::Open(workspace) if workspaces_to_remove.contains(workspace) - ); - let (fallback_paths, project_group_key) = neighbor - .as_ref() - .map(|neighbor| neighbor.project_location(cx)) - .unwrap_or_else(|| { - workspaces_to_remove - .first() - .map(|workspace| { - let key = workspace.read(cx).project_group_key(cx); - (key.path_list().clone(), key) - }) - .unwrap_or_default() - }); + let terminal_workspace_removed = matches!( + workspace, + ThreadEntryWorkspace::Open(workspace) if workspaces_to_remove.contains(workspace) + ); + let metadata = metadata.clone(); + let workspace = workspace.clone(); - let excluded = workspaces_to_remove.clone(); - let remove_task = multi_workspace.update(cx, |multi_workspace, cx| { - multi_workspace.remove( - workspaces_to_remove, - move |this, window, cx| { - let active_workspace = this.workspace().clone(); - this.find_or_create_workspace( - fallback_paths, - project_group_key.host(), - Some(project_group_key), - |options, window, cx| { - connect_remote(active_workspace, options, window, cx) - }, - &excluded, - None, - OpenMode::Activate, - window, - cx, - ) - }, + self.remove_workspaces_then( + workspaces_to_remove, + close_item_tasks, + window, + cx, + move |this, window, cx| { + if terminal_workspace_removed { + this.delete_empty_drafts_for_archive_paths( + metadata.folder_paths(), + metadata.remote_connection.as_ref(), + cx, + ); + } + // If the terminal's workspace has already been removed, don't + // synthesize a fallback draft in the detached AgentPanel. + this.close_terminal_entry( + &metadata, + &workspace, + is_active, + neighbor.as_ref(), + !terminal_workspace_removed, + roots_to_archive, window, cx, - ) - }); - - let metadata = metadata.clone(); - let workspace = workspace.clone(); - cx.spawn_in(window, async move |this, cx| { - if !remove_task.await? { - return anyhow::Ok(()); - } - - for task in close_item_tasks { - let result: anyhow::Result<()> = task.await; - result.log_err(); - } - - this.update_in(cx, |this, window, cx| { - if terminal_workspace_removed { - this.delete_empty_drafts_for_archive_paths( - metadata.folder_paths(), - metadata.remote_connection.as_ref(), - cx, - ); - } - // If the terminal's workspace has already been removed, - // don't synthesize a fallback draft in the detached - // AgentPanel. - this.close_terminal_entry( - &metadata, - &workspace, - is_active, - neighbor.as_ref(), - !terminal_workspace_removed, - roots_to_archive, - window, - cx, - ); - })?; - anyhow::Ok(()) - }) - .detach_and_log_err(cx); - } else if !close_item_tasks.is_empty() { - let metadata = metadata.clone(); - let workspace = workspace.clone(); - cx.spawn_in(window, async move |this, cx| { - for task in close_item_tasks { - let result: anyhow::Result<()> = task.await; - result.log_err(); - } - - this.update_in(cx, |this, window, cx| { - this.close_terminal_entry( - &metadata, - &workspace, - is_active, - neighbor.as_ref(), - true, - roots_to_archive, - window, - cx, - ); - })?; - anyhow::Ok(()) - }) - .detach_and_log_err(cx); - } else { - self.close_terminal_entry( - metadata, - workspace, - is_active, - neighbor.as_ref(), - true, - roots_to_archive, - window, - cx, - ); - } + ); + }, + ); } fn close_terminal_entry( @@ -5256,6 +5123,48 @@ impl Sidebar { self.update_entries(cx); } + fn remove_workspaces_then( + &mut self, + workspaces_to_remove: Vec>, + close_item_tasks: Vec>>, + window: &mut Window, + cx: &mut Context, + finish: impl FnOnce(&mut Self, &mut Window, &mut Context) + 'static, + ) { + if workspaces_to_remove.is_empty() && close_item_tasks.is_empty() { + finish(self, window, cx); + return; + } + + let remove_task = if workspaces_to_remove.is_empty() { + None + } else { + let Some(multi_workspace) = self.multi_workspace.upgrade() else { + return; + }; + Some(multi_workspace.update(cx, |multi_workspace, cx| { + multi_workspace.remove(workspaces_to_remove, RemovalIntent::KeepProject, window, cx) + })) + }; + + cx.spawn_in(window, async move |this, cx| { + if let Some(remove_task) = remove_task + && !remove_task.await? + { + return anyhow::Ok(()); + } + + for task in close_item_tasks { + let result: anyhow::Result<()> = task.await; + result.log_err(); + } + + this.update_in(cx, |this, window, cx| finish(this, window, cx))?; + anyhow::Ok(()) + }) + .detach_and_log_err(cx); + } + fn close_items_for_archived_worktrees( &self, roots_to_archive: &[thread_worktree_archive::RootPlan], @@ -5397,12 +5306,16 @@ impl Sidebar { cx, ) { - self.open_workspace_and_archive_thread( - session_id.clone(), + let session_id = session_id.clone(); + self.open_workspace_for_archive( folder_paths, project_group_key, window, cx, + move |this, _workspace, window, cx| { + this.update_entries(cx); + this.archive_thread(&session_id, window, cx); + }, ); return; } @@ -5466,133 +5379,40 @@ impl Sidebar { cx, ); - if !workspaces_to_remove.is_empty() { - let multi_workspace = self.multi_workspace.upgrade().unwrap(); - let session_id = session_id.clone(); + let removed_workspace = !workspaces_to_remove.is_empty(); + let session_id = session_id.clone(); + let thread_remote_connection = metadata + .as_ref() + .and_then(|metadata| metadata.remote_connection.clone()); - let (fallback_paths, project_group_key) = neighbor - .as_ref() - .map(|neighbor| neighbor.project_location(cx)) - .unwrap_or_else(|| { - workspaces_to_remove - .first() - .map(|workspace| { - let key = workspace.read(cx).project_group_key(cx); - (key.path_list().clone(), key) - }) - .unwrap_or_default() - }); - - let excluded = workspaces_to_remove.clone(); - let remove_task = multi_workspace.update(cx, |mw, cx| { - mw.remove( - workspaces_to_remove, - move |this, window, cx| { - let active_workspace = this.workspace().clone(); - this.find_or_create_workspace( - fallback_paths, - project_group_key.host(), - Some(project_group_key), - |options, window, cx| { - connect_remote(active_workspace, options, window, cx) - }, - &excluded, - None, - OpenMode::Activate, - window, - cx, - ) - }, + self.remove_workspaces_then( + workspaces_to_remove, + close_item_tasks, + window, + cx, + move |this, window, cx| { + if removed_workspace && let Some(thread_folder_paths) = thread_folder_paths.as_ref() + { + this.delete_empty_drafts_for_archive_paths( + thread_folder_paths, + thread_remote_connection.as_ref(), + cx, + ); + } + let in_flight = thread_id + .and_then(|tid| this.start_archive_worktree_task(tid, roots_to_archive, cx)); + this.archive_and_activate( + &session_id, + thread_id, + neighbor.as_ref(), + thread_folder_paths.as_ref(), + thread_remote_connection.as_ref(), + in_flight, window, cx, - ) - }); - - let thread_folder_paths = thread_folder_paths.clone(); - let thread_remote_connection = metadata - .as_ref() - .and_then(|metadata| metadata.remote_connection.clone()); - cx.spawn_in(window, async move |this, cx| { - if !remove_task.await? { - return anyhow::Ok(()); - } - - for task in close_item_tasks { - let result: anyhow::Result<()> = task.await; - result.log_err(); - } - - this.update_in(cx, |this, window, cx| { - if let Some(thread_folder_paths) = thread_folder_paths.as_ref() { - this.delete_empty_drafts_for_archive_paths( - thread_folder_paths, - thread_remote_connection.as_ref(), - cx, - ); - } - let in_flight = thread_id.and_then(|tid| { - this.start_archive_worktree_task(tid, roots_to_archive, cx) - }); - this.archive_and_activate( - &session_id, - thread_id, - neighbor.as_ref(), - thread_folder_paths.as_ref(), - thread_remote_connection.as_ref(), - in_flight, - window, - cx, - ); - })?; - anyhow::Ok(()) - }) - .detach_and_log_err(cx); - } else if !close_item_tasks.is_empty() { - let session_id = session_id.clone(); - let thread_folder_paths = thread_folder_paths.clone(); - let thread_remote_connection = metadata - .as_ref() - .and_then(|metadata| metadata.remote_connection.clone()); - cx.spawn_in(window, async move |this, cx| { - for task in close_item_tasks { - let result: anyhow::Result<()> = task.await; - result.log_err(); - } - - this.update_in(cx, |this, window, cx| { - let in_flight = thread_id.and_then(|tid| { - this.start_archive_worktree_task(tid, roots_to_archive, cx) - }); - this.archive_and_activate( - &session_id, - thread_id, - neighbor.as_ref(), - thread_folder_paths.as_ref(), - thread_remote_connection.as_ref(), - in_flight, - window, - cx, - ); - })?; - anyhow::Ok(()) - }) - .detach_and_log_err(cx); - } else { - let in_flight = thread_id - .and_then(|tid| self.start_archive_worktree_task(tid, roots_to_archive, cx)); - self.archive_and_activate( - session_id, - thread_id, - neighbor.as_ref(), - thread_folder_paths.as_ref(), - metadata - .as_ref() - .and_then(|metadata| metadata.remote_connection.as_ref()), - in_flight, - window, - cx, - ); - } + ); + }, + ); } /// Archive a thread and activate the nearest neighbor or a draft. @@ -6831,37 +6651,6 @@ impl Sidebar { } } - /// Closed linked-worktree drafts need an open workspace so archive root - /// planning can inspect repositories before deleting the worktree. - fn open_workspace_and_remove_draft( - &mut self, - draft_id: ThreadId, - folder_paths: PathList, - project_group_key: ProjectGroupKey, - window: &mut Window, - cx: &mut Context, - ) { - let Some((open_task, modal_workspace)) = - self.open_workspace_for_archive(folder_paths, project_group_key, window, cx) - else { - return; - }; - - cx.spawn_in(window, async move |this, cx| { - let result = open_task.await; - remote_connection::dismiss_connection_modal(&modal_workspace, cx); - let workspace = result?; - Self::wait_for_archive_workspace_metadata(&workspace, cx).await; - - this.update_in(cx, |this, window, cx| { - let workspace = ThreadEntryWorkspace::Open(workspace); - this.remove_draft(draft_id, &workspace, window, cx); - })?; - anyhow::Ok(()) - }) - .detach_and_log_err(cx); - } - fn remove_draft( &mut self, draft_id: ThreadId, @@ -6889,12 +6678,14 @@ impl Sidebar { cx, ) { - self.open_workspace_and_remove_draft( - draft_id, + self.open_workspace_for_archive( folder_paths.clone(), project_group_key.clone(), window, cx, + move |this, workspace, window, cx| { + this.remove_draft(draft_id, &ThreadEntryWorkspace::Open(workspace), window, cx); + }, ); return; } @@ -6959,122 +6750,39 @@ impl Sidebar { cx, ); - if !workspaces_to_remove.is_empty() { - let Some(multi_workspace) = self.multi_workspace.upgrade() else { - return; - }; - let draft_workspace_removed = matches!( - workspace, - ThreadEntryWorkspace::Open(workspace) if workspaces_to_remove.contains(workspace) - ); - let (fallback_paths, project_group_key) = neighbor - .as_ref() - .map(|neighbor| neighbor.project_location(cx)) - .unwrap_or_else(|| { - workspaces_to_remove - .first() - .map(|workspace| { - let key = workspace.read(cx).project_group_key(cx); - (key.path_list().clone(), key) - }) - .unwrap_or_default() - }); + let draft_workspace_removed = matches!( + workspace, + ThreadEntryWorkspace::Open(workspace) if workspaces_to_remove.contains(workspace) + ); + let workspace = workspace.clone(); - let excluded = workspaces_to_remove.clone(); - let remove_task = multi_workspace.update(cx, |multi_workspace, cx| { - multi_workspace.remove( - workspaces_to_remove, - move |this, window, cx| { - let active_workspace = this.workspace().clone(); - this.find_or_create_workspace( - fallback_paths, - project_group_key.host(), - Some(project_group_key), - |options, window, cx| { - connect_remote(active_workspace, options, window, cx) - }, - &excluded, - None, - OpenMode::Activate, - window, - cx, - ) - }, + self.remove_workspaces_then( + workspaces_to_remove, + close_item_tasks, + window, + cx, + move |this, window, cx| { + if draft_workspace_removed + && let Some(draft_folder_paths) = draft_folder_paths.as_ref() + { + this.delete_empty_drafts_for_archive_paths( + draft_folder_paths, + draft_remote_connection.as_ref(), + cx, + ); + } + this.remove_draft_entry( + draft_id, + &workspace, + was_active, + neighbor.as_ref(), + !draft_workspace_removed, + roots_to_archive, window, cx, - ) - }); - - let workspace = workspace.clone(); - cx.spawn_in(window, async move |this, cx| { - if !remove_task.await? { - return anyhow::Ok(()); - } - - for task in close_item_tasks { - let result: anyhow::Result<()> = task.await; - result.log_err(); - } - - this.update_in(cx, |this, window, cx| { - if draft_workspace_removed { - if let Some(draft_folder_paths) = draft_folder_paths.as_ref() { - this.delete_empty_drafts_for_archive_paths( - draft_folder_paths, - draft_remote_connection.as_ref(), - cx, - ); - } - } - this.remove_draft_entry( - draft_id, - &workspace, - was_active, - neighbor.as_ref(), - !draft_workspace_removed, - roots_to_archive, - window, - cx, - ); - })?; - anyhow::Ok(()) - }) - .detach_and_log_err(cx); - } else if !close_item_tasks.is_empty() { - let workspace = workspace.clone(); - cx.spawn_in(window, async move |this, cx| { - for task in close_item_tasks { - let result: anyhow::Result<()> = task.await; - result.log_err(); - } - - this.update_in(cx, |this, window, cx| { - this.remove_draft_entry( - draft_id, - &workspace, - was_active, - neighbor.as_ref(), - true, - roots_to_archive, - window, - cx, - ); - })?; - anyhow::Ok(()) - }) - .detach_and_log_err(cx); - } else { - self.remove_draft_entry( - draft_id, - workspace, - was_active, - neighbor.as_ref(), - true, - roots_to_archive, - window, - cx, - ); - } + ); + }, + ); } fn remove_draft_entry( diff --git a/crates/sidebar/src/sidebar_tests.rs b/crates/sidebar/src/sidebar_tests.rs index b88b00281e..aff7eaff3f 100644 --- a/crates/sidebar/src/sidebar_tests.rs +++ b/crates/sidebar/src/sidebar_tests.rs @@ -1055,6 +1055,80 @@ async fn test_collapse_state_survives_worktree_key_change(cx: &mut TestAppContex ); } +#[gpui::test] +async fn test_neighboring_activatable_entry_stays_within_project(cx: &mut TestAppContext) { + let project = init_test_project("/my-project", cx).await; + let (multi_workspace, cx) = + cx.add_window_view(|window, cx| MultiWorkspace::test_new(project, window, cx)); + let sidebar = setup_sidebar(&multi_workspace, cx); + let workspace = multi_workspace.read_with(cx, |mw, _| mw.workspace().clone()); + + let header = |path: &str| ListEntry::ProjectHeader { + key: ProjectGroupKey::new(None, PathList::new(&[std::path::PathBuf::from(path)])), + label: path.into(), + highlight_positions: Vec::new(), + has_running_threads: false, + waiting_thread_count: 0, + has_notifications: false, + is_active: false, + has_threads: true, + }; + let thread = |name: &str| { + ListEntry::Thread(Arc::new(ThreadEntry { + metadata: ThreadMetadata { + thread_id: ThreadId::new(), + session_id: Some(acp::SessionId::new(Arc::from(name))), + agent_id: AgentId::new("zed-agent"), + worktree_paths: WorktreePaths::default(), + title: Some(name.to_string().into()), + title_override: None, + updated_at: Utc::now(), + created_at: Some(Utc::now()), + interacted_at: None, + archived: false, + remote_connection: None, + }, + icon: IconName::ZedAgent, + icon_from_external_svg: None, + status: AgentThreadStatus::Completed, + workspace: ThreadEntryWorkspace::Open(workspace.clone()), + is_live: false, + is_background: false, + is_title_generating: false, + draft: None, + highlight_positions: Vec::new(), + worktrees: Vec::new(), + diff_stats: DiffStats::default(), + })) + }; + + sidebar.update_in(cx, |s, _window, _cx| { + s.contents.entries = vec![ + header("/project-a"), + thread("a-newest"), + thread("a-oldest"), + header("/project-b"), + thread("b-newest"), + ]; + + let neighbor_session = |position: usize| match s.neighboring_activatable_entry(position) { + Some(ActivatableEntry::Thread { metadata, .. }) => metadata.session_id, + _ => None, + }; + + assert_eq!( + neighbor_session(2), + Some(acp::SessionId::new(Arc::from("a-newest"))), + "the neighbor should be the sibling in the same project, not the next project's thread" + ); + assert_eq!( + neighbor_session(4), + Some(acp::SessionId::new(Arc::from("a-oldest"))), + "an empty project should fall back to another project" + ); + }); +} + #[gpui::test] async fn test_visible_entries_as_strings(cx: &mut TestAppContext) { use workspace::ProjectGroup; @@ -8236,6 +8310,7 @@ async fn test_archive_last_worktree_thread_removes_workspace(cx: &mut TestAppCon let (multi_workspace, cx) = cx.add_window_view(|window, cx| MultiWorkspace::test_new(main_project.clone(), window, cx)); let sidebar = setup_sidebar(&multi_workspace, cx); + let main_workspace = multi_workspace.read_with(cx, |mw, _| mw.workspace().clone()); let _worktree_workspace = multi_workspace.update_in(cx, |mw, window, cx| { mw.test_add_workspace(worktree_project.clone(), window, cx) @@ -8265,6 +8340,38 @@ async fn test_archive_last_worktree_thread_removes_workspace(cx: &mut TestAppCon ); cx.run_until_parked(); + let remote_host = + remote::RemoteConnectionOptions::Mock(remote::MockConnectionOptions { id: 99 }); + multi_workspace.update(cx, |mw, _cx| { + mw.test_add_project_group(workspace::ProjectGroup { + key: ProjectGroupKey::new( + Some(remote_host.clone()), + PathList::new(&[PathBuf::from("/remote/project")]), + ), + workspaces: Vec::new(), + expanded: true, + }); + }); + cx.update(|_window, cx| { + let metadata = ThreadMetadata { + thread_id: ThreadId::new(), + session_id: Some(acp::SessionId::new(Arc::from("remote-thread"))), + agent_id: agent::ZED_AGENT_ID.clone(), + title: Some("Remote Thread".into()), + title_override: None, + updated_at: chrono::TimeZone::with_ymd_and_hms(&Utc, 2024, 1, 3, 0, 0, 0).unwrap(), + created_at: None, + interacted_at: None, + worktree_paths: WorktreePaths::from_folder_paths(&PathList::new(&[PathBuf::from( + "/remote/project", + )])), + archived: false, + remote_connection: Some(remote_host), + }; + ThreadMetadataStore::global(cx).update(cx, |store, cx| store.save(metadata, cx)); + }); + cx.run_until_parked(); + multi_workspace.update_in(cx, |_, _window, cx| cx.notify()); cx.run_until_parked(); @@ -8294,6 +8401,14 @@ async fn test_archive_last_worktree_thread_removes_workspace(cx: &mut TestAppCon "linked worktree workspace should be removed after archiving its last thread" ); + multi_workspace.read_with(cx, |mw, _| { + assert_eq!( + mw.workspace(), + &main_workspace, + "archiving the worktree's last thread should activate its own project, not the remote one" + ); + }); + // The linked worktree checkout directory should also be removed from disk. assert!( !fs.is_dir(Path::new("/worktrees/project/feature-a/project")) @@ -8746,12 +8861,10 @@ async fn test_restore_worktree_thread_uses_main_repo_project_group_key(cx: &mut cx.run_until_parked(); // Remove the worktree workspace and delete the worktree from disk. - let main_workspace = - multi_workspace.read_with(cx, |mw, _| mw.workspaces().next().unwrap().clone()); let remove_task = multi_workspace.update_in(cx, |mw, window, cx| { mw.remove( vec![worktree_workspace], - move |_this, _window, _cx| Task::ready(Ok(main_workspace)), + RemovalIntent::KeepProject, window, cx, ) @@ -12459,7 +12572,8 @@ mod property_test { let key = &keys[project_group_index]; let ws = mw .workspaces_for_project_group(key, cx) - .and_then(|ws| ws.first().cloned()) + .first() + .cloned() .unwrap_or_else(|| mw.workspace().clone()); let project = ws.read(cx).project().clone(); (ws, project) @@ -12612,7 +12726,8 @@ mod property_test { let keys = mw.project_group_keys(); let key = &keys[index]; mw.workspaces_for_project_group(key, cx) - .and_then(|ws| ws.first().cloned()) + .first() + .cloned() .unwrap_or_else(|| mw.workspace().clone()) }); multi_workspace.update_in(cx, |mw, window, cx| { @@ -12681,7 +12796,8 @@ mod property_test { let keys = mw.project_group_keys(); let key = &keys[project_group_index]; mw.workspaces_for_project_group(key, cx) - .and_then(|ws| ws.first().cloned()) + .first() + .cloned() .unwrap() }); let main_project = main_workspace.read_with(cx, |ws, _| ws.project().clone()); @@ -12700,8 +12816,7 @@ mod property_test { let workspace = multi_workspace.read_with(cx, |mw, cx| { let keys = mw.project_group_keys(); let key = &keys[project_group_index]; - mw.workspaces_for_project_group(key, cx) - .and_then(|ws| ws.first().cloned()) + mw.workspaces_for_project_group(key, cx).first().cloned() }); let Some(workspace) = workspace else { return }; let project = workspace.read_with(cx, |ws, _| ws.project().clone()); @@ -12728,8 +12843,7 @@ mod property_test { let workspace = multi_workspace.read_with(cx, |mw, cx| { let keys = mw.project_group_keys(); let key = &keys[project_group_index]; - mw.workspaces_for_project_group(key, cx) - .and_then(|ws| ws.first().cloned()) + mw.workspaces_for_project_group(key, cx).first().cloned() }); let Some(workspace) = workspace else { return }; let project = workspace.read_with(cx, |ws, _| ws.project().clone()); @@ -14978,9 +15092,9 @@ async fn test_find_or_create_workspace_returns_the_created_remote_workspace( Some(opts), Some(key), move |_, _, _| Task::ready(Ok(Some(remote_client))), - &[], None, workspace::OpenMode::Activate, + None, window, cx, ) diff --git a/crates/workspace/src/multi_workspace.rs b/crates/workspace/src/multi_workspace.rs index 4e6ec5b38c..df49cf8fca 100644 --- a/crates/workspace/src/multi_workspace.rs +++ b/crates/workspace/src/multi_workspace.rs @@ -281,14 +281,28 @@ pub struct SerializedProjectGroupState { pub struct ProjectGroupState { pub key: ProjectGroupKey, pub expanded: bool, - pub last_active_workspace: Option>, +} + +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum RemovalIntent { + KeepProject, + CloseProject, +} + +/// One row per workspace held by this window. The displayed workspace is +/// always one of these rows. `pinned` records whether the workspace survives +/// being navigated away from; `activated_at` records when it was last +/// displayed. +struct HeldWorkspace { + workspace: Entity, + pinned: bool, + activated_at: Option, } pub struct MultiWorkspace { window_id: WindowId, - retained_workspaces: Vec>, + held: Vec, project_groups: Vec, - active_workspace: Entity, /// Source of truth for which workspace is presented in this window, shared /// with each member `Workspace` so they can tell whether they own the /// platform window's title and edited indicator. This only exists to prevent @@ -351,9 +365,12 @@ impl MultiWorkspace { }); Self { window_id: window.window_handle().window_id(), - retained_workspaces: Vec::new(), + held: vec![HeldWorkspace { + workspace, + pinned: false, + activated_at: Some(0), + }], project_groups: Vec::new(), - active_workspace: workspace, active_workspace_id, sidebar: None, sidebar_open: false, @@ -488,7 +505,7 @@ impl MultiWorkspace { self.sidebar_open = true; self.retain_active_workspace(cx); let sidebar_focus_handle = self.sidebar.as_ref().map(|s| s.focus_handle(cx)); - for workspace in self.retained_workspaces.clone() { + for workspace in self.workspaces().cloned().collect::>() { workspace.update(cx, |workspace, _cx| { workspace.set_sidebar_focus_handle(sidebar_focus_handle.clone()); }); @@ -504,7 +521,7 @@ impl MultiWorkspace { }; telemetry::event!("Sidebar Toggled", action = "close", side = side); self.sidebar_open = false; - for workspace in self.retained_workspaces.clone() { + for workspace in self.workspaces().cloned().collect::>() { workspace.update(cx, |workspace, _cx| { workspace.set_sidebar_focus_handle(None); }); @@ -619,18 +636,30 @@ impl MultiWorkspace { cx.notify(); } - pub fn is_workspace_retained(&self, workspace: &Entity) -> bool { - self.retained_workspaces + fn held_index(&self, workspace: &Entity) -> Option { + self.held .iter() - .any(|retained| retained == workspace) + .position(|held| held.workspace == *workspace) + } + + pub fn is_workspace_retained(&self, workspace: &Entity) -> bool { + self.held + .iter() + .any(|held| held.pinned && held.workspace == *workspace) } pub fn active_workspace_is_retained(&self) -> bool { - self.is_workspace_retained(&self.active_workspace) + self.held[self.displayed_index()].pinned } - pub fn retained_workspaces(&self) -> &[Entity] { - &self.retained_workspaces + /// The displayed workspace is the most recently activated row. + fn displayed_index(&self) -> usize { + self.held + .iter() + .enumerate() + .max_by_key(|(_, held)| held.activated_at) + .expect("a window always holds at least one workspace") + .0 } /// Ensures a project group exists for `key`, creating one if needed. @@ -648,7 +677,6 @@ impl MultiWorkspace { ProjectGroupState { key, expanded: true, - last_active_workspace: None, }, ); } @@ -685,7 +713,7 @@ impl MultiWorkspace { } if new_key_exists { - let active_key = self.active_workspace.read(cx).project_group_key(cx); + let active_key = self.workspace().read(cx).project_group_key(cx); if active_key == *new_key { self.project_groups.retain(|g| g.key != *old_key); } else { @@ -704,27 +732,46 @@ impl MultiWorkspace { // linked worktree workspace), re-create the old group so it // remains reachable in the sidebar. let other_workspace_needs_old_key = self - .retained_workspaces + .held .iter() - .any(|ws| ws.read(cx).project_group_key(cx) == *old_key); + .any(|held| held.pinned && held.workspace.read(cx).project_group_key(cx) == *old_key); if other_workspace_needs_old_key { self.ensure_project_group_state(old_key.clone()); } } - pub(crate) fn retain_workspace( + /// Ensures `workspace` has a row in `held`, registering it on first + /// insert, and returns the row's index. + fn hold( &mut self, workspace: Entity, - key: ProjectGroupKey, + window: &Window, cx: &mut Context, - ) { - self.ensure_project_group_state(key); - if self.is_workspace_retained(&workspace) { + ) -> usize { + if let Some(index) = self.held_index(&workspace) { + return index; + } + self.register_workspace(&workspace, window, cx); + self.held.push(HeldWorkspace { + workspace, + pinned: false, + activated_at: None, + }); + self.held.len() - 1 + } + + /// Pins the row so the workspace survives navigating away, recording + /// `group` as the project group it was pinned under. No-op if already + /// pinned. + fn pin(&mut self, index: usize, group: ProjectGroupKey, cx: &mut Context) { + if self.held[index].pinned { return; } - - self.retained_workspaces.push(workspace.clone()); - cx.emit(MultiWorkspaceEvent::WorkspaceAdded(workspace)); + self.held[index].pinned = true; + self.ensure_project_group_state(group); + cx.emit(MultiWorkspaceEvent::WorkspaceAdded( + self.held[index].workspace.clone(), + )); } pub(crate) fn activate_provisional_workspace( @@ -734,17 +781,9 @@ impl MultiWorkspace { window: &mut Window, cx: &mut Context, ) { - if workspace != self.active_workspace { - self.register_workspace(&workspace, window, cx); - } - - self.ensure_project_group_state(provisional_key); - if !self.is_workspace_retained(&workspace) { - self.retained_workspaces.push(workspace.clone()); - } - - self.activate(workspace.clone(), None, window, cx); - cx.emit(MultiWorkspaceEvent::WorkspaceAdded(workspace)); + let index = self.hold(workspace.clone(), window, cx); + self.pin(index, provisional_key, cx); + self.activate(workspace, None, window, cx); } fn register_workspace( @@ -792,11 +831,7 @@ impl MultiWorkspace { if restored.iter().any(|group| group.key == key) { continue; } - restored.push(ProjectGroupState { - key, - expanded, - last_active_workspace: None, - }); + restored.push(ProjectGroupState { key, expanded }); } for existing in std::mem::take(&mut self.project_groups) { if !restored.iter().any(|group| group.key == existing.key) { @@ -813,35 +848,28 @@ impl MultiWorkspace { .collect() } - fn derived_project_groups(&self, cx: &App) -> Vec { + pub fn project_groups(&self, cx: &App) -> Vec { self.project_groups .iter() .map(|group| ProjectGroup { key: group.key.clone(), - workspaces: self - .retained_workspaces - .iter() - .filter(|workspace| workspace.read(cx).project_group_key(cx) == group.key) - .cloned() - .collect(), + workspaces: self.workspaces_for_project_group(&group.key, cx), expanded: group.expanded, }) .collect() } - pub fn project_groups(&self, cx: &App) -> Vec { - self.derived_project_groups(cx) - } - pub fn last_active_workspace_for_group( &self, key: &ProjectGroupKey, cx: &App, ) -> Option> { - let group = self.project_groups.iter().find(|g| g.key == *key)?; - let weak = group.last_active_workspace.as_ref()?; - let workspace = weak.upgrade()?; - (workspace.read(cx).project_group_key(cx) == *key).then_some(workspace) + self.held + .iter() + .filter(|held| held.workspace.read(cx).project_group_key(cx) == *key) + .filter_map(|held| Some((held.activated_at?, &held.workspace))) + .max_by_key(|(activated_at, _)| *activated_at) + .map(|(_, workspace)| workspace.clone()) } pub fn group_state_by_key(&self, key: &ProjectGroupKey) -> Option<&ProjectGroupState> { @@ -907,57 +935,13 @@ impl MultiWorkspace { &self, key: &ProjectGroupKey, cx: &App, - ) -> Option>> { - let has_group = self.project_groups.iter().any(|group| group.key == *key) - || self - .retained_workspaces - .iter() - .any(|workspace| workspace.read(cx).project_group_key(cx) == *key); - - has_group.then(|| { - self.retained_workspaces - .iter() - .filter(|workspace| workspace.read(cx).project_group_key(cx) == *key) - .cloned() - .collect() - }) - } - - pub fn close_workspace( - &mut self, - workspace: &Entity, - window: &mut Window, - cx: &mut Context, - ) -> Task> { - let group_key = workspace.read(cx).project_group_key(cx); - let excluded_workspace = workspace.clone(); - - self.remove( - [workspace.clone()], - move |this, window, cx| { - if let Some(workspace) = this - .workspaces_for_project_group(&group_key, cx) - .unwrap_or_default() - .into_iter() - .find(|candidate| candidate != &excluded_workspace) - { - return Task::ready(Ok(workspace)); - } - - let group_index = this - .project_groups - .iter() - .position(|group| group.key == group_key); - this.fallback_workspace( - group_index, - std::slice::from_ref(&excluded_workspace), - window, - cx, - ) - }, - window, - cx, - ) + ) -> Vec> { + self.held + .iter() + .filter(|held| held.pinned) + .map(|held| held.workspace.clone()) + .filter(|workspace| workspace.read(cx).project_group_key(cx) == *key) + .collect() } pub fn remove_project_group( @@ -966,97 +950,27 @@ impl MultiWorkspace { window: &mut Window, cx: &mut Context, ) -> Task> { - let group_index = self - .project_groups - .iter() - .position(|group| group.key == *group_key); - - // The active workspace can remain provisional while the sidebar is - // closed. Retain it before removal so switching to the fallback does - // not recreate the project group that is being removed. + // The active workspace can remain unpinned while the sidebar is + // closed. Pin it first: this puts it in the removal set below, and + // stops `activate` from pinning it while switching to the + // replacement, which would recreate the project group row this + // function just deleted. let active_workspace = self.workspace().clone(); if active_workspace.read(cx).project_group_key(cx) == *group_key && !self.is_workspace_retained(&active_workspace) { - self.retain_workspace(active_workspace, group_key.clone(), cx); + let index = self.hold(active_workspace, window, cx); + self.pin(index, group_key.clone(), cx); } - let workspaces = self - .workspaces_for_project_group(group_key, cx) - .unwrap_or_default(); - let excluded_workspaces = workspaces.clone(); - let removal_task = self.remove( - workspaces, - move |this, window, cx| { - this.fallback_workspace(group_index, &excluded_workspaces, window, cx) - }, - window, - cx, - ); + let workspaces = self.workspaces_for_project_group(group_key, cx); + + let task = self.remove(workspaces, RemovalIntent::CloseProject, window, cx); self.project_groups.retain(|group| group.key != *group_key); cx.emit(MultiWorkspaceEvent::ProjectGroupsChanged); - removal_task - } - - /// Finds a fallback workspace outside the project group at `group_index`. - /// - /// Prefers the nearest retained workspace in another group. If none exists, - /// opens the immediately neighboring group when it is local, and otherwise - /// creates an empty workspace. - fn fallback_workspace( - &mut self, - group_index: Option, - excluded_workspaces: &[Entity], - window: &mut Window, - cx: &mut Context, - ) -> Task>> { - if let Some(group_index) = group_index { - if let Some(workspace) = - self.nearest_retained_workspace(group_index, excluded_workspaces, cx) - { - return Task::ready(Ok(workspace)); - } - - let neighboring_group_key = self - .project_groups - .get(group_index + 1) - .or_else(|| { - group_index - .checked_sub(1) - .and_then(|previous| self.project_groups.get(previous)) - }) - .map(|group| group.key.clone()); - - if let Some(key) = neighboring_group_key - && key.host().is_none() - { - return self.find_or_create_local_workspace( - key.path_list().clone(), - Some(key), - excluded_workspaces, - None, - OpenMode::Activate, - window, - cx, - ); - } - } - - let app_state = self.workspace().read(cx).app_state().clone(); - let project = Project::local( - app_state.client.clone(), - app_state.node_runtime.clone(), - app_state.user_store.clone(), - app_state.languages.clone(), - app_state.fs.clone(), - None, - project::LocalProjectFlags::default(), - cx, - ); - let new_workspace = cx.new(|cx| Workspace::new(None, project, app_state, window, cx)); - Task::ready(Ok(new_workspace)) + task } /// Returns the nearest retained workspace outside the project group at @@ -1070,43 +984,36 @@ impl MultiWorkspace { /// /// The `group_index` must identify a project group that is still present in /// [`Self::project_groups`]. + /// The keys of the other project groups, nearest first, preferring the + /// following group over the preceding group at equal distances. Without an + /// index, every group key in display order. + fn neighbor_group_keys(&self, group_index: Option) -> Vec { + let Some(index) = group_index else { + return self + .project_groups + .iter() + .map(|group| group.key.clone()) + .collect(); + }; + + (1..self.project_groups.len()) + .flat_map(|distance| [index.checked_add(distance), index.checked_sub(distance)]) + .flatten() + .filter_map(|index| self.project_groups.get(index)) + .map(|group| group.key.clone()) + .collect() + } + + #[cfg(test)] pub(super) fn nearest_retained_workspace( &self, group_index: usize, excluded_workspaces: &[Entity], cx: &App, ) -> Option> { - for distance in 1..self.project_groups.len() { - for index in [ - group_index.checked_add(distance), - group_index.checked_sub(distance), - ] + self.neighbor_group_keys(Some(group_index)) .into_iter() - .flatten() - { - if let Some(group) = self.project_groups.get(index) { - let workspace_is_available = |workspace: &Entity| { - !excluded_workspaces.contains(workspace) - && !workspace.read(cx).project().read(cx).is_disconnected(cx) - }; - let workspace = self - .last_active_workspace_for_group(&group.key, cx) - .filter(&workspace_is_available) - .or_else(|| { - self.workspaces_for_project_group(&group.key, cx) - .unwrap_or_default() - .into_iter() - .find(workspace_is_available) - }); - - if workspace.is_some() { - return workspace; - } - } - } - } - - None + .find_map(|key| self.live_member_for_group(&key, excluded_workspaces, cx)) } /// Goes through sqlite: serialize -> close -> open new window @@ -1124,9 +1031,7 @@ impl MultiWorkspace { let app_state = self.workspace().read(cx).app_state().clone(); - let workspaces: Vec<_> = self - .workspaces_for_project_group(key, cx) - .unwrap_or_default(); + let workspaces: Vec<_> = self.workspaces_for_project_group(key, cx); let mut serialization_tasks = Vec::new(); for workspace in &workspaces { serialization_tasks.push(workspace.update(cx, |workspace, inner_cx| { @@ -1159,21 +1064,8 @@ impl MultiWorkspace { path_list: &PathList, host: Option<&RemoteConnectionOptions>, cx: &App, - ) -> Option> { - self.workspace_for_paths_excluding(path_list, host, &[], cx) - } - - fn workspace_for_paths_excluding( - &self, - path_list: &PathList, - host: Option<&RemoteConnectionOptions>, - excluding: &[Entity], - cx: &App, ) -> Option> { for workspace in self.workspaces() { - if excluding.contains(workspace) { - continue; - } let root_paths = PathList::new(&workspace.read(cx).root_paths(cx)); let key = workspace.read(cx).project_group_key(cx); let host_matches = key.host().as_ref() == host; @@ -1210,56 +1102,21 @@ impl MultiWorkspace { &mut Context, ) -> Task>>> + 'static, - excluding: &[Entity], - init: Option) + Send>>, - open_mode: OpenMode, - window: &mut Window, - cx: &mut Context, - ) -> Task>> { - self.find_or_create_workspace_with_source_workspace( - paths, - host, - provisional_project_group_key, - connect_remote, - excluding, - init, - open_mode, - None, - window, - cx, - ) - } - - pub fn find_or_create_workspace_with_source_workspace( - &mut self, - paths: PathList, - host: Option, - provisional_project_group_key: Option, - connect_remote: impl FnOnce( - RemoteConnectionOptions, - &mut Window, - &mut Context, - ) -> Task>>> - + 'static, - excluding: &[Entity], init: Option) + Send>>, open_mode: OpenMode, source_workspace: Option>, window: &mut Window, cx: &mut Context, ) -> Task>> { - if let Some(workspace) = - self.workspace_for_paths_excluding(&paths, host.as_ref(), excluding, cx) - { + if let Some(workspace) = self.workspace_for_paths(&paths, host.as_ref(), cx) { self.activate(workspace.clone(), source_workspace, window, cx); return Task::ready(Ok(workspace)); } let Some(connection_options) = host else { - return self.find_or_create_local_workspace_with_source_workspace( + return self.find_or_create_local_workspace( paths, provisional_project_group_key, - excluding, init, open_mode, source_workspace, @@ -1344,44 +1201,17 @@ impl MultiWorkspace { /// or creates a new one (deserializing its saved state from the database). /// Never searches other windows or matches workspaces with a superset of /// the requested paths. - /// - /// `excluding` lists workspaces that should be skipped during the search - /// (e.g. workspaces that are about to be removed). pub fn find_or_create_local_workspace( &mut self, path_list: PathList, project_group: Option, - excluding: &[Entity], - init: Option) + Send>>, - open_mode: OpenMode, - window: &mut Window, - cx: &mut Context, - ) -> Task>> { - self.find_or_create_local_workspace_with_source_workspace( - path_list, - project_group, - excluding, - init, - open_mode, - None, - window, - cx, - ) - } - - pub fn find_or_create_local_workspace_with_source_workspace( - &mut self, - path_list: PathList, - project_group: Option, - excluding: &[Entity], init: Option) + Send>>, open_mode: OpenMode, source_workspace: Option>, window: &mut Window, cx: &mut Context, ) -> Task>> { - if let Some(workspace) = self.workspace_for_paths_excluding(&path_list, None, excluding, cx) - { + if let Some(workspace) = self.workspace_for_paths(&path_list, None, cx) { self.activate(workspace.clone(), source_workspace, window, cx); return Task::ready(Ok(workspace)); } @@ -1390,7 +1220,6 @@ impl MultiWorkspace { let app_state = self.workspace().read(cx).app_state().clone(); let requesting_window = window.window_handle().downcast::(); let fs = ::global(cx); - let excluding = excluding.to_vec(); cx.spawn(async move |_this, cx| { let effective_path_list = if let Some(project_group) = project_group { @@ -1420,12 +1249,7 @@ impl MultiWorkspace { && let Some(workspace) = requesting_window .update(cx, |multi_workspace, window, cx| { multi_workspace - .workspace_for_paths_excluding( - &effective_path_list, - None, - &excluding, - cx, - ) + .workspace_for_paths(&effective_path_list, None, cx) .inspect(|workspace| { multi_workspace.activate( workspace.clone(), @@ -1459,14 +1283,11 @@ impl MultiWorkspace { } pub fn workspace(&self) -> &Entity { - &self.active_workspace + &self.held[self.displayed_index()].workspace } pub fn workspaces(&self) -> impl Iterator> { - let active_is_retained = self.is_workspace_retained(&self.active_workspace); - self.retained_workspaces - .iter() - .chain(std::iter::once(&self.active_workspace).filter(move |_| !active_is_retained)) + self.held.iter().map(|held| &held.workspace) } /// Adds a workspace to this window as persistent without changing which @@ -1477,16 +1298,12 @@ impl MultiWorkspace { if self.is_workspace_retained(&workspace) { return; } - - if workspace != self.active_workspace { - self.register_workspace(&workspace, window, cx); - } - let key = workspace.read(cx).project_group_key(cx); - self.retain_workspace(workspace, key, cx); + let index = self.hold(workspace, window, cx); + self.pin(index, key, cx); telemetry::event!( "Workspace Added", - workspace_count = self.retained_workspaces.len() + workspace_count = self.held.iter().filter(|held| held.pinned).count() ); cx.notify(); } @@ -1504,35 +1321,33 @@ impl MultiWorkspace { return; } - let old_active_workspace = self.active_workspace.clone(); + let old_active_workspace = self.workspace().clone(); let old_active_was_retained = self.active_workspace_is_retained(); - let workspace_was_retained = self.is_workspace_retained(&workspace); let should_retain_workspaces = self.multi_workspace_enabled(cx); if should_retain_workspaces && !old_active_was_retained { let key = old_active_workspace.read(cx).project_group_key(cx); - self.retain_workspace(old_active_workspace.clone(), key, cx); + let index = self.hold(old_active_workspace.clone(), window, cx); + self.pin(index, key, cx); } - if !workspace_was_retained { - self.register_workspace(&workspace, window, cx); - - if should_retain_workspaces { - let key = workspace.read(cx).project_group_key(cx); - self.retain_workspace(workspace.clone(), key, cx); - } + let displayed = self.hold(workspace.clone(), window, cx); + if should_retain_workspaces { + let key = workspace.read(cx).project_group_key(cx); + self.pin(displayed, key, cx); } - self.active_workspace = workspace; // Publish the new active workspace before anyone reads the shared cell // to decide who owns the window chrome. - self.active_workspace_id - .set(self.active_workspace.entity_id()); + self.active_workspace_id.set(workspace.entity_id()); - let active_key = self.active_workspace.read(cx).project_group_key(cx); - if let Some(group) = self.project_groups.iter_mut().find(|g| g.key == active_key) { - group.last_active_workspace = Some(self.active_workspace.downgrade()); - } + let stamp = self + .held + .iter() + .filter_map(|held| held.activated_at) + .max() + .map_or(0, |max| max + 1); + self.held[displayed].activated_at = Some(stamp); if !should_retain_workspaces && !old_active_was_retained { self.detach_workspace(&old_active_workspace, cx); @@ -1542,7 +1357,7 @@ impl MultiWorkspace { // The previously-active workspace left the title and edited indicator // reflecting its own state, so re-apply them from the newly-active // workspace (which is now the chrome owner per `owns_window_chrome`). - self.active_workspace.update(cx, |workspace, cx| { + workspace.update(cx, |workspace, cx| { workspace.refresh_window_state(window, cx); }); @@ -1552,40 +1367,16 @@ impl MultiWorkspace { cx.notify(); } - /// Adds `workspace` as a retained background tab without switching the - /// active workspace to it or moving focus. Mirrors the registration and - /// retention bookkeeping `activate` performs for the incoming workspace, - /// but leaves the currently-active workspace focused. - /// - /// Used when something opens a workspace the user should not be yanked - /// into — e.g. the agent's `create_thread` tool spawning a sibling - /// worktree in the background. - pub fn add_background_workspace( - &mut self, - workspace: Entity, - window: &mut Window, - cx: &mut Context, - ) { - if self.workspace() == &workspace || self.is_workspace_retained(&workspace) { - return; - } - self.register_workspace(&workspace, window, cx); - let key = workspace.read(cx).project_group_key(cx); - self.retain_workspace(workspace, key, cx); - cx.notify(); - } - /// Promotes the currently active workspace to persistent if it is /// transient, so it is retained across workspace switches even when /// the sidebar is closed. No-op if the workspace is already persistent. pub fn retain_active_workspace(&mut self, cx: &mut Context) { - let workspace = self.active_workspace.clone(); - if self.is_workspace_retained(&workspace) { + let index = self.displayed_index(); + if self.held[index].pinned { return; } - - let key = workspace.read(cx).project_group_key(cx); - self.retain_workspace(workspace, key, cx); + let key = self.held[index].workspace.read(cx).project_group_key(cx); + self.pin(index, key, cx); self.serialize(cx); cx.notify(); } @@ -1597,14 +1388,16 @@ impl MultiWorkspace { self.close_sidebar(window, cx); } - let active_workspace = self.active_workspace.clone(); - for workspace in self.retained_workspaces.clone() { - if workspace != active_workspace { + let displayed_workspace = self.workspace().clone(); + for workspace in self.workspaces().cloned().collect::>() { + if workspace != displayed_workspace { self.detach_workspace(&workspace, cx); } } - self.retained_workspaces.clear(); + for held in &mut self.held { + held.pinned = false; + } self.project_groups.clear(); cx.notify(); } @@ -1613,18 +1406,13 @@ impl MultiWorkspace { /// group key, and emits `WorkspaceRemoved`. The DB row is preserved /// so the workspace still appears in the recent-projects list. fn detach_workspace(&mut self, workspace: &Entity, cx: &mut Context) { - self.retained_workspaces - .retain(|retained| retained != workspace); - for group in &mut self.project_groups { - if group - .last_active_workspace - .as_ref() - .and_then(WeakEntity::upgrade) - .as_ref() - == Some(workspace) - { - group.last_active_workspace = None; - } + if let Some(index) = self.held_index(workspace) { + assert_ne!( + index, + self.displayed_index(), + "the displayed workspace must be re-pointed before it is detached" + ); + self.held.remove(index); } cx.emit(MultiWorkspaceEvent::WorkspaceRemoved(workspace.entity_id())); workspace.update(cx, |workspace, _cx| { @@ -1813,7 +1601,12 @@ impl MultiWorkspace { #[cfg(any(test, feature = "test-support"))] pub fn assert_project_group_key_integrity(&self, cx: &App) -> anyhow::Result<()> { let mut retained_ids: collections::HashSet = Default::default(); - for workspace in &self.retained_workspaces { + for workspace in self + .held + .iter() + .filter(|held| held.pinned) + .map(|held| &held.workspace) + { anyhow::ensure!( retained_ids.insert(workspace.entity_id()), "workspace {:?} is retained more than once", @@ -1863,7 +1656,6 @@ impl MultiWorkspace { self.project_groups.push(ProjectGroupState { key: group.key, expanded: group.expanded, - last_active_workspace: None, }); } @@ -1951,23 +1743,47 @@ impl MultiWorkspace { tasks } + /// The best replacement candidate within one project group: its most + /// recently displayed member, else its first pinned member, skipping + /// `excluding` and disconnected projects. + fn live_member_for_group( + &self, + key: &ProjectGroupKey, + excluding: &[Entity], + cx: &App, + ) -> Option> { + let available = |workspace: &Entity| { + !excluding.contains(workspace) + && !workspace.read(cx).project().read(cx).is_disconnected(cx) + }; + self.last_active_workspace_for_group(key, cx) + .filter(&available) + .or_else(|| { + self.held + .iter() + .filter(|held| held.pinned) + .map(|held| held.workspace.clone()) + .filter(|workspace| workspace.read(cx).project_group_key(cx) == *key) + .find(available) + }) + } + /// Removes one or more workspaces from this multi-workspace. /// - /// If the active workspace is among those being removed, - /// `fallback_workspace` is called **synchronously before the removal - /// begins** to produce a `Task` that resolves to the workspace that - /// should become active. The fallback must not be one of the - /// workspaces being removed. + /// Every workspace is first asked for consent (save prompts); no state + /// changes until all consent. The rows are then deleted and, when the + /// displayed workspace was among them, a replacement is chosen from what + /// remains: another workspace in the same project, then a workspace in the + /// nearest neighboring project, then an empty workspace. When the intent is + /// `KeepProject` and the project has no other workspace, its root worktrees + /// are reopened afterwards; the same applies to the adjacent local project + /// when nothing at all remains. /// /// Returns `true` if any workspaces were actually removed. pub fn remove( &mut self, workspaces: impl IntoIterator>, - fallback_workspace: impl FnOnce( - &mut Self, - &mut Window, - &mut Context, - ) -> Task>>, + intent: RemovalIntent, window: &mut Window, cx: &mut Context, ) -> Task> { @@ -1977,15 +1793,31 @@ impl MultiWorkspace { return Task::ready(Ok(false)); } - let removing_active = workspaces.iter().any(|ws| ws == self.workspace()); let original_active = self.workspace().clone(); + let group_key = original_active.read(cx).project_group_key(cx); - let fallback_task = removing_active.then(|| fallback_workspace(self, window, cx)); + // Record the neighborhood of the project as the user sees it now: + // callers like `remove_project_group` delete the project row itself + // before the removal task runs. + let group_index = self + .project_groups + .iter() + .position(|group| group.key == group_key); + let neighbor_keys = self.neighbor_group_keys(group_index); + let adjacent_key = group_index.and_then(|index| { + self.project_groups + .get(index + 1) + .or_else(|| { + index + .checked_sub(1) + .and_then(|previous| self.project_groups.get(previous)) + }) + .map(|group| group.key.clone()) + }); cx.spawn_in(window, async move |this, cx| { - // Run the standard workspace close lifecycle for every workspace - // being removed from this window. This handles save prompting and - // session cleanup consistently with other replace-in-window flows. + // Consent phase: run the standard close lifecycle for every + // workspace being removed. Prompts only; no state changes. for workspace in &workspaces { let should_continue = workspace .update_in(cx, |workspace, window, cx| { @@ -1998,47 +1830,101 @@ impl MultiWorkspace { } } - // If we're removing the active workspace, await the - // fallback and switch to it before tearing anything down. - // Otherwise restore the original active workspace in case - // prompting switched away from it. - if let Some(fallback_task) = fallback_task { - let new_active = fallback_task.await?; - - this.update_in(cx, |this, window, cx| { - assert!( - !workspaces.contains(&new_active), - "fallback workspace must not be one of the workspaces being removed" - ); - this.activate(new_active, None, window, cx); - })?; - } else { - this.update_in(cx, |this, window, cx| { - if *this.workspace() != original_active { - this.activate(original_active, None, window, cx); - } - })?; - } - - // Actually remove the workspaces. - this.update_in(cx, |this, _, cx| { + // Edit phase: one synchronous update. Delete the rows, then pick + // the replacement from the rows that actually remain. + let (removed_any, reopen_key) = this.update_in(cx, |this, window, cx| { let mut removed_any = false; + let displayed_workspace = this.workspace().clone(); for workspace in &workspaces { - let was_retained = this.is_workspace_retained(workspace); - if was_retained { + if *workspace == displayed_workspace { + continue; + } + if this.held_index(workspace).is_some() { this.detach_workspace(workspace, cx); removed_any = true; } } + let mut reopen_key = None; + if workspaces.contains(&displayed_workspace) { + let doomed = std::slice::from_ref(&displayed_workspace); + + let same_group = this + .held + .iter() + .filter(|held| held.pinned && held.workspace != displayed_workspace) + .map(|held| held.workspace.clone()) + .find(|workspace| workspace.read(cx).project_group_key(cx) == group_key); + if intent == RemovalIntent::KeepProject + && same_group.is_none() + && group_key.host().is_none() + && !group_key.path_list().is_empty() + { + reopen_key = Some(group_key.clone()); + } + + let replacement = same_group + .or_else(|| { + neighbor_keys + .iter() + .find_map(|key| this.live_member_for_group(key, doomed, cx)) + }) + .unwrap_or_else(|| { + if reopen_key.is_none() { + reopen_key = adjacent_key.clone().filter(|key| { + key.host().is_none() && !key.path_list().is_empty() + }); + } + let app_state = displayed_workspace.read(cx).app_state().clone(); + let project = Project::local( + app_state.client.clone(), + app_state.node_runtime.clone(), + app_state.user_store.clone(), + app_state.languages.clone(), + app_state.fs.clone(), + None, + project::LocalProjectFlags::default(), + cx, + ); + cx.new(|cx| Workspace::new(None, project, app_state, window, cx)) + }); + + this.activate(replacement, None, window, cx); + this.detach_workspace(&displayed_workspace, cx); + removed_any = true; + } else if *this.workspace() != original_active + && !workspaces.contains(&original_active) + { + // Prompting switched the display away from where the user + // was; go back. + this.activate(original_active.clone(), None, window, cx); + } + if removed_any { this.serialize(cx); cx.notify(); } - Ok(removed_any) - })? + (removed_any, reopen_key) + })?; + + if let Some(key) = reopen_key { + this.update_in(cx, |this, window, cx| { + this.find_or_create_local_workspace( + key.path_list().clone(), + Some(key), + None, + OpenMode::Activate, + None, + window, + cx, + ) + })? + .await?; + } + + Ok(removed_any) }) } @@ -2051,7 +1937,7 @@ impl MultiWorkspace { ) -> Task>> { if self.multi_workspace_enabled(cx) { let empty_workspace = if self - .active_workspace + .workspace() .read(cx) .project() .read(cx) @@ -2059,7 +1945,7 @@ impl MultiWorkspace { .next() .is_none() { - Some(self.active_workspace.clone()) + Some(self.workspace().clone()) } else { None }; @@ -2080,16 +1966,18 @@ impl MultiWorkspace { this.find_or_create_local_workspace( PathList::new(&paths), None, - empty_workspace.as_slice(), None, OpenMode::Activate, + None, window, cx, ) })?; let new_workspace = create_task.await?; - if let Some(empty_workspace) = empty_workspace { + if let Some(empty_workspace) = empty_workspace + && empty_workspace != new_workspace + { this.update(cx, |this, cx| { if this.is_workspace_retained(&empty_workspace) { this.detach_workspace(&empty_workspace, cx); diff --git a/crates/workspace/src/multi_workspace_tests.rs b/crates/workspace/src/multi_workspace_tests.rs index e642e1821b..8c767eee1b 100644 --- a/crates/workspace/src/multi_workspace_tests.rs +++ b/crates/workspace/src/multi_workspace_tests.rs @@ -432,9 +432,9 @@ async fn test_find_or_create_local_workspace_reuses_active_workspace_when_sideba mw.find_or_create_local_workspace( PathList::new(&[PathBuf::from("/root_a")]), None, - &[], None, OpenMode::Activate, + None, window, cx, ) @@ -497,9 +497,9 @@ async fn test_find_or_create_workspace_uses_project_group_key_when_paths_are_mis None, Some(project_group_key.clone()), |_options, _window, _cx| Task::ready(Ok(None)), - &[], None, OpenMode::Activate, + None, window, cx, ) @@ -555,22 +555,9 @@ async fn test_remove_fallback_via_find_or_create_skips_removed_workspaces(cx: &m let removed = multi_workspace .update_in(cx, |mw, window, cx| { - let excluded = vec![workspace_a.clone()]; mw.remove( - excluded.clone(), - move |this, window, cx| { - this.find_or_create_workspace( - PathList::new(&[PathBuf::from("/root_a")]), - None, - None, - |_options, _window, _cx| Task::ready(Ok(None)), - &excluded, - None, - OpenMode::Activate, - window, - cx, - ) - }, + vec![workspace_a.clone()], + RemovalIntent::CloseProject, window, cx, ) @@ -593,6 +580,52 @@ async fn test_remove_fallback_via_find_or_create_skips_removed_workspaces(cx: &m }); } +#[gpui::test] +async fn test_remove_keeping_the_project_does_not_switch_projects(cx: &mut TestAppContext) { + init_test(cx); + let fs = FakeFs::new(cx.executor()); + fs.insert_tree("/root_a", json!({ "file.txt": "" })).await; + fs.insert_tree("/root_b", json!({ "file.txt": "" })).await; + cx.update(|cx| ::set_global(fs.clone(), cx)); + let project_a = Project::test(fs.clone(), ["/root_a".as_ref()], cx).await; + let project_b = Project::test(fs, ["/root_b".as_ref()], cx).await; + + let (multi_workspace, cx) = + cx.add_window_view(|window, cx| MultiWorkspace::test_new(project_a, window, cx)); + + let workspace_a = multi_workspace.read_with(cx, |mw, _cx| mw.workspace().clone()); + let _workspace_b = multi_workspace.update_in(cx, |mw, window, cx| { + mw.test_add_workspace(project_b, window, cx) + }); + cx.run_until_parked(); + + multi_workspace.update_in(cx, |mw, window, cx| { + mw.activate(workspace_a.clone(), None, window, cx); + }); + cx.run_until_parked(); + + multi_workspace + .update_in(cx, |mw, window, cx| { + mw.remove( + vec![workspace_a.clone()], + RemovalIntent::KeepProject, + window, + cx, + ) + }) + .await + .expect("removing the active workspace should succeed"); + cx.run_until_parked(); + + multi_workspace.read_with(cx, |mw, cx| { + assert_eq!( + PathList::new(&mw.workspace().read(cx).root_paths(cx)), + PathList::new(&[PathBuf::from("/root_a")]), + "the replacement workspace should be in the removed workspace's project" + ); + }); +} + #[gpui::test] async fn test_find_or_create_local_workspace_reuses_active_workspace_after_sidebar_open( cx: &mut TestAppContext, @@ -625,9 +658,9 @@ async fn test_find_or_create_local_workspace_reuses_active_workspace_after_sideb mw.find_or_create_local_workspace( PathList::new(&[PathBuf::from("/root_a")]), None, - &[], None, OpenMode::Activate, + None, window, cx, ) @@ -711,7 +744,12 @@ async fn test_close_workspace_prefers_already_loaded_neighboring_workspace( let closed = multi_workspace .update_in(cx, |multi_workspace, window, cx| { - multi_workspace.close_workspace(&workspace_a, window, cx) + multi_workspace.remove( + [workspace_a.clone()], + RemovalIntent::CloseProject, + window, + cx, + ) }) .await .expect("closing the active workspace should succeed"); @@ -735,7 +773,6 @@ async fn test_close_workspace_prefers_already_loaded_neighboring_workspace( assert!( multi_workspace .workspaces_for_project_group(&project_c_key, cx) - .unwrap_or_default() .is_empty(), "the unloaded neighboring group C should remain unopened" ); @@ -758,7 +795,6 @@ async fn test_close_workspace_prefers_workspace_in_same_project_group(cx: &mut T let (workspace_a_1, workspace_a_2) = multi_workspace.read_with(cx, |multi_workspace, cx| { let mut workspaces = multi_workspace .workspaces_for_project_group(&key_a, cx) - .expect("project group A should have retained workspaces") .into_iter(); let first = workspaces .next() @@ -781,7 +817,12 @@ async fn test_close_workspace_prefers_workspace_in_same_project_group(cx: &mut T let closed = multi_workspace .update_in(cx, |multi_workspace, window, cx| { - multi_workspace.close_workspace(&workspace_a_1, window, cx) + multi_workspace.remove( + [workspace_a_1.clone()], + RemovalIntent::CloseProject, + window, + cx, + ) }) .await .expect("closing the active workspace should succeed"); @@ -795,9 +836,7 @@ async fn test_close_workspace_prefers_workspace_in_same_project_group(cx: &mut T ); assert_eq!( - multi_workspace - .workspaces_for_project_group(&key_a, cx) - .expect("project group A should remain"), + multi_workspace.workspaces_for_project_group(&key_a, cx), vec![workspace_a_2], "only the fallback workspace should remain in project group A" ); @@ -830,7 +869,12 @@ async fn test_close_workspace_opens_unloaded_local_neighbor(cx: &mut TestAppCont let closed = multi_workspace .update_in(cx, |multi_workspace, window, cx| { - multi_workspace.close_workspace(&workspace_a, window, cx) + multi_workspace.remove( + [workspace_a.clone()], + RemovalIntent::CloseProject, + window, + cx, + ) }) .await .expect("closing the active workspace should succeed"); @@ -1294,7 +1338,12 @@ async fn test_close_workspace_with_remote_neighbor_does_not_create_local_workspa // to creating an empty workspace instead. multi_workspace .update_in(cx, |mw, window, cx| { - mw.close_workspace(&workspace_a, window, cx) + mw.remove( + [workspace_a.clone()], + RemovalIntent::CloseProject, + window, + cx, + ) }) .await .expect("close_workspace should succeed"); @@ -1418,15 +1467,16 @@ async fn test_nearest_retained_workspace(cx: &mut TestAppContext) { .expect("project group for project-c should exist"); let workspace_b = multi_workspace .workspaces_for_project_group(&key_b, cx) - .and_then(|workspaces| workspaces.into_iter().next()) + .into_iter() + .next() .expect("workspace for project-b should exist"); let workspace_d = multi_workspace .workspaces_for_project_group(&key_d, cx) - .and_then(|workspaces| workspaces.into_iter().next()) + .into_iter() + .next() .expect("workspace for project-d should exist"); let retained_workspaces_a = multi_workspace - .workspaces_for_project_group(&key_a, cx) - .expect("project group A should have retained workspaces"); + .workspaces_for_project_group(&key_a, cx); assert_eq!( retained_workspaces_a.len(), 2, @@ -1477,23 +1527,20 @@ async fn test_nearest_retained_workspace(cx: &mut TestAppContext) { "the farther group's last active workspace should be preferred" ); - // We'll now clear the project group's last active workspace. - // In that state, the search falls back to the group's first retained - // workspace. - multi_workspace - .group_state_by_key_mut(&key_a) - .expect("project group A should exist") - .last_active_workspace - .take(); - + // With the group's most recently activated workspace excluded, the + // search falls back to the member activated before it. assert_eq!( multi_workspace.nearest_retained_workspace( group_c_index, - &[workspace_b.clone(), workspace_d.clone()], + &[ + workspace_b.clone(), + workspace_d.clone(), + workspace_a_2.clone() + ], cx ), Some(workspace_a_1.clone()), - "the first retained workspace should be used without a last active workspace" + "the previously activated workspace should be used when the last active one is excluded" ); // Excluding every neighboring workspace exhausts the search. diff --git a/crates/workspace/src/persistence.rs b/crates/workspace/src/persistence.rs index c64150eae1..f08aaba6c4 100644 --- a/crates/workspace/src/persistence.rs +++ b/crates/workspace/src/persistence.rs @@ -2764,9 +2764,9 @@ pub fn delete_unloaded_items( #[cfg(test)] mod tests { use super::*; - use crate::OpenMode; use crate::PathList; use crate::ProjectGroupKey; + use crate::RemovalIntent; use crate::{ multi_workspace::MultiWorkspace, persistence::{ @@ -2843,7 +2843,7 @@ mod tests { .workspaces() .find(|ws| *ws != &active) .expect("should have a non-active workspace"); - mw.remove([ws.clone()], |_, _, _| unreachable!(), _window, cx) + mw.remove([ws.clone()], RemovalIntent::CloseProject, _window, cx) .detach_and_log_err(cx); }); @@ -4811,7 +4811,7 @@ mod tests { // Remove workspace at index 1 (the second workspace). multi_workspace.update_in(cx, |mw, window, cx| { let ws = mw.workspaces().nth(1).unwrap().clone(); - mw.remove([ws], |_, _, _| unreachable!(), window, cx) + mw.remove([ws], RemovalIntent::CloseProject, window, cx) .detach_and_log_err(cx); }); @@ -4921,7 +4921,7 @@ mod tests { // Remove workspace2 (index 1). multi_workspace.update_in(cx, |mw, window, cx| { let ws = mw.workspaces().nth(1).unwrap().clone(); - mw.remove([ws], |_, _, _| unreachable!(), window, cx) + mw.remove([ws], RemovalIntent::CloseProject, window, cx) .detach_and_log_err(cx); }); @@ -5002,7 +5002,7 @@ mod tests { // Remove workspace2 — this pushes a task to pending_removal_tasks. multi_workspace.update_in(cx, |mw, window, cx| { let ws = mw.workspaces().nth(1).unwrap().clone(); - mw.remove([ws], |_, _, _| unreachable!(), window, cx) + mw.remove([ws], RemovalIntent::CloseProject, window, cx) .detach_and_log_err(cx); }); @@ -5955,27 +5955,12 @@ mod tests { }); cx.run_until_parked(); - // Remove workspace_a. The fallback searches for the same paths. - // Without the `excluding` parameter, `workspace_for_paths` would - // return workspace_a (first match) and the assert in `remove` - // would fire. With the fix, workspace_a is skipped and - // workspace_b is found instead. - let path_list = PathList::new(std::slice::from_ref(&dir)); - let excluded = vec![workspace_a.clone()]; + // Remove workspace_a. Its replacement is looked up by the same paths, + // so workspace_a itself has to be skipped or it would be picked again. multi_workspace.update_in(cx, |mw, window, cx| { mw.remove( vec![workspace_a.clone()], - move |this, window, cx| { - this.find_or_create_local_workspace( - path_list, - None, - &excluded, - None, - OpenMode::Activate, - window, - cx, - ) - }, + RemovalIntent::CloseProject, window, cx, ) diff --git a/crates/workspace/src/workspace.rs b/crates/workspace/src/workspace.rs index 0e79764e19..4c0e31e72e 100644 --- a/crates/workspace/src/workspace.rs +++ b/crates/workspace/src/workspace.rs @@ -33,8 +33,8 @@ pub use dock::Panel; pub use multi_workspace::{ CloseWorkspaceSidebar, DraggedSidebar, FocusWorkspaceSidebar, MoveProjectToNewWindow, MultiWorkspace, MultiWorkspaceEvent, NewThread, NextProject, NextThread, PreviousProject, - PreviousThread, ProjectGroup, ProjectGroupKey, SerializedProjectGroupState, Sidebar, - SidebarEvent, SidebarHandle, SidebarRenderState, SidebarSide, ToggleWorkspaceSidebar, + PreviousThread, ProjectGroup, ProjectGroupKey, RemovalIntent, SerializedProjectGroupState, + Sidebar, SidebarEvent, SidebarHandle, SidebarRenderState, SidebarSide, ToggleWorkspaceSidebar, sidebar_side_context_menu, }; pub use path_list::{PathList, SerializedPathList}; @@ -12056,7 +12056,12 @@ mod tests { // Try to remove workspace B. It should prompt because of the dirty item. let remove_task = multi_workspace_handle .update(cx, |mw, window, cx| { - mw.remove([workspace_b.clone()], |_, _, _| unreachable!(), window, cx) + mw.remove( + [workspace_b.clone()], + RemovalIntent::CloseProject, + window, + cx, + ) }) .unwrap(); cx.run_until_parked(); @@ -12094,7 +12099,12 @@ mod tests { .update(cx, |mw, window, cx| { // First switch back to A. mw.activate(workspace_a.clone(), None, window, cx); - mw.remove([workspace_b.clone()], |_, _, _| unreachable!(), window, cx) + mw.remove( + [workspace_b.clone()], + RemovalIntent::CloseProject, + window, + cx, + ) }) .unwrap(); cx.run_until_parked();