Improve auto watch (#56126)

Joseph T. Lyons created 1 day ago

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: 

<img width="324" height="61"
alt="589131021-c967dfe1-9026-4a1d-a399-b735303f2de0"
src="https://github.com/user-attachments/assets/7cd414cd-5a13-4e16-ab6e-5de6d2cd64ed"
/>

   After:

<img width="373" height="77"
alt="589131282-607e15a5-e50c-4a8e-b22c-327f2e7b8ab5"
src="https://github.com/user-attachments/assets/7c19e0c8-8c50-4f8c-b966-f2a824eea4a0"
/>


- 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

Change summary

crates/call/src/call_impl/room.rs                   |   5 
crates/collab/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(-)

Detailed changes

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
                         }
                     });

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<Project>,
+    client_b: TestClient,
+    client_c: TestClient,
+    channel_id: ChannelId,
+    user_a_project: Entity<Project>,
+    user_b_project: Entity<Project>,
 }
 
 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::<SharedScreen>().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::<SharedScreen>()
+        .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

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 {

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 {