diff --git a/crates/channel/src/channel_store.rs b/crates/channel/src/channel_store.rs index ee1929f720c9a7a6d0023fc32681e862dfbe0419..a5a0a922468710cd49e0178515052b0157c876b9 100644 --- a/crates/channel/src/channel_store.rs +++ b/crates/channel/src/channel_store.rs @@ -3,10 +3,7 @@ mod channel_index; use crate::{channel_buffer::ChannelBuffer, channel_chat::ChannelChat}; use anyhow::{anyhow, Result}; use client::{Client, Subscription, User, UserId, UserStore}; -use collections::{ - hash_map::{self, DefaultHasher}, - HashMap, HashSet, -}; +use collections::{hash_map, HashMap, HashSet}; use futures::{channel::mpsc, future::Shared, Future, FutureExt, StreamExt}; use gpui::{AppContext, AsyncAppContext, Entity, ModelContext, ModelHandle, Task, WeakModelHandle}; use rpc::{ @@ -14,14 +11,7 @@ use rpc::{ TypedEnvelope, }; use serde_derive::{Deserialize, Serialize}; -use std::{ - borrow::Cow, - hash::{Hash, Hasher}, - mem, - ops::Deref, - sync::Arc, - time::Duration, -}; +use std::{borrow::Cow, hash::Hash, mem, ops::Deref, sync::Arc, time::Duration}; use util::ResultExt; use self::channel_index::ChannelIndex; @@ -910,12 +900,6 @@ impl ChannelPath { pub fn channel_id(&self) -> ChannelId { self.0[self.0.len() - 1] } - - pub fn unique_id(&self) -> u64 { - let mut hasher = DefaultHasher::new(); - self.0.deref().hash(&mut hasher); - hasher.finish() - } } impl From for Cow<'static, ChannelPath> { diff --git a/crates/collab/src/db/queries/channels.rs b/crates/collab/src/db/queries/channels.rs index 2d65139a9889210bd7356833e4aa1d27e6e08f77..16a0891d160ffad81abfda6d46d0e4300112961e 100644 --- a/crates/collab/src/db/queries/channels.rs +++ b/crates/collab/src/db/queries/channels.rs @@ -828,68 +828,53 @@ impl Database { ) -> Result { self.check_user_is_channel_admin(to, user, &*tx).await?; - let to_ancestors = self.get_channel_ancestors(to, &*tx).await?; - let mut channel_descendants = self.get_channel_descendants([channel], &*tx).await?; - for ancestor in to_ancestors { - if channel_descendants.contains_key(&ancestor) { - return Err(anyhow!("Cannot create a channel cycle").into()); + let paths = channel_path::Entity::find() + .filter(channel_path::Column::IdPath.like(&format!("%/{}/%", channel))) + .all(tx) + .await?; + + let mut new_path_suffixes = HashSet::default(); + for path in paths { + if let Some(start_offset) = path.id_path.find(&format!("/{}/", channel)) { + new_path_suffixes.insert(( + path.channel_id, + path.id_path[(start_offset + 1)..].to_string(), + )); } } - // Now insert all of the new paths - let sql = r#" - INSERT INTO channel_paths - (id_path, channel_id) - SELECT - id_path || $1 || '/', $2 - FROM - channel_paths - WHERE - channel_id = $3 - ON CONFLICT (id_path) DO NOTHING; - "#; - let channel_paths_stmt = Statement::from_sql_and_values( - self.pool.get_database_backend(), - sql, - [ - channel.to_proto().into(), - channel.to_proto().into(), - to.to_proto().into(), - ], - ); - tx.execute(channel_paths_stmt).await?; - for (descdenant_id, descendant_parent_ids) in - channel_descendants.iter().filter(|(id, _)| id != &&channel) - { - for descendant_parent_id in descendant_parent_ids.iter() { - let channel_paths_stmt = Statement::from_sql_and_values( - self.pool.get_database_backend(), - sql, - [ - descdenant_id.to_proto().into(), - descdenant_id.to_proto().into(), - descendant_parent_id.to_proto().into(), - ], - ); - tx.execute(channel_paths_stmt).await?; + let paths_to_new_parent = channel_path::Entity::find() + .filter(channel_path::Column::ChannelId.eq(to)) + .all(tx) + .await?; + + let mut new_paths = Vec::new(); + for path in paths_to_new_parent { + if path.id_path.contains(&format!("/{}/", channel)) { + Err(anyhow!("cycle"))?; } + + new_paths.extend(new_path_suffixes.iter().map(|(channel_id, path_suffix)| { + channel_path::ActiveModel { + channel_id: ActiveValue::Set(*channel_id), + id_path: ActiveValue::Set(format!("{}{}", &path.id_path, path_suffix)), + } + })); } - // If we're linking a channel, remove any root edges for the channel + channel_path::Entity::insert_many(new_paths) + .exec(&*tx) + .await?; + + // remove any root edges for the channel we just linked { - 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?; + channel_path::Entity::delete_many() + .filter(channel_path::Column::IdPath.like(&format!("/{}/%", channel))) + .exec(&*tx) + .await?; } + let mut channel_descendants = self.get_channel_descendants([channel], &*tx).await?; if let Some(channel) = channel_descendants.get_mut(&channel) { // Remove the other parents channel.clear(); @@ -936,35 +921,43 @@ impl Database { 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?; + DELETE FROM channel_paths + WHERE + id_path LIKE '%/' || $1 || '/' || $2 || '/%' + RETURNING id_path, channel_id + "#; + + let paths = channel_path::Entity::find() + .from_raw_sql(Statement::from_sql_and_values( + self.pool.get_database_backend(), + sql, + [from.to_proto().into(), channel.to_proto().into()], + )) + .all(&*tx) + .await?; + + let is_stranded = channel_path::Entity::find() + .filter(channel_path::Column::ChannelId.eq(channel)) + .count(&*tx) + .await? + == 0; // 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, - [channel.to_proto().into(), channel.to_proto().into()], - ); - tx.execute(channel_paths_stmt).await?; + if is_stranded { + let root_paths: Vec<_> = paths + .iter() + .map(|path| { + let start_offset = path.id_path.find(&format!("/{}/", channel)).unwrap(); + channel_path::ActiveModel { + channel_id: ActiveValue::Set(path.channel_id), + id_path: ActiveValue::Set(path.id_path[start_offset..].to_string()), + } + }) + .collect(); + channel_path::Entity::insert_many(root_paths) + .exec(&*tx) + .await?; + } Ok(()) } @@ -978,6 +971,13 @@ impl Database { from: ChannelId, to: ChannelId, ) -> Result { + if from == to { + return Ok(ChannelGraph { + channels: vec![], + edges: vec![], + }); + } + self.transaction(|tx| async move { self.check_user_is_channel_admin(channel, user, &*tx) .await?; diff --git a/crates/collab/src/db/tests/channel_tests.rs b/crates/collab/src/db/tests/channel_tests.rs index 8a03014cf9ef844fb743319720dd5846784577d6..429852d12870a232da68d165d75de68d3a7b9be0 100644 --- a/crates/collab/src/db/tests/channel_tests.rs +++ b/crates/collab/src/db/tests/channel_tests.rs @@ -791,6 +791,64 @@ async fn test_db_channel_moving(db: &Arc) { assert!(result.channels.is_empty()) } +test_both_dbs!( + test_db_channel_moving_bugs, + test_db_channel_moving_bugs_postgres, + test_db_channel_moving_bugs_sqlite +); + +async fn test_db_channel_moving_bugs(db: &Arc) { + let user_id = db + .create_user( + "user1@example.com", + false, + NewUserParams { + github_login: "user1".into(), + github_user_id: 5, + invite_count: 0, + }, + ) + .await + .unwrap() + .user_id; + + let zed_id = db.create_root_channel("zed", "1", user_id).await.unwrap(); + + let projects_id = db + .create_channel("projects", Some(zed_id), "2", user_id) + .await + .unwrap(); + + let livestreaming_id = db + .create_channel("livestreaming", Some(projects_id), "3", user_id) + .await + .unwrap(); + + // Dag is: zed - projects - livestreaming + + // Move to same parent should be a no-op + assert!(db + .move_channel(user_id, projects_id, zed_id, zed_id) + .await + .unwrap() + .is_empty()); + + // Stranding a channel should retain it's sub channels + db.unlink_channel(user_id, projects_id, zed_id) + .await + .unwrap(); + + let result = db.get_channels_for_user(user_id).await.unwrap(); + assert_dag( + result.channels, + &[ + (zed_id, None), + (projects_id, None), + (livestreaming_id, Some(projects_id)), + ], + ); +} + #[track_caller] fn assert_dag(actual: ChannelGraph, expected: &[(ChannelId, Option)]) { let mut actual_map: HashMap> = HashMap::default(); diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index 87b77202356accb6594ab0193c12cc0ce648cd71..fa9cc5ef1e97c05871c4ad07c9372ae9d47f4f3f 100644 --- a/crates/collab/src/rpc.rs +++ b/crates/collab/src/rpc.rs @@ -2474,6 +2474,11 @@ async fn move_channel( .move_channel(session.user_id, channel_id, from_parent, to) .await?; + if channels_to_send.is_empty() { + response.send(Ack {})?; + return Ok(()); + } + let members_from = db.get_channel_members(from_parent).await?; let members_to = db.get_channel_members(to).await?; diff --git a/crates/collab/src/tests/channel_tests.rs b/crates/collab/src/tests/channel_tests.rs index 6b300282a1740c2d349cf1bdd2a0b82a304a16d5..6bdcee6af3eebc5a885f7a60b780b9f13e7ca0a3 100644 --- a/crates/collab/src/tests/channel_tests.rs +++ b/crates/collab/src/tests/channel_tests.rs @@ -145,8 +145,6 @@ async fn test_core_channels( ], ); - println!("STARTING CREATE CHANNEL C"); - let channel_c_id = client_a .channel_store() .update(cx_a, |channel_store, cx| { @@ -1028,10 +1026,6 @@ async fn test_channel_moving( // - ep assert_channels_list_shape(client_c.channel_store(), cx_c, &[(channel_ep_id, 0)]); - println!("*******************************************"); - println!("********** STARTING LINK CHANNEL **********"); - println!("*******************************************"); - dbg!(client_b.user_id()); client_b .channel_store() .update(cx_b, |channel_store, cx| { @@ -1199,5 +1193,5 @@ fn assert_channels_list_shape( .map(|(depth, channel)| (channel.id, depth)) .collect::>() }); - pretty_assertions::assert_eq!(dbg!(actual), expected_channels); + pretty_assertions::assert_eq!(actual, expected_channels); } diff --git a/crates/collab_ui/src/collab_panel.rs b/crates/collab_ui/src/collab_panel.rs index 66f5de1cdea14d348ed2e221412a6c6af4d8503b..6013ea4907dd7d703a7c107b6c726b7cef9ae201 100644 --- a/crates/collab_ui/src/collab_panel.rs +++ b/crates/collab_ui/src/collab_panel.rs @@ -1787,7 +1787,7 @@ impl CollabPanel { is_dragged_over = true; } - MouseEventHandler::new::(path.unique_id() as usize, cx, |state, cx| { + MouseEventHandler::new::(ix, cx, |state, cx| { let row_hovered = state.hovered(); let mut select_state = |interactive: &Interactive| { @@ -1822,47 +1822,43 @@ impl CollabPanel { .flex(1., true), ) .with_child( - MouseEventHandler::new::( - channel.id as usize, - cx, - move |_, cx| { - let participants = - self.channel_store.read(cx).channel_participants(channel_id); - if !participants.is_empty() { - let extra_count = participants.len().saturating_sub(FACEPILE_LIMIT); - - FacePile::new(theme.face_overlap) - .with_children( - participants - .iter() - .filter_map(|user| { - Some( - Image::from_data(user.avatar.clone()?) - .with_style(theme.channel_avatar), - ) - }) - .take(FACEPILE_LIMIT), + MouseEventHandler::new::(ix, cx, move |_, cx| { + let participants = + self.channel_store.read(cx).channel_participants(channel_id); + if !participants.is_empty() { + let extra_count = participants.len().saturating_sub(FACEPILE_LIMIT); + + FacePile::new(theme.face_overlap) + .with_children( + participants + .iter() + .filter_map(|user| { + Some( + Image::from_data(user.avatar.clone()?) + .with_style(theme.channel_avatar), + ) + }) + .take(FACEPILE_LIMIT), + ) + .with_children((extra_count > 0).then(|| { + Label::new( + format!("+{}", extra_count), + theme.extra_participant_label.text.clone(), ) - .with_children((extra_count > 0).then(|| { - Label::new( - format!("+{}", extra_count), - theme.extra_participant_label.text.clone(), - ) - .contained() - .with_style(theme.extra_participant_label.container) - })) - .into_any() - } else if row_hovered { - Svg::new("icons/speaker-loud.svg") - .with_color(theme.channel_hash.color) - .constrained() - .with_width(theme.channel_hash.width) - .into_any() - } else { - Empty::new().into_any() - } - }, - ) + .contained() + .with_style(theme.extra_participant_label.container) + })) + .into_any() + } else if row_hovered { + Svg::new("icons/speaker-loud.svg") + .with_color(theme.channel_hash.color) + .constrained() + .with_width(theme.channel_hash.width) + .into_any() + } else { + Empty::new().into_any() + } + }) .on_click(MouseButton::Left, move |_, this, cx| { this.join_channel_call(channel_id, cx); }), @@ -1875,7 +1871,7 @@ impl CollabPanel { location: path.clone(), }), ) - .with_id(path.unique_id() as usize) + .with_id(ix) .with_style(theme.disclosure.clone()) .element() .constrained() @@ -1955,11 +1951,11 @@ impl CollabPanel { }) .as_draggable( (channel.clone(), path.parent_id()), - move |e, (channel, _), cx: &mut ViewContext| { + move |modifiers, (channel, _), cx: &mut ViewContext| { let theme = &theme::current(cx).collab_panel; Flex::::row() - .with_children(e.alt.then(|| { + .with_children(modifiers.alt.then(|| { Svg::new("icons/plus.svg") .with_color(theme.channel_hash.color) .constrained() diff --git a/crates/drag_and_drop/src/drag_and_drop.rs b/crates/drag_and_drop/src/drag_and_drop.rs index dc778759facb66bcaef2a1ec96811077b38d25a9..a945b3bb5e7d5ee57fd9accdad873372b9dc7ba0 100644 --- a/crates/drag_and_drop/src/drag_and_drop.rs +++ b/crates/drag_and_drop/src/drag_and_drop.rs @@ -364,9 +364,15 @@ impl Draggable for MouseEventHandler { DragAndDrop::::drag_started(e, cx); }) .on_drag(MouseButton::Left, move |e, _, cx| { - let payload = payload.clone(); - let render = render.clone(); - DragAndDrop::::dragging(e, payload, cx, render) + if e.end { + cx.update_global::, _, _>(|drag_and_drop, cx| { + drag_and_drop.finish_dragging(cx) + }) + } else { + let payload = payload.clone(); + let render = render.clone(); + DragAndDrop::::dragging(e, payload, cx, render) + } }) } }