Fix bug that allowed following multiple people in one pane (#3108)

Max Brunsfeld created

I've also simplified the representation of a workspace's leaders, so
that it encodes in the type that there can only be one leader per pane.

Release Notes:

- Fixed a bug where you could accidentally follow multiple collaborators
in one pane at the same time.

Change summary

crates/collab/src/tests/following_tests.rs | 156 ++++++++++-------------
crates/workspace/src/pane_group.rs         |  33 +---
crates/workspace/src/workspace.rs          | 135 ++++++++++----------
3 files changed, 146 insertions(+), 178 deletions(-)

Detailed changes

crates/collab/src/tests/following_tests.rs 🔗

@@ -184,20 +184,12 @@ async fn test_basic_following(
 
     // All clients see that clients B and C are following client A.
     cx_c.foreground().run_until_parked();
-    for (name, active_call, cx) in [
-        ("A", &active_call_a, &cx_a),
-        ("B", &active_call_b, &cx_b),
-        ("C", &active_call_c, &cx_c),
-        ("D", &active_call_d, &cx_d),
-    ] {
-        active_call.read_with(*cx, |call, cx| {
-            let room = call.room().unwrap().read(cx);
-            assert_eq!(
-                room.followers_for(peer_id_a, project_id),
-                &[peer_id_b, peer_id_c],
-                "checking followers for A as {name}"
-            );
-        });
+    for (name, cx) in [("A", &cx_a), ("B", &cx_b), ("C", &cx_c), ("D", &cx_d)] {
+        assert_eq!(
+            followers_by_leader(project_id, cx),
+            &[(peer_id_a, vec![peer_id_b, peer_id_c])],
+            "followers seen by {name}"
+        );
     }
 
     // Client C unfollows client A.
@@ -207,46 +199,39 @@ async fn test_basic_following(
 
     // All clients see that clients B is following client A.
     cx_c.foreground().run_until_parked();
-    for (name, active_call, cx) in [
-        ("A", &active_call_a, &cx_a),
-        ("B", &active_call_b, &cx_b),
-        ("C", &active_call_c, &cx_c),
-        ("D", &active_call_d, &cx_d),
-    ] {
-        active_call.read_with(*cx, |call, cx| {
-            let room = call.room().unwrap().read(cx);
-            assert_eq!(
-                room.followers_for(peer_id_a, project_id),
-                &[peer_id_b],
-                "checking followers for A as {name}"
-            );
-        });
+    for (name, cx) in [("A", &cx_a), ("B", &cx_b), ("C", &cx_c), ("D", &cx_d)] {
+        assert_eq!(
+            followers_by_leader(project_id, cx),
+            &[(peer_id_a, vec![peer_id_b])],
+            "followers seen by {name}"
+        );
     }
 
     // Client C re-follows client A.
-    workspace_c.update(cx_c, |workspace, cx| {
-        workspace.follow(peer_id_a, cx);
-    });
+    workspace_c
+        .update(cx_c, |workspace, cx| {
+            workspace.follow(peer_id_a, cx).unwrap()
+        })
+        .await
+        .unwrap();
 
     // All clients see that clients B and C are following client A.
     cx_c.foreground().run_until_parked();
-    for (name, active_call, cx) in [
-        ("A", &active_call_a, &cx_a),
-        ("B", &active_call_b, &cx_b),
-        ("C", &active_call_c, &cx_c),
-        ("D", &active_call_d, &cx_d),
-    ] {
-        active_call.read_with(*cx, |call, cx| {
-            let room = call.room().unwrap().read(cx);
-            assert_eq!(
-                room.followers_for(peer_id_a, project_id),
-                &[peer_id_b, peer_id_c],
-                "checking followers for A as {name}"
-            );
-        });
+    for (name, cx) in [("A", &cx_a), ("B", &cx_b), ("C", &cx_c), ("D", &cx_d)] {
+        assert_eq!(
+            followers_by_leader(project_id, cx),
+            &[(peer_id_a, vec![peer_id_b, peer_id_c])],
+            "followers seen by {name}"
+        );
     }
 
-    // Client D follows client C.
+    // Client D follows client B, then switches to following client C.
+    workspace_d
+        .update(cx_d, |workspace, cx| {
+            workspace.follow(peer_id_b, cx).unwrap()
+        })
+        .await
+        .unwrap();
     workspace_d
         .update(cx_d, |workspace, cx| {
             workspace.follow(peer_id_c, cx).unwrap()
@@ -256,20 +241,15 @@ async fn test_basic_following(
 
     // All clients see that D is following C
     cx_d.foreground().run_until_parked();
-    for (name, active_call, cx) in [
-        ("A", &active_call_a, &cx_a),
-        ("B", &active_call_b, &cx_b),
-        ("C", &active_call_c, &cx_c),
-        ("D", &active_call_d, &cx_d),
-    ] {
-        active_call.read_with(*cx, |call, cx| {
-            let room = call.room().unwrap().read(cx);
-            assert_eq!(
-                room.followers_for(peer_id_c, project_id),
-                &[peer_id_d],
-                "checking followers for C as {name}"
-            );
-        });
+    for (name, cx) in [("A", &cx_a), ("B", &cx_b), ("C", &cx_c), ("D", &cx_d)] {
+        assert_eq!(
+            followers_by_leader(project_id, cx),
+            &[
+                (peer_id_a, vec![peer_id_b, peer_id_c]),
+                (peer_id_c, vec![peer_id_d])
+            ],
+            "followers seen by {name}"
+        );
     }
 
     // Client C closes the project.
@@ -278,32 +258,12 @@ async fn test_basic_following(
 
     // Clients A and B see that client B is following A, and client C is not present in the followers.
     cx_c.foreground().run_until_parked();
-    for (name, active_call, cx) in [("A", &active_call_a, &cx_a), ("B", &active_call_b, &cx_b)] {
-        active_call.read_with(*cx, |call, cx| {
-            let room = call.room().unwrap().read(cx);
-            assert_eq!(
-                room.followers_for(peer_id_a, project_id),
-                &[peer_id_b],
-                "checking followers for A as {name}"
-            );
-        });
-    }
-
-    // All clients see that no-one is following C
-    for (name, active_call, cx) in [
-        ("A", &active_call_a, &cx_a),
-        ("B", &active_call_b, &cx_b),
-        ("C", &active_call_c, &cx_c),
-        ("D", &active_call_d, &cx_d),
-    ] {
-        active_call.read_with(*cx, |call, cx| {
-            let room = call.room().unwrap().read(cx);
-            assert_eq!(
-                room.followers_for(peer_id_c, project_id),
-                &[],
-                "checking followers for C as {name}"
-            );
-        });
+    for (name, cx) in [("A", &cx_a), ("B", &cx_b), ("C", &cx_c), ("D", &cx_d)] {
+        assert_eq!(
+            followers_by_leader(project_id, cx),
+            &[(peer_id_a, vec![peer_id_b]),],
+            "followers seen by {name}"
+        );
     }
 
     // When client A activates a different editor, client B does so as well.
@@ -1667,6 +1627,30 @@ struct PaneSummary {
     items: Vec<(bool, String)>,
 }
 
+fn followers_by_leader(project_id: u64, cx: &TestAppContext) -> Vec<(PeerId, Vec<PeerId>)> {
+    cx.read(|cx| {
+        let active_call = ActiveCall::global(cx).read(cx);
+        let peer_id = active_call.client().peer_id();
+        let room = active_call.room().unwrap().read(cx);
+        let mut result = room
+            .remote_participants()
+            .values()
+            .map(|participant| participant.peer_id)
+            .chain(peer_id)
+            .filter_map(|peer_id| {
+                let followers = room.followers_for(peer_id, project_id);
+                if followers.is_empty() {
+                    None
+                } else {
+                    Some((peer_id, followers.to_vec()))
+                }
+            })
+            .collect::<Vec<_>>();
+        result.sort_by_key(|e| e.0);
+        result
+    })
+}
+
 fn pane_summaries(workspace: &ViewHandle<Workspace>, cx: &mut TestAppContext) -> Vec<PaneSummary> {
     workspace.read_with(cx, |workspace, cx| {
         let active_pane = workspace.active_pane();

crates/workspace/src/pane_group.rs 🔗

@@ -1,10 +1,7 @@
-use std::{cell::RefCell, rc::Rc, sync::Arc};
-
-use crate::{
-    pane_group::element::PaneAxisElement, AppState, FollowerStatesByLeader, Pane, Workspace,
-};
+use crate::{pane_group::element::PaneAxisElement, AppState, FollowerState, Pane, Workspace};
 use anyhow::{anyhow, Result};
 use call::{ActiveCall, ParticipantLocation};
+use collections::HashMap;
 use gpui::{
     elements::*,
     geometry::{rect::RectF, vector::Vector2F},
@@ -13,6 +10,7 @@ use gpui::{
 };
 use project::Project;
 use serde::Deserialize;
+use std::{cell::RefCell, rc::Rc, sync::Arc};
 use theme::Theme;
 
 const HANDLE_HITBOX_SIZE: f32 = 4.0;
@@ -95,7 +93,7 @@ impl PaneGroup {
         &self,
         project: &ModelHandle<Project>,
         theme: &Theme,
-        follower_states: &FollowerStatesByLeader,
+        follower_states: &HashMap<ViewHandle<Pane>, FollowerState>,
         active_call: Option<&ModelHandle<ActiveCall>>,
         active_pane: &ViewHandle<Pane>,
         zoomed: Option<&AnyViewHandle>,
@@ -162,7 +160,7 @@ impl Member {
         project: &ModelHandle<Project>,
         basis: usize,
         theme: &Theme,
-        follower_states: &FollowerStatesByLeader,
+        follower_states: &HashMap<ViewHandle<Pane>, FollowerState>,
         active_call: Option<&ModelHandle<ActiveCall>>,
         active_pane: &ViewHandle<Pane>,
         zoomed: Option<&AnyViewHandle>,
@@ -179,19 +177,10 @@ impl Member {
                     ChildView::new(pane, cx).into_any()
                 };
 
-                let leader = follower_states
-                    .iter()
-                    .find_map(|(leader_id, follower_states)| {
-                        if follower_states.contains_key(pane) {
-                            Some(leader_id)
-                        } else {
-                            None
-                        }
-                    })
-                    .and_then(|leader_id| {
-                        let room = active_call?.read(cx).room()?.read(cx);
-                        room.remote_participant_for_peer_id(*leader_id)
-                    });
+                let leader = follower_states.get(pane).and_then(|state| {
+                    let room = active_call?.read(cx).room()?.read(cx);
+                    room.remote_participant_for_peer_id(state.leader_id)
+                });
 
                 let mut leader_border = Border::default();
                 let mut leader_status_box = None;
@@ -486,7 +475,7 @@ impl PaneAxis {
         project: &ModelHandle<Project>,
         basis: usize,
         theme: &Theme,
-        follower_state: &FollowerStatesByLeader,
+        follower_states: &HashMap<ViewHandle<Pane>, FollowerState>,
         active_call: Option<&ModelHandle<ActiveCall>>,
         active_pane: &ViewHandle<Pane>,
         zoomed: Option<&AnyViewHandle>,
@@ -515,7 +504,7 @@ impl PaneAxis {
                 project,
                 (basis + ix) * 10,
                 theme,
-                follower_state,
+                follower_states,
                 active_call,
                 active_pane,
                 zoomed,

crates/workspace/src/workspace.rs 🔗

@@ -574,7 +574,7 @@ pub struct Workspace {
     titlebar_item: Option<AnyViewHandle>,
     notifications: Vec<(TypeId, usize, Box<dyn NotificationHandle>)>,
     project: ModelHandle<Project>,
-    follower_states_by_leader: FollowerStatesByLeader,
+    follower_states: HashMap<ViewHandle<Pane>, FollowerState>,
     last_leaders_by_pane: HashMap<WeakViewHandle<Pane>, PeerId>,
     window_edited: bool,
     active_call: Option<(ModelHandle<ActiveCall>, Vec<Subscription>)>,
@@ -599,10 +599,9 @@ pub struct ViewId {
     pub id: u64,
 }
 
-type FollowerStatesByLeader = HashMap<PeerId, HashMap<ViewHandle<Pane>, FollowerState>>;
-
 #[derive(Default)]
 struct FollowerState {
+    leader_id: PeerId,
     active_view_id: Option<ViewId>,
     items_by_leader_view_id: HashMap<ViewId, Box<dyn FollowableItemHandle>>,
 }
@@ -791,7 +790,7 @@ impl Workspace {
             bottom_dock,
             right_dock,
             project: project.clone(),
-            follower_states_by_leader: Default::default(),
+            follower_states: Default::default(),
             last_leaders_by_pane: Default::default(),
             window_edited: false,
             active_call,
@@ -2509,13 +2508,16 @@ impl Workspace {
     }
 
     fn collaborator_left(&mut self, peer_id: PeerId, cx: &mut ViewContext<Self>) {
-        if let Some(states_by_pane) = self.follower_states_by_leader.remove(&peer_id) {
-            for state in states_by_pane.into_values() {
-                for item in state.items_by_leader_view_id.into_values() {
+        self.follower_states.retain(|_, state| {
+            if state.leader_id == peer_id {
+                for item in state.items_by_leader_view_id.values() {
                     item.set_leader_peer_id(None, cx);
                 }
+                false
+            } else {
+                true
             }
-        }
+        });
         cx.notify();
     }
 
@@ -2528,10 +2530,15 @@ impl Workspace {
 
         self.last_leaders_by_pane
             .insert(pane.downgrade(), leader_id);
-        self.follower_states_by_leader
-            .entry(leader_id)
-            .or_default()
-            .insert(pane.clone(), Default::default());
+        self.unfollow(&pane, cx);
+        self.follower_states.insert(
+            pane.clone(),
+            FollowerState {
+                leader_id,
+                active_view_id: None,
+                items_by_leader_view_id: Default::default(),
+            },
+        );
         cx.notify();
 
         let room_id = self.active_call()?.read(cx).room()?.read(cx).id();
@@ -2546,9 +2553,8 @@ impl Workspace {
             let response = request.await?;
             this.update(&mut cx, |this, _| {
                 let state = this
-                    .follower_states_by_leader
-                    .get_mut(&leader_id)
-                    .and_then(|states_by_pane| states_by_pane.get_mut(&pane))
+                    .follower_states
+                    .get_mut(&pane)
                     .ok_or_else(|| anyhow!("following interrupted"))?;
                 state.active_view_id = if let Some(active_view_id) = response.active_view_id {
                     Some(ViewId::from_proto(active_view_id)?)
@@ -2643,12 +2649,10 @@ impl Workspace {
         }
 
         // if you're already following, find the right pane and focus it.
-        for (existing_leader_id, states_by_pane) in &mut self.follower_states_by_leader {
-            if leader_id == *existing_leader_id {
-                for (pane, _) in states_by_pane {
-                    cx.focus(pane);
-                    return None;
-                }
+        for (pane, state) in &self.follower_states {
+            if leader_id == state.leader_id {
+                cx.focus(pane);
+                return None;
             }
         }
 
@@ -2661,36 +2665,37 @@ impl Workspace {
         pane: &ViewHandle<Pane>,
         cx: &mut ViewContext<Self>,
     ) -> Option<PeerId> {
-        for (leader_id, states_by_pane) in &mut self.follower_states_by_leader {
-            let leader_id = *leader_id;
-            if let Some(state) = states_by_pane.remove(pane) {
-                for (_, item) in state.items_by_leader_view_id {
-                    item.set_leader_peer_id(None, cx);
-                }
-
-                if states_by_pane.is_empty() {
-                    self.follower_states_by_leader.remove(&leader_id);
-                    let project_id = self.project.read(cx).remote_id();
-                    let room_id = self.active_call()?.read(cx).room()?.read(cx).id();
-                    self.app_state
-                        .client
-                        .send(proto::Unfollow {
-                            room_id,
-                            project_id,
-                            leader_id: Some(leader_id),
-                        })
-                        .log_err();
-                }
+        let state = self.follower_states.remove(pane)?;
+        let leader_id = state.leader_id;
+        for (_, item) in state.items_by_leader_view_id {
+            item.set_leader_peer_id(None, cx);
+        }
 
-                cx.notify();
-                return Some(leader_id);
-            }
+        if self
+            .follower_states
+            .values()
+            .all(|state| state.leader_id != state.leader_id)
+        {
+            let project_id = self.project.read(cx).remote_id();
+            let room_id = self.active_call()?.read(cx).room()?.read(cx).id();
+            self.app_state
+                .client
+                .send(proto::Unfollow {
+                    room_id,
+                    project_id,
+                    leader_id: Some(leader_id),
+                })
+                .log_err();
         }
-        None
+
+        cx.notify();
+        Some(leader_id)
     }
 
     pub fn is_being_followed(&self, peer_id: PeerId) -> bool {
-        self.follower_states_by_leader.contains_key(&peer_id)
+        self.follower_states
+            .values()
+            .any(|state| state.leader_id == peer_id)
     }
 
     fn render_titlebar(&self, theme: &Theme, cx: &mut ViewContext<Self>) -> AnyElement<Self> {
@@ -2913,8 +2918,8 @@ impl Workspace {
         match update.variant.ok_or_else(|| anyhow!("invalid update"))? {
             proto::update_followers::Variant::UpdateActiveView(update_active_view) => {
                 this.update(cx, |this, _| {
-                    if let Some(state) = this.follower_states_by_leader.get_mut(&leader_id) {
-                        for state in state.values_mut() {
+                    for (_, state) in &mut this.follower_states {
+                        if state.leader_id == leader_id {
                             state.active_view_id =
                                 if let Some(active_view_id) = update_active_view.id.clone() {
                                     Some(ViewId::from_proto(active_view_id)?)
@@ -2936,8 +2941,8 @@ impl Workspace {
                 let mut tasks = Vec::new();
                 this.update(cx, |this, cx| {
                     let project = this.project.clone();
-                    if let Some(state) = this.follower_states_by_leader.get_mut(&leader_id) {
-                        for state in state.values_mut() {
+                    for (_, state) in &mut this.follower_states {
+                        if state.leader_id == leader_id {
                             let view_id = ViewId::from_proto(id.clone())?;
                             if let Some(item) = state.items_by_leader_view_id.get(&view_id) {
                                 tasks.push(item.apply_update_proto(&project, variant.clone(), cx));
@@ -2950,10 +2955,9 @@ impl Workspace {
             }
             proto::update_followers::Variant::CreateView(view) => {
                 let panes = this.read_with(cx, |this, _| {
-                    this.follower_states_by_leader
-                        .get(&leader_id)
-                        .into_iter()
-                        .flat_map(|states_by_pane| states_by_pane.keys())
+                    this.follower_states
+                        .iter()
+                        .filter_map(|(pane, state)| (state.leader_id == leader_id).then_some(pane))
                         .cloned()
                         .collect()
                 })?;
@@ -3012,11 +3016,7 @@ impl Workspace {
         for (pane, (item_tasks, leader_view_ids)) in item_tasks_by_pane {
             let items = futures::future::try_join_all(item_tasks).await?;
             this.update(cx, |this, cx| {
-                let state = this
-                    .follower_states_by_leader
-                    .get_mut(&leader_id)?
-                    .get_mut(&pane)?;
-
+                let state = this.follower_states.get_mut(&pane)?;
                 for (id, item) in leader_view_ids.into_iter().zip(items) {
                     item.set_leader_peer_id(Some(leader_id), cx);
                     state.items_by_leader_view_id.insert(id, item);
@@ -3073,15 +3073,7 @@ impl Workspace {
     }
 
     pub fn leader_for_pane(&self, pane: &ViewHandle<Pane>) -> Option<PeerId> {
-        self.follower_states_by_leader
-            .iter()
-            .find_map(|(leader_id, state)| {
-                if state.contains_key(pane) {
-                    Some(*leader_id)
-                } else {
-                    None
-                }
-            })
+        self.follower_states.get(pane).map(|state| state.leader_id)
     }
 
     fn leader_updated(&mut self, leader_id: PeerId, cx: &mut ViewContext<Self>) -> Option<()> {
@@ -3109,7 +3101,10 @@ impl Workspace {
             }
         };
 
-        for (pane, state) in self.follower_states_by_leader.get(&leader_id)? {
+        for (pane, state) in &self.follower_states {
+            if state.leader_id != leader_id {
+                continue;
+            }
             if leader_in_this_app {
                 let item = state
                     .active_view_id
@@ -3804,7 +3799,7 @@ impl View for Workspace {
                                                     self.center.render(
                                                         &project,
                                                         &theme,
-                                                        &self.follower_states_by_leader,
+                                                        &self.follower_states,
                                                         self.active_call(),
                                                         self.active_pane(),
                                                         self.zoomed