From af90077a6aaf12a2af5871a506ce544bbf0e8046 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Mon, 9 Oct 2023 13:30:14 -0700 Subject: [PATCH 1/2] Add failing test for switching leaders in a pane --- crates/collab/src/tests/following_tests.rs | 156 +++++++++------------ 1 file changed, 70 insertions(+), 86 deletions(-) diff --git a/crates/collab/src/tests/following_tests.rs b/crates/collab/src/tests/following_tests.rs index 3a489b9ac3..f3857e3db3 100644 --- a/crates/collab/src/tests/following_tests.rs +++ b/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)> { + 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::>(); + result.sort_by_key(|e| e.0); + result + }) +} + fn pane_summaries(workspace: &ViewHandle, cx: &mut TestAppContext) -> Vec { workspace.read_with(cx, |workspace, cx| { let active_pane = workspace.active_pane(); From ca735ad70f5229693466e9fa3511a9e3447c47b2 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Mon, 9 Oct 2023 13:32:38 -0700 Subject: [PATCH 2/2] Ensure there's only one leader per pane --- crates/workspace/src/pane_group.rs | 33 +++---- crates/workspace/src/workspace.rs | 137 ++++++++++++++--------------- 2 files changed, 77 insertions(+), 93 deletions(-) diff --git a/crates/workspace/src/pane_group.rs b/crates/workspace/src/pane_group.rs index c12cb261c8..aef03dcda0 100644 --- a/crates/workspace/src/pane_group.rs +++ b/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, theme: &Theme, - follower_states: &FollowerStatesByLeader, + follower_states: &HashMap, FollowerState>, active_call: Option<&ModelHandle>, active_pane: &ViewHandle, zoomed: Option<&AnyViewHandle>, @@ -162,7 +160,7 @@ impl Member { project: &ModelHandle, basis: usize, theme: &Theme, - follower_states: &FollowerStatesByLeader, + follower_states: &HashMap, FollowerState>, active_call: Option<&ModelHandle>, active_pane: &ViewHandle, 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, basis: usize, theme: &Theme, - follower_state: &FollowerStatesByLeader, + follower_states: &HashMap, FollowerState>, active_call: Option<&ModelHandle>, active_pane: &ViewHandle, zoomed: Option<&AnyViewHandle>, @@ -515,7 +504,7 @@ impl PaneAxis { project, (basis + ix) * 10, theme, - follower_state, + follower_states, active_call, active_pane, zoomed, diff --git a/crates/workspace/src/workspace.rs b/crates/workspace/src/workspace.rs index d39c1cce76..d97e444eb6 100644 --- a/crates/workspace/src/workspace.rs +++ b/crates/workspace/src/workspace.rs @@ -574,7 +574,7 @@ pub struct Workspace { titlebar_item: Option, notifications: Vec<(TypeId, usize, Box)>, project: ModelHandle, - follower_states_by_leader: FollowerStatesByLeader, + follower_states: HashMap, FollowerState>, last_leaders_by_pane: HashMap, PeerId>, window_edited: bool, active_call: Option<(ModelHandle, Vec)>, @@ -599,10 +599,9 @@ pub struct ViewId { pub id: u64, } -type FollowerStatesByLeader = HashMap, FollowerState>>; - #[derive(Default)] struct FollowerState { + leader_id: PeerId, active_view_id: Option, items_by_leader_view_id: HashMap>, } @@ -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) { - 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, cx: &mut ViewContext, ) -> Option { - 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(); - } - - cx.notify(); - return Some(leader_id); - } + 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); } - None + + 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(); + } + + 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) -> AnyElement { @@ -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) -> Option { - 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) -> 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