diff --git a/crates/channel/src/channel_store.rs b/crates/channel/src/channel_store.rs index 1778e09a74..f95ae6bd9b 100644 --- a/crates/channel/src/channel_store.rs +++ b/crates/channel/src/channel_store.rs @@ -133,7 +133,7 @@ impl ChannelStore { } pub fn index_of_channel(&self, channel_id: ChannelId) -> Option { - self.channel_paths + self.channel_index .iter() .position(|path| path.ends_with(&[channel_id])) } @@ -327,11 +327,43 @@ impl ChannelStore { }) } + pub fn link_channel( + &mut self, + channel_id: ChannelId, + to: ChannelId, + cx: &mut ModelContext, + ) -> Task> { + let client = self.client.clone(); + cx.spawn(|_, _| async move { + let _ = client + .request(proto::LinkChannel { channel_id, to }) + .await?; + + Ok(()) + }) + } + + pub fn unlink_channel( + &mut self, + channel_id: ChannelId, + from: Option, + cx: &mut ModelContext, + ) -> Task> { + let client = self.client.clone(); + cx.spawn(|_, _| async move { + let _ = client + .request(proto::UnlinkChannel { channel_id, from }) + .await?; + + Ok(()) + }) + } + pub fn move_channel( &mut self, channel_id: ChannelId, - from_parent: Option, - to: Option, + from: Option, + to: ChannelId, cx: &mut ModelContext, ) -> Task> { let client = self.client.clone(); @@ -339,7 +371,7 @@ impl ChannelStore { let _ = client .request(proto::MoveChannel { channel_id, - from_parent, + from, to, }) .await?; @@ -802,6 +834,4 @@ impl ChannelStore { anyhow::Ok(()) })) } - - } diff --git a/crates/collab/src/db/queries/channels.rs b/crates/collab/src/db/queries/channels.rs index d347be6c4f..2bfbb7abdb 100644 --- a/crates/collab/src/db/queries/channels.rs +++ b/crates/collab/src/db/queries/channels.rs @@ -761,20 +761,41 @@ impl Database { .await } - async fn link_channel( + // Insert an edge from the given channel to the given other channel. + pub async fn link_channel( &self, - from: ChannelId, + user: UserId, + channel: ChannelId, + to: ChannelId, + ) -> Result> { + self.transaction(|tx| async move { + // Note that even with these maxed permissions, this linking operation + // is still insecure because you can't remove someone's permissions to a + // channel if they've linked the channel to one where they're an admin. + self.check_user_is_channel_admin(channel, user, &*tx) + .await?; + + self.link_channel_internal(user, channel, to, &*tx).await + }) + .await + } + + pub async fn link_channel_internal( + &self, + user: UserId, + channel: ChannelId, to: ChannelId, tx: &DatabaseTransaction, - ) -> Result { + ) -> Result> { + self.check_user_is_channel_admin(to, user, &*tx).await?; + let to_ancestors = self.get_channel_ancestors(to, &*tx).await?; - let from_descendants = self.get_channel_descendants([from], &*tx).await?; + let mut from_descendants = self.get_channel_descendants([channel], &*tx).await?; for ancestor in to_ancestors { if from_descendants.contains_key(&ancestor) { return Err(anyhow!("Cannot create a channel cycle").into()); } } - let sql = r#" INSERT INTO channel_paths (id_path, channel_id) @@ -790,14 +811,13 @@ impl Database { self.pool.get_database_backend(), sql, [ - from.to_proto().into(), - from.to_proto().into(), + channel.to_proto().into(), + channel.to_proto().into(), to.to_proto().into(), ], ); tx.execute(channel_paths_stmt).await?; - - for (from_id, to_ids) in from_descendants.iter().filter(|(id, _)| id != &&from) { + for (from_id, to_ids) in from_descendants.iter().filter(|(id, _)| id != &&channel) { for to_id in to_ids { let channel_paths_stmt = Statement::from_sql_and_values( self.pool.get_database_backend(), @@ -812,94 +832,116 @@ impl Database { } } - Ok(from_descendants) + if let Some(channel) = from_descendants.get_mut(&channel) { + // Remove the other parents + channel.clear(); + channel.insert(to); + } + + let channels = self.get_all_channels(from_descendants, &*tx).await?; + + Ok(channels) } - async fn remove_channel_from_parent( + /// Unlink a channel from a given parent. This will add in a root edge if + /// the channel has no other parents after this operation. + pub async fn unlink_channel( &self, - from: ChannelId, - parent: ChannelId, + user: UserId, + channel: ChannelId, + from: Option, + ) -> Result<()> { + self.transaction(|tx| async move { + // Note that even with these maxed permissions, this linking operation + // is still insecure because you can't remove someone's permissions to a + // channel if they've linked the channel to one where they're an admin. + self.check_user_is_channel_admin(channel, user, &*tx) + .await?; + + self.unlink_channel_internal(user, channel, from, &*tx) + .await?; + + Ok(()) + }) + .await + } + + pub async fn unlink_channel_internal( + &self, + user: UserId, + channel: ChannelId, + from: Option, tx: &DatabaseTransaction, ) -> Result<()> { - let sql = r#" + if let Some(from) = from { + self.check_user_is_channel_admin(from, user, &*tx).await?; + + let sql = r#" DELETE FROM channel_paths WHERE id_path LIKE '%' || $1 || '/' || $2 || '%' "#; + let channel_paths_stmt = Statement::from_sql_and_values( + self.pool.get_database_backend(), + sql, + [from.to_proto().into(), channel.to_proto().into()], + ); + tx.execute(channel_paths_stmt).await?; + } else { + let sql = r#" + DELETE FROM channel_paths + WHERE + id_path = '/' || $1 || '/' + "#; + let channel_paths_stmt = Statement::from_sql_and_values( + self.pool.get_database_backend(), + sql, + [channel.to_proto().into()], + ); + tx.execute(channel_paths_stmt).await?; + } + + // Make sure that there is always at least one path to the channel + let sql = r#" + INSERT INTO channel_paths + (id_path, channel_id) + SELECT + '/' || $1 || '/', $2 + WHERE NOT EXISTS + (SELECT * + FROM channel_paths + WHERE channel_id = $2) + "#; + let channel_paths_stmt = Statement::from_sql_and_values( self.pool.get_database_backend(), sql, - [parent.to_proto().into(), from.to_proto().into()], + [channel.to_proto().into(), channel.to_proto().into()], ); tx.execute(channel_paths_stmt).await?; Ok(()) } - /// Move a channel from one parent to another. - /// Note that this requires a valid parent_id in the 'from_parent' field. - /// As channels are a DAG, we need to know which parent to remove the channel from. - /// Here's a list of the parameters to this function and their behavior: - /// - /// - (`None`, `None`) No op - /// - (`None`, `Some(id)`) Link the channel without removing it from any of it's parents - /// - (`Some(id)`, `None`) Remove a channel from a given parent, and leave other parents - /// - (`Some(id)`, `Some(id)`) Move channel from one parent to another, leaving other parents - /// - /// Returns the channel that was moved + it's sub channels for use - /// by the members for `to` + /// Move a channel from one parent to another, returns the + /// Channels that were moved for notifying clients pub async fn move_channel( &self, user: UserId, - from: ChannelId, - from_parent: Option, - to: Option, + channel: ChannelId, + from: Option, + to: ChannelId, ) -> Result> { self.transaction(|tx| async move { - // Note that even with these maxed permissions, this linking operation - // is still insecure because you can't remove someone's permissions to a - // channel if they've linked the channel to one where they're an admin. - self.check_user_is_channel_admin(from, user, &*tx).await?; + self.check_user_is_channel_admin(channel, user, &*tx) + .await?; - let mut channel_descendants = None; + let moved_channels = self.link_channel_internal(user, channel, to, &*tx).await?; - // Note that we have to do the linking before the removal, so that we - // can leave the channel_path table in a consistent state. - if let Some(to) = to { - self.check_user_is_channel_admin(to, user, &*tx).await?; + self.unlink_channel_internal(user, channel, from, &*tx) + .await?; - channel_descendants = Some(self.link_channel(from, to, &*tx).await?); - } - - let mut channel_descendants = match channel_descendants { - Some(channel_descendants) => channel_descendants, - None => self.get_channel_descendants([from], &*tx).await?, - }; - - if let Some(from_parent) = from_parent { - self.check_user_is_channel_admin(from_parent, user, &*tx) - .await?; - - self.remove_channel_from_parent(from, from_parent, &*tx) - .await?; - } - - let channels; - if let Some(to) = to { - if let Some(channel) = channel_descendants.get_mut(&from) { - // Remove the other parents - channel.clear(); - channel.insert(to); - } - - channels = self - .get_all_channels(channel_descendants, &*tx) - .await?; - } else { - channels = vec![]; - } - - Ok(channels) + Ok(moved_channels) }) .await } diff --git a/crates/collab/src/db/tests/channel_tests.rs b/crates/collab/src/db/tests/channel_tests.rs index 7f159ea0bd..edf4bbef5a 100644 --- a/crates/collab/src/db/tests/channel_tests.rs +++ b/crates/collab/src/db/tests/channel_tests.rs @@ -1,7 +1,7 @@ use rpc::{proto, ConnectionId}; use crate::{ - db::{Channel, Database, NewUserParams}, + db::{Channel, ChannelId, Database, NewUserParams}, test_both_dbs, }; use std::sync::Arc; @@ -501,50 +501,32 @@ async fn test_channels_moving(db: &Arc) { .await .unwrap(); + // ======================================================================== // sanity check // Initial DAG: // /- gpui2 // zed -- crdb - livestreaming - livestreaming_dag let result = db.get_channels_for_user(a_id).await.unwrap(); - pretty_assertions::assert_eq!( + assert_dag( result.channels, - vec![ - Channel { - id: zed_id, - name: "zed".to_string(), - parent_id: None, - }, - Channel { - id: crdb_id, - name: "crdb".to_string(), - parent_id: Some(zed_id), - }, - Channel { - id: gpui2_id, - name: "gpui2".to_string(), - parent_id: Some(zed_id), - }, - Channel { - id: livestreaming_id, - name: "livestreaming".to_string(), - parent_id: Some(crdb_id), - }, - Channel { - id: livestreaming_dag_id, - name: "livestreaming_dag".to_string(), - parent_id: Some(livestreaming_id), - }, - ] + &[ + (zed_id, None), + (crdb_id, Some(zed_id)), + (gpui2_id, Some(zed_id)), + (livestreaming_id, Some(crdb_id)), + (livestreaming_dag_id, Some(livestreaming_id)), + ], ); // Attempt to make a cycle assert!(db - .move_channel(a_id, zed_id, None, Some(livestreaming_id)) + .link_channel(a_id, zed_id, livestreaming_id) .await .is_err()); + // ======================================================================== // Make a link - db.move_channel(a_id, livestreaming_id, None, Some(zed_id)) + db.link_channel(a_id, livestreaming_id, zed_id) .await .unwrap(); @@ -553,42 +535,16 @@ async fn test_channels_moving(db: &Arc) { // zed -- crdb - livestreaming - livestreaming_dag // \---------/ let result = db.get_channels_for_user(a_id).await.unwrap(); - pretty_assertions::assert_eq!( - result.channels, - vec![ - Channel { - id: zed_id, - name: "zed".to_string(), - parent_id: None, - }, - Channel { - id: crdb_id, - name: "crdb".to_string(), - parent_id: Some(zed_id), - }, - Channel { - id: gpui2_id, - name: "gpui2".to_string(), - parent_id: Some(zed_id), - }, - Channel { - id: livestreaming_id, - name: "livestreaming".to_string(), - parent_id: Some(zed_id), - }, - Channel { - id: livestreaming_id, - name: "livestreaming".to_string(), - parent_id: Some(crdb_id), - }, - Channel { - id: livestreaming_dag_id, - name: "livestreaming_dag".to_string(), - parent_id: Some(livestreaming_id), - }, - ] - ); + assert_dag(result.channels, &[ + (zed_id, None), + (crdb_id, Some(zed_id)), + (gpui2_id, Some(zed_id)), + (livestreaming_id, Some(zed_id)), + (livestreaming_id, Some(crdb_id)), + (livestreaming_dag_id, Some(livestreaming_id)), + ]); + // ======================================================================== // Create a new channel below a channel with multiple parents let livestreaming_dag_sub_id = db .create_channel( @@ -605,50 +561,20 @@ async fn test_channels_moving(db: &Arc) { // zed -- crdb - livestreaming - livestreaming_dag - livestreaming_dag_sub_id // \---------/ let result = db.get_channels_for_user(a_id).await.unwrap(); - pretty_assertions::assert_eq!( - result.channels, - vec![ - Channel { - id: zed_id, - name: "zed".to_string(), - parent_id: None, - }, - Channel { - id: crdb_id, - name: "crdb".to_string(), - parent_id: Some(zed_id), - }, - Channel { - id: gpui2_id, - name: "gpui2".to_string(), - parent_id: Some(zed_id), - }, - Channel { - id: livestreaming_id, - name: "livestreaming".to_string(), - parent_id: Some(zed_id), - }, - Channel { - id: livestreaming_id, - name: "livestreaming".to_string(), - parent_id: Some(crdb_id), - }, - Channel { - id: livestreaming_dag_id, - name: "livestreaming_dag".to_string(), - parent_id: Some(livestreaming_id), - }, - Channel { - id: livestreaming_dag_sub_id, - name: "livestreaming_dag_sub".to_string(), - parent_id: Some(livestreaming_dag_id), - }, - ] - ); + assert_dag(result.channels, &[ + (zed_id, None), + (crdb_id, Some(zed_id)), + (gpui2_id, Some(zed_id)), + (livestreaming_id, Some(zed_id)), + (livestreaming_id, Some(crdb_id)), + (livestreaming_dag_id, Some(livestreaming_id)), + (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), + ]); - // Make a link - let channels = db - .move_channel(a_id, livestreaming_dag_sub_id, None, Some(livestreaming_id)) + // ======================================================================== + // Test a complex DAG by making another link + let returned_channels = db + .link_channel(a_id, livestreaming_dag_sub_id, livestreaming_id) .await .unwrap(); @@ -658,66 +584,32 @@ async fn test_channels_moving(db: &Arc) { // \--------/ // make sure we're getting just the new link + // Not using the assert_dag helper because we want to make sure we're returning the full data pretty_assertions::assert_eq!( - channels, - vec![ - Channel { - id: livestreaming_dag_sub_id, - name: "livestreaming_dag_sub".to_string(), - parent_id: Some(livestreaming_id), - } - ] + returned_channels, + vec![Channel { + id: livestreaming_dag_sub_id, + name: "livestreaming_dag_sub".to_string(), + parent_id: Some(livestreaming_id), + }] ); let result = db.get_channels_for_user(a_id).await.unwrap(); - pretty_assertions::assert_eq!( - result.channels, - vec![ - Channel { - id: zed_id, - name: "zed".to_string(), - parent_id: None, - }, - Channel { - id: crdb_id, - name: "crdb".to_string(), - parent_id: Some(zed_id), - }, - Channel { - id: gpui2_id, - name: "gpui2".to_string(), - parent_id: Some(zed_id), - }, - Channel { - id: livestreaming_id, - name: "livestreaming".to_string(), - parent_id: Some(zed_id), - }, - Channel { - id: livestreaming_id, - name: "livestreaming".to_string(), - parent_id: Some(crdb_id), - }, - Channel { - id: livestreaming_dag_id, - name: "livestreaming_dag".to_string(), - parent_id: Some(livestreaming_id), - }, - Channel { - id: livestreaming_dag_sub_id, - name: "livestreaming_dag_sub".to_string(), - parent_id: Some(livestreaming_id), - }, - Channel { - id: livestreaming_dag_sub_id, - name: "livestreaming_dag_sub".to_string(), - parent_id: Some(livestreaming_dag_id), - }, - ] - ); + assert_dag(result.channels, &[ + (zed_id, None), + (crdb_id, Some(zed_id)), + (gpui2_id, Some(zed_id)), + (livestreaming_id, Some(zed_id)), + (livestreaming_id, Some(crdb_id)), + (livestreaming_dag_id, Some(livestreaming_id)), + (livestreaming_dag_sub_id, Some(livestreaming_id)), + (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), + ]); - // Make another link - let channels = db.move_channel(a_id, livestreaming_id, None, Some(gpui2_id)) + // ======================================================================== + // Test a complex DAG by making another link + let returned_channels = db + .link_channel(a_id, livestreaming_id, gpui2_id) .await .unwrap(); @@ -727,62 +619,14 @@ async fn test_channels_moving(db: &Arc) { // \---------/ // Make sure that we're correctly getting the full sub-dag - pretty_assertions::assert_eq!(channels, - vec![Channel { - id: livestreaming_id, - name: "livestreaming".to_string(), - parent_id: Some(gpui2_id), - }, - Channel { - id: livestreaming_dag_id, - name: "livestreaming_dag".to_string(), - parent_id: Some(livestreaming_id), - }, - Channel { - id: livestreaming_dag_sub_id, - name: "livestreaming_dag_sub".to_string(), - parent_id: Some(livestreaming_id), - }, - Channel { - id: livestreaming_dag_sub_id, - name: "livestreaming_dag_sub".to_string(), - parent_id: Some(livestreaming_dag_id), - }]); - - let result = db.get_channels_for_user(a_id).await.unwrap(); pretty_assertions::assert_eq!( - result.channels, + returned_channels, vec![ - Channel { - id: zed_id, - name: "zed".to_string(), - parent_id: None, - }, - Channel { - id: crdb_id, - name: "crdb".to_string(), - parent_id: Some(zed_id), - }, - Channel { - id: gpui2_id, - name: "gpui2".to_string(), - parent_id: Some(zed_id), - }, Channel { id: livestreaming_id, name: "livestreaming".to_string(), parent_id: Some(gpui2_id), }, - Channel { - id: livestreaming_id, - name: "livestreaming".to_string(), - parent_id: Some(zed_id), - }, - Channel { - id: livestreaming_id, - name: "livestreaming".to_string(), - parent_id: Some(crdb_id), - }, Channel { id: livestreaming_dag_id, name: "livestreaming_dag".to_string(), @@ -797,12 +641,31 @@ async fn test_channels_moving(db: &Arc) { id: livestreaming_dag_sub_id, name: "livestreaming_dag_sub".to_string(), parent_id: Some(livestreaming_dag_id), - }, + } ] ); - // Remove that inner link - let channels = db.move_channel(a_id, livestreaming_dag_sub_id, Some(livestreaming_id), None) + let result = db.get_channels_for_user(a_id).await.unwrap(); + assert_dag(result.channels, &[ + (zed_id, None), + (crdb_id, Some(zed_id)), + (gpui2_id, Some(zed_id)), + (livestreaming_id, Some(gpui2_id)), + (livestreaming_id, Some(zed_id)), + (livestreaming_id, Some(crdb_id)), + (livestreaming_dag_id, Some(livestreaming_id)), + (livestreaming_dag_sub_id, Some(livestreaming_id)), + (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), + ]); + + // ======================================================================== + // Test unlinking in a complex DAG by removing the inner link + db + .unlink_channel( + a_id, + livestreaming_dag_sub_id, + Some(livestreaming_id), + ) .await .unwrap(); @@ -811,62 +674,21 @@ async fn test_channels_moving(db: &Arc) { // zed - crdb -- livestreaming - livestreaming_dag - livestreaming_dag_sub // \---------/ - // Since we're not moving it to anywhere, there's nothing to notify anyone about - pretty_assertions::assert_eq!( - channels, - vec![] - ); - - let result = db.get_channels_for_user(a_id).await.unwrap(); - pretty_assertions::assert_eq!( - result.channels, - vec![ - Channel { - id: zed_id, - name: "zed".to_string(), - parent_id: None, - }, - Channel { - id: crdb_id, - name: "crdb".to_string(), - parent_id: Some(zed_id), - }, - Channel { - id: gpui2_id, - name: "gpui2".to_string(), - parent_id: Some(zed_id), - }, - Channel { - id: livestreaming_id, - name: "livestreaming".to_string(), - parent_id: Some(gpui2_id), - }, - Channel { - id: livestreaming_id, - name: "livestreaming".to_string(), - parent_id: Some(zed_id), - }, - Channel { - id: livestreaming_id, - name: "livestreaming".to_string(), - parent_id: Some(crdb_id), - }, - Channel { - id: livestreaming_dag_id, - name: "livestreaming_dag".to_string(), - parent_id: Some(livestreaming_id), - }, - Channel { - id: livestreaming_dag_sub_id, - name: "livestreaming_dag_sub".to_string(), - parent_id: Some(livestreaming_dag_id), - }, - ] - ); + assert_dag(result.channels, &[ + (zed_id, None), + (crdb_id, Some(zed_id)), + (gpui2_id, Some(zed_id)), + (livestreaming_id, Some(gpui2_id)), + (livestreaming_id, Some(zed_id)), + (livestreaming_id, Some(crdb_id)), + (livestreaming_dag_id, Some(livestreaming_id)), + (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), + ]); - // Remove that outer link - db.move_channel(a_id, livestreaming_id, Some(gpui2_id), None) + // ======================================================================== + // Test unlinking in a complex DAG by removing the inner link + db.unlink_channel(a_id, livestreaming_id, Some(gpui2_id)) .await .unwrap(); @@ -875,49 +697,19 @@ async fn test_channels_moving(db: &Arc) { // zed - crdb -- livestreaming - livestreaming_dag - livestreaming_dag_sub // \---------/ let result = db.get_channels_for_user(a_id).await.unwrap(); - pretty_assertions::assert_eq!( - result.channels, - vec![ - Channel { - id: zed_id, - name: "zed".to_string(), - parent_id: None, - }, - Channel { - id: crdb_id, - name: "crdb".to_string(), - parent_id: Some(zed_id), - }, - Channel { - id: gpui2_id, - name: "gpui2".to_string(), - parent_id: Some(zed_id), - }, - Channel { - id: livestreaming_id, - name: "livestreaming".to_string(), - parent_id: Some(zed_id), - }, - Channel { - id: livestreaming_id, - name: "livestreaming".to_string(), - parent_id: Some(crdb_id), - }, - Channel { - id: livestreaming_dag_id, - name: "livestreaming_dag".to_string(), - parent_id: Some(livestreaming_id), - }, - Channel { - id: livestreaming_dag_sub_id, - name: "livestreaming_dag_sub".to_string(), - parent_id: Some(livestreaming_dag_id), - }, - ] - ); + assert_dag(result.channels, &[ + (zed_id, None), + (crdb_id, Some(zed_id)), + (gpui2_id, Some(zed_id)), + (livestreaming_id, Some(zed_id)), + (livestreaming_id, Some(crdb_id)), + (livestreaming_dag_id, Some(livestreaming_id)), + (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), + ]); - // Move livestreaming to be below gpui2 - db.move_channel(a_id, livestreaming_id, Some(crdb_id), Some(gpui2_id)) + // ======================================================================== + // Test moving DAG nodes by moving livestreaming to be below gpui2 + db.move_channel(a_id, livestreaming_id, Some(crdb_id), gpui2_id) .await .unwrap(); @@ -926,47 +718,17 @@ async fn test_channels_moving(db: &Arc) { // zed - crdb / // \---------/ let result = db.get_channels_for_user(a_id).await.unwrap(); - pretty_assertions::assert_eq!( - result.channels, - vec![ - Channel { - id: zed_id, - name: "zed".to_string(), - parent_id: None, - }, - Channel { - id: crdb_id, - name: "crdb".to_string(), - parent_id: Some(zed_id), - }, - Channel { - id: gpui2_id, - name: "gpui2".to_string(), - parent_id: Some(zed_id), - }, - Channel { - id: livestreaming_id, - name: "livestreaming".to_string(), - parent_id: Some(zed_id), - }, - Channel { - id: livestreaming_id, - name: "livestreaming".to_string(), - parent_id: Some(gpui2_id), - }, - Channel { - id: livestreaming_dag_id, - name: "livestreaming_dag".to_string(), - parent_id: Some(livestreaming_id), - }, - Channel { - id: livestreaming_dag_sub_id, - name: "livestreaming_dag_sub".to_string(), - parent_id: Some(livestreaming_dag_id), - }, - ] - ); + assert_dag(result.channels, &[ + (zed_id, None), + (crdb_id, Some(zed_id)), + (gpui2_id, Some(zed_id)), + (livestreaming_id, Some(zed_id)), + (livestreaming_id, Some(gpui2_id)), + (livestreaming_dag_id, Some(livestreaming_id)), + (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), + ]); + // ======================================================================== // Deleting a channel should not delete children that still have other parents db.delete_channel(gpui2_id, a_id).await.unwrap(); @@ -974,46 +736,109 @@ async fn test_channels_moving(db: &Arc) { // zed - crdb // \- livestreaming - livestreaming_dag - livestreaming_dag_sub let result = db.get_channels_for_user(a_id).await.unwrap(); - pretty_assertions::assert_eq!( - result.channels, - vec![ - Channel { - id: zed_id, - name: "zed".to_string(), - parent_id: None, - }, - Channel { - id: crdb_id, - name: "crdb".to_string(), - parent_id: Some(zed_id), - }, - Channel { - id: livestreaming_id, - name: "livestreaming".to_string(), - parent_id: Some(zed_id), - }, - Channel { - id: livestreaming_dag_id, - name: "livestreaming_dag".to_string(), - parent_id: Some(livestreaming_id), - }, - Channel { - id: livestreaming_dag_sub_id, - name: "livestreaming_dag_sub".to_string(), - parent_id: Some(livestreaming_dag_id), - }, - ] - ); + assert_dag(result.channels, &[ + (zed_id, None), + (crdb_id, Some(zed_id)), + (livestreaming_id, Some(zed_id)), + (livestreaming_dag_id, Some(livestreaming_id)), + (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), + ]); - // But deleting a parent of a DAG should delete the whole DAG: - db.move_channel(a_id, livestreaming_id, None, Some(crdb_id)) + // ======================================================================== + // Unlinking a channel from it's parent should automatically promote it to a root channel + db.unlink_channel(a_id, crdb_id, Some(zed_id)) .await .unwrap(); + + // DAG is now: + // crdb + // zed + // \- livestreaming - livestreaming_dag - livestreaming_dag_sub + + let result = db.get_channels_for_user(a_id).await.unwrap(); + assert_dag(result.channels, &[ + (zed_id, None), + (crdb_id, None), + (livestreaming_id, Some(zed_id)), + (livestreaming_dag_id, Some(livestreaming_id)), + (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), + ]); + + // ======================================================================== + // Unlinking a root channel should not have any effect + db.unlink_channel(a_id, crdb_id, None) + .await + .unwrap(); + + // DAG is now: + // crdb + // zed + // \- livestreaming - livestreaming_dag - livestreaming_dag_sub + // + let result = db.get_channels_for_user(a_id).await.unwrap(); + assert_dag(result.channels, &[ + (zed_id, None), + (crdb_id, None), + (livestreaming_id, Some(zed_id)), + (livestreaming_dag_id, Some(livestreaming_id)), + (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), + ]); + + // ======================================================================== + // You should be able to move a root channel into a non-root channel + db.move_channel(a_id, crdb_id, None, zed_id) + .await + .unwrap(); + + // DAG is now: + // zed - crdb + // \- livestreaming - livestreaming_dag - livestreaming_dag_sub + + let result = db.get_channels_for_user(a_id).await.unwrap(); + assert_dag(result.channels, &[ + (zed_id, None), + (crdb_id, Some(zed_id)), + (livestreaming_id, Some(zed_id)), + (livestreaming_dag_id, Some(livestreaming_id)), + (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), + ]); + + + // ======================================================================== + // Moving a non-root channel without a parent id should be the equivalent of a link operation + db.move_channel(a_id, livestreaming_id, None, crdb_id) + .await + .unwrap(); + // DAG is now: // zed - crdb - livestreaming - livestreaming_dag - livestreaming_dag_sub // \--------/ + let result = db.get_channels_for_user(a_id).await.unwrap(); + assert_dag(result.channels, &[ + (zed_id, None), + (crdb_id, Some(zed_id)), + (livestreaming_id, Some(zed_id)), + (livestreaming_id, Some(crdb_id)), + (livestreaming_dag_id, Some(livestreaming_id)), + (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), + ]); + + // ======================================================================== + // Deleting a parent of a DAG should delete the whole DAG: db.delete_channel(zed_id, a_id).await.unwrap(); let result = db.get_channels_for_user(a_id).await.unwrap(); - assert!(result.channels.is_empty()) + assert!( + result.channels.is_empty() + ) +} + +#[track_caller] +fn assert_dag(actual: Vec, expected: &[(ChannelId, Option)]) { + let actual = actual + .iter() + .map(|channel| (channel.id, channel.parent_id)) + .collect::>(); + + pretty_assertions::assert_eq!(actual, expected) } diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index d15bc704ef..325d2e390b 100644 --- a/crates/collab/src/rpc.rs +++ b/crates/collab/src/rpc.rs @@ -3,7 +3,7 @@ mod connection_pool; use crate::{ auth, db::{ - self, ChannelId, ChannelsForUser, Database, MessageId, ProjectId, RoomId, ServerId, User, + self, Channel, ChannelId, ChannelsForUser, Database, MessageId, ProjectId, RoomId, ServerId, User, UserId, }, executor::Executor, @@ -267,6 +267,8 @@ impl Server { .add_request_handler(send_channel_message) .add_request_handler(remove_channel_message) .add_request_handler(get_channel_messages) + .add_request_handler(link_channel) + .add_request_handler(unlink_channel) .add_request_handler(move_channel) .add_request_handler(follow) .add_message_handler(unfollow) @@ -2391,6 +2393,72 @@ async fn rename_channel( Ok(()) } +async fn link_channel( + request: proto::LinkChannel, + response: Response, + session: Session, +) -> Result<()> { + let db = session.db().await; + let channel_id = ChannelId::from_proto(request.channel_id); + let to = ChannelId::from_proto(request.to); + let channels_to_send = db.link_channel(session.user_id, channel_id, to).await?; + + let members = db.get_channel_members(to).await?; + let connection_pool = session.connection_pool().await; + let update = proto::UpdateChannels { + channels: channels_to_send + .into_iter() + .map(|channel| proto::Channel { + id: channel.id.to_proto(), + name: channel.name, + parent_id: channel.parent_id.map(ChannelId::to_proto), + }) + .collect(), + ..Default::default() + }; + for member_id in members { + for connection_id in connection_pool.user_connection_ids(member_id) { + session.peer.send(connection_id, update.clone())?; + } + } + + response.send(Ack {})?; + + Ok(()) +} + +async fn unlink_channel( + request: proto::UnlinkChannel, + response: Response, + session: Session, +) -> Result<()> { + let db = session.db().await; + let channel_id = ChannelId::from_proto(request.channel_id); + let from = request.from.map(ChannelId::from_proto); + db.unlink_channel(session.user_id, channel_id, from).await?; + + if let Some(from_parent) = from { + let members = db.get_channel_members(from_parent).await?; + let update = proto::UpdateChannels { + delete_channel_edge: vec![proto::ChannelEdge { + channel_id: channel_id.to_proto(), + parent_id: from_parent.to_proto(), + }], + ..Default::default() + }; + let connection_pool = session.connection_pool().await; + for member_id in members { + for connection_id in connection_pool.user_connection_ids(member_id) { + session.peer.send(connection_id, update.clone())?; + } + } + } + + response.send(Ack {})?; + + Ok(()) +} + async fn move_channel( request: proto::MoveChannel, response: Response, @@ -2398,18 +2466,12 @@ async fn move_channel( ) -> Result<()> { let db = session.db().await; let channel_id = ChannelId::from_proto(request.channel_id); - let from_parent = request.from_parent.map(ChannelId::from_proto); - let to = request.to.map(ChannelId::from_proto); - let channels_to_send = db - .move_channel( - session.user_id, - channel_id, - from_parent, - to, - ) + let from_parent = request.from.map(ChannelId::from_proto); + let to = ChannelId::from_proto(request.to); + let channels_to_send: Vec = db + .move_channel(session.user_id, channel_id, from_parent, to) .await?; - if let Some(from_parent) = from_parent { let members = db.get_channel_members(from_parent).await?; let update = proto::UpdateChannels { @@ -2425,24 +2487,24 @@ async fn move_channel( session.peer.send(connection_id, update.clone())?; } } - } - if let Some(to) = to { - let members = db.get_channel_members(to).await?; - let connection_pool = session.connection_pool().await; - let update = proto::UpdateChannels { - channels: channels_to_send.into_iter().map(|channel| proto::Channel { + let members = db.get_channel_members(to).await?; + let connection_pool = session.connection_pool().await; + let update = proto::UpdateChannels { + channels: channels_to_send + .into_iter() + .map(|channel| proto::Channel { id: channel.id.to_proto(), name: channel.name, parent_id: channel.parent_id.map(ChannelId::to_proto), - }).collect(), - ..Default::default() - }; - for member_id in members { - for connection_id in connection_pool.user_connection_ids(member_id) { - session.peer.send(connection_id, update.clone())?; - } + }) + .collect(), + ..Default::default() + }; + for member_id in members { + for connection_id in connection_pool.user_connection_ids(member_id) { + session.peer.send(connection_id, update.clone())?; } } diff --git a/crates/collab/src/tests/channel_tests.rs b/crates/collab/src/tests/channel_tests.rs index 77045d9174..a0d480e57f 100644 --- a/crates/collab/src/tests/channel_tests.rs +++ b/crates/collab/src/tests/channel_tests.rs @@ -933,7 +933,7 @@ async fn test_channel_moving(deterministic: Arc, cx_a: &mut TestA client_a .channel_store() .update(cx_a, |channel_store, cx| { - channel_store.move_channel(channel_c_id, Some(channel_b_id), Some(channel_a_id), cx) + channel_store.move_channel(channel_c_id, Some(channel_b_id), channel_a_id, cx) }) .await .unwrap(); @@ -970,7 +970,7 @@ async fn test_channel_moving(deterministic: Arc, cx_a: &mut TestA client_a .channel_store() .update(cx_a, |channel_store, cx| { - channel_store.move_channel(channel_c_id, None, Some(channel_b_id), cx) + channel_store.link_channel(channel_c_id, channel_b_id, cx) }) .await .unwrap(); diff --git a/crates/collab_ui/src/collab_panel.rs b/crates/collab_ui/src/collab_panel.rs index f0b10dffa4..8914b7d901 100644 --- a/crates/collab_ui/src/collab_panel.rs +++ b/crates/collab_ui/src/collab_panel.rs @@ -114,7 +114,7 @@ struct PutChannel { #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] struct UnlinkChannel { channel_id: ChannelId, - parent_id: ChannelId, + parent_id: Option, } actions!( @@ -199,13 +199,13 @@ pub fn init(cx: &mut AppContext) { cx.add_action( |panel: &mut CollabPanel, action: &LinkChannel, _: &mut ViewContext| { - panel.copy = Some(ChannelCopy::Link(action.channel_id)); + panel.link_or_move = Some(ChannelCopy::Link(action.channel_id)); }, ); cx.add_action( |panel: &mut CollabPanel, action: &MoveChannel, _: &mut ViewContext| { - panel.copy = Some(ChannelCopy::Move { + panel.link_or_move = Some(ChannelCopy::Move { channel_id: action.channel_id, parent_id: action.parent_id, }); @@ -214,20 +214,20 @@ pub fn init(cx: &mut AppContext) { cx.add_action( |panel: &mut CollabPanel, action: &PutChannel, cx: &mut ViewContext| { - if let Some(copy) = panel.copy.take() { + if let Some(copy) = panel.link_or_move.take() { match copy { ChannelCopy::Move { channel_id, parent_id, } => panel.channel_store.update(cx, |channel_store, cx| { channel_store - .move_channel(channel_id, parent_id, Some(action.to), cx) + .move_channel(channel_id, parent_id, action.to, cx) .detach_and_log_err(cx) }), ChannelCopy::Link(channel) => { panel.channel_store.update(cx, |channel_store, cx| { channel_store - .move_channel(channel, None, Some(action.to), cx) + .link_channel(channel, action.to, cx) .detach_and_log_err(cx) }) } @@ -240,7 +240,7 @@ pub fn init(cx: &mut AppContext) { |panel: &mut CollabPanel, action: &UnlinkChannel, cx: &mut ViewContext| { panel.channel_store.update(cx, |channel_store, cx| { channel_store - .move_channel(action.channel_id, Some(action.parent_id), None, cx) + .unlink_channel(action.channel_id, action.parent_id, cx) .detach_and_log_err(cx) }) }, @@ -284,13 +284,20 @@ impl ChannelCopy { ChannelCopy::Link(channel_id) => *channel_id, } } + + fn is_move(&self) -> bool { + match self { + ChannelCopy::Move { .. } => true, + ChannelCopy::Link(_) => false, + } + } } pub struct CollabPanel { width: Option, fs: Arc, has_focus: bool, - copy: Option, + link_or_move: Option, pending_serialization: Task>, context_menu: ViewHandle, filter_editor: ViewHandle, @@ -553,7 +560,7 @@ impl CollabPanel { let mut this = Self { width: None, has_focus: false, - copy: None, + link_or_move: None, fs: workspace.app_state().fs.clone(), pending_serialization: Task::ready(None), context_menu: cx.add_view(|cx| ContextMenu::new(view_id, cx)), @@ -2062,15 +2069,14 @@ impl CollabPanel { ) { self.context_menu_on_selected = position.is_none(); - let copy_channel = self - .copy - .as_ref() - .and_then(|copy| { - self.channel_store - .read(cx) - .channel_for_id(copy.channel_id()) - }) - .map(|channel| channel.name.clone()); + let operation_details = self.link_or_move.as_ref().and_then(|link_or_move| { + let channel_name = self + .channel_store + .read(cx) + .channel_for_id(link_or_move.channel_id()) + .map(|channel| channel.name.clone())?; + Some((channel_name, link_or_move.is_move())) + }); self.context_menu.update(cx, |context_menu, cx| { context_menu.set_position_mode(if self.context_menu_on_selected { @@ -2079,13 +2085,29 @@ impl CollabPanel { OverlayPositionMode::Window }); + let mut items = Vec::new(); + + if let Some((channel_name, is_move)) = operation_details { + items.push(ContextMenuItem::action( + format!( + "{} '#{}' here", + if is_move { "Move" } else { "Link" }, + channel_name + ), + PutChannel { + to: location.channel, + }, + )); + items.push(ContextMenuItem::Separator) + } + let expand_action_name = if self.is_channel_collapsed(&location) { "Expand Subchannels" } else { "Collapse Subchannels" }; - let mut items = vec![ + items.extend([ ContextMenuItem::action( expand_action_name, ToggleCollapse { @@ -2098,7 +2120,7 @@ impl CollabPanel { channel_id: location.channel, }, ), - ]; + ]); if self.channel_store.read(cx).is_user_admin(location.channel) { let parent_id = location.path.parent_id(); @@ -2120,25 +2142,27 @@ impl CollabPanel { ContextMenuItem::Separator, ]); - if let Some(parent) = parent_id { - items.push(ContextMenuItem::action( - "Unlink from parent", - UnlinkChannel { - channel_id: location.channel, - parent_id: parent, - }, - )) - } + items.push(ContextMenuItem::action( + if parent_id.is_some() { + "Unlink from parent" + } else { + "Unlink from root" + }, + UnlinkChannel { + channel_id: location.channel, + parent_id, + }, + )); items.extend([ ContextMenuItem::action( - "Link to new parent", + "Link this channel", LinkChannel { channel_id: location.channel, }, ), ContextMenuItem::action( - "Move", + "Move this channel", MoveChannel { channel_id: location.channel, parent_id, @@ -2146,15 +2170,6 @@ impl CollabPanel { ), ]); - if let Some(copy_channel) = copy_channel { - items.push(ContextMenuItem::action( - format!("Put '#{}'", copy_channel), - PutChannel { - to: location.channel, - }, - )); - } - items.extend([ ContextMenuItem::Separator, ContextMenuItem::action( diff --git a/crates/rpc/proto/zed.proto b/crates/rpc/proto/zed.proto index f252efaa14..c12a559355 100644 --- a/crates/rpc/proto/zed.proto +++ b/crates/rpc/proto/zed.proto @@ -167,7 +167,9 @@ message Envelope { GetChannelMessagesResponse get_channel_messages_response = 149; RemoveChannelMessage remove_channel_message = 150; - MoveChannel move_channel = 151; // Current max + LinkChannel link_channel = 151; + UnlinkChannel unlink_channel = 152; + MoveChannel move_channel = 153; // Current max } } @@ -1082,10 +1084,20 @@ message GetChannelMessagesResponse { bool done = 2; } +message LinkChannel { + uint64 channel_id = 1; + uint64 to = 2; +} + +message UnlinkChannel { + uint64 channel_id = 1; + optional uint64 from = 2; +} + message MoveChannel { uint64 channel_id = 1; - optional uint64 from_parent = 2; - optional uint64 to = 3; + optional uint64 from = 2; + uint64 to = 3; } message JoinChannelBuffer { diff --git a/crates/rpc/src/proto.rs b/crates/rpc/src/proto.rs index ccff1526f0..0a2c4a9d7d 100644 --- a/crates/rpc/src/proto.rs +++ b/crates/rpc/src/proto.rs @@ -248,6 +248,8 @@ messages!( (UpdateContacts, Foreground), (DeleteChannel, Foreground), (MoveChannel, Foreground), + (LinkChannel, Foreground), + (UnlinkChannel, Foreground), (UpdateChannels, Foreground), (UpdateDiagnosticSummary, Foreground), (UpdateFollowers, Foreground), @@ -332,6 +334,8 @@ request_messages!( (DeleteChannel, Ack), (RenameProjectEntry, ProjectEntryResponse), (RenameChannel, ChannelResponse), + (LinkChannel, Ack), + (UnlinkChannel, Ack), (MoveChannel, Ack), (SaveBuffer, BufferSaved), (SearchProject, SearchProjectResponse),