Fix bugs in handling mutual following (#3091)

Max Brunsfeld created 2 years ago

This fixes some bugs in our following logic, due to our attempts to
prevent infinite loops when two people follow each other.

* Propagate all of leader's views to a new follower, even if those views
were originally created by that follower.
* Propagate active view changes to followers, even if the active view is
following that follower.
* Avoid redundant active view updates on the client.

Release Notes:

- Fixed bugs where it was impossible to follow someone into a view that
they previously following you into.

Change summary

crates/collab/src/rpc.rs                   |  16 
crates/collab/src/tests/following_tests.rs | 541 ++++++++++++++++++++---
crates/workspace/src/workspace.rs          |  18 
3 files changed, 487 insertions(+), 88 deletions(-)

Detailed changes

crates/collab/src/rpc.rs

@@ -1906,13 +1906,10 @@ async fn follow(
         .check_room_participants(room_id, leader_id, session.connection_id)
         .await?;
 
-    let mut response_payload = session
+    let response_payload = session
         .peer
         .forward_request(session.connection_id, leader_id, request)
         .await?;
-    response_payload
-        .views
-        .retain(|view| view.leader_id != Some(follower_id.into()));
     response.send(response_payload)?;
 
     if let Some(project_id) = project_id {
@@ -1973,14 +1970,17 @@ async fn update_followers(request: proto::UpdateFollowers, session: Session) ->
             .await?
     };
 
-    let leader_id = request.variant.as_ref().and_then(|variant| match variant {
-        proto::update_followers::Variant::CreateView(payload) => payload.leader_id,
+    // For now, don't send view update messages back to that view's current leader.
+    let connection_id_to_omit = request.variant.as_ref().and_then(|variant| match variant {
         proto::update_followers::Variant::UpdateView(payload) => payload.leader_id,
-        proto::update_followers::Variant::UpdateActiveView(payload) => payload.leader_id,
+        _ => None,
     });
+
     for follower_peer_id in request.follower_ids.iter().copied() {
         let follower_connection_id = follower_peer_id.into();
-        if Some(follower_peer_id) != leader_id && connection_ids.contains(&follower_connection_id) {
+        if Some(follower_peer_id) != connection_id_to_omit
+            && connection_ids.contains(&follower_connection_id)
+        {
             session.peer.forward_send(
                 session.connection_id,
                 follower_connection_id,

crates/collab/src/tests/following_tests.rs

@@ -4,6 +4,7 @@ use collab_ui::project_shared_notification::ProjectSharedNotification;
 use editor::{Editor, ExcerptRange, MultiBuffer};
 use gpui::{executor::Deterministic, geometry::vector::vec2f, TestAppContext, ViewHandle};
 use live_kit_client::MacOSDisplay;
+use rpc::proto::PeerId;
 use serde_json::json;
 use std::{borrow::Cow, sync::Arc};
 use workspace::{
@@ -724,10 +725,9 @@ async fn test_peers_following_each_other(
         .await
         .unwrap();
 
-    // Client A opens some editors.
+    // Client A opens a file.
     let workspace_a = client_a.build_workspace(&project_a, cx_a).root(cx_a);
-    let pane_a1 = workspace_a.read_with(cx_a, |workspace, _| workspace.active_pane().clone());
-    let _editor_a1 = workspace_a
+    workspace_a
         .update(cx_a, |workspace, cx| {
             workspace.open_path((worktree_id, "1.txt"), None, true, cx)
         })
@@ -736,10 +736,9 @@ async fn test_peers_following_each_other(
         .downcast::<Editor>()
         .unwrap();
 
-    // Client B opens an editor.
+    // Client B opens a different file.
     let workspace_b = client_b.build_workspace(&project_b, cx_b).root(cx_b);
-    let pane_b1 = workspace_b.read_with(cx_b, |workspace, _| workspace.active_pane().clone());
-    let _editor_b1 = workspace_b
+    workspace_b
         .update(cx_b, |workspace, cx| {
             workspace.open_path((worktree_id, "2.txt"), None, true, cx)
         })
@@ -754,9 +753,7 @@ async fn test_peers_following_each_other(
     });
     workspace_a
         .update(cx_a, |workspace, cx| {
-            assert_ne!(*workspace.active_pane(), pane_a1);
-            let leader_id = *project_a.read(cx).collaborators().keys().next().unwrap();
-            workspace.follow(leader_id, cx).unwrap()
+            workspace.follow(client_b.peer_id().unwrap(), cx).unwrap()
         })
         .await
         .unwrap();
@@ -765,85 +762,443 @@ async fn test_peers_following_each_other(
     });
     workspace_b
         .update(cx_b, |workspace, cx| {
-            assert_ne!(*workspace.active_pane(), pane_b1);
-            let leader_id = *project_b.read(cx).collaborators().keys().next().unwrap();
-            workspace.follow(leader_id, cx).unwrap()
+            workspace.follow(client_a.peer_id().unwrap(), cx).unwrap()
         })
         .await
         .unwrap();
 
-    workspace_a.update(cx_a, |workspace, cx| {
-        workspace.activate_next_pane(cx);
-    });
-    // Wait for focus effects to be fully flushed
-    workspace_a.update(cx_a, |workspace, _| {
-        assert_eq!(*workspace.active_pane(), pane_a1);
-    });
+    // Clients A and B return focus to the original files they had open
+    workspace_a.update(cx_a, |workspace, cx| workspace.activate_next_pane(cx));
+    workspace_b.update(cx_b, |workspace, cx| workspace.activate_next_pane(cx));
+    deterministic.run_until_parked();
 
+    // Both clients see the other client's focused file in their right pane.
+    assert_eq!(
+        pane_summaries(&workspace_a, cx_a),
+        &[
+            PaneSummary {
+                active: true,
+                leader: None,
+                items: vec![(true, "1.txt".into())]
+            },
+            PaneSummary {
+                active: false,
+                leader: client_b.peer_id(),
+                items: vec![(false, "1.txt".into()), (true, "2.txt".into())]
+            },
+        ]
+    );
+    assert_eq!(
+        pane_summaries(&workspace_b, cx_b),
+        &[
+            PaneSummary {
+                active: true,
+                leader: None,
+                items: vec![(true, "2.txt".into())]
+            },
+            PaneSummary {
+                active: false,
+                leader: client_a.peer_id(),
+                items: vec![(false, "2.txt".into()), (true, "1.txt".into())]
+            },
+        ]
+    );
+
+    // Clients A and B each open a new file.
     workspace_a
         .update(cx_a, |workspace, cx| {
             workspace.open_path((worktree_id, "3.txt"), None, true, cx)
         })
         .await
         .unwrap();
-    workspace_b.update(cx_b, |workspace, cx| {
-        workspace.activate_next_pane(cx);
-    });
 
     workspace_b
         .update(cx_b, |workspace, cx| {
-            assert_eq!(*workspace.active_pane(), pane_b1);
             workspace.open_path((worktree_id, "4.txt"), None, true, cx)
         })
         .await
         .unwrap();
-    cx_a.foreground().run_until_parked();
+    deterministic.run_until_parked();
 
-    // Ensure leader updates don't change the active pane of followers
-    workspace_a.read_with(cx_a, |workspace, _| {
-        assert_eq!(*workspace.active_pane(), pane_a1);
+    // Both client's see the other client open the new file, but keep their
+    // focus on their own active pane.
+    assert_eq!(
+        pane_summaries(&workspace_a, cx_a),
+        &[
+            PaneSummary {
+                active: true,
+                leader: None,
+                items: vec![(false, "1.txt".into()), (true, "3.txt".into())]
+            },
+            PaneSummary {
+                active: false,
+                leader: client_b.peer_id(),
+                items: vec![
+                    (false, "1.txt".into()),
+                    (false, "2.txt".into()),
+                    (true, "4.txt".into())
+                ]
+            },
+        ]
+    );
+    assert_eq!(
+        pane_summaries(&workspace_b, cx_b),
+        &[
+            PaneSummary {
+                active: true,
+                leader: None,
+                items: vec![(false, "2.txt".into()), (true, "4.txt".into())]
+            },
+            PaneSummary {
+                active: false,
+                leader: client_a.peer_id(),
+                items: vec![
+                    (false, "2.txt".into()),
+                    (false, "1.txt".into()),
+                    (true, "3.txt".into())
+                ]
+            },
+        ]
+    );
+
+    // Client A focuses their right pane, in which they're following client B.
+    workspace_a.update(cx_a, |workspace, cx| workspace.activate_next_pane(cx));
+    deterministic.run_until_parked();
+
+    // Client B sees that client A is now looking at the same file as them.
+    assert_eq!(
+        pane_summaries(&workspace_a, cx_a),
+        &[
+            PaneSummary {
+                active: false,
+                leader: None,
+                items: vec![(false, "1.txt".into()), (true, "3.txt".into())]
+            },
+            PaneSummary {
+                active: true,
+                leader: client_b.peer_id(),
+                items: vec![
+                    (false, "1.txt".into()),
+                    (false, "2.txt".into()),
+                    (true, "4.txt".into())
+                ]
+            },
+        ]
+    );
+    assert_eq!(
+        pane_summaries(&workspace_b, cx_b),
+        &[
+            PaneSummary {
+                active: true,
+                leader: None,
+                items: vec![(false, "2.txt".into()), (true, "4.txt".into())]
+            },
+            PaneSummary {
+                active: false,
+                leader: client_a.peer_id(),
+                items: vec![
+                    (false, "2.txt".into()),
+                    (false, "1.txt".into()),
+                    (false, "3.txt".into()),
+                    (true, "4.txt".into())
+                ]
+            },
+        ]
+    );
+
+    // Client B focuses their right pane, in which they're following client A,
+    // who is following them.
+    workspace_b.update(cx_b, |workspace, cx| workspace.activate_next_pane(cx));
+    deterministic.run_until_parked();
+
+    // Client A sees that client B is now looking at the same file as them.
+    assert_eq!(
+        pane_summaries(&workspace_b, cx_b),
+        &[
+            PaneSummary {
+                active: false,
+                leader: None,
+                items: vec![(false, "2.txt".into()), (true, "4.txt".into())]
+            },
+            PaneSummary {
+                active: true,
+                leader: client_a.peer_id(),
+                items: vec![
+                    (false, "2.txt".into()),
+                    (false, "1.txt".into()),
+                    (false, "3.txt".into()),
+                    (true, "4.txt".into())
+                ]
+            },
+        ]
+    );
+    assert_eq!(
+        pane_summaries(&workspace_a, cx_a),
+        &[
+            PaneSummary {
+                active: false,
+                leader: None,
+                items: vec![(false, "1.txt".into()), (true, "3.txt".into())]
+            },
+            PaneSummary {
+                active: true,
+                leader: client_b.peer_id(),
+                items: vec![
+                    (false, "1.txt".into()),
+                    (false, "2.txt".into()),
+                    (true, "4.txt".into())
+                ]
+            },
+        ]
+    );
+
+    // Client B focuses a file that they previously followed A to, breaking
+    // the follow.
+    workspace_b.update(cx_b, |workspace, cx| {
+        workspace.active_pane().update(cx, |pane, cx| {
+            pane.activate_prev_item(true, cx);
+        });
     });
-    workspace_b.read_with(cx_b, |workspace, _| {
-        assert_eq!(*workspace.active_pane(), pane_b1);
+    deterministic.run_until_parked();
+
+    // Both clients see that client B is looking at that previous file.
+    assert_eq!(
+        pane_summaries(&workspace_b, cx_b),
+        &[
+            PaneSummary {
+                active: false,
+                leader: None,
+                items: vec![(false, "2.txt".into()), (true, "4.txt".into())]
+            },
+            PaneSummary {
+                active: true,
+                leader: None,
+                items: vec![
+                    (false, "2.txt".into()),
+                    (false, "1.txt".into()),
+                    (true, "3.txt".into()),
+                    (false, "4.txt".into())
+                ]
+            },
+        ]
+    );
+    assert_eq!(
+        pane_summaries(&workspace_a, cx_a),
+        &[
+            PaneSummary {
+                active: false,
+                leader: None,
+                items: vec![(false, "1.txt".into()), (true, "3.txt".into())]
+            },
+            PaneSummary {
+                active: true,
+                leader: client_b.peer_id(),
+                items: vec![
+                    (false, "1.txt".into()),
+                    (false, "2.txt".into()),
+                    (false, "4.txt".into()),
+                    (true, "3.txt".into()),
+                ]
+            },
+        ]
+    );
+
+    // Client B closes tabs, some of which were originally opened by client A,
+    // and some of which were originally opened by client B.
+    workspace_b.update(cx_b, |workspace, cx| {
+        workspace.active_pane().update(cx, |pane, cx| {
+            pane.close_inactive_items(&Default::default(), cx)
+                .unwrap()
+                .detach();
+        });
     });
 
-    // Ensure peers following each other doesn't cause an infinite loop.
+    deterministic.run_until_parked();
+
+    // Both clients see that Client B is looking at the previous tab.
     assert_eq!(
-        workspace_a.read_with(cx_a, |workspace, cx| workspace
-            .active_item(cx)
-            .unwrap()
-            .project_path(cx)),
-        Some((worktree_id, "3.txt").into())
+        pane_summaries(&workspace_b, cx_b),
+        &[
+            PaneSummary {
+                active: false,
+                leader: None,
+                items: vec![(false, "2.txt".into()), (true, "4.txt".into())]
+            },
+            PaneSummary {
+                active: true,
+                leader: None,
+                items: vec![(true, "3.txt".into()),]
+            },
+        ]
     );
+    assert_eq!(
+        pane_summaries(&workspace_a, cx_a),
+        &[
+            PaneSummary {
+                active: false,
+                leader: None,
+                items: vec![(false, "1.txt".into()), (true, "3.txt".into())]
+            },
+            PaneSummary {
+                active: true,
+                leader: client_b.peer_id(),
+                items: vec![
+                    (false, "1.txt".into()),
+                    (false, "2.txt".into()),
+                    (false, "4.txt".into()),
+                    (true, "3.txt".into()),
+                ]
+            },
+        ]
+    );
+
+    // Client B follows client A again.
+    workspace_b
+        .update(cx_b, |workspace, cx| {
+            workspace.follow(client_a.peer_id().unwrap(), cx).unwrap()
+        })
+        .await
+        .unwrap();
+
+    // Client A cycles through some tabs.
     workspace_a.update(cx_a, |workspace, cx| {
-        assert_eq!(
-            workspace.active_item(cx).unwrap().project_path(cx),
-            Some((worktree_id, "3.txt").into())
-        );
-        workspace.activate_next_pane(cx);
+        workspace.active_pane().update(cx, |pane, cx| {
+            pane.activate_prev_item(true, cx);
+        });
     });
+    deterministic.run_until_parked();
+
+    // Client B follows client A into those tabs.
+    assert_eq!(
+        pane_summaries(&workspace_a, cx_a),
+        &[
+            PaneSummary {
+                active: false,
+                leader: None,
+                items: vec![(false, "1.txt".into()), (true, "3.txt".into())]
+            },
+            PaneSummary {
+                active: true,
+                leader: None,
+                items: vec![
+                    (false, "1.txt".into()),
+                    (false, "2.txt".into()),
+                    (true, "4.txt".into()),
+                    (false, "3.txt".into()),
+                ]
+            },
+        ]
+    );
+    assert_eq!(
+        pane_summaries(&workspace_b, cx_b),
+        &[
+            PaneSummary {
+                active: false,
+                leader: None,
+                items: vec![(false, "2.txt".into()), (true, "4.txt".into())]
+            },
+            PaneSummary {
+                active: true,
+                leader: client_a.peer_id(),
+                items: vec![(false, "3.txt".into()), (true, "4.txt".into())]
+            },
+        ]
+    );
 
     workspace_a.update(cx_a, |workspace, cx| {
-        assert_eq!(
-            workspace.active_item(cx).unwrap().project_path(cx),
-            Some((worktree_id, "4.txt").into())
-        );
+        workspace.active_pane().update(cx, |pane, cx| {
+            pane.activate_prev_item(true, cx);
+        });
     });
+    deterministic.run_until_parked();
 
-    workspace_b.update(cx_b, |workspace, cx| {
-        assert_eq!(
-            workspace.active_item(cx).unwrap().project_path(cx),
-            Some((worktree_id, "4.txt").into())
-        );
-        workspace.activate_next_pane(cx);
-    });
+    assert_eq!(
+        pane_summaries(&workspace_a, cx_a),
+        &[
+            PaneSummary {
+                active: false,
+                leader: None,
+                items: vec![(false, "1.txt".into()), (true, "3.txt".into())]
+            },
+            PaneSummary {
+                active: true,
+                leader: None,
+                items: vec![
+                    (false, "1.txt".into()),
+                    (true, "2.txt".into()),
+                    (false, "4.txt".into()),
+                    (false, "3.txt".into()),
+                ]
+            },
+        ]
+    );
+    assert_eq!(
+        pane_summaries(&workspace_b, cx_b),
+        &[
+            PaneSummary {
+                active: false,
+                leader: None,
+                items: vec![(false, "2.txt".into()), (true, "4.txt".into())]
+            },
+            PaneSummary {
+                active: true,
+                leader: client_a.peer_id(),
+                items: vec![
+                    (false, "3.txt".into()),
+                    (false, "4.txt".into()),
+                    (true, "2.txt".into())
+                ]
+            },
+        ]
+    );
 
-    workspace_b.update(cx_b, |workspace, cx| {
-        assert_eq!(
-            workspace.active_item(cx).unwrap().project_path(cx),
-            Some((worktree_id, "3.txt").into())
-        );
+    workspace_a.update(cx_a, |workspace, cx| {
+        workspace.active_pane().update(cx, |pane, cx| {
+            pane.activate_prev_item(true, cx);
+        });
     });
+    deterministic.run_until_parked();
+
+    assert_eq!(
+        pane_summaries(&workspace_a, cx_a),
+        &[
+            PaneSummary {
+                active: false,
+                leader: None,
+                items: vec![(false, "1.txt".into()), (true, "3.txt".into())]
+            },
+            PaneSummary {
+                active: true,
+                leader: None,
+                items: vec![
+                    (true, "1.txt".into()),
+                    (false, "2.txt".into()),
+                    (false, "4.txt".into()),
+                    (false, "3.txt".into()),
+                ]
+            },
+        ]
+    );
+    assert_eq!(
+        pane_summaries(&workspace_b, cx_b),
+        &[
+            PaneSummary {
+                active: false,
+                leader: None,
+                items: vec![(false, "2.txt".into()), (true, "4.txt".into())]
+            },
+            PaneSummary {
+                active: true,
+                leader: client_a.peer_id(),
+                items: vec![
+                    (false, "3.txt".into()),
+                    (false, "4.txt".into()),
+                    (false, "2.txt".into()),
+                    (true, "1.txt".into()),
+                ]
+            },
+        ]
+    );
 }
 
 #[gpui::test(iterations = 10)]
@@ -1074,24 +1429,6 @@ async fn test_peers_simultaneously_following_each_other(
     });
 }
 
-fn visible_push_notifications(
-    cx: &mut TestAppContext,
-) -> Vec<gpui::ViewHandle<ProjectSharedNotification>> {
-    let mut ret = Vec::new();
-    for window in cx.windows() {
-        window.read_with(cx, |window| {
-            if let Some(handle) = window
-                .root_view()
-                .clone()
-                .downcast::<ProjectSharedNotification>()
-            {
-                ret.push(handle)
-            }
-        });
-    }
-    ret
-}
-
 #[gpui::test(iterations = 10)]
 async fn test_following_across_workspaces(
     deterministic: Arc<Deterministic>,
@@ -1304,3 +1641,59 @@ async fn test_following_across_workspaces(
         assert_eq!(item.tab_description(0, cx).unwrap(), Cow::Borrowed("y.rs"));
     });
 }
+
+fn visible_push_notifications(
+    cx: &mut TestAppContext,
+) -> Vec<gpui::ViewHandle<ProjectSharedNotification>> {
+    let mut ret = Vec::new();
+    for window in cx.windows() {
+        window.read_with(cx, |window| {
+            if let Some(handle) = window
+                .root_view()
+                .clone()
+                .downcast::<ProjectSharedNotification>()
+            {
+                ret.push(handle)
+            }
+        });
+    }
+    ret
+}
+
+#[derive(Debug, PartialEq, Eq)]
+struct PaneSummary {
+    active: bool,
+    leader: Option<PeerId>,
+    items: Vec<(bool, String)>,
+}
+
+fn pane_summaries(workspace: &ViewHandle<Workspace>, cx: &mut TestAppContext) -> Vec<PaneSummary> {
+    workspace.read_with(cx, |workspace, cx| {
+        let active_pane = workspace.active_pane();
+        workspace
+            .panes()
+            .iter()
+            .map(|pane| {
+                let leader = workspace.leader_for_pane(pane);
+                let active = pane == active_pane;
+                let pane = pane.read(cx);
+                let active_ix = pane.active_item_index();
+                PaneSummary {
+                    active,
+                    leader,
+                    items: pane
+                        .items()
+                        .enumerate()
+                        .map(|(ix, item)| {
+                            (
+                                ix == active_ix,
+                                item.tab_description(0, cx)
+                                    .map_or(String::new(), |s| s.to_string()),
+                            )
+                        })
+                        .collect(),
+                }
+            })
+            .collect()
+    })
+}

crates/workspace/src/workspace.rs

@@ -573,6 +573,7 @@ pub struct Workspace {
     panes_by_item: HashMap<usize, WeakViewHandle<Pane>>,
     active_pane: ViewHandle<Pane>,
     last_active_center_pane: Option<WeakViewHandle<Pane>>,
+    last_active_view_id: Option<proto::ViewId>,
     status_bar: ViewHandle<StatusBar>,
     titlebar_item: Option<AnyViewHandle>,
     notifications: Vec<(TypeId, usize, Box<dyn NotificationHandle>)>,
@@ -786,6 +787,7 @@ impl Workspace {
             panes_by_item: Default::default(),
             active_pane: center_pane.clone(),
             last_active_center_pane: Some(center_pane.downgrade()),
+            last_active_view_id: None,
             status_bar,
             titlebar_item: None,
             notifications: Default::default(),
@@ -2862,6 +2864,7 @@ impl Workspace {
 
         cx.notify();
 
+        self.last_active_view_id = active_view_id.clone();
         proto::FollowResponse {
             active_view_id,
             views: self
@@ -3028,7 +3031,7 @@ impl Workspace {
         Ok(())
     }
 
-    fn update_active_view_for_followers(&self, cx: &AppContext) {
+    fn update_active_view_for_followers(&mut self, cx: &AppContext) {
         let mut is_project_item = true;
         let mut update = proto::UpdateActiveView::default();
         if self.active_pane.read(cx).has_focus() {
@@ -3046,11 +3049,14 @@ impl Workspace {
             }
         }
 
-        self.update_followers(
-            is_project_item,
-            proto::update_followers::Variant::UpdateActiveView(update),
-            cx,
-        );
+        if update.id != self.last_active_view_id {
+            self.last_active_view_id = update.id.clone();
+            self.update_followers(
+                is_project_item,
+                proto::update_followers::Variant::UpdateActiveView(update),
+                cx,
+            );
+        }
     }
 
     fn update_followers(