diff --git a/crates/channel/src/channel_store.rs b/crates/channel/src/channel_store.rs index f95ae6bd9b..08ff11f85d 100644 --- a/crates/channel/src/channel_store.rs +++ b/crates/channel/src/channel_store.rs @@ -346,7 +346,7 @@ impl ChannelStore { pub fn unlink_channel( &mut self, channel_id: ChannelId, - from: Option, + from: ChannelId, cx: &mut ModelContext, ) -> Task> { let client = self.client.clone(); @@ -362,7 +362,7 @@ impl ChannelStore { pub fn move_channel( &mut self, channel_id: ChannelId, - from: Option, + from: ChannelId, to: ChannelId, cx: &mut ModelContext, ) -> Task> { diff --git a/crates/channel/src/channel_store/channel_index.rs b/crates/channel/src/channel_store/channel_index.rs index 90cde46e90..08c5060196 100644 --- a/crates/channel/src/channel_store/channel_index.rs +++ b/crates/channel/src/channel_store/channel_index.rs @@ -62,16 +62,13 @@ impl ChannelIndex { /// Remove the given edge from this index. This will not remove the channel. /// If this operation would result in a dangling edge, re-insert it. - pub fn delete_edge(&mut self, parent_id: Option, channel_id: ChannelId) { - if let Some(parent_id) = parent_id { - self.paths.retain(|path| { - !path - .windows(2) - .any(|window| window == [parent_id, channel_id]) - }); - } else { - self.paths.retain(|path| path.first() != Some(&channel_id)); - } + pub fn delete_edge(&mut self, parent_id: ChannelId, channel_id: ChannelId) { + self.paths.retain(|path| { + !path + .windows(2) + .any(|window| window == [parent_id, channel_id]) + }); + // Ensure that there is at least one channel path in the index if !self diff --git a/crates/collab/src/db/queries/channels.rs b/crates/collab/src/db/queries/channels.rs index 449f48992f..f8ad453632 100644 --- a/crates/collab/src/db/queries/channels.rs +++ b/crates/collab/src/db/queries/channels.rs @@ -48,7 +48,6 @@ impl Database { .insert(&*tx) .await?; - let channel_paths_stmt; if let Some(parent) = parent { let sql = r#" INSERT INTO channel_paths @@ -60,7 +59,7 @@ impl Database { WHERE channel_id = $3 "#; - channel_paths_stmt = Statement::from_sql_and_values( + let channel_paths_stmt = Statement::from_sql_and_values( self.pool.get_database_backend(), sql, [ @@ -796,6 +795,8 @@ impl Database { return Err(anyhow!("Cannot create a channel cycle").into()); } } + + // Now insert all of the new paths let sql = r#" INSERT INTO channel_paths (id_path, channel_id) @@ -832,6 +833,21 @@ impl Database { } } + // If we're linking a channel, remove any root edges for the channel + { + 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?; + } + if let Some(channel) = from_descendants.get_mut(&channel) { // Remove the other parents channel.clear(); @@ -849,7 +865,7 @@ impl Database { &self, user: UserId, channel: ChannelId, - from: Option, + from: ChannelId, ) -> Result<()> { self.transaction(|tx| async move { // Note that even with these maxed permissions, this linking operation @@ -927,10 +943,6 @@ impl Database { self.unlink_channel_internal(user, channel, from, &*tx) .await?; - dbg!(channel_path::Entity::find().all(&*tx).await); - - dbg!(&moved_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 edf4bbef5a..50faa2a910 100644 --- a/crates/collab/src/db/tests/channel_tests.rs +++ b/crates/collab/src/db/tests/channel_tests.rs @@ -664,7 +664,7 @@ async fn test_channels_moving(db: &Arc) { .unlink_channel( a_id, livestreaming_dag_sub_id, - Some(livestreaming_id), + livestreaming_id, ) .await .unwrap(); @@ -688,7 +688,7 @@ async fn test_channels_moving(db: &Arc) { // ======================================================================== // Test unlinking in a complex DAG by removing the inner link - db.unlink_channel(a_id, livestreaming_id, Some(gpui2_id)) + db.unlink_channel(a_id, livestreaming_id, gpui2_id) .await .unwrap(); @@ -709,7 +709,7 @@ async fn test_channels_moving(db: &Arc) { // ======================================================================== // Test moving DAG nodes by moving livestreaming to be below gpui2 - db.move_channel(a_id, livestreaming_id, Some(crdb_id), gpui2_id) + db.move_channel(a_id, livestreaming_id, crdb_id, gpui2_id) .await .unwrap(); @@ -746,7 +746,7 @@ async fn test_channels_moving(db: &Arc) { // ======================================================================== // 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)) + db.unlink_channel(a_id, crdb_id, zed_id) .await .unwrap(); @@ -764,29 +764,9 @@ async fn test_channels_moving(db: &Arc) { (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) + db.link_channel(a_id, crdb_id, zed_id) .await .unwrap(); @@ -805,8 +785,8 @@ async fn test_channels_moving(db: &Arc) { // ======================================================================== - // 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) + // Prep for DAG deletion test + db.link_channel(a_id, livestreaming_id, crdb_id) .await .unwrap(); @@ -824,10 +804,10 @@ async fn test_channels_moving(db: &Arc) { (livestreaming_dag_sub_id, Some(livestreaming_dag_id)), ]); - // ======================================================================== - // Deleting a parent of a DAG should delete the whole DAG: + // Deleting the 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() ) diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index 8a6488459b..6a5ba3e5d4 100644 --- a/crates/collab/src/rpc.rs +++ b/crates/collab/src/rpc.rs @@ -2434,17 +2434,16 @@ async fn unlink_channel( ) -> Result<()> { let db = session.db().await; let channel_id = ChannelId::from_proto(request.channel_id); - let from = request.from.map(ChannelId::from_proto); - - // Get the members before we remove it, so we know who to notify - let members = db.get_channel_members(channel_id).await?; + let from = ChannelId::from_proto(request.from); db.unlink_channel(session.user_id, channel_id, from).await?; + let members = db.get_channel_members(from).await?; + let update = proto::UpdateChannels { delete_channel_edge: vec![proto::ChannelEdge { channel_id: channel_id.to_proto(), - parent_id: from.map(ChannelId::to_proto), + parent_id: from.to_proto(), }], ..Default::default() }; @@ -2467,38 +2466,31 @@ async fn move_channel( ) -> Result<()> { let db = session.db().await; let channel_id = ChannelId::from_proto(request.channel_id); - let from_parent = request.from.map(ChannelId::from_proto); + let from_parent = ChannelId::from_proto(request.from); let to = ChannelId::from_proto(request.to); - let mut members = db.get_channel_members(channel_id).await?; + let members_from = db.get_channel_members(channel_id).await?; let channels_to_send: Vec = db .move_channel(session.user_id, channel_id, from_parent, to) .await?; - let members_after = db.get_channel_members(channel_id).await?; + let members_to = db.get_channel_members(channel_id).await?; - members.extend(members_after); - members.sort(); - members.dedup(); - - if let Some(from_parent) = from_parent { - let update = proto::UpdateChannels { - delete_channel_edge: vec![proto::ChannelEdge { - channel_id: channel_id.to_proto(), - parent_id: Some(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())?; - } + 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_from { + for connection_id in connection_pool.user_connection_ids(member_id) { + session.peer.send(connection_id, update.clone())?; } } - let connection_pool = session.connection_pool().await; let update = proto::UpdateChannels { channels: channels_to_send .into_iter() @@ -2510,7 +2502,7 @@ async fn move_channel( .collect(), ..Default::default() }; - for member_id in members { + for member_id in members_to { 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 008dc0abc5..09583fda29 100644 --- a/crates/collab/src/tests/channel_tests.rs +++ b/crates/collab/src/tests/channel_tests.rs @@ -905,10 +905,15 @@ async fn test_channel_moving( (&client_a, cx_a), ) .await; - let channel_a_a_id = channels[0]; - let channel_a_b_id = channels[1]; - let channel_a_c_id = channels[2]; - let channel_a_d_id = channels[3]; + let channel_a_id = channels[0]; + let channel_b_id = channels[1]; + let channel_c_id = channels[2]; + let channel_d_id = channels[3]; + + dbg!(channel_a_id); + dbg!(channel_b_id); + dbg!(channel_c_id); + dbg!(channel_d_id); // Current shape: // a - b - c - d @@ -916,17 +921,17 @@ async fn test_channel_moving( client_a.channel_store(), cx_a, &[ - (channel_a_a_id, 0), - (channel_a_b_id, 1), - (channel_a_c_id, 2), - (channel_a_d_id, 3), + (channel_a_id, 0), + (channel_b_id, 1), + (channel_c_id, 2), + (channel_d_id, 3), ], ); client_a .channel_store() .update(cx_a, |channel_store, cx| { - channel_store.move_channel(channel_a_d_id, Some(channel_a_c_id), channel_a_b_id, cx) + channel_store.move_channel(channel_d_id, channel_c_id, channel_b_id, cx) }) .await .unwrap(); @@ -938,17 +943,17 @@ async fn test_channel_moving( client_a.channel_store(), cx_a, &[ - (channel_a_a_id, 0), - (channel_a_b_id, 1), - (channel_a_c_id, 2), - (channel_a_d_id, 2), + (channel_a_id, 0), + (channel_b_id, 1), + (channel_c_id, 2), + (channel_d_id, 2), ], ); client_a .channel_store() .update(cx_a, |channel_store, cx| { - channel_store.link_channel(channel_a_d_id, channel_a_c_id, cx) + channel_store.link_channel(channel_d_id, channel_c_id, cx) }) .await .unwrap(); @@ -960,11 +965,11 @@ async fn test_channel_moving( client_a.channel_store(), cx_a, &[ - (channel_a_a_id, 0), - (channel_a_b_id, 1), - (channel_a_c_id, 2), - (channel_a_d_id, 3), - (channel_a_d_id, 2), + (channel_a_id, 0), + (channel_b_id, 1), + (channel_c_id, 2), + (channel_d_id, 3), + (channel_d_id, 2), ], ); @@ -978,9 +983,9 @@ async fn test_channel_moving( (&client_b, cx_b), ) .await; - let channel_b_mu_id = b_channels[0]; - let channel_b_gamma_id = b_channels[1]; - let channel_b_epsilon_id = b_channels[2]; + let channel_mu_id = b_channels[0]; + let channel_ga_id = b_channels[1]; + let channel_ep_id = b_channels[2]; // Current shape for B: // /- ep @@ -989,13 +994,13 @@ async fn test_channel_moving( client_b.channel_store(), cx_b, &[ - (channel_b_mu_id, 0), - (channel_b_gamma_id, 1), - (channel_b_epsilon_id, 1) + (channel_mu_id, 0), + (channel_ga_id, 1), + (channel_ep_id, 1) ], ); - client_a.add_admin_to_channel((&client_b, cx_b), channel_a_b_id, cx_a).await; + client_a.add_admin_to_channel((&client_b, cx_b), channel_b_id, cx_a).await; // Current shape for B: // /- ep // mu -- ga @@ -1006,51 +1011,51 @@ async fn test_channel_moving( cx_b, &[ // B's old channels - (channel_b_mu_id, 0), - (channel_b_gamma_id, 1), - (channel_b_epsilon_id, 1), + (channel_mu_id, 0), + (channel_ga_id, 1), + (channel_ep_id, 1), // New channels from a - (channel_a_b_id, 0), - (channel_a_c_id, 1), - (channel_a_d_id, 1), - (channel_a_d_id, 2), + (channel_b_id, 0), + (channel_c_id, 1), + (channel_d_id, 1), + (channel_d_id, 2), ], ); + // client_b + // .channel_store() + // .update(cx_a, |channel_store, cx| { + // channel_store.move_channel(channel_a_b_id, None, channel_b_epsilon_id, cx) + // }) + // .await + // .unwrap(); + + // // Current shape for B: + // // /---------\ + // // /- ep -- b -- c -- d + // // mu -- ga + // assert_channels_list_shape( + // client_b.channel_store(), + // cx_b, + // &[ + // // B's old channels + // (channel_b_mu_id, 0), + // (channel_b_gamma_id, 1), + // (channel_b_epsilon_id, 1), + + // // New channels from a, now under epsilon + // (channel_a_b_id, 2), + // (channel_a_c_id, 3), + // (channel_a_d_id, 3), + // (channel_a_d_id, 4), + // ], + // ); + client_b .channel_store() .update(cx_a, |channel_store, cx| { - channel_store.move_channel(channel_a_b_id, None, channel_b_epsilon_id, cx) - }) - .await - .unwrap(); - - // Current shape for B: - // /---------\ - // /- ep -- b -- c -- d - // mu -- ga - assert_channels_list_shape( - client_b.channel_store(), - cx_b, - &[ - // B's old channels - (channel_b_mu_id, 0), - (channel_b_gamma_id, 1), - (channel_b_epsilon_id, 1), - - // New channels from a, now under epsilon - (channel_a_b_id, 2), - (channel_a_c_id, 3), - (channel_a_d_id, 3), - (channel_a_d_id, 4), - ], - ); - - client_b - .channel_store() - .update(cx_a, |channel_store, cx| { - channel_store.link_channel(channel_b_gamma_id, channel_a_b_id, cx) + channel_store.link_channel(channel_ga_id, channel_b_id, cx) }) .await .unwrap(); @@ -1065,16 +1070,16 @@ async fn test_channel_moving( cx_b, &[ // B's old channels - (channel_b_mu_id, 0), - (channel_b_gamma_id, 1), - (channel_b_epsilon_id, 1), + (channel_mu_id, 0), + (channel_ga_id, 1), + (channel_ep_id, 1), // New channels from a, now under epsilon, with gamma - (channel_a_b_id, 2), - (channel_b_gamma_id, 3), - (channel_a_c_id, 3), - (channel_a_d_id, 3), - (channel_a_d_id, 4), + (channel_b_id, 2), + (channel_ga_id, 3), + (channel_c_id, 3), + (channel_d_id, 3), + (channel_d_id, 4), ], ); @@ -1083,12 +1088,12 @@ async fn test_channel_moving( client_a.channel_store(), cx_a, &[ - (channel_a_a_id, 0), - (channel_a_b_id, 1), - (channel_b_gamma_id, 1), - (channel_a_c_id, 2), - (channel_a_d_id, 3), - (channel_a_d_id, 2), + (channel_a_id, 0), + (channel_b_id, 1), + (channel_ga_id, 1), + (channel_c_id, 2), + (channel_d_id, 3), + (channel_d_id, 2), ], ); // TODO: Make sure to test that non-local root removing problem I was thinking about diff --git a/crates/collab_ui/src/collab_panel.rs b/crates/collab_ui/src/collab_panel.rs index 8914b7d901..4e889d35d4 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: Option, + parent_id: ChannelId, } actions!( @@ -218,19 +218,21 @@ pub fn init(cx: &mut AppContext) { match copy { ChannelCopy::Move { channel_id, - parent_id, + parent_id: Some(parent_id), } => panel.channel_store.update(cx, |channel_store, cx| { channel_store .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 - .link_channel(channel, action.to, cx) - .detach_and_log_err(cx) - }) - } + ChannelCopy::Link(channel) + | ChannelCopy::Move { + channel_id: channel, + parent_id: None, + } => panel.channel_store.update(cx, |channel_store, cx| { + channel_store + .link_channel(channel, action.to, cx) + .detach_and_log_err(cx) + }), } } }, @@ -2142,17 +2144,15 @@ impl CollabPanel { ContextMenuItem::Separator, ]); - items.push(ContextMenuItem::action( - if parent_id.is_some() { - "Unlink from parent" - } else { - "Unlink from root" - }, - UnlinkChannel { - channel_id: location.channel, - parent_id, - }, - )); + if let Some(parent_id) = parent_id { + items.push(ContextMenuItem::action( + "Unlink from parent", + UnlinkChannel { + channel_id: location.channel, + parent_id, + }, + )); + } items.extend([ ContextMenuItem::action( diff --git a/crates/rpc/proto/zed.proto b/crates/rpc/proto/zed.proto index c12a559355..54414f38f4 100644 --- a/crates/rpc/proto/zed.proto +++ b/crates/rpc/proto/zed.proto @@ -1091,12 +1091,12 @@ message LinkChannel { message UnlinkChannel { uint64 channel_id = 1; - optional uint64 from = 2; + uint64 from = 2; } message MoveChannel { uint64 channel_id = 1; - optional uint64 from = 2; + uint64 from = 2; uint64 to = 3; }