From 6766514599c6f8ce6530ccc685db5e0d68c44f32 Mon Sep 17 00:00:00 2001 From: "Joseph T. Lyons" Date: Fri, 8 May 2026 01:45:12 -0400 Subject: [PATCH] Improve auto watch (#56126) This PR fixes a few bugs, updates some UI, and improves testing of auto watch. It'll likely be easier to review commit by commit: - Swapped the Copy Channel Link and Auto Watch buttons so Auto Watch appears in a better position. The UI is still not great, but I think this tweak will improve it until someone on design can help. Before: 589131021-c967dfe1-9026-4a1d-a399-b735303f2de0 After: 589131282-607e15a5-e50c-4a8e-b22c-327f2e7b8ab5 - Disable Auto Watch when following another collaborator, with test coverage for that behavior. We currently disable following when engaging auto watch, and now we disable auto watch when following. They are mutually exclusive and I think the feels correct. - Refactored Auto Watch integration tests to use channels API instead of room API. - Improved test robustness by using assertions to identify `SharedScreen` items by type and `peer_id` instead of tab title text. - Fixed Auto Watch for returning channel participants by emitting `RemoteVideoTracksChanged` when removing a participant with active video tracks, with regression coverage for leave/rejoin/share. Self-Review Checklist: - [X] I've reviewed my own diff for quality, security, and reliability - [ ] Unsafe blocks (if any) have justifying comments - [X] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [X] Tests cover the new/changed behavior - [X] Performance impact has been considered and is acceptable Closes Release Notes: - N/A --- crates/call/src/call_impl/room.rs | 5 + .../tests/integration/auto_watch_tests.rs | 312 +++++++++++++++--- crates/collab_ui/src/collab_panel.rs | 14 +- crates/workspace/src/workspace.rs | 1 + 4 files changed, 275 insertions(+), 57 deletions(-) diff --git a/crates/call/src/call_impl/room.rs b/crates/call/src/call_impl/room.rs index 658c2b620643f50c3dc347f9ae99f7c7ef8cae43..21b40822cf05187cde558d8a389e4a8e4628a024 100644 --- a/crates/call/src/call_impl/room.rs +++ b/crates/call/src/call_impl/room.rs @@ -935,6 +935,11 @@ impl Room { for sid in participant.video_tracks.keys() { cx.emit(Event::RemoteVideoTrackUnsubscribed { sid: sid.clone() }); } + if !participant.video_tracks.is_empty() { + cx.emit(Event::RemoteVideoTracksChanged { + participant_id: participant.peer_id, + }); + } false } }); diff --git a/crates/collab/tests/integration/auto_watch_tests.rs b/crates/collab/tests/integration/auto_watch_tests.rs index c8d395407b362b2ccebccb2fc2f859542ca0aeea..f119e1a4af4d94d391d9c549e77a2ffb9ff1b95f 100644 --- a/crates/collab/tests/integration/auto_watch_tests.rs +++ b/crates/collab/tests/integration/auto_watch_tests.rs @@ -1,18 +1,20 @@ use crate::TestServer; use call::ActiveCall; +use client::ChannelId; use gpui::{App, BackgroundExecutor, Entity, TestAppContext, TestScreenCaptureSource}; use project::Project; -use serde_json::json; -use util::path; -use workspace::Workspace; +use rpc::proto::PeerId; +use workspace::{AutoWatch, SharedScreen, Workspace}; use super::TestClient; struct AutoWatchTestSetup { client_a: TestClient, - _client_b: TestClient, - _client_c: TestClient, - project_a: Entity, + client_b: TestClient, + client_c: TestClient, + channel_id: ChannelId, + user_a_project: Entity, + user_b_project: Entity, } async fn setup_auto_watch_test( @@ -20,35 +22,67 @@ async fn setup_auto_watch_test( user_a: &mut TestAppContext, user_b: &mut TestAppContext, user_c: &mut TestAppContext, +) -> AutoWatchTestSetup { + setup_auto_watch_test_with_initial_participants(server, user_a, user_b, user_c, true).await +} + +async fn setup_auto_watch_late_joiner_test( + server: &mut TestServer, + user_a: &mut TestAppContext, + user_b: &mut TestAppContext, + user_c: &mut TestAppContext, +) -> AutoWatchTestSetup { + setup_auto_watch_test_with_initial_participants(server, user_a, user_b, user_c, false).await +} + +async fn setup_auto_watch_test_with_initial_participants( + server: &mut TestServer, + user_a: &mut TestAppContext, + user_b: &mut TestAppContext, + user_c: &mut TestAppContext, + join_user_c: bool, ) -> AutoWatchTestSetup { let client_a = server.create_client(user_a, "user_a").await; let client_b = server.create_client(user_b, "user_b").await; let client_c = server.create_client(user_c, "user_c").await; - server - .create_room(&mut [ + let channel_id = server + .make_channel( + "the-channel", + None, (&client_a, user_a), - (&client_b, user_b), - (&client_c, user_c), - ]) + &mut [(&client_b, user_b), (&client_c, user_c)], + ) .await; - let active_call_a = user_a.read(ActiveCall::global); + let user_a_project = client_a.build_empty_local_project(false, user_a); + let user_b_project = client_b.build_empty_local_project(false, user_b); - client_a - .fs() - .insert_tree(path!("/a"), json!({ "file.txt": "content" })) - .await; - let (project_a, _worktree_id) = client_a.build_local_project(path!("/a"), user_a).await; + let active_call_a = user_a.read(ActiveCall::global); active_call_a - .update(user_a, |call, cx| call.set_location(Some(&project_a), cx)) + .update(user_a, |call, cx| call.join_channel(channel_id, cx)) .await .unwrap(); + let active_call_b = user_b.read(ActiveCall::global); + active_call_b + .update(user_b, |call, cx| call.join_channel(channel_id, cx)) + .await + .unwrap(); + + if join_user_c { + let active_call_c = user_c.read(ActiveCall::global); + active_call_c + .update(user_c, |call, cx| call.join_channel(channel_id, cx)) + .await + .unwrap(); + } AutoWatchTestSetup { client_a, - _client_b: client_b, - _client_c: client_c, - project_a, + client_b, + client_c, + channel_id, + user_a_project, + user_b_project, } } @@ -61,7 +95,9 @@ async fn test_auto_watch_opens_existing_share_on_toggle( ) { let mut server = TestServer::start(executor.clone()).await; let setup = setup_auto_watch_test(&mut server, user_a, user_b, user_c).await; - let (workspace_a, user_a) = setup.client_a.build_workspace(&setup.project_a, user_a); + let (workspace_a, user_a) = setup + .client_a + .build_workspace(&setup.user_a_project, user_a); executor.run_until_parked(); start_screen_share(user_b).await; @@ -73,7 +109,11 @@ async fn test_auto_watch_opens_existing_share_on_toggle( executor.run_until_parked(); workspace_a.update(user_a, |workspace, cx| { - assert_active_matches_title(workspace, "user_b's screen", cx); + assert_active_item_is_screen_share_for_peer( + workspace, + setup.client_b.peer_id().unwrap(), + cx, + ); }); } @@ -86,7 +126,9 @@ async fn test_auto_watch_opens_share_when_no_one_is_sharing_yet( ) { let mut server = TestServer::start(executor.clone()).await; let setup = setup_auto_watch_test(&mut server, user_a, user_b, user_c).await; - let (workspace_a, user_a) = setup.client_a.build_workspace(&setup.project_a, user_a); + let (workspace_a, user_a) = setup + .client_a + .build_workspace(&setup.user_a_project, user_a); workspace_a.update_in(user_a, |workspace, window, cx| { workspace.toggle_auto_watch(window, cx); @@ -96,7 +138,11 @@ async fn test_auto_watch_opens_share_when_no_one_is_sharing_yet( executor.run_until_parked(); workspace_a.update(user_a, |workspace, cx| { - assert_active_matches_title(workspace, "user_b's screen", cx); + assert_active_item_is_screen_share_for_peer( + workspace, + setup.client_b.peer_id().unwrap(), + cx, + ); }); } @@ -109,7 +155,9 @@ async fn test_auto_watch_switches_to_next_share_on_share_end( ) { let mut server = TestServer::start(executor.clone()).await; let setup = setup_auto_watch_test(&mut server, user_a, user_b, user_c).await; - let (workspace_a, user_a) = setup.client_a.build_workspace(&setup.project_a, user_a); + let (workspace_a, user_a) = setup + .client_a + .build_workspace(&setup.user_a_project, user_a); workspace_a.update_in(user_a, |workspace, window, cx| { workspace.toggle_auto_watch(window, cx); @@ -119,7 +167,11 @@ async fn test_auto_watch_switches_to_next_share_on_share_end( executor.run_until_parked(); workspace_a.update(user_a, |workspace, cx| { - assert_active_matches_title(workspace, "user_b's screen", cx); + assert_active_item_is_screen_share_for_peer( + workspace, + setup.client_b.peer_id().unwrap(), + cx, + ); }); start_screen_share(user_c).await; @@ -129,7 +181,11 @@ async fn test_auto_watch_switches_to_next_share_on_share_end( executor.run_until_parked(); workspace_a.update(user_a, |workspace, cx| { - assert_active_matches_title(workspace, "user_c's screen", cx); + assert_active_item_is_screen_share_for_peer( + workspace, + setup.client_c.peer_id().unwrap(), + cx, + ); }); } @@ -142,7 +198,9 @@ async fn test_auto_watch_ignores_shares_while_user_is_sharing( ) { let mut server = TestServer::start(executor.clone()).await; let setup = setup_auto_watch_test(&mut server, user_a, user_b, user_c).await; - let (workspace_a, user_a) = setup.client_a.build_workspace(&setup.project_a, user_a); + let (workspace_a, user_a) = setup + .client_a + .build_workspace(&setup.user_a_project, user_a); start_screen_share(user_a).await; executor.run_until_parked(); @@ -155,16 +213,11 @@ async fn test_auto_watch_ignores_shares_while_user_is_sharing( }); executor.run_until_parked(); - // Ensure that no screen share is found in user a's tab bar workspace_a.update(user_a, |workspace, cx| { - let has_shared_screen_tab = workspace - .active_pane() - .read(cx) - .items() - .any(|item| item.tab_content_text(0, cx).contains("screen")); - assert!( - !has_shared_screen_tab, - "should not open anyone's screen share when toggling on while sharing" + assert_no_screen_share_tabs_exist( + workspace, + "should not open anyone's screen share when toggling on while sharing", + cx, ); }); } @@ -178,7 +231,9 @@ async fn test_auto_watch_opens_share_after_local_user_stops_sharing( ) { let mut server = TestServer::start(executor.clone()).await; let setup = setup_auto_watch_test(&mut server, user_a, user_b, user_c).await; - let (workspace_a, user_a) = setup.client_a.build_workspace(&setup.project_a, user_a); + let (workspace_a, user_a) = setup + .client_a + .build_workspace(&setup.user_a_project, user_a); workspace_a.update_in(user_a, |workspace, window, cx| { workspace.toggle_auto_watch(window, cx); @@ -193,7 +248,11 @@ async fn test_auto_watch_opens_share_after_local_user_stops_sharing( executor.run_until_parked(); workspace_a.update(user_a, |workspace, cx| { - assert_active_matches_title(workspace, "user_b's screen", cx); + assert_active_item_is_screen_share_for_peer( + workspace, + setup.client_b.peer_id().unwrap(), + cx, + ); }); } @@ -206,7 +265,9 @@ async fn test_auto_watch_toggle_off_leaves_tabs_open( ) { let mut server = TestServer::start(executor.clone()).await; let setup = setup_auto_watch_test(&mut server, user_a, user_b, user_c).await; - let (workspace_a, user_a) = setup.client_a.build_workspace(&setup.project_a, user_a); + let (workspace_a, user_a) = setup + .client_a + .build_workspace(&setup.user_a_project, user_a); workspace_a.update_in(user_a, |workspace, window, cx| { workspace.toggle_auto_watch(window, cx); @@ -215,27 +276,177 @@ async fn test_auto_watch_toggle_off_leaves_tabs_open( executor.run_until_parked(); workspace_a.update(user_a, |workspace, cx| { - assert_active_matches_title(workspace, "user_b's screen", cx); + assert_active_item_is_screen_share_for_peer( + workspace, + setup.client_b.peer_id().unwrap(), + cx, + ); + }); + + workspace_a.update_in(user_a, |workspace, window, cx| { + workspace.toggle_auto_watch(window, cx); + }); + + workspace_a.update(user_a, |workspace, cx| { + assert_active_item_is_screen_share_for_peer( + workspace, + setup.client_b.peer_id().unwrap(), + cx, + ); + }); +} + +#[gpui::test] +async fn test_auto_watch_reopens_screen_share_from_returning_channel_participant( + executor: BackgroundExecutor, + user_a: &mut TestAppContext, + user_b: &mut TestAppContext, + user_c: &mut TestAppContext, +) { + let mut server = TestServer::start(executor.clone()).await; + let setup = setup_auto_watch_late_joiner_test(&mut server, user_a, user_b, user_c).await; + let (workspace_a, user_a) = setup + .client_a + .build_workspace(&setup.user_a_project, user_a); + let (workspace_b, user_b) = setup + .client_b + .build_workspace(&setup.user_b_project, user_b); + + workspace_a.update_in(user_a, |workspace, window, cx| { + workspace.toggle_auto_watch(window, cx); + }); + workspace_b.update_in(user_b, |workspace, window, cx| { + workspace.toggle_auto_watch(window, cx); + }); + executor.run_until_parked(); + + let active_call_c = user_c.read(ActiveCall::global); + active_call_c + .update(user_c, |call, cx| call.join_channel(setup.channel_id, cx)) + .await + .unwrap(); + executor.run_until_parked(); + + start_screen_share(user_c).await; + executor.run_until_parked(); + + workspace_a.update(user_a, |workspace, cx| { + assert_active_item_is_screen_share_for_peer( + workspace, + setup.client_c.peer_id().unwrap(), + cx, + ); + }); + workspace_b.update(user_b, |workspace, cx| { + assert_active_item_is_screen_share_for_peer( + workspace, + setup.client_c.peer_id().unwrap(), + cx, + ); + }); + + active_call_c + .update(user_c, |call, cx| call.hang_up(cx)) + .await + .unwrap(); + executor.run_until_parked(); + + workspace_a.update(user_a, |workspace, cx| { + assert_no_screen_share_tabs_exist( + workspace, + "user A should stop seeing user C's screen after user C hangs up", + cx, + ); }); + workspace_b.update(user_b, |workspace, cx| { + assert_no_screen_share_tabs_exist( + workspace, + "user B should stop seeing user C's screen after user C hangs up", + cx, + ); + }); + + let active_call_c = user_c.read(ActiveCall::global); + active_call_c + .update(user_c, |call, cx| call.join_channel(setup.channel_id, cx)) + .await + .unwrap(); + executor.run_until_parked(); + + start_screen_share(user_c).await; + executor.run_until_parked(); + + workspace_a.update(user_a, |workspace, cx| { + assert_active_item_is_screen_share_for_peer( + workspace, + setup.client_c.peer_id().unwrap(), + cx, + ); + }); + workspace_b.update(user_b, |workspace, cx| { + assert_active_item_is_screen_share_for_peer( + workspace, + setup.client_c.peer_id().unwrap(), + cx, + ); + }); +} + +#[gpui::test] +async fn test_auto_watch_is_disabled_when_following_collaborator( + executor: BackgroundExecutor, + user_a: &mut TestAppContext, + user_b: &mut TestAppContext, + user_c: &mut TestAppContext, +) { + let mut server = TestServer::start(executor.clone()).await; + let setup = setup_auto_watch_test(&mut server, user_a, user_b, user_c).await; + let (workspace_a, user_a) = setup + .client_a + .build_workspace(&setup.user_a_project, user_a); + let user_b_peer_id = setup.client_b.peer_id().unwrap(); workspace_a.update_in(user_a, |workspace, window, cx| { workspace.toggle_auto_watch(window, cx); }); + start_screen_share(user_b).await; + executor.run_until_parked(); workspace_a.update(user_a, |workspace, cx| { - assert_active_matches_title(workspace, "user_b's screen", cx); + assert_active_item_is_screen_share_for_peer( + workspace, + setup.client_b.peer_id().unwrap(), + cx, + ); }); + + workspace_a.update_in(user_a, |workspace, window, cx| { + workspace.follow(user_b_peer_id, window, cx); + }); + executor.run_until_parked(); + + workspace_a.update(user_a, |workspace, _cx| { + assert_eq!(*workspace.auto_watch_state(), AutoWatch::Off); + }); +} + +#[track_caller] +fn assert_no_screen_share_tabs_exist(workspace: &Workspace, message: &str, cx: &App) { + let has_shared_screen_tab = workspace + .active_pane() + .read(cx) + .items() + .any(|item| item.downcast::().is_some()); + assert!(!has_shared_screen_tab, "{message}"); } #[track_caller] -fn assert_active_matches_title(workspace: &Workspace, expected_title: &str, cx: &App) { +fn assert_active_item_is_screen_share_for_peer(workspace: &Workspace, peer_id: PeerId, cx: &App) { let active_item = workspace.active_item(cx).expect("no active item"); - assert_eq!( - active_item.tab_content_text(0, cx), - expected_title, - "expected active item to be '{}'", - expected_title - ); + let shared_screen = active_item + .downcast::() + .expect("expected active item to be a shared screen"); + assert_eq!(shared_screen.read(cx).peer_id, peer_id); } async fn start_screen_share(cx: &mut TestAppContext) { @@ -260,6 +471,7 @@ async fn start_screen_share(cx: &mut TestAppContext) { .unwrap(); } +#[track_caller] fn stop_screen_share(cx: &mut TestAppContext) { let active_call = cx.read(ActiveCall::global); active_call diff --git a/crates/collab_ui/src/collab_panel.rs b/crates/collab_ui/src/collab_panel.rs index cea3806edb3e01a56caaf6e9e68cd5d2951596f1..5fe6d839569b63d56eb1b766698d0e1b48802ef7 100644 --- a/crates/collab_ui/src/collab_panel.rs +++ b/crates/collab_ui/src/collab_panel.rs @@ -2913,6 +2913,13 @@ impl CollabPanel { if show_auto_watch || show_copy { Some( h_flex() + .when_some(channel_link, |this, channel_link| { + this.child( + CopyButton::new("copy-channel-link", channel_link) + .visible_on_hover("section-header") + .tooltip_label("Copy Channel Link"), + ) + }) .when(has_auto_watch_flag, |this| { this.child( IconButton::new( @@ -2952,13 +2959,6 @@ impl CollabPanel { )), ) }) - .when_some(channel_link, |this, channel_link| { - this.child( - CopyButton::new("copy-channel-link", channel_link) - .visible_on_hover("section-header") - .tooltip_label("Copy Channel Link"), - ) - }) .into_any_element(), ) } else { diff --git a/crates/workspace/src/workspace.rs b/crates/workspace/src/workspace.rs index 9cc1fa30865f81f764e5bc2e3bc3abb9031108fd..79b7e28ffb34ff8279b207b160c8664986c076e6 100644 --- a/crates/workspace/src/workspace.rs +++ b/crates/workspace/src/workspace.rs @@ -5742,6 +5742,7 @@ impl Workspace { .insert(pane.downgrade(), leader_id); self.unfollow(leader_id, window, cx); self.unfollow_in_pane(&pane, window, cx); + self.auto_watch = AutoWatch::Off; self.follower_states.insert( leader_id, FollowerState {