diff --git a/crates/channel/src/channel.rs b/crates/channel/src/channel.rs index b6db304a70..d0a32e16ff 100644 --- a/crates/channel/src/channel.rs +++ b/crates/channel/src/channel.rs @@ -11,9 +11,7 @@ pub use channel_chat::{ mentions_to_proto, ChannelChat, ChannelChatEvent, ChannelMessage, ChannelMessageId, MessageParams, }; -pub use channel_store::{ - Channel, ChannelData, ChannelEvent, ChannelId, ChannelMembership, ChannelPath, ChannelStore, -}; +pub use channel_store::{Channel, ChannelEvent, ChannelId, ChannelMembership, ChannelStore}; #[cfg(test)] mod channel_store_tests; diff --git a/crates/channel/src/channel_store.rs b/crates/channel/src/channel_store.rs index 2665c2e1ec..efa05d51a9 100644 --- a/crates/channel/src/channel_store.rs +++ b/crates/channel/src/channel_store.rs @@ -9,11 +9,10 @@ use db::RELEASE_CHANNEL; use futures::{channel::mpsc, future::Shared, Future, FutureExt, StreamExt}; use gpui::{AppContext, AsyncAppContext, Entity, ModelContext, ModelHandle, Task, WeakModelHandle}; use rpc::{ - proto::{self, ChannelEdge, ChannelVisibility}, + proto::{self, ChannelVisibility}, TypedEnvelope, }; -use serde_derive::{Deserialize, Serialize}; -use std::{borrow::Cow, hash::Hash, mem, ops::Deref, sync::Arc, time::Duration}; +use std::{mem, sync::Arc, time::Duration}; use util::ResultExt; pub fn init(client: &Arc, user_store: ModelHandle, cx: &mut AppContext) { @@ -27,7 +26,7 @@ pub const RECONNECT_TIMEOUT: Duration = Duration::from_secs(30); pub type ChannelId = u64; pub struct ChannelStore { - channel_index: ChannelIndex, + pub channel_index: ChannelIndex, channel_invitations: Vec>, channel_participants: HashMap>>, outgoing_invites: HashSet<(ChannelId, UserId)>, @@ -42,8 +41,6 @@ pub struct ChannelStore { _update_channels: Task<()>, } -pub type ChannelData = (Channel, ChannelPath); - #[derive(Clone, Debug, PartialEq)] pub struct Channel { pub id: ChannelId, @@ -52,6 +49,7 @@ pub struct Channel { pub role: proto::ChannelRole, pub unseen_note_version: Option<(u64, clock::Global)>, pub unseen_message_id: Option, + pub parent_path: Vec, } impl Channel { @@ -78,9 +76,6 @@ impl Channel { } } -#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Serialize, Deserialize)] -pub struct ChannelPath(Arc<[ChannelId]>); - pub struct ChannelMembership { pub user: Arc, pub kind: proto::channel_member::Kind, @@ -193,16 +188,6 @@ impl ChannelStore { self.client.clone() } - pub fn has_children(&self, channel_id: ChannelId) -> bool { - self.channel_index.iter().any(|path| { - if let Some(ix) = path.iter().position(|id| *id == channel_id) { - path.len() > ix + 1 - } else { - false - } - }) - } - /// Returns the number of unique channels in the store pub fn channel_count(&self) -> usize { self.channel_index.by_id().len() @@ -222,20 +207,19 @@ impl ChannelStore { } /// Iterate over all entries in the channel DAG - pub fn channel_dag_entries(&self) -> impl '_ + Iterator)> { - self.channel_index.iter().map(move |path| { - let id = path.last().unwrap(); - let channel = self.channel_for_id(*id).unwrap(); - (path.len() - 1, channel) - }) + pub fn ordered_channels(&self) -> impl '_ + Iterator)> { + self.channel_index + .ordered_channels() + .iter() + .filter_map(move |id| { + let channel = self.channel_index.by_id().get(id)?; + Some((channel.parent_path.len(), channel)) + }) } - pub fn channel_dag_entry_at(&self, ix: usize) -> Option<(&Arc, &ChannelPath)> { - let path = self.channel_index.get(ix)?; - let id = path.last().unwrap(); - let channel = self.channel_for_id(*id).unwrap(); - - Some((channel, path)) + pub fn channel_at_index(&self, ix: usize) -> Option<&Arc> { + let channel_id = self.channel_index.ordered_channels().get(ix)?; + self.channel_index.by_id().get(channel_id) } pub fn channel_at(&self, ix: usize) -> Option<&Arc> { @@ -484,20 +468,19 @@ impl ChannelStore { .ok_or_else(|| anyhow!("missing channel in response"))?; let channel_id = channel.id; - let parent_edge = if let Some(parent_id) = parent_id { - vec![ChannelEdge { - channel_id: channel.id, - parent_id, - }] - } else { - vec![] - }; + // let parent_edge = if let Some(parent_id) = parent_id { + // vec![ChannelEdge { + // channel_id: channel.id, + // parent_id, + // }] + // } else { + // vec![] + // }; this.update(&mut cx, |this, cx| { let task = this.update_channels( proto::UpdateChannels { channels: vec![channel], - insert_edge: parent_edge, ..Default::default() }, cx, @@ -515,53 +498,16 @@ 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: ChannelId, - 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: ChannelId, - to: ChannelId, + to: Option, cx: &mut ModelContext, ) -> Task> { let client = self.client.clone(); cx.spawn(|_, _| async move { let _ = client - .request(proto::MoveChannel { - channel_id, - from, - to, - }) + .request(proto::MoveChannel { channel_id, to }) .await?; Ok(()) @@ -956,6 +902,7 @@ impl ChannelStore { name: channel.name, unseen_note_version: None, unseen_message_id: None, + parent_path: channel.parent_path, }), ), } @@ -963,8 +910,6 @@ impl ChannelStore { let channels_changed = !payload.channels.is_empty() || !payload.delete_channels.is_empty() - || !payload.insert_edge.is_empty() - || !payload.delete_edge.is_empty() || !payload.unseen_channel_messages.is_empty() || !payload.unseen_channel_buffer_changes.is_empty(); @@ -1022,14 +967,6 @@ impl ChannelStore { unseen_channel_message.message_id, ); } - - for edge in payload.insert_edge { - index.insert_edge(edge.channel_id, edge.parent_id); - } - - for edge in payload.delete_edge { - index.delete_edge(edge.parent_id, edge.channel_id); - } } cx.notify(); @@ -1078,44 +1015,3 @@ impl ChannelStore { })) } } - -impl Deref for ChannelPath { - type Target = [ChannelId]; - - fn deref(&self) -> &Self::Target { - &self.0 - } -} - -impl ChannelPath { - pub fn new(path: Arc<[ChannelId]>) -> Self { - debug_assert!(path.len() >= 1); - Self(path) - } - - pub fn parent_id(&self) -> Option { - self.0.len().checked_sub(2).map(|i| self.0[i]) - } - - pub fn channel_id(&self) -> ChannelId { - self.0[self.0.len() - 1] - } -} - -impl From for Cow<'static, ChannelPath> { - fn from(value: ChannelPath) -> Self { - Cow::Owned(value) - } -} - -impl<'a> From<&'a ChannelPath> for Cow<'a, ChannelPath> { - fn from(value: &'a ChannelPath) -> Self { - Cow::Borrowed(value) - } -} - -impl Default for ChannelPath { - fn default() -> Self { - ChannelPath(Arc::from([])) - } -} diff --git a/crates/channel/src/channel_store/channel_index.rs b/crates/channel/src/channel_store/channel_index.rs index b221ce1b02..97b2ab6318 100644 --- a/crates/channel/src/channel_store/channel_index.rs +++ b/crates/channel/src/channel_store/channel_index.rs @@ -1,14 +1,11 @@ -use std::{ops::Deref, sync::Arc}; - use crate::{Channel, ChannelId}; use collections::BTreeMap; use rpc::proto; - -use super::ChannelPath; +use std::sync::Arc; #[derive(Default, Debug)] pub struct ChannelIndex { - paths: Vec, + channels_ordered: Vec, channels_by_id: BTreeMap>, } @@ -17,8 +14,12 @@ impl ChannelIndex { &self.channels_by_id } + pub fn ordered_channels(&self) -> &[ChannelId] { + &self.channels_ordered + } + pub fn clear(&mut self) { - self.paths.clear(); + self.channels_ordered.clear(); self.channels_by_id.clear(); } @@ -26,13 +27,13 @@ impl ChannelIndex { pub fn delete_channels(&mut self, channels: &[ChannelId]) { self.channels_by_id .retain(|channel_id, _| !channels.contains(channel_id)); - self.paths - .retain(|path| !path.iter().any(|channel_id| channels.contains(channel_id))); + self.channels_ordered + .retain(|channel_id| !channels.contains(channel_id)); } pub fn bulk_insert(&mut self) -> ChannelPathsInsertGuard { ChannelPathsInsertGuard { - paths: &mut self.paths, + channels_ordered: &mut self.channels_ordered, channels_by_id: &mut self.channels_by_id, } } @@ -75,42 +76,15 @@ impl ChannelIndex { } } -impl Deref for ChannelIndex { - type Target = [ChannelPath]; - - fn deref(&self) -> &Self::Target { - &self.paths - } -} - /// A guard for ensuring that the paths index maintains its sort and uniqueness /// invariants after a series of insertions #[derive(Debug)] pub struct ChannelPathsInsertGuard<'a> { - paths: &'a mut Vec, + channels_ordered: &'a mut Vec, channels_by_id: &'a mut BTreeMap>, } impl<'a> ChannelPathsInsertGuard<'a> { - /// 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: 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 - .paths - .iter() - .any(|path| path.iter().any(|id| id == &channel_id)) - { - self.insert_root(channel_id); - } - } - pub fn note_changed(&mut self, channel_id: ChannelId, epoch: u64, version: &clock::Global) { insert_note_changed(&mut self.channels_by_id, channel_id, epoch, &version); } @@ -141,6 +115,7 @@ impl<'a> ChannelPathsInsertGuard<'a> { name: channel_proto.name, unseen_note_version: None, unseen_message_id: None, + parent_path: channel_proto.parent_path, }), ); self.insert_root(channel_proto.id); @@ -148,74 +123,35 @@ impl<'a> ChannelPathsInsertGuard<'a> { ret } - pub fn insert_edge(&mut self, channel_id: ChannelId, parent_id: ChannelId) { - let mut parents = Vec::new(); - let mut descendants = Vec::new(); - let mut ixs_to_remove = Vec::new(); - - for (ix, path) in self.paths.iter().enumerate() { - if path - .windows(2) - .any(|window| window[0] == parent_id && window[1] == channel_id) - { - // We already have this edge in the index - return; - } - if path.ends_with(&[parent_id]) { - parents.push(path); - } else if let Some(position) = path.iter().position(|id| id == &channel_id) { - if position == 0 { - ixs_to_remove.push(ix); - } - descendants.push(path.split_at(position).1); - } - } - - let mut new_paths = Vec::new(); - for parent in parents.iter() { - if descendants.is_empty() { - let mut new_path = Vec::with_capacity(parent.len() + 1); - new_path.extend_from_slice(parent); - new_path.push(channel_id); - new_paths.push(ChannelPath::new(new_path.into())); - } else { - for descendant in descendants.iter() { - let mut new_path = Vec::with_capacity(parent.len() + descendant.len()); - new_path.extend_from_slice(parent); - new_path.extend_from_slice(descendant); - new_paths.push(ChannelPath::new(new_path.into())); - } - } - } - - for ix in ixs_to_remove.into_iter().rev() { - self.paths.swap_remove(ix); - } - self.paths.extend(new_paths) - } - fn insert_root(&mut self, channel_id: ChannelId) { - self.paths.push(ChannelPath::new(Arc::from([channel_id]))); + self.channels_ordered.push(channel_id); } } impl<'a> Drop for ChannelPathsInsertGuard<'a> { fn drop(&mut self) { - self.paths.sort_by(|a, b| { - let a = channel_path_sorting_key(a, &self.channels_by_id); - let b = channel_path_sorting_key(b, &self.channels_by_id); + self.channels_ordered.sort_by(|a, b| { + let a = channel_path_sorting_key(*a, &self.channels_by_id); + let b = channel_path_sorting_key(*b, &self.channels_by_id); a.cmp(b) }); - self.paths.dedup(); + self.channels_ordered.dedup(); } } fn channel_path_sorting_key<'a>( - path: &'a [ChannelId], + id: ChannelId, channels_by_id: &'a BTreeMap>, -) -> impl 'a + Iterator> { - path.iter() - .map(|id| Some(channels_by_id.get(id)?.name.as_str())) +) -> impl Iterator { + let (parent_path, name) = channels_by_id + .get(&id) + .map_or((&[] as &[_], None), |channel| { + (channel.parent_path.as_slice(), Some(channel.name.as_str())) + }); + parent_path + .iter() + .filter_map(|id| Some(channels_by_id.get(id)?.name.as_str())) + .chain(name) } fn insert_note_changed( diff --git a/crates/channel/src/channel_store_tests.rs b/crates/channel/src/channel_store_tests.rs index dd4f24e2ca..ff8761ee91 100644 --- a/crates/channel/src/channel_store_tests.rs +++ b/crates/channel/src/channel_store_tests.rs @@ -20,12 +20,14 @@ fn test_update_channels(cx: &mut AppContext) { name: "b".to_string(), visibility: proto::ChannelVisibility::Members as i32, role: proto::ChannelRole::Admin.into(), + parent_path: Vec::new(), }, proto::Channel { id: 2, name: "a".to_string(), visibility: proto::ChannelVisibility::Members as i32, role: proto::ChannelRole::Member.into(), + parent_path: Vec::new(), }, ], ..Default::default() @@ -51,22 +53,14 @@ fn test_update_channels(cx: &mut AppContext) { name: "x".to_string(), visibility: proto::ChannelVisibility::Members as i32, role: proto::ChannelRole::Admin.into(), + parent_path: vec![1], }, proto::Channel { id: 4, name: "y".to_string(), visibility: proto::ChannelVisibility::Members as i32, role: proto::ChannelRole::Member.into(), - }, - ], - insert_edge: vec![ - proto::ChannelEdge { - parent_id: 1, - channel_id: 3, - }, - proto::ChannelEdge { - parent_id: 2, - channel_id: 4, + parent_path: vec![2], }, ], ..Default::default() @@ -98,28 +92,21 @@ fn test_dangling_channel_paths(cx: &mut AppContext) { name: "a".to_string(), visibility: proto::ChannelVisibility::Members as i32, role: proto::ChannelRole::Admin.into(), + parent_path: vec![], }, proto::Channel { id: 1, name: "b".to_string(), visibility: proto::ChannelVisibility::Members as i32, role: proto::ChannelRole::Admin.into(), + parent_path: vec![0], }, proto::Channel { id: 2, name: "c".to_string(), visibility: proto::ChannelVisibility::Members as i32, role: proto::ChannelRole::Admin.into(), - }, - ], - insert_edge: vec![ - proto::ChannelEdge { - parent_id: 0, - channel_id: 1, - }, - proto::ChannelEdge { - parent_id: 1, - channel_id: 2, + parent_path: vec![0, 1], }, ], ..Default::default() @@ -170,6 +157,7 @@ async fn test_channel_messages(cx: &mut TestAppContext) { name: "the-channel".to_string(), visibility: proto::ChannelVisibility::Members as i32, role: proto::ChannelRole::Member.into(), + parent_path: vec![], }], ..Default::default() }); @@ -197,7 +185,7 @@ async fn test_channel_messages(cx: &mut TestAppContext) { // Join a channel and populate its existing messages. let channel = channel_store.update(cx, |store, cx| { - let channel_id = store.channel_dag_entries().next().unwrap().1.id; + let channel_id = store.ordered_channels().next().unwrap().1.id; store.open_channel_chat(channel_id, cx) }); let join_channel = server.receive::().await.unwrap(); @@ -384,7 +372,7 @@ fn assert_channels( ) { let actual = channel_store.read_with(cx, |store, _| { store - .channel_dag_entries() + .ordered_channels() .map(|(depth, channel)| (depth, channel.name.to_string(), channel.role)) .collect::>() }); diff --git a/crates/collab/migrations.sqlite/20221109000000_test_schema.sql b/crates/collab/migrations.sqlite/20221109000000_test_schema.sql index 7fa808b498..775a4c1bbe 100644 --- a/crates/collab/migrations.sqlite/20221109000000_test_schema.sql +++ b/crates/collab/migrations.sqlite/20221109000000_test_schema.sql @@ -193,9 +193,12 @@ CREATE TABLE "channels" ( "id" INTEGER PRIMARY KEY AUTOINCREMENT, "name" VARCHAR NOT NULL, "created_at" TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, - "visibility" VARCHAR NOT NULL + "visibility" VARCHAR NOT NULL, + "parent_path" TEXT ); +CREATE INDEX "index_channels_on_parent_path" ON "channels" ("parent_path"); + CREATE TABLE IF NOT EXISTS "channel_chat_participants" ( "id" INTEGER PRIMARY KEY AUTOINCREMENT, "user_id" INTEGER NOT NULL REFERENCES users (id), @@ -224,12 +227,6 @@ CREATE TABLE "channel_message_mentions" ( PRIMARY KEY(message_id, start_offset) ); -CREATE TABLE "channel_paths" ( - "id_path" TEXT NOT NULL PRIMARY KEY, - "channel_id" INTEGER NOT NULL REFERENCES channels (id) ON DELETE CASCADE -); -CREATE INDEX "index_channel_paths_on_channel_id" ON "channel_paths" ("channel_id"); - CREATE TABLE "channel_members" ( "id" INTEGER PRIMARY KEY AUTOINCREMENT, "channel_id" INTEGER NOT NULL REFERENCES channels (id) ON DELETE CASCADE, diff --git a/crates/collab/migrations/20231024085546_move_channel_paths_to_channels_table.sql b/crates/collab/migrations/20231024085546_move_channel_paths_to_channels_table.sql new file mode 100644 index 0000000000..d9fc6c8722 --- /dev/null +++ b/crates/collab/migrations/20231024085546_move_channel_paths_to_channels_table.sql @@ -0,0 +1,12 @@ +ALTER TABLE channels ADD COLUMN parent_path TEXT; + +UPDATE channels +SET parent_path = substr( + channel_paths.id_path, + 2, + length(channel_paths.id_path) - length('/' || channel_paths.channel_id::text || '/') +) +FROM channel_paths +WHERE channel_paths.channel_id = channels.id; + +CREATE INDEX "index_channels_on_parent_path" ON "channels" ("parent_path"); diff --git a/crates/collab/src/db.rs b/crates/collab/src/db.rs index f4e2602cc6..df33416a46 100644 --- a/crates/collab/src/db.rs +++ b/crates/collab/src/db.rs @@ -13,7 +13,6 @@ use anyhow::anyhow; use collections::{BTreeMap, HashMap, HashSet}; use dashmap::DashMap; use futures::StreamExt; -use queries::channels::ChannelGraph; use rand::{prelude::StdRng, Rng, SeedableRng}; use rpc::{ proto::{self}, @@ -492,21 +491,33 @@ pub struct RemoveChannelMemberResult { pub notification_id: Option, } -#[derive(FromQueryResult, Debug, PartialEq, Eq, Hash)] +#[derive(Debug, PartialEq, Eq, Hash)] pub struct Channel { pub id: ChannelId, pub name: String, pub visibility: ChannelVisibility, pub role: ChannelRole, + pub parent_path: Vec, } impl Channel { + fn from_model(value: channel::Model, role: ChannelRole) -> Self { + Channel { + id: value.id, + visibility: value.visibility, + name: value.clone().name, + role, + parent_path: value.ancestors().collect(), + } + } + pub fn to_proto(&self) -> proto::Channel { proto::Channel { id: self.id.to_proto(), name: self.name.clone(), visibility: self.visibility.into(), role: self.role.into(), + parent_path: self.parent_path.iter().map(|c| c.to_proto()).collect(), } } } @@ -530,7 +541,7 @@ impl ChannelMember { #[derive(Debug, PartialEq)] pub struct ChannelsForUser { - pub channels: ChannelGraph, + pub channels: Vec, pub channel_participants: HashMap>, pub unseen_buffer_changes: Vec, pub channel_messages: Vec, diff --git a/crates/collab/src/db/queries/buffers.rs b/crates/collab/src/db/queries/buffers.rs index 3aa9cff171..9eddb1f618 100644 --- a/crates/collab/src/db/queries/buffers.rs +++ b/crates/collab/src/db/queries/buffers.rs @@ -16,7 +16,8 @@ impl Database { connection: ConnectionId, ) -> Result { self.transaction(|tx| async move { - self.check_user_is_channel_participant(channel_id, user_id, &tx) + let channel = self.get_channel_internal(channel_id, &*tx).await?; + self.check_user_is_channel_participant(&channel, user_id, &tx) .await?; let buffer = channel::Model { @@ -129,9 +130,11 @@ impl Database { self.transaction(|tx| async move { let mut results = Vec::new(); for client_buffer in buffers { - let channel_id = ChannelId::from_proto(client_buffer.channel_id); + let channel = self + .get_channel_internal(ChannelId::from_proto(client_buffer.channel_id), &*tx) + .await?; if self - .check_user_is_channel_participant(channel_id, user_id, &*tx) + .check_user_is_channel_participant(&channel, user_id, &*tx) .await .is_err() { @@ -139,9 +142,9 @@ impl Database { continue; } - let buffer = self.get_channel_buffer(channel_id, &*tx).await?; + let buffer = self.get_channel_buffer(channel.id, &*tx).await?; let mut collaborators = channel_buffer_collaborator::Entity::find() - .filter(channel_buffer_collaborator::Column::ChannelId.eq(channel_id)) + .filter(channel_buffer_collaborator::Column::ChannelId.eq(channel.id)) .all(&*tx) .await?; @@ -439,7 +442,8 @@ impl Database { Vec, )> { self.transaction(move |tx| async move { - self.check_user_is_channel_member(channel_id, user, &*tx) + let channel = self.get_channel_internal(channel_id, &*tx).await?; + self.check_user_is_channel_member(&channel, user, &*tx) .await?; let buffer = buffer::Entity::find() @@ -482,7 +486,7 @@ impl Database { ) .await?; - channel_members = self.get_channel_participants(channel_id, &*tx).await?; + channel_members = self.get_channel_participants(&channel, &*tx).await?; let collaborators = self .get_channel_buffer_collaborators_internal(channel_id, &*tx) .await?; diff --git a/crates/collab/src/db/queries/channels.rs b/crates/collab/src/db/queries/channels.rs index 9e7b1cabf5..68b06e435d 100644 --- a/crates/collab/src/db/queries/channels.rs +++ b/crates/collab/src/db/queries/channels.rs @@ -1,5 +1,6 @@ use super::*; -use rpc::proto::{channel_member::Kind, ChannelEdge}; +use rpc::proto::channel_member::Kind; +use sea_orm::TryGetableMany; impl Database { #[cfg(test)] @@ -42,55 +43,41 @@ impl Database { pub async fn create_channel( &self, name: &str, - parent: Option, + parent_channel_id: Option, admin_id: UserId, ) -> Result { let name = Self::sanitize_channel_name(name)?; self.transaction(move |tx| async move { - if let Some(parent) = parent { - self.check_user_is_channel_admin(parent, admin_id, &*tx) + let mut parent = None; + + if let Some(parent_channel_id) = parent_channel_id { + let parent_channel = self.get_channel_internal(parent_channel_id, &*tx).await?; + self.check_user_is_channel_admin(&parent_channel, admin_id, &*tx) .await?; + parent = Some(parent_channel); } let channel = channel::ActiveModel { id: ActiveValue::NotSet, name: ActiveValue::Set(name.to_string()), visibility: ActiveValue::Set(ChannelVisibility::Members), + parent_path: ActiveValue::Set( + parent + .as_ref() + .map_or(String::new(), |parent| parent.path()), + ), } .insert(&*tx) .await?; - if let Some(parent) = parent { - let sql = r#" - INSERT INTO channel_paths - (id_path, channel_id) - SELECT - id_path || $1 || '/', $2 - FROM - channel_paths - WHERE - channel_id = $3 - "#; - let channel_paths_stmt = Statement::from_sql_and_values( - self.pool.get_database_backend(), - sql, - [ - channel.id.to_proto().into(), - channel.id.to_proto().into(), - parent.to_proto().into(), - ], - ); - tx.execute(channel_paths_stmt).await?; + let participants_to_update; + if let Some(parent) = &parent { + participants_to_update = self + .participants_to_notify_for_channel_change(parent, &*tx) + .await?; } else { - channel_path::Entity::insert(channel_path::ActiveModel { - channel_id: ActiveValue::Set(channel.id), - id_path: ActiveValue::Set(format!("/{}/", channel.id)), - }) - .exec(&*tx) - .await?; - } + participants_to_update = vec![]; - if parent.is_none() { channel_member::ActiveModel { id: ActiveValue::NotSet, channel_id: ActiveValue::Set(channel.id), @@ -100,22 +87,10 @@ impl Database { } .insert(&*tx) .await?; - } - - let participants_to_update = if let Some(parent) = parent { - self.participants_to_notify_for_channel_change(parent, &*tx) - .await? - } else { - vec![] }; Ok(CreateChannelResult { - channel: Channel { - id: channel.id, - visibility: channel.visibility, - name: channel.name, - role: ChannelRole::Admin, - }, + channel: Channel::from_model(channel, ChannelRole::Admin), participants_to_update, }) }) @@ -130,20 +105,14 @@ impl Database { environment: &str, ) -> Result<(JoinRoom, Option, ChannelRole)> { self.transaction(move |tx| async move { + let channel = self.get_channel_internal(channel_id, &*tx).await?; + let mut role = self.channel_role_for_user(&channel, user_id, &*tx).await?; + let mut accept_invite_result = None; - let channel = channel::Entity::find() - .filter(channel::Column::Id.eq(channel_id)) - .one(&*tx) - .await?; - - let mut role = self - .channel_role_for_user(channel_id, user_id, &*tx) - .await?; - - if role.is_none() && channel.is_some() { + if role.is_none() { if let Some(invitation) = self - .pending_invite_for_channel(channel_id, user_id, &*tx) + .pending_invite_for_channel(&channel, user_id, &*tx) .await? { // note, this may be a parent channel @@ -156,31 +125,28 @@ impl Database { .await?; accept_invite_result = Some( - self.calculate_membership_updated(channel_id, user_id, &*tx) + self.calculate_membership_updated(&channel, user_id, &*tx) .await?, ); debug_assert!( - self.channel_role_for_user(channel_id, user_id, &*tx) - .await? - == role + self.channel_role_for_user(&channel, user_id, &*tx).await? == role ); } } - if role.is_none() - && channel.as_ref().map(|c| c.visibility) == Some(ChannelVisibility::Public) - { + + if channel.visibility == ChannelVisibility::Public { role = Some(ChannelRole::Guest); - let channel_id_to_join = self - .public_path_to_channel(channel_id, &*tx) + let channel_to_join = self + .public_ancestors_including_self(&channel, &*tx) .await? .first() .cloned() - .unwrap_or(channel_id); + .unwrap_or(channel.clone()); channel_member::Entity::insert(channel_member::ActiveModel { id: ActiveValue::NotSet, - channel_id: ActiveValue::Set(channel_id_to_join), + channel_id: ActiveValue::Set(channel_to_join.id), user_id: ActiveValue::Set(user_id), accepted: ActiveValue::Set(true), role: ActiveValue::Set(ChannelRole::Guest), @@ -189,19 +155,15 @@ impl Database { .await?; accept_invite_result = Some( - self.calculate_membership_updated(channel_id, user_id, &*tx) + self.calculate_membership_updated(&channel_to_join, user_id, &*tx) .await?, ); - debug_assert!( - self.channel_role_for_user(channel_id, user_id, &*tx) - .await? - == role - ); + debug_assert!(self.channel_role_for_user(&channel, user_id, &*tx).await? == role); } - if channel.is_none() || role.is_none() || role == Some(ChannelRole::Banned) { - Err(anyhow!("no such channel, or not allowed"))? + if role.is_none() || role == Some(ChannelRole::Banned) { + Err(anyhow!("not allowed"))? } let live_kit_room = format!("channel-{}", nanoid::nanoid!(30)); @@ -209,7 +171,7 @@ impl Database { .get_or_create_channel_room(channel_id, &live_kit_room, environment, &*tx) .await?; - self.join_channel_room_internal(channel_id, room_id, user_id, connection, &*tx) + self.join_channel_room_internal(room_id, user_id, connection, &*tx) .await .map(|jr| (jr, accept_invite_result, role.unwrap())) }) @@ -223,23 +185,21 @@ impl Database { admin_id: UserId, ) -> Result { self.transaction(move |tx| async move { - self.check_user_is_channel_admin(channel_id, admin_id, &*tx) + let channel = self.get_channel_internal(channel_id, &*tx).await?; + + self.check_user_is_channel_admin(&channel, admin_id, &*tx) .await?; let previous_members = self - .get_channel_participant_details_internal(channel_id, &*tx) + .get_channel_participant_details_internal(&channel, &*tx) .await?; - channel::ActiveModel { - id: ActiveValue::Unchanged(channel_id), - visibility: ActiveValue::Set(visibility), - ..Default::default() - } - .update(&*tx) - .await?; + let mut model = channel.into_active_model(); + model.visibility = ActiveValue::Set(visibility); + let channel = model.update(&*tx).await?; let mut participants_to_update: HashMap = self - .participants_to_notify_for_channel_change(channel_id, &*tx) + .participants_to_notify_for_channel_change(&channel, &*tx) .await? .into_iter() .collect(); @@ -249,10 +209,10 @@ impl Database { match visibility { ChannelVisibility::Members => { let all_descendents: Vec = self - .get_channel_descendants(vec![channel_id], &*tx) + .get_channel_descendants_including_self(vec![channel_id], &*tx) .await? .into_iter() - .map(|edge| ChannelId::from_proto(edge.channel_id)) + .map(|channel| channel.id) .collect(); channels_to_remove = channel::Entity::find() @@ -268,6 +228,7 @@ impl Database { .collect(); channels_to_remove.push(channel_id); + for member in previous_members { if member.role.can_only_see_public_descendants() { participants_to_remove.insert(member.user_id); @@ -275,11 +236,9 @@ impl Database { } } ChannelVisibility::Public => { - if let Some(public_parent_id) = - self.public_parent_channel_id(channel_id, &*tx).await? - { + if let Some(public_parent) = self.public_parent_channel(&channel, &*tx).await? { let parent_updates = self - .participants_to_notify_for_channel_change(public_parent_id, &*tx) + .participants_to_notify_for_channel_change(&public_parent, &*tx) .await?; for (user_id, channels) in parent_updates { @@ -304,39 +263,12 @@ impl Database { user_id: UserId, ) -> Result<(Vec, Vec)> { self.transaction(move |tx| async move { - self.check_user_is_channel_admin(channel_id, user_id, &*tx) + let channel = self.get_channel_internal(channel_id, &*tx).await?; + self.check_user_is_channel_admin(&channel, user_id, &*tx) .await?; - // Don't remove descendant channels that have additional parents. - let mut channels_to_remove: HashSet = HashSet::default(); - channels_to_remove.insert(channel_id); - - let graph = self.get_channel_descendants([channel_id], &*tx).await?; - for edge in graph.iter() { - channels_to_remove.insert(ChannelId::from_proto(edge.channel_id)); - } - - { - let mut channels_to_keep = channel_path::Entity::find() - .filter( - channel_path::Column::ChannelId - .is_in(channels_to_remove.iter().copied()) - .and( - channel_path::Column::IdPath - .not_like(&format!("%/{}/%", channel_id)), - ), - ) - .stream(&*tx) - .await?; - while let Some(row) = channels_to_keep.next().await { - let row = row?; - channels_to_remove.remove(&row.channel_id); - } - } - - let channel_ancestors = self.get_channel_ancestors(channel_id, &*tx).await?; let members_to_notify: Vec = channel_member::Entity::find() - .filter(channel_member::Column::ChannelId.is_in(channel_ancestors)) + .filter(channel_member::Column::ChannelId.is_in(channel.ancestors_including_self())) .select_only() .column(channel_member::Column::UserId) .distinct() @@ -344,25 +276,19 @@ impl Database { .all(&*tx) .await?; + let channels_to_remove = self + .get_channel_descendants_including_self(vec![channel.id], &*tx) + .await? + .into_iter() + .map(|channel| channel.id) + .collect::>(); + channel::Entity::delete_many() .filter(channel::Column::Id.is_in(channels_to_remove.iter().copied())) .exec(&*tx) .await?; - // Delete any other paths that include this channel - let sql = r#" - DELETE FROM channel_paths - WHERE - id_path LIKE '%' || $1 || '%' - "#; - let channel_paths_stmt = Statement::from_sql_and_values( - self.pool.get_database_backend(), - sql, - [channel_id.to_proto().into()], - ); - tx.execute(channel_paths_stmt).await?; - - Ok((channels_to_remove.into_iter().collect(), members_to_notify)) + Ok((channels_to_remove, members_to_notify)) }) .await } @@ -375,7 +301,8 @@ impl Database { role: ChannelRole, ) -> Result { self.transaction(move |tx| async move { - self.check_user_is_channel_admin(channel_id, inviter_id, &*tx) + let channel = self.get_channel_internal(channel_id, &*tx).await?; + self.check_user_is_channel_admin(&channel, inviter_id, &*tx) .await?; channel_member::ActiveModel { @@ -388,17 +315,7 @@ impl Database { .insert(&*tx) .await?; - let channel = channel::Entity::find_by_id(channel_id) - .one(&*tx) - .await? - .unwrap(); - - let channel = Channel { - id: channel.id, - visibility: channel.visibility, - name: channel.name, - role, - }; + let channel = Channel::from_model(channel, role); let notifications = self .create_notification( @@ -440,40 +357,27 @@ impl Database { self.transaction(move |tx| async move { let new_name = Self::sanitize_channel_name(new_name)?.to_string(); + let channel = self.get_channel_internal(channel_id, &*tx).await?; let role = self - .check_user_is_channel_admin(channel_id, admin_id, &*tx) + .check_user_is_channel_admin(&channel, admin_id, &*tx) .await?; - let channel = channel::ActiveModel { - id: ActiveValue::Unchanged(channel_id), - name: ActiveValue::Set(new_name.clone()), - ..Default::default() - } - .update(&*tx) - .await?; + let mut model = channel.into_active_model(); + model.name = ActiveValue::Set(new_name.clone()); + let channel = model.update(&*tx).await?; let participants = self - .get_channel_participant_details_internal(channel_id, &*tx) + .get_channel_participant_details_internal(&channel, &*tx) .await?; Ok(RenameChannelResult { - channel: Channel { - id: channel.id, - name: channel.name, - visibility: channel.visibility, - role, - }, + channel: Channel::from_model(channel.clone(), role), participants_to_update: participants .iter() .map(|participant| { ( participant.user_id, - Channel { - id: channel.id, - name: new_name.clone(), - visibility: channel.visibility, - role: participant.role, - }, + Channel::from_model(channel.clone(), participant.role), ) }) .collect(), @@ -489,6 +393,8 @@ impl Database { accept: bool, ) -> Result { self.transaction(move |tx| async move { + let channel = self.get_channel_internal(channel_id, &*tx).await?; + let membership_update = if accept { let rows_affected = channel_member::Entity::update_many() .set(channel_member::ActiveModel { @@ -510,7 +416,7 @@ impl Database { } Some( - self.calculate_membership_updated(channel_id, user_id, &*tx) + self.calculate_membership_updated(&channel, user_id, &*tx) .await?, ) } else { @@ -554,53 +460,26 @@ impl Database { async fn calculate_membership_updated( &self, - channel_id: ChannelId, + channel: &channel::Model, user_id: UserId, tx: &DatabaseTransaction, ) -> Result { - let mut channel_to_refresh = channel_id; - let mut removed_channels: Vec = Vec::new(); - - // if the user was previously a guest of a parent public channel they may have seen this - // channel (or its descendants) in the tree already. - // Now they have new permissions, the graph of channels needs updating from that point. - if let Some(public_parent) = self.public_parent_channel_id(channel_id, &*tx).await? { - if self - .channel_role_for_user(public_parent, user_id, &*tx) - .await? - == Some(ChannelRole::Guest) - { - channel_to_refresh = public_parent; - } - } - - // remove all descendant channels from the user's tree - removed_channels.append( - &mut self - .get_channel_descendants(vec![channel_to_refresh], &*tx) - .await? - .into_iter() - .map(|edge| ChannelId::from_proto(edge.channel_id)) - .collect(), - ); - - let new_channels = self - .get_user_channels(user_id, Some(channel_to_refresh), &*tx) - .await?; - - // We only add the current channel to "moved" if the user has lost access, - // otherwise it would be made a root channel on the client. - if !new_channels - .channels - .channels - .iter() - .any(|c| c.id == channel_id) - { - removed_channels.push(channel_id); - } + let new_channels = self.get_user_channels(user_id, Some(channel), &*tx).await?; + let removed_channels = self + .get_channel_descendants_including_self(vec![channel.id], &*tx) + .await? + .into_iter() + .filter_map(|channel| { + if !new_channels.channels.iter().any(|c| c.id == channel.id) { + Some(channel.id) + } else { + None + } + }) + .collect::>(); Ok(MembershipUpdated { - channel_id, + channel_id: channel.id, new_channels, removed_channels, }) @@ -613,7 +492,8 @@ impl Database { admin_id: UserId, ) -> Result { self.transaction(|tx| async move { - self.check_user_is_channel_admin(channel_id, admin_id, &*tx) + let channel = self.get_channel_internal(channel_id, &*tx).await?; + self.check_user_is_channel_admin(&channel, admin_id, &*tx) .await?; let result = channel_member::Entity::delete_many() @@ -631,7 +511,7 @@ impl Database { Ok(RemoveChannelMemberResult { membership_update: self - .calculate_membership_updated(channel_id, member_id, &*tx) + .calculate_membership_updated(&channel, member_id, &*tx) .await?, notification_id: self .remove_notification( @@ -673,11 +553,9 @@ impl Database { let channels = channels .into_iter() - .map(|channel| Channel { - id: channel.id, - name: channel.name, - visibility: channel.visibility, - role: role_for_channel[&channel.id], + .filter_map(|channel| { + let role = *role_for_channel.get(&channel.id)?; + Some(Channel::from_model(channel, role)) }) .collect(); @@ -698,15 +576,9 @@ impl Database { pub async fn get_user_channels( &self, user_id: UserId, - parent_channel_id: Option, + ancestor_channel: Option<&channel::Model>, tx: &DatabaseTransaction, ) -> Result { - // note: we could (maybe) win some efficiency here when parent_channel_id - // is set by getting just the role for that channel, then getting descendants - // with roles attached; but that's not as straightforward as it sounds - // because we need to calculate the path to the channel to make the query - // efficient, which currently requires an extra round trip to the database. - // Fix this later... let channel_memberships = channel_member::Entity::find() .filter( channel_member::Column::UserId @@ -716,117 +588,65 @@ impl Database { .all(&*tx) .await?; - let mut edges = self - .get_channel_descendants(channel_memberships.iter().map(|m| m.channel_id), &*tx) + let descendants = self + .get_channel_descendants_including_self( + channel_memberships.iter().map(|m| m.channel_id), + &*tx, + ) .await?; - let mut role_for_channel: HashMap = HashMap::default(); - + let mut roles_by_channel_id: HashMap = HashMap::default(); for membership in channel_memberships.iter() { - let included = - parent_channel_id.is_none() || membership.channel_id == parent_channel_id.unwrap(); - role_for_channel.insert(membership.channel_id, (membership.role, included)); + roles_by_channel_id.insert(membership.channel_id, membership.role); } - for ChannelEdge { - parent_id, - channel_id, - } in edges.iter() - { - let parent_id = ChannelId::from_proto(*parent_id); - let channel_id = ChannelId::from_proto(*channel_id); - debug_assert!(role_for_channel.get(&parent_id).is_some()); - let (parent_role, parent_included) = role_for_channel[&parent_id]; + let mut visible_channel_ids: HashSet = HashSet::default(); - if let Some((existing_role, included)) = role_for_channel.get(&channel_id) { - role_for_channel.insert( - channel_id, - (existing_role.max(parent_role), *included || parent_included), - ); - } else { - role_for_channel.insert( - channel_id, - ( - parent_role, - parent_included - || parent_channel_id.is_none() - || Some(channel_id) == parent_channel_id, - ), - ); - } - } + let channels: Vec = descendants + .into_iter() + .filter_map(|channel| { + let parent_role = channel + .parent_id() + .and_then(|parent_id| roles_by_channel_id.get(&parent_id)); - let mut channels: Vec = Vec::new(); - let mut channels_to_remove: HashSet = HashSet::default(); - - let mut rows = channel::Entity::find() - .filter(channel::Column::Id.is_in(role_for_channel.keys().copied())) - .stream(&*tx) - .await?; - - while let Some(row) = rows.next().await { - let channel = row?; - let (role, included) = role_for_channel[&channel.id]; - - if !included - || role == ChannelRole::Banned - || role == ChannelRole::Guest && channel.visibility != ChannelVisibility::Public - { - channels_to_remove.insert(channel.id.0 as u64); - continue; - } - - channels.push(Channel { - id: channel.id, - name: channel.name, - visibility: channel.visibility, - role, - }); - } - drop(rows); - - if !channels_to_remove.is_empty() { - // Note: this code assumes each channel has one parent. - // If there are multiple valid public paths to a channel, - // e.g. - // If both of these paths are present (* indicating public): - // - zed* -> projects -> vim* - // - zed* -> conrad -> public-projects* -> vim* - // Users would only see one of them (based on edge sort order) - let mut replacement_parent: HashMap = HashMap::default(); - for ChannelEdge { - parent_id, - channel_id, - } in edges.iter() - { - if channels_to_remove.contains(channel_id) { - replacement_parent.insert(*channel_id, *parent_id); - } - } - - let mut new_edges: Vec = Vec::new(); - 'outer: for ChannelEdge { - mut parent_id, - channel_id, - } in edges.iter() - { - if channels_to_remove.contains(channel_id) { - continue; - } - while channels_to_remove.contains(&parent_id) { - if let Some(new_parent_id) = replacement_parent.get(&parent_id) { - parent_id = *new_parent_id; + let role = if let Some(parent_role) = parent_role { + let role = if let Some(existing_role) = roles_by_channel_id.get(&channel.id) { + existing_role.max(*parent_role) } else { - continue 'outer; + *parent_role + }; + roles_by_channel_id.insert(channel.id, role); + role + } else { + *roles_by_channel_id.get(&channel.id)? + }; + + let can_see_parent_paths = role.can_see_all_descendants() + || role.can_only_see_public_descendants() + && channel.visibility == ChannelVisibility::Public; + if !can_see_parent_paths { + return None; + } + + visible_channel_ids.insert(channel.id); + + if let Some(ancestor) = ancestor_channel { + if !channel + .ancestors_including_self() + .any(|id| id == ancestor.id) + { + return None; } } - new_edges.push(ChannelEdge { - parent_id, - channel_id: *channel_id, - }) - } - edges = new_edges; - } + + let mut channel = Channel::from_model(channel, role); + channel + .parent_path + .retain(|id| visible_channel_ids.contains(&id)); + + Some(channel) + }) + .collect(); #[derive(Copy, Clone, Debug, EnumIter, DeriveColumn)] enum QueryUserIdsAndChannelIds { @@ -861,7 +681,7 @@ impl Database { .await?; Ok(ChannelsForUser { - channels: ChannelGraph { channels, edges }, + channels, channel_participants, unseen_buffer_changes: channel_buffer_changes, channel_messages: unseen_messages, @@ -870,7 +690,7 @@ impl Database { async fn participants_to_notify_for_channel_change( &self, - new_parent: ChannelId, + new_parent: &channel::Model, tx: &DatabaseTransaction, ) -> Result> { let mut results: Vec<(UserId, ChannelsForUser)> = Vec::new(); @@ -890,11 +710,10 @@ impl Database { )) } - let public_parent = self - .public_path_to_channel(new_parent, &*tx) - .await? - .last() - .copied(); + let public_parents = self + .public_ancestors_including_self(new_parent, &*tx) + .await?; + let public_parent = public_parents.last(); let Some(public_parent) = public_parent else { return Ok(results); @@ -931,7 +750,8 @@ impl Database { role: ChannelRole, ) -> Result { self.transaction(|tx| async move { - self.check_user_is_channel_admin(channel_id, admin_id, &*tx) + let channel = self.get_channel_internal(channel_id, &*tx).await?; + self.check_user_is_channel_admin(&channel, admin_id, &*tx) .await?; let membership = channel_member::Entity::find() @@ -951,24 +771,16 @@ impl Database { update.role = ActiveValue::Set(role); let updated = channel_member::Entity::update(update).exec(&*tx).await?; - if !updated.accepted { - let channel = channel::Entity::find_by_id(channel_id) - .one(&*tx) - .await? - .unwrap(); - - return Ok(SetMemberRoleResult::InviteUpdated(Channel { - id: channel.id, - visibility: channel.visibility, - name: channel.name, - role, - })); + if updated.accepted { + Ok(SetMemberRoleResult::MembershipUpdated( + self.calculate_membership_updated(&channel, for_user, &*tx) + .await?, + )) + } else { + Ok(SetMemberRoleResult::InviteUpdated(Channel::from_model( + channel, role, + ))) } - - Ok(SetMemberRoleResult::MembershipUpdated( - self.calculate_membership_updated(channel_id, for_user, &*tx) - .await?, - )) }) .await } @@ -980,12 +792,13 @@ impl Database { ) -> Result> { let (role, members) = self .transaction(move |tx| async move { + let channel = self.get_channel_internal(channel_id, &*tx).await?; let role = self - .check_user_is_channel_participant(channel_id, user_id, &*tx) + .check_user_is_channel_participant(&channel, user_id, &*tx) .await?; Ok(( role, - self.get_channel_participant_details_internal(channel_id, &*tx) + self.get_channel_participant_details_internal(&channel, &*tx) .await?, )) }) @@ -1016,16 +829,9 @@ impl Database { async fn get_channel_participant_details_internal( &self, - channel_id: ChannelId, + channel: &channel::Model, tx: &DatabaseTransaction, ) -> Result> { - let channel_visibility = channel::Entity::find() - .filter(channel::Column::Id.eq(channel_id)) - .one(&*tx) - .await? - .map(|channel| channel.visibility) - .unwrap_or(ChannelVisibility::Members); - #[derive(Copy, Clone, Debug, EnumIter, DeriveColumn)] enum QueryMemberDetails { UserId, @@ -1035,16 +841,14 @@ impl Database { Visibility, } - let tx = tx; - let ancestor_ids = self.get_channel_ancestors(channel_id, &*tx).await?; let mut stream = channel_member::Entity::find() .left_join(channel::Entity) - .filter(channel_member::Column::ChannelId.is_in(ancestor_ids.iter().copied())) + .filter(channel_member::Column::ChannelId.is_in(channel.ancestors_including_self())) .select_only() .column(channel_member::Column::UserId) .column(channel_member::Column::Role) .column_as( - channel_member::Column::ChannelId.eq(channel_id), + channel_member::Column::ChannelId.eq(channel.id), QueryMemberDetails::IsDirectMember, ) .column(channel_member::Column::Accepted) @@ -1072,7 +876,7 @@ impl Database { if channel_role == ChannelRole::Guest && visibility != ChannelVisibility::Public - && channel_visibility != ChannelVisibility::Public + && channel.visibility != ChannelVisibility::Public { continue; } @@ -1108,11 +912,11 @@ impl Database { pub async fn get_channel_participants( &self, - channel_id: ChannelId, + channel: &channel::Model, tx: &DatabaseTransaction, ) -> Result> { let participants = self - .get_channel_participant_details_internal(channel_id, &*tx) + .get_channel_participant_details_internal(channel, &*tx) .await?; Ok(participants .into_iter() @@ -1122,11 +926,11 @@ impl Database { pub async fn check_user_is_channel_admin( &self, - channel_id: ChannelId, + channel: &channel::Model, user_id: UserId, tx: &DatabaseTransaction, ) -> Result { - let role = self.channel_role_for_user(channel_id, user_id, tx).await?; + let role = self.channel_role_for_user(channel, user_id, tx).await?; match role { Some(ChannelRole::Admin) => Ok(role.unwrap()), Some(ChannelRole::Member) @@ -1140,11 +944,11 @@ impl Database { pub async fn check_user_is_channel_member( &self, - channel_id: ChannelId, + channel: &channel::Model, user_id: UserId, tx: &DatabaseTransaction, ) -> Result { - let channel_role = self.channel_role_for_user(channel_id, user_id, tx).await?; + let channel_role = self.channel_role_for_user(channel, user_id, tx).await?; match channel_role { Some(ChannelRole::Admin) | Some(ChannelRole::Member) => Ok(channel_role.unwrap()), Some(ChannelRole::Banned) | Some(ChannelRole::Guest) | None => Err(anyhow!( @@ -1155,11 +959,11 @@ impl Database { pub async fn check_user_is_channel_participant( &self, - channel_id: ChannelId, + channel: &channel::Model, user_id: UserId, tx: &DatabaseTransaction, ) -> Result { - let role = self.channel_role_for_user(channel_id, user_id, tx).await?; + let role = self.channel_role_for_user(channel, user_id, tx).await?; match role { Some(ChannelRole::Admin) | Some(ChannelRole::Member) | Some(ChannelRole::Guest) => { Ok(role.unwrap()) @@ -1172,14 +976,12 @@ impl Database { pub async fn pending_invite_for_channel( &self, - channel_id: ChannelId, + channel: &channel::Model, user_id: UserId, tx: &DatabaseTransaction, ) -> Result> { - let channel_ids = self.get_channel_ancestors(channel_id, tx).await?; - let row = channel_member::Entity::find() - .filter(channel_member::Column::ChannelId.is_in(channel_ids)) + .filter(channel_member::Column::ChannelId.is_in(channel.ancestors_including_self())) .filter(channel_member::Column::UserId.eq(user_id)) .filter(channel_member::Column::Accepted.eq(false)) .one(&*tx) @@ -1188,88 +990,39 @@ impl Database { Ok(row) } - pub async fn parent_channel_id( + pub async fn public_parent_channel( &self, - channel_id: ChannelId, + channel: &channel::Model, tx: &DatabaseTransaction, - ) -> Result> { - let path = self.path_to_channel(channel_id, &*tx).await?; - if path.len() >= 2 { - Ok(Some(path[path.len() - 2])) - } else { - Ok(None) + ) -> Result> { + let mut path = self.public_ancestors_including_self(channel, &*tx).await?; + if path.last().unwrap().id == channel.id { + path.pop(); } + Ok(path.pop()) } - pub async fn public_parent_channel_id( + pub async fn public_ancestors_including_self( &self, - channel_id: ChannelId, + channel: &channel::Model, tx: &DatabaseTransaction, - ) -> Result> { - let path = self.public_path_to_channel(channel_id, &*tx).await?; - if path.len() >= 2 && path.last().copied() == Some(channel_id) { - Ok(Some(path[path.len() - 2])) - } else { - Ok(path.last().copied()) - } - } - - pub async fn path_to_channel( - &self, - channel_id: ChannelId, - tx: &DatabaseTransaction, - ) -> Result> { - let arbitary_path = channel_path::Entity::find() - .filter(channel_path::Column::ChannelId.eq(channel_id)) - .order_by(channel_path::Column::IdPath, sea_orm::Order::Desc) - .one(tx) - .await?; - - let Some(path) = arbitary_path else { - return Ok(vec![]); - }; - - Ok(path - .id_path - .trim_matches('/') - .split('/') - .map(|id| ChannelId::from_proto(id.parse().unwrap())) - .collect()) - } - - pub async fn public_path_to_channel( - &self, - channel_id: ChannelId, - tx: &DatabaseTransaction, - ) -> Result> { - let ancestor_ids = self.path_to_channel(channel_id, &*tx).await?; - - let rows = channel::Entity::find() - .filter(channel::Column::Id.is_in(ancestor_ids.iter().copied())) + ) -> Result> { + let visible_channels = channel::Entity::find() + .filter(channel::Column::Id.is_in(channel.ancestors_including_self())) .filter(channel::Column::Visibility.eq(ChannelVisibility::Public)) + .order_by_asc(channel::Column::ParentPath) .all(&*tx) .await?; - let mut visible_channels: HashSet = HashSet::default(); - - for row in rows { - visible_channels.insert(row.id); - } - - Ok(ancestor_ids - .into_iter() - .filter(|id| visible_channels.contains(id)) - .collect()) + Ok(visible_channels) } pub async fn channel_role_for_user( &self, - channel_id: ChannelId, + channel: &channel::Model, user_id: UserId, tx: &DatabaseTransaction, ) -> Result> { - let channel_ids = self.get_channel_ancestors(channel_id, tx).await?; - #[derive(Copy, Clone, Debug, EnumIter, DeriveColumn)] enum QueryChannelMembership { ChannelId, @@ -1281,7 +1034,7 @@ impl Database { .left_join(channel::Entity) .filter( channel_member::Column::ChannelId - .is_in(channel_ids) + .is_in(channel.ancestors_including_self()) .and(channel_member::Column::UserId.eq(user_id)) .and(channel_member::Column::Accepted.eq(true)), ) @@ -1320,7 +1073,7 @@ impl Database { } ChannelRole::Guest => {} } - if channel_id == membership_channel { + if channel.id == membership_channel { current_channel_visibility = Some(visibility); } } @@ -1330,7 +1083,7 @@ impl Database { if is_participant && user_role.is_none() { if current_channel_visibility.is_none() { current_channel_visibility = channel::Entity::find() - .filter(channel::Column::Id.eq(channel_id)) + .filter(channel::Column::Id.eq(channel.id)) .one(&*tx) .await? .map(|channel| channel.visibility); @@ -1343,39 +1096,13 @@ impl Database { Ok(user_role) } - /// Returns the channel ancestors in arbitrary order - pub async fn get_channel_ancestors( - &self, - channel_id: ChannelId, - tx: &DatabaseTransaction, - ) -> Result> { - let paths = channel_path::Entity::find() - .filter(channel_path::Column::ChannelId.eq(channel_id)) - .order_by(channel_path::Column::IdPath, sea_orm::Order::Desc) - .all(tx) - .await?; - let mut channel_ids = Vec::new(); - for path in paths { - for id in path.id_path.trim_matches('/').split('/') { - if let Ok(id) = id.parse() { - let id = ChannelId::from_proto(id); - if let Err(ix) = channel_ids.binary_search(&id) { - channel_ids.insert(ix, id); - } - } - } - } - Ok(channel_ids) - } - - // Returns the channel desendants as a sorted list of edges for further processing. - // The edges are sorted such that you will see unknown channel ids as children - // before you see them as parents. - async fn get_channel_descendants( + // Get the descendants of the given set if channels, ordered by their + // path. + async fn get_channel_descendants_including_self( &self, channel_ids: impl IntoIterator, tx: &DatabaseTransaction, - ) -> Result> { + ) -> Result> { let mut values = String::new(); for id in channel_ids { if !values.is_empty() { @@ -1390,65 +1117,55 @@ impl Database { let sql = format!( r#" - SELECT - descendant_paths.* + SELECT DISTINCT + descendant_channels.*, + descendant_channels.parent_path || descendant_channels.id as full_path FROM - channel_paths parent_paths, channel_paths descendant_paths + channels parent_channels, channels descendant_channels WHERE - parent_paths.channel_id IN ({values}) AND - descendant_paths.id_path != parent_paths.id_path AND - descendant_paths.id_path LIKE (parent_paths.id_path || '%') + descendant_channels.id IN ({values}) OR + ( + parent_channels.id IN ({values}) AND + descendant_channels.parent_path LIKE (parent_channels.parent_path || parent_channels.id || '/%') + ) ORDER BY - descendant_paths.id_path - "# + full_path ASC + "# ); - let stmt = Statement::from_string(self.pool.get_database_backend(), sql); - - let mut paths = channel_path::Entity::find() - .from_raw_sql(stmt) - .stream(tx) - .await?; - - let mut results: Vec = Vec::new(); - while let Some(path) = paths.next().await { - let path = path?; - let ids: Vec<&str> = path.id_path.trim_matches('/').split('/').collect(); - - debug_assert!(ids.len() >= 2); - debug_assert!(ids[ids.len() - 1] == path.channel_id.to_string()); - - results.push(ChannelEdge { - parent_id: ids[ids.len() - 2].parse().unwrap(), - channel_id: ids[ids.len() - 1].parse().unwrap(), - }) - } - - Ok(results) + Ok(channel::Entity::find() + .from_raw_sql(Statement::from_string( + self.pool.get_database_backend(), + sql, + )) + .all(tx) + .await?) } /// Returns the channel with the given ID pub async fn get_channel(&self, channel_id: ChannelId, user_id: UserId) -> Result { self.transaction(|tx| async move { + let channel = self.get_channel_internal(channel_id, &*tx).await?; let role = self - .check_user_is_channel_participant(channel_id, user_id, &*tx) + .check_user_is_channel_participant(&channel, user_id, &*tx) .await?; - let channel = channel::Entity::find_by_id(channel_id).one(&*tx).await?; - let Some(channel) = channel else { - Err(anyhow!("no such channel"))? - }; - - Ok(Channel { - id: channel.id, - visibility: channel.visibility, - role, - name: channel.name, - }) + Ok(Channel::from_model(channel, role)) }) .await } + pub async fn get_channel_internal( + &self, + channel_id: ChannelId, + tx: &DatabaseTransaction, + ) -> Result { + Ok(channel::Entity::find_by_id(channel_id) + .one(&*tx) + .await? + .ok_or_else(|| anyhow!("no such channel"))?) + } + pub(crate) async fn get_or_create_channel_room( &self, channel_id: ChannelId, @@ -1484,203 +1201,86 @@ impl Database { Ok(room_id) } - // Insert an edge from the given channel to the given other channel. - pub async fn link_channel( - &self, - user: UserId, - channel: ChannelId, - to: ChannelId, - ) -> Result { - self.transaction(|tx| async move { - 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, - new_parent: ChannelId, - tx: &DatabaseTransaction, - ) -> Result { - self.check_user_is_channel_admin(new_parent, user, &*tx) - .await?; - - 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(), - )); - } - } - - let paths_to_new_parent = channel_path::Entity::find() - .filter(channel_path::Column::ChannelId.eq(new_parent)) - .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)), - } - })); - } - - channel_path::Entity::insert_many(new_paths) - .exec(&*tx) - .await?; - - // remove any root edges for the channel we just linked - { - channel_path::Entity::delete_many() - .filter(channel_path::Column::IdPath.like(&format!("/{}/%", channel))) - .exec(&*tx) - .await?; - } - - let mut channel_info = self.get_user_channels(user, Some(channel), &*tx).await?; - - channel_info.channels.edges.push(ChannelEdge { - channel_id: channel.to_proto(), - parent_id: new_parent.to_proto(), - }); - - Ok(channel_info.channels) - } - - /// 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, - user: UserId, - channel: ChannelId, - from: ChannelId, - ) -> Result<()> { - self.transaction(|tx| async move { - 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: ChannelId, - tx: &DatabaseTransaction, - ) -> Result<()> { - self.check_user_is_channel_admin(from, user, &*tx).await?; - - let sql = r#" - 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 - 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(()) - } - /// Move a channel from one parent to another pub async fn move_channel( &self, channel_id: ChannelId, - old_parent_id: Option, - new_parent_id: ChannelId, + new_parent_id: Option, admin_id: UserId, ) -> Result> { self.transaction(|tx| async move { - self.check_user_is_channel_admin(channel_id, admin_id, &*tx) + let channel = self.get_channel_internal(channel_id, &*tx).await?; + self.check_user_is_channel_admin(&channel, admin_id, &*tx) .await?; - debug_assert_eq!( - self.parent_channel_id(channel_id, &*tx).await?, - old_parent_id - ); + let new_parent_path; + let new_parent_channel; + if let Some(new_parent_id) = new_parent_id { + let new_parent = self.get_channel_internal(new_parent_id, &*tx).await?; + self.check_user_is_channel_admin(&new_parent, admin_id, &*tx) + .await?; - if old_parent_id == Some(new_parent_id) { + new_parent_path = new_parent.path(); + new_parent_channel = Some(new_parent); + } else { + new_parent_path = String::new(); + new_parent_channel = None; + }; + + let previous_participants = self + .get_channel_participant_details_internal(&channel, &*tx) + .await?; + + let old_path = format!("{}{}/", channel.parent_path, channel.id); + let new_path = format!("{}{}/", new_parent_path, channel.id); + + if old_path == new_path { return Ok(None); } - let previous_participants = self - .get_channel_participant_details_internal(channel_id, &*tx) - .await?; - self.link_channel_internal(admin_id, channel_id, new_parent_id, &*tx) - .await?; + let mut model = channel.into_active_model(); + model.parent_path = ActiveValue::Set(new_parent_path); + let channel = model.update(&*tx).await?; - if let Some(from) = old_parent_id { - self.unlink_channel_internal(admin_id, channel_id, from, &*tx) - .await?; + if new_parent_channel.is_none() { + channel_member::ActiveModel { + id: ActiveValue::NotSet, + channel_id: ActiveValue::Set(channel_id), + user_id: ActiveValue::Set(admin_id), + accepted: ActiveValue::Set(true), + role: ActiveValue::Set(ChannelRole::Admin), + } + .insert(&*tx) + .await?; } - let participants_to_update: HashMap = self - .participants_to_notify_for_channel_change(new_parent_id, &*tx) + let descendent_ids = + ChannelId::find_by_statement::(Statement::from_sql_and_values( + self.pool.get_database_backend(), + " + UPDATE channels SET parent_path = REPLACE(parent_path, $1, $2) + WHERE parent_path LIKE $3 || '%' + RETURNING id + ", + [old_path.clone().into(), new_path.into(), old_path.into()], + )) + .all(&*tx) + .await?; + + let participants_to_update: HashMap<_, _> = self + .participants_to_notify_for_channel_change( + new_parent_channel.as_ref().unwrap_or(&channel), + &*tx, + ) .await? .into_iter() .collect(); let mut moved_channels: HashSet = HashSet::default(); - moved_channels.insert(channel_id); - for edge in self.get_channel_descendants([channel_id], &*tx).await? { - moved_channels.insert(ChannelId::from_proto(edge.channel_id)); + for id in descendent_ids { + moved_channels.insert(id); } + moved_channels.insert(channel_id); let mut participants_to_remove: HashSet = HashSet::default(); for participant in previous_participants { @@ -1701,47 +1301,12 @@ impl Database { } } +#[derive(Copy, Clone, Debug, EnumIter, DeriveColumn)] +enum QueryIds { + Id, +} + #[derive(Copy, Clone, Debug, EnumIter, DeriveColumn)] enum QueryUserIds { UserId, } - -#[derive(Debug)] -pub struct ChannelGraph { - pub channels: Vec, - pub edges: Vec, -} - -impl ChannelGraph { - pub fn is_empty(&self) -> bool { - self.channels.is_empty() && self.edges.is_empty() - } -} - -#[cfg(test)] -impl PartialEq for ChannelGraph { - fn eq(&self, other: &Self) -> bool { - // Order independent comparison for tests - let channels_set = self.channels.iter().collect::>(); - let other_channels_set = other.channels.iter().collect::>(); - let edges_set = self - .edges - .iter() - .map(|edge| (edge.channel_id, edge.parent_id)) - .collect::>(); - let other_edges_set = other - .edges - .iter() - .map(|edge| (edge.channel_id, edge.parent_id)) - .collect::>(); - - channels_set == other_channels_set && edges_set == other_edges_set - } -} - -#[cfg(not(test))] -impl PartialEq for ChannelGraph { - fn eq(&self, other: &Self) -> bool { - self.channels == other.channels && self.edges == other.edges - } -} diff --git a/crates/collab/src/db/queries/messages.rs b/crates/collab/src/db/queries/messages.rs index 562c4e45c4..47bb27df39 100644 --- a/crates/collab/src/db/queries/messages.rs +++ b/crates/collab/src/db/queries/messages.rs @@ -1,5 +1,4 @@ use super::*; -use futures::Stream; use rpc::Notification; use sea_orm::TryInsertResult; use time::OffsetDateTime; @@ -12,7 +11,8 @@ impl Database { user_id: UserId, ) -> Result<()> { self.transaction(|tx| async move { - self.check_user_is_channel_participant(channel_id, user_id, &*tx) + let channel = self.get_channel_internal(channel_id, &*tx).await?; + self.check_user_is_channel_participant(&channel, user_id, &*tx) .await?; channel_chat_participant::ActiveModel { id: ActiveValue::NotSet, @@ -80,7 +80,8 @@ impl Database { before_message_id: Option, ) -> Result> { self.transaction(|tx| async move { - self.check_user_is_channel_participant(channel_id, user_id, &*tx) + let channel = self.get_channel_internal(channel_id, &*tx).await?; + self.check_user_is_channel_participant(&channel, user_id, &*tx) .await?; let mut condition = @@ -94,7 +95,7 @@ impl Database { .filter(condition) .order_by_desc(channel_message::Column::Id) .limit(count as u64) - .stream(&*tx) + .all(&*tx) .await?; self.load_channel_messages(rows, &*tx).await @@ -111,27 +112,23 @@ impl Database { let rows = channel_message::Entity::find() .filter(channel_message::Column::Id.is_in(message_ids.iter().copied())) .order_by_desc(channel_message::Column::Id) - .stream(&*tx) + .all(&*tx) .await?; - let mut channel_ids = HashSet::::default(); - let messages = self - .load_channel_messages( - rows.map(|row| { - row.map(|row| { - channel_ids.insert(row.channel_id); - row - }) - }), - &*tx, - ) - .await?; + let mut channels = HashMap::::default(); + for row in &rows { + channels.insert( + row.channel_id, + self.get_channel_internal(row.channel_id, &*tx).await?, + ); + } - for channel_id in channel_ids { - self.check_user_is_channel_member(channel_id, user_id, &*tx) + for (_, channel) in channels { + self.check_user_is_channel_participant(&channel, user_id, &*tx) .await?; } + let messages = self.load_channel_messages(rows, &*tx).await?; Ok(messages) }) .await @@ -139,26 +136,26 @@ impl Database { async fn load_channel_messages( &self, - mut rows: impl Send + Unpin + Stream>, + rows: Vec, tx: &DatabaseTransaction, ) -> Result> { - let mut messages = Vec::new(); - while let Some(row) = rows.next().await { - let row = row?; - let nonce = row.nonce.as_u64_pair(); - messages.push(proto::ChannelMessage { - id: row.id.to_proto(), - sender_id: row.sender_id.to_proto(), - body: row.body, - timestamp: row.sent_at.assume_utc().unix_timestamp() as u64, - mentions: vec![], - nonce: Some(proto::Nonce { - upper_half: nonce.0, - lower_half: nonce.1, - }), - }); - } - drop(rows); + let mut messages = rows + .into_iter() + .map(|row| { + let nonce = row.nonce.as_u64_pair(); + proto::ChannelMessage { + id: row.id.to_proto(), + sender_id: row.sender_id.to_proto(), + body: row.body, + timestamp: row.sent_at.assume_utc().unix_timestamp() as u64, + mentions: vec![], + nonce: Some(proto::Nonce { + upper_half: nonce.0, + lower_half: nonce.1, + }), + } + }) + .collect::>(); messages.reverse(); let mut mentions = channel_message_mention::Entity::find() @@ -203,7 +200,8 @@ impl Database { nonce: u128, ) -> Result { self.transaction(|tx| async move { - self.check_user_is_channel_participant(channel_id, user_id, &*tx) + let channel = self.get_channel_internal(channel_id, &*tx).await?; + self.check_user_is_channel_participant(&channel, user_id, &*tx) .await?; let mut rows = channel_chat_participant::Entity::find() @@ -310,7 +308,7 @@ impl Database { } } - let mut channel_members = self.get_channel_participants(channel_id, &*tx).await?; + let mut channel_members = self.get_channel_participants(&channel, &*tx).await?; channel_members.retain(|member| !participant_user_ids.contains(member)); Ok(CreatedChannelMessage { @@ -483,8 +481,9 @@ impl Database { .await?; if result.rows_affected == 0 { + let channel = self.get_channel_internal(channel_id, &*tx).await?; if self - .check_user_is_channel_admin(channel_id, user_id, &*tx) + .check_user_is_channel_admin(&channel, user_id, &*tx) .await .is_ok() { diff --git a/crates/collab/src/db/queries/rooms.rs b/crates/collab/src/db/queries/rooms.rs index 630d51cfe6..40fdf5d58f 100644 --- a/crates/collab/src/db/queries/rooms.rs +++ b/crates/collab/src/db/queries/rooms.rs @@ -50,10 +50,10 @@ impl Database { .map(|participant| participant.user_id), ); - let (channel_id, room) = self.get_channel_room(room_id, &tx).await?; + let (channel, room) = self.get_channel_room(room_id, &tx).await?; let channel_members; - if let Some(channel_id) = channel_id { - channel_members = self.get_channel_participants(channel_id, &tx).await?; + if let Some(channel) = &channel { + channel_members = self.get_channel_participants(channel, &tx).await?; } else { channel_members = Vec::new(); @@ -69,7 +69,7 @@ impl Database { Ok(RefreshedRoom { room, - channel_id, + channel_id: channel.map(|channel| channel.id), channel_members, stale_participant_user_ids, canceled_calls_to_user_ids, @@ -381,7 +381,6 @@ impl Database { pub(crate) async fn join_channel_room_internal( &self, - channel_id: ChannelId, room_id: RoomId, user_id: UserId, connection: ConnectionId, @@ -420,11 +419,12 @@ impl Database { .exec(&*tx) .await?; - let room = self.get_room(room_id, &tx).await?; - let channel_members = self.get_channel_participants(channel_id, &tx).await?; + let (channel, room) = self.get_channel_room(room_id, &tx).await?; + let channel = channel.ok_or_else(|| anyhow!("no channel for room"))?; + let channel_members = self.get_channel_participants(&channel, &*tx).await?; Ok(JoinRoom { room, - channel_id: Some(channel_id), + channel_id: Some(channel.id), channel_members, }) } @@ -718,16 +718,16 @@ impl Database { }); } - let (channel_id, room) = self.get_channel_room(room_id, &tx).await?; - let channel_members = if let Some(channel_id) = channel_id { - self.get_channel_participants(channel_id, &tx).await? + let (channel, room) = self.get_channel_room(room_id, &tx).await?; + let channel_members = if let Some(channel) = &channel { + self.get_channel_participants(&channel, &tx).await? } else { Vec::new() }; Ok(RejoinedRoom { room, - channel_id, + channel_id: channel.map(|channel| channel.id), channel_members, rejoined_projects, reshared_projects, @@ -869,7 +869,7 @@ impl Database { .exec(&*tx) .await?; - let (channel_id, room) = self.get_channel_room(room_id, &tx).await?; + let (channel, room) = self.get_channel_room(room_id, &tx).await?; let deleted = if room.participants.is_empty() { let result = room::Entity::delete_by_id(room_id).exec(&*tx).await?; result.rows_affected > 0 @@ -877,14 +877,14 @@ impl Database { false }; - let channel_members = if let Some(channel_id) = channel_id { - self.get_channel_participants(channel_id, &tx).await? + let channel_members = if let Some(channel) = &channel { + self.get_channel_participants(channel, &tx).await? } else { Vec::new() }; let left_room = LeftRoom { room, - channel_id, + channel_id: channel.map(|channel| channel.id), channel_members, left_projects, canceled_calls_to_user_ids, @@ -1072,7 +1072,7 @@ impl Database { &self, room_id: RoomId, tx: &DatabaseTransaction, - ) -> Result<(Option, proto::Room)> { + ) -> Result<(Option, proto::Room)> { let db_room = room::Entity::find_by_id(room_id) .one(tx) .await? @@ -1181,9 +1181,16 @@ impl Database { project_id: db_follower.project_id.to_proto(), }); } + drop(db_followers); + + let channel = if let Some(channel_id) = db_room.channel_id { + Some(self.get_channel_internal(channel_id, &*tx).await?) + } else { + None + }; Ok(( - db_room.channel_id, + channel, proto::Room { id: db_room.id.to_proto(), live_kit_room: db_room.live_kit_room, diff --git a/crates/collab/src/db/tables.rs b/crates/collab/src/db/tables.rs index 0acb266d9d..4f28ce4fbd 100644 --- a/crates/collab/src/db/tables.rs +++ b/crates/collab/src/db/tables.rs @@ -8,7 +8,6 @@ pub mod channel_chat_participant; pub mod channel_member; pub mod channel_message; pub mod channel_message_mention; -pub mod channel_path; pub mod contact; pub mod feature_flag; pub mod follower; diff --git a/crates/collab/src/db/tables/channel.rs b/crates/collab/src/db/tables/channel.rs index 0975a8cc30..e30ec9af61 100644 --- a/crates/collab/src/db/tables/channel.rs +++ b/crates/collab/src/db/tables/channel.rs @@ -8,6 +8,28 @@ pub struct Model { pub id: ChannelId, pub name: String, pub visibility: ChannelVisibility, + pub parent_path: String, +} + +impl Model { + pub fn parent_id(&self) -> Option { + self.ancestors().last() + } + + pub fn ancestors(&self) -> impl Iterator + '_ { + self.parent_path + .trim_end_matches('/') + .split('/') + .filter_map(|id| Some(ChannelId::from_proto(id.parse().ok()?))) + } + + pub fn ancestors_including_self(&self) -> impl Iterator + '_ { + self.ancestors().chain(Some(self.id)) + } + + pub fn path(&self) -> String { + format!("{}{}/", self.parent_path, self.id) + } } impl ActiveModelBehavior for ActiveModel {} diff --git a/crates/collab/src/db/tables/channel_path.rs b/crates/collab/src/db/tables/channel_path.rs deleted file mode 100644 index 323f116dae..0000000000 --- a/crates/collab/src/db/tables/channel_path.rs +++ /dev/null @@ -1,15 +0,0 @@ -use crate::db::ChannelId; -use sea_orm::entity::prelude::*; - -#[derive(Clone, Debug, Default, PartialEq, Eq, DeriveEntityModel)] -#[sea_orm(table_name = "channel_paths")] -pub struct Model { - #[sea_orm(primary_key)] - pub id_path: String, - pub channel_id: ChannelId, -} - -impl ActiveModelBehavior for ActiveModel {} - -#[derive(Copy, Clone, Debug, EnumIter, DeriveRelation)] -pub enum Relation {} diff --git a/crates/collab/src/db/tests.rs b/crates/collab/src/db/tests.rs index 8f2939040d..b6a89ff6f8 100644 --- a/crates/collab/src/db/tests.rs +++ b/crates/collab/src/db/tests.rs @@ -7,7 +7,6 @@ mod message_tests; use super::*; use gpui::executor::Background; use parking_lot::Mutex; -use rpc::proto::ChannelEdge; use sea_orm::ConnectionTrait; use sqlx::migrate::MigrateDatabase; use std::sync::{ @@ -153,33 +152,17 @@ impl Drop for TestDb { } } -/// The second tuples are (channel_id, parent) -fn graph( - channels: &[(ChannelId, &'static str, ChannelRole)], - edges: &[(ChannelId, ChannelId)], -) -> ChannelGraph { - let mut graph = ChannelGraph { - channels: vec![], - edges: vec![], - }; - - for (id, name, role) in channels { - graph.channels.push(Channel { +fn channel_tree(channels: &[(ChannelId, &[ChannelId], &'static str, ChannelRole)]) -> Vec { + channels + .iter() + .map(|(id, parent_path, name, role)| Channel { id: *id, name: name.to_string(), visibility: ChannelVisibility::Members, role: *role, + parent_path: parent_path.to_vec(), }) - } - - for (channel, parent) in edges { - graph.edges.push(ChannelEdge { - channel_id: channel.to_proto(), - parent_id: parent.to_proto(), - }) - } - - graph + .collect() } static GITHUB_USER_ID: AtomicI32 = AtomicI32::new(5); diff --git a/crates/collab/src/db/tests/channel_tests.rs b/crates/collab/src/db/tests/channel_tests.rs index 32cba2fdd9..43526c7f24 100644 --- a/crates/collab/src/db/tests/channel_tests.rs +++ b/crates/collab/src/db/tests/channel_tests.rs @@ -1,18 +1,15 @@ -use std::sync::Arc; - use crate::{ db::{ - queries::channels::ChannelGraph, - tests::{graph, new_test_connection, new_test_user, TEST_RELEASE_CHANNEL}, - ChannelId, ChannelRole, Database, NewUserParams, RoomId, + tests::{channel_tree, new_test_connection, new_test_user, TEST_RELEASE_CHANNEL}, + Channel, ChannelId, ChannelRole, Database, NewUserParams, RoomId, }, test_both_dbs, }; -use collections::{HashMap, HashSet}; use rpc::{ proto::{self}, ConnectionId, }; +use std::sync::Arc; test_both_dbs!(test_channels, test_channels_postgres, test_channels_sqlite); @@ -44,7 +41,10 @@ async fn test_channels(db: &Arc) { .unwrap(); let mut members = db - .transaction(|tx| async move { Ok(db.get_channel_participants(replace_id, &*tx).await?) }) + .transaction(|tx| async move { + let channel = db.get_channel_internal(replace_id, &*tx).await?; + Ok(db.get_channel_participants(&channel, &*tx).await?) + }) .await .unwrap(); members.sort(); @@ -61,42 +61,41 @@ async fn test_channels(db: &Arc) { let result = db.get_channels_for_user(a_id).await.unwrap(); assert_eq!( result.channels, - graph( - &[ - (zed_id, "zed", ChannelRole::Admin), - (crdb_id, "crdb", ChannelRole::Admin), - (livestreaming_id, "livestreaming", ChannelRole::Admin), - (replace_id, "replace", ChannelRole::Admin), - (rust_id, "rust", ChannelRole::Admin), - (cargo_id, "cargo", ChannelRole::Admin), - (cargo_ra_id, "cargo-ra", ChannelRole::Admin) - ], - &[ - (crdb_id, zed_id), - (livestreaming_id, zed_id), - (replace_id, zed_id), - (cargo_id, rust_id), - (cargo_ra_id, cargo_id), - ] - ) + channel_tree(&[ + (zed_id, &[], "zed", ChannelRole::Admin), + (crdb_id, &[zed_id], "crdb", ChannelRole::Admin), + ( + livestreaming_id, + &[zed_id], + "livestreaming", + ChannelRole::Admin + ), + (replace_id, &[zed_id], "replace", ChannelRole::Admin), + (rust_id, &[], "rust", ChannelRole::Admin), + (cargo_id, &[rust_id], "cargo", ChannelRole::Admin), + ( + cargo_ra_id, + &[rust_id, cargo_id], + "cargo-ra", + ChannelRole::Admin + ) + ],) ); let result = db.get_channels_for_user(b_id).await.unwrap(); assert_eq!( result.channels, - graph( - &[ - (zed_id, "zed", ChannelRole::Member), - (crdb_id, "crdb", ChannelRole::Member), - (livestreaming_id, "livestreaming", ChannelRole::Member), - (replace_id, "replace", ChannelRole::Member) - ], - &[ - (crdb_id, zed_id), - (livestreaming_id, zed_id), - (replace_id, zed_id) - ] - ) + channel_tree(&[ + (zed_id, &[], "zed", ChannelRole::Member), + (crdb_id, &[zed_id], "crdb", ChannelRole::Member), + ( + livestreaming_id, + &[zed_id], + "livestreaming", + ChannelRole::Member + ), + (replace_id, &[zed_id], "replace", ChannelRole::Member) + ],) ); // Update member permissions @@ -112,19 +111,17 @@ async fn test_channels(db: &Arc) { let result = db.get_channels_for_user(b_id).await.unwrap(); assert_eq!( result.channels, - graph( - &[ - (zed_id, "zed", ChannelRole::Admin), - (crdb_id, "crdb", ChannelRole::Admin), - (livestreaming_id, "livestreaming", ChannelRole::Admin), - (replace_id, "replace", ChannelRole::Admin) - ], - &[ - (crdb_id, zed_id), - (livestreaming_id, zed_id), - (replace_id, zed_id) - ] - ) + channel_tree(&[ + (zed_id, &[], "zed", ChannelRole::Admin), + (crdb_id, &[zed_id], "crdb", ChannelRole::Admin), + ( + livestreaming_id, + &[zed_id], + "livestreaming", + ChannelRole::Admin + ), + (replace_id, &[zed_id], "replace", ChannelRole::Admin) + ],) ); // Remove a single channel @@ -327,14 +324,10 @@ async fn test_channel_renames(db: &Arc) { .await .unwrap(); - let zed_archive_id = zed_id; - - let channel = db.get_channel(zed_archive_id, user_1).await.unwrap(); + let channel = db.get_channel(zed_id, user_1).await.unwrap(); assert_eq!(channel.name, "zed-archive"); - let non_permissioned_rename = db - .rename_channel(zed_archive_id, user_2, "hacked-lol") - .await; + let non_permissioned_rename = db.rename_channel(zed_id, user_2, "hacked-lol").await; assert!(non_permissioned_rename.is_err()); let bad_name_rename = db.rename_channel(zed_id, user_1, "#").await; @@ -383,328 +376,16 @@ async fn test_db_channel_moving(db: &Arc) { // /- gpui2 // zed -- crdb - livestreaming - livestreaming_dag let result = db.get_channels_for_user(a_id).await.unwrap(); - assert_dag( + assert_channel_tree( result.channels, &[ - (zed_id, None), - (crdb_id, Some(zed_id)), - (gpui2_id, Some(zed_id)), - (livestreaming_id, Some(crdb_id)), - (livestreaming_dag_id, Some(livestreaming_id)), + (zed_id, &[]), + (crdb_id, &[zed_id]), + (livestreaming_id, &[zed_id, crdb_id]), + (livestreaming_dag_id, &[zed_id, crdb_id, livestreaming_id]), + (gpui2_id, &[zed_id]), ], ); - - // Attempt to make a cycle - assert!(db - .link_channel(a_id, zed_id, livestreaming_id) - .await - .is_err()); - - // // ======================================================================== - // // Make a link - // db.link_channel(a_id, livestreaming_id, zed_id) - // .await - // .unwrap(); - - // // DAG is now: - // // /- gpui2 - // // zed -- crdb - livestreaming - livestreaming_dag - // // \---------/ - // 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(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("livestreaming_dag_sub", Some(livestreaming_dag_id), a_id) - // .await - // .unwrap(); - - // // DAG is now: - // // /- gpui2 - // // zed -- crdb - livestreaming - livestreaming_dag - livestreaming_dag_sub_id - // // \---------/ - // 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(zed_id)), - // (livestreaming_id, Some(crdb_id)), - // (livestreaming_dag_id, Some(livestreaming_id)), - // (livestreaming_dag_sub_id, Some(livestreaming_dag_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(); - - // // DAG is now: - // // /- gpui2 /---------------------\ - // // zed - crdb - livestreaming - livestreaming_dag - livestreaming_dag_sub_id - // // \--------/ - - // // 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!( - // returned_channels, - // graph( - // &[( - // livestreaming_dag_sub_id, - // "livestreaming_dag_sub", - // ChannelRole::Admin - // )], - // &[(livestreaming_dag_sub_id, livestreaming_id)] - // ) - // ); - - // 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(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 a complex DAG by making another link - // let returned_channels = db - // .link_channel(a_id, livestreaming_id, gpui2_id) - // .await - // .unwrap(); - - // // DAG is now: - // // /- gpui2 -\ /---------------------\ - // // zed - crdb -- livestreaming - livestreaming_dag - livestreaming_dag_sub_id - // // \---------/ - - // // Make sure that we're correctly getting the full sub-dag - // pretty_assertions::assert_eq!( - // returned_channels, - // graph( - // &[ - // (livestreaming_id, "livestreaming", ChannelRole::Admin), - // ( - // livestreaming_dag_id, - // "livestreaming_dag", - // ChannelRole::Admin - // ), - // ( - // livestreaming_dag_sub_id, - // "livestreaming_dag_sub", - // ChannelRole::Admin - // ), - // ], - // &[ - // (livestreaming_id, gpui2_id), - // (livestreaming_dag_id, livestreaming_id), - // (livestreaming_dag_sub_id, livestreaming_id), - // (livestreaming_dag_sub_id, livestreaming_dag_id), - // ] - // ) - // ); - - // 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(zed_id)), - // (livestreaming_id, Some(crdb_id)), - // (livestreaming_id, Some(gpui2_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, livestreaming_id) - // .await - // .unwrap(); - - // // DAG is now: - // // /- gpui2 -\ - // // 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)), - // (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)), - // ], - // ); - - // // ======================================================================== - // // Test unlinking in a complex DAG by removing the inner link - // db.unlink_channel(a_id, livestreaming_id, gpui2_id) - // .await - // .unwrap(); - - // // DAG is now: - // // /- gpui2 - // // 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)), - // (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)), - // ], - // ); - - // // ======================================================================== - // // Test moving DAG nodes by moving livestreaming to be below gpui2 - // db.move_channel(livestreaming_id, Some(crdb_id), gpui2_id, a_id) - // .await - // .unwrap(); - - // // DAG is now: - // // /- gpui2 -- livestreaming - livestreaming_dag - livestreaming_dag_sub - // // zed - crdb / - // // \---------/ - // 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(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(); - - // // 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)), - // ], - // ); - - // // ======================================================================== - // // Unlinking a channel from it's parent should automatically promote it to a root channel - // db.unlink_channel(a_id, crdb_id, 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)), - // ], - // ); - - // // ======================================================================== - // // You should be able to move a root channel into a non-root channel - // db.link_channel(a_id, crdb_id, 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)), - // ], - // ); - - // // ======================================================================== - // // Prep for DAG deletion test - // db.link_channel(a_id, livestreaming_id, 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 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()) } test_both_dbs!( @@ -743,37 +424,32 @@ async fn test_db_channel_moving_bugs(db: &Arc) { // Move to same parent should be a no-op assert!(db - .move_channel(projects_id, Some(zed_id), zed_id, user_id) + .move_channel(projects_id, Some(zed_id), user_id) .await .unwrap() .is_none()); let result = db.get_channels_for_user(user_id).await.unwrap(); - assert_dag( + assert_channel_tree( result.channels, &[ - (zed_id, None), - (projects_id, Some(zed_id)), - (livestreaming_id, Some(projects_id)), + (zed_id, &[]), + (projects_id, &[zed_id]), + (livestreaming_id, &[zed_id, projects_id]), ], ); - // Stranding a channel should retain it's sub channels - // Commented out as we don't fix permissions when this happens yet. - // - // 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)), - // ], - // ); + // Move the project channel to the root + db.move_channel(projects_id, None, user_id).await.unwrap(); + let result = db.get_channels_for_user(user_id).await.unwrap(); + assert_channel_tree( + result.channels, + &[ + (zed_id, &[]), + (projects_id, &[]), + (livestreaming_id, &[projects_id]), + ], + ); } test_both_dbs!( @@ -788,44 +464,52 @@ async fn test_user_is_channel_participant(db: &Arc) { let guest = new_test_user(db, "guest@example.com").await; let zed_channel = db.create_root_channel("zed", admin).await.unwrap(); - let active_channel = db + let active_channel_id = db .create_sub_channel("active", zed_channel, admin) .await .unwrap(); - let vim_channel = db - .create_sub_channel("vim", active_channel, admin) + let vim_channel_id = db + .create_sub_channel("vim", active_channel_id, admin) .await .unwrap(); - db.set_channel_visibility(vim_channel, crate::db::ChannelVisibility::Public, admin) + db.set_channel_visibility(vim_channel_id, crate::db::ChannelVisibility::Public, admin) .await .unwrap(); - db.invite_channel_member(active_channel, member, admin, ChannelRole::Member) + db.invite_channel_member(active_channel_id, member, admin, ChannelRole::Member) .await .unwrap(); - db.invite_channel_member(vim_channel, guest, admin, ChannelRole::Guest) + db.invite_channel_member(vim_channel_id, guest, admin, ChannelRole::Guest) .await .unwrap(); - db.respond_to_channel_invite(active_channel, member, true) + db.respond_to_channel_invite(active_channel_id, member, true) .await .unwrap(); db.transaction(|tx| async move { - db.check_user_is_channel_participant(vim_channel, admin, &*tx) - .await + db.check_user_is_channel_participant( + &db.get_channel_internal(vim_channel_id, &*tx).await?, + admin, + &*tx, + ) + .await }) .await .unwrap(); db.transaction(|tx| async move { - db.check_user_is_channel_participant(vim_channel, member, &*tx) - .await + db.check_user_is_channel_participant( + &db.get_channel_internal(vim_channel_id, &*tx).await?, + member, + &*tx, + ) + .await }) .await .unwrap(); let mut members = db - .get_channel_participant_details(vim_channel, admin) + .get_channel_participant_details(vim_channel_id, admin) .await .unwrap(); @@ -852,38 +536,49 @@ async fn test_user_is_channel_participant(db: &Arc) { ] ); - db.respond_to_channel_invite(vim_channel, guest, true) + db.respond_to_channel_invite(vim_channel_id, guest, true) .await .unwrap(); db.transaction(|tx| async move { - db.check_user_is_channel_participant(vim_channel, guest, &*tx) - .await + db.check_user_is_channel_participant( + &db.get_channel_internal(vim_channel_id, &*tx).await?, + guest, + &*tx, + ) + .await }) .await .unwrap(); let channels = db.get_channels_for_user(guest).await.unwrap().channels; - assert_dag(channels, &[(vim_channel, None)]); + assert_channel_tree(channels, &[(vim_channel_id, &[])]); let channels = db.get_channels_for_user(member).await.unwrap().channels; - assert_dag( + assert_channel_tree( channels, - &[(active_channel, None), (vim_channel, Some(active_channel))], + &[ + (active_channel_id, &[]), + (vim_channel_id, &[active_channel_id]), + ], ); - db.set_channel_member_role(vim_channel, admin, guest, ChannelRole::Banned) + db.set_channel_member_role(vim_channel_id, admin, guest, ChannelRole::Banned) .await .unwrap(); assert!(db .transaction(|tx| async move { - db.check_user_is_channel_participant(vim_channel, guest, &*tx) - .await + db.check_user_is_channel_participant( + &db.get_channel_internal(vim_channel_id, &*tx).await.unwrap(), + guest, + &*tx, + ) + .await }) .await .is_err()); let mut members = db - .get_channel_participant_details(vim_channel, admin) + .get_channel_participant_details(vim_channel_id, admin) .await .unwrap(); @@ -910,7 +605,7 @@ async fn test_user_is_channel_participant(db: &Arc) { ] ); - db.remove_channel_member(vim_channel, guest, admin) + db.remove_channel_member(vim_channel_id, guest, admin) .await .unwrap(); @@ -924,7 +619,7 @@ async fn test_user_is_channel_participant(db: &Arc) { // currently people invited to parent channels are not shown here let mut members = db - .get_channel_participant_details(vim_channel, admin) + .get_channel_participant_details(vim_channel_id, admin) .await .unwrap(); @@ -951,28 +646,42 @@ async fn test_user_is_channel_participant(db: &Arc) { .unwrap(); db.transaction(|tx| async move { - db.check_user_is_channel_participant(zed_channel, guest, &*tx) - .await + db.check_user_is_channel_participant( + &db.get_channel_internal(zed_channel, &*tx).await.unwrap(), + guest, + &*tx, + ) + .await }) .await .unwrap(); assert!(db .transaction(|tx| async move { - db.check_user_is_channel_participant(active_channel, guest, &*tx) - .await + db.check_user_is_channel_participant( + &db.get_channel_internal(active_channel_id, &*tx) + .await + .unwrap(), + guest, + &*tx, + ) + .await }) .await .is_err(),); db.transaction(|tx| async move { - db.check_user_is_channel_participant(vim_channel, guest, &*tx) - .await + db.check_user_is_channel_participant( + &db.get_channel_internal(vim_channel_id, &*tx).await.unwrap(), + guest, + &*tx, + ) + .await }) .await .unwrap(); let mut members = db - .get_channel_participant_details(vim_channel, admin) + .get_channel_participant_details(vim_channel_id, admin) .await .unwrap(); @@ -1000,9 +709,9 @@ async fn test_user_is_channel_participant(db: &Arc) { ); let channels = db.get_channels_for_user(guest).await.unwrap().channels; - assert_dag( + assert_channel_tree( channels, - &[(zed_channel, None), (vim_channel, Some(zed_channel))], + &[(zed_channel, &[]), (vim_channel_id, &[zed_channel])], ) } @@ -1047,15 +756,20 @@ async fn test_user_joins_correct_channel(db: &Arc) { let most_public = db .transaction(|tx| async move { Ok(db - .public_path_to_channel(vim_channel, &tx) + .public_ancestors_including_self( + &db.get_channel_internal(vim_channel, &*tx).await.unwrap(), + &tx, + ) .await? .first() .cloned()) }) .await - .unwrap(); + .unwrap() + .unwrap() + .id; - assert_eq!(most_public, Some(zed_channel)) + assert_eq!(most_public, zed_channel) } test_both_dbs!( @@ -1092,26 +806,14 @@ async fn test_guest_access(db: &Arc) { } #[track_caller] -fn assert_dag(actual: ChannelGraph, expected: &[(ChannelId, Option)]) { - let mut actual_map: HashMap> = HashMap::default(); - for channel in actual.channels { - actual_map.insert(channel.id, HashSet::default()); - } - for edge in actual.edges { - actual_map - .get_mut(&ChannelId::from_proto(edge.channel_id)) - .unwrap() - .insert(ChannelId::from_proto(edge.parent_id)); - } - - let mut expected_map: HashMap> = HashMap::default(); - - for (child, parent) in expected { - let entry = expected_map.entry(*child).or_default(); - if let Some(parent) = parent { - entry.insert(*parent); - } - } - - pretty_assertions::assert_eq!(actual_map, expected_map) +fn assert_channel_tree(actual: Vec, expected: &[(ChannelId, &[ChannelId])]) { + let actual = actual + .iter() + .map(|channel| (channel.id, channel.parent_path.as_slice())) + .collect::>(); + pretty_assertions::assert_eq!( + actual, + expected.to_vec(), + "wrong channel ids and parent paths" + ); } diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index dda638e107..a0ec7da392 100644 --- a/crates/collab/src/rpc.rs +++ b/crates/collab/src/rpc.rs @@ -277,8 +277,6 @@ impl Server { .add_request_handler(get_channel_messages_by_id) .add_request_handler(get_notifications) .add_request_handler(mark_notification_as_read) - .add_request_handler(link_channel) - .add_request_handler(unlink_channel) .add_request_handler(move_channel) .add_request_handler(follow) .add_message_handler(unfollow) @@ -2472,52 +2470,19 @@ async fn rename_channel( Ok(()) } -// TODO: Implement in terms of symlinks -// Current behavior of this is more like 'Move root channel' -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 result = db - .move_channel(channel_id, None, to, session.user_id) - .await?; - drop(db); - - notify_channel_moved(result, session).await?; - - response.send(Ack {})?; - - Ok(()) -} - -// TODO: Implement in terms of symlinks -async fn unlink_channel( - _request: proto::UnlinkChannel, - _response: Response, - _session: Session, -) -> Result<()> { - Err(anyhow!("unimplemented").into()) -} - async fn move_channel( request: proto::MoveChannel, response: Response, session: Session, ) -> Result<()> { - let db = session.db().await; let channel_id = ChannelId::from_proto(request.channel_id); - let from_parent = ChannelId::from_proto(request.from); - let to = ChannelId::from_proto(request.to); + let to = request.to.map(ChannelId::from_proto); - let result = db - .move_channel(channel_id, Some(from_parent), to, session.user_id) + let result = session + .db() + .await + .move_channel(channel_id, to, session.user_id) .await?; - drop(db); notify_channel_moved(result, session).await?; @@ -3244,18 +3209,12 @@ fn build_channels_update( ) -> proto::UpdateChannels { let mut update = proto::UpdateChannels::default(); - for channel in channels.channels.channels { - update.channels.push(proto::Channel { - id: channel.id.to_proto(), - name: channel.name, - visibility: channel.visibility.into(), - role: channel.role.into(), - }); + for channel in channels.channels { + update.channels.push(channel.to_proto()); } update.unseen_channel_buffer_changes = channels.unseen_buffer_changes; update.unseen_channel_messages = channels.channel_messages; - update.insert_edge = channels.channels.edges; for (channel_id, participants) in channels.channel_participants { update @@ -3267,12 +3226,7 @@ fn build_channels_update( } for channel in channel_invites { - update.channel_invitations.push(proto::Channel { - id: channel.id.to_proto(), - name: channel.name, - visibility: channel.visibility.into(), - role: channel.role.into(), - }); + update.channel_invitations.push(channel.to_proto()); } update diff --git a/crates/collab/src/tests/channel_buffer_tests.rs b/crates/collab/src/tests/channel_buffer_tests.rs index 931610f5ff..5ca40a3c2d 100644 --- a/crates/collab/src/tests/channel_buffer_tests.rs +++ b/crates/collab/src/tests/channel_buffer_tests.rs @@ -3,7 +3,7 @@ use crate::{ tests::TestServer, }; use call::ActiveCall; -use channel::{Channel, ACKNOWLEDGE_DEBOUNCE_INTERVAL}; +use channel::ACKNOWLEDGE_DEBOUNCE_INTERVAL; use client::ParticipantIndex; use client::{Collaborator, UserId}; use collab_ui::channel_view::ChannelView; @@ -11,10 +11,7 @@ use collections::HashMap; use editor::{Anchor, Editor, ToOffset}; use futures::future; use gpui::{executor::Deterministic, ModelHandle, TestAppContext, ViewContext}; -use rpc::{ - proto::{self, PeerId}, - RECEIVE_TIMEOUT, -}; +use rpc::{proto::PeerId, RECEIVE_TIMEOUT}; use serde_json::json; use std::{ops::Range, sync::Arc}; @@ -411,10 +408,7 @@ async fn test_channel_buffer_disconnect( deterministic.advance_clock(RECEIVE_TIMEOUT + RECONNECT_TIMEOUT); channel_buffer_a.update(cx_a, |buffer, cx| { - assert_eq!( - buffer.channel(cx).unwrap().as_ref(), - &channel(channel_id, "the-channel", proto::ChannelRole::Admin) - ); + assert_eq!(buffer.channel(cx).unwrap().name, "the-channel"); assert!(!buffer.is_connected()); }); @@ -441,17 +435,6 @@ async fn test_channel_buffer_disconnect( }); } -fn channel(id: u64, name: &'static str, role: proto::ChannelRole) -> Channel { - Channel { - id, - role, - name: name.to_string(), - visibility: proto::ChannelVisibility::Members, - unseen_note_version: None, - unseen_message_id: None, - } -} - #[gpui::test] async fn test_rejoin_channel_buffer( deterministic: Arc, diff --git a/crates/collab/src/tests/channel_tests.rs b/crates/collab/src/tests/channel_tests.rs index 5b0bf02f8f..a33ded6492 100644 --- a/crates/collab/src/tests/channel_tests.rs +++ b/crates/collab/src/tests/channel_tests.rs @@ -61,10 +61,7 @@ async fn test_core_channels( ); client_b.channel_store().read_with(cx_b, |channels, _| { - assert!(channels - .channel_dag_entries() - .collect::>() - .is_empty()) + assert!(channels.ordered_channels().collect::>().is_empty()) }); // Invite client B to channel A as client A. @@ -1019,7 +1016,7 @@ async fn test_channel_link_notifications( client_a .channel_store() .update(cx_a, |channel_store, cx| { - channel_store.link_channel(vim_channel, active_channel, cx) + channel_store.move_channel(vim_channel, Some(active_channel), cx) }) .await .unwrap(); @@ -1054,7 +1051,7 @@ async fn test_channel_link_notifications( client_a .channel_store() .update(cx_a, |channel_store, cx| { - channel_store.link_channel(helix_channel, vim_channel, cx) + channel_store.move_channel(helix_channel, Some(vim_channel), cx) }) .await .unwrap(); @@ -1427,7 +1424,7 @@ async fn test_channel_moving( client_a .channel_store() .update(cx_a, |channel_store, cx| { - channel_store.move_channel(channel_d_id, channel_c_id, channel_b_id, cx) + channel_store.move_channel(channel_d_id, Some(channel_b_id), cx) }) .await .unwrap(); @@ -1445,189 +1442,6 @@ async fn test_channel_moving( (channel_d_id, 2), ], ); - - // TODO: Restore this test once we have a way to make channel symlinks - // client_a - // .channel_store() - // .update(cx_a, |channel_store, cx| { - // channel_store.link_channel(channel_d_id, channel_c_id, cx) - // }) - // .await - // .unwrap(); - - // // Current shape for A: - // // /------\ - // // a - b -- c -- d - // assert_channels_list_shape( - // client_a.channel_store(), - // cx_a, - // &[ - // (channel_a_id, 0), - // (channel_b_id, 1), - // (channel_c_id, 2), - // (channel_d_id, 3), - // (channel_d_id, 2), - // ], - // ); - // - // let b_channels = server - // .make_channel_tree( - // &[ - // ("channel-mu", None), - // ("channel-gamma", Some("channel-mu")), - // ("channel-epsilon", Some("channel-mu")), - // ], - // (&client_b, cx_b), - // ) - // .await; - // 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 - // // mu -- ga - // assert_channels_list_shape( - // client_b.channel_store(), - // cx_b, - // &[(channel_mu_id, 0), (channel_ep_id, 1), (channel_ga_id, 1)], - // ); - - // client_a - // .add_admin_to_channel((&client_b, cx_b), channel_b_id, cx_a) - // .await; - - // // Current shape for B: - // // /- ep - // // mu -- ga - // // /---------\ - // // b -- c -- d - // assert_channels_list_shape( - // client_b.channel_store(), - // cx_b, - // &[ - // // New channels from a - // (channel_b_id, 0), - // (channel_c_id, 1), - // (channel_d_id, 2), - // (channel_d_id, 1), - // // B's old channels - // (channel_mu_id, 0), - // (channel_ep_id, 1), - // (channel_ga_id, 1), - // ], - // ); - - // client_b - // .add_admin_to_channel((&client_c, cx_c), channel_ep_id, cx_b) - // .await; - - // // Current shape for C: - // // - ep - // assert_channels_list_shape(client_c.channel_store(), cx_c, &[(channel_ep_id, 0)]); - - // client_b - // .channel_store() - // .update(cx_b, |channel_store, cx| { - // channel_store.link_channel(channel_b_id, channel_ep_id, cx) - // }) - // .await - // .unwrap(); - - // // Current shape for B: - // // /---------\ - // // /- ep -- b -- c -- d - // // mu -- ga - // assert_channels_list_shape( - // client_b.channel_store(), - // cx_b, - // &[ - // (channel_mu_id, 0), - // (channel_ep_id, 1), - // (channel_b_id, 2), - // (channel_c_id, 3), - // (channel_d_id, 4), - // (channel_d_id, 3), - // (channel_ga_id, 1), - // ], - // ); - - // // Current shape for C: - // // /---------\ - // // ep -- b -- c -- d - // assert_channels_list_shape( - // client_c.channel_store(), - // cx_c, - // &[ - // (channel_ep_id, 0), - // (channel_b_id, 1), - // (channel_c_id, 2), - // (channel_d_id, 3), - // (channel_d_id, 2), - // ], - // ); - - // client_b - // .channel_store() - // .update(cx_b, |channel_store, cx| { - // channel_store.link_channel(channel_ga_id, channel_b_id, cx) - // }) - // .await - // .unwrap(); - - // // Current shape for B: - // // /---------\ - // // /- ep -- b -- c -- d - // // / \ - // // mu ---------- ga - // assert_channels_list_shape( - // client_b.channel_store(), - // cx_b, - // &[ - // (channel_mu_id, 0), - // (channel_ep_id, 1), - // (channel_b_id, 2), - // (channel_c_id, 3), - // (channel_d_id, 4), - // (channel_d_id, 3), - // (channel_ga_id, 3), - // (channel_ga_id, 1), - // ], - // ); - - // // Current shape for A: - // // /------\ - // // a - b -- c -- d - // // \-- ga - // assert_channels_list_shape( - // client_a.channel_store(), - // cx_a, - // &[ - // (channel_a_id, 0), - // (channel_b_id, 1), - // (channel_c_id, 2), - // (channel_d_id, 3), - // (channel_d_id, 2), - // (channel_ga_id, 2), - // ], - // ); - - // // Current shape for C: - // // /-------\ - // // ep -- b -- c -- d - // // \-- ga - // assert_channels_list_shape( - // client_c.channel_store(), - // cx_c, - // &[ - // (channel_ep_id, 0), - // (channel_b_id, 1), - // (channel_c_id, 2), - // (channel_d_id, 3), - // (channel_d_id, 2), - // (channel_ga_id, 2), - // ], - // ); } #[derive(Debug, PartialEq)] @@ -1667,7 +1481,7 @@ fn assert_channels( ) { let actual = channel_store.read_with(cx, |store, _| { store - .channel_dag_entries() + .ordered_channels() .map(|(depth, channel)| ExpectedChannel { depth, name: channel.name.clone(), @@ -1689,7 +1503,7 @@ fn assert_channels_list_shape( let actual = channel_store.read_with(cx, |store, _| { store - .channel_dag_entries() + .ordered_channels() .map(|(depth, channel)| (channel.id, depth)) .collect::>() }); diff --git a/crates/collab/src/tests/random_channel_buffer_tests.rs b/crates/collab/src/tests/random_channel_buffer_tests.rs index 64fecd5e62..38bc3f7c12 100644 --- a/crates/collab/src/tests/random_channel_buffer_tests.rs +++ b/crates/collab/src/tests/random_channel_buffer_tests.rs @@ -83,7 +83,7 @@ impl RandomizedTest for RandomChannelBufferTest { match rng.gen_range(0..100_u32) { 0..=29 => { let channel_name = client.channel_store().read_with(cx, |store, cx| { - store.channel_dag_entries().find_map(|(_, channel)| { + store.ordered_channels().find_map(|(_, channel)| { if store.has_open_channel_buffer(channel.id, cx) { None } else { @@ -131,7 +131,7 @@ impl RandomizedTest for RandomChannelBufferTest { ChannelBufferOperation::JoinChannelNotes { channel_name } => { let buffer = client.channel_store().update(cx, |store, cx| { let channel_id = store - .channel_dag_entries() + .ordered_channels() .find(|(_, c)| c.name == channel_name) .unwrap() .1 diff --git a/crates/collab_ui/src/collab_panel.rs b/crates/collab_ui/src/collab_panel.rs index 8c33727cfb..8d68ee12c0 100644 --- a/crates/collab_ui/src/collab_panel.rs +++ b/crates/collab_ui/src/collab_panel.rs @@ -9,7 +9,7 @@ use crate::{ }; use anyhow::Result; use call::ActiveCall; -use channel::{Channel, ChannelData, ChannelEvent, ChannelId, ChannelPath, ChannelStore}; +use channel::{Channel, ChannelEvent, ChannelId, ChannelStore}; use channel_modal::ChannelModal; use client::{ proto::{self, PeerId}, @@ -55,17 +55,17 @@ use workspace::{ #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] struct ToggleCollapse { - location: ChannelPath, + location: ChannelId, } #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] struct NewChannel { - location: ChannelPath, + location: ChannelId, } #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] struct RenameChannel { - location: ChannelPath, + channel_id: ChannelId, } #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] @@ -111,13 +111,6 @@ pub struct CopyChannelLink { #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] struct StartMoveChannelFor { channel_id: ChannelId, - parent_id: Option, -} - -#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] -struct StartLinkChannelFor { - channel_id: ChannelId, - parent_id: Option, } #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] @@ -125,8 +118,6 @@ struct MoveChannel { to: ChannelId, } -type DraggedChannel = (Channel, Option); - actions!( collab_panel, [ @@ -163,14 +154,6 @@ impl_actions!( #[derive(Debug, Copy, Clone, PartialEq, Eq)] struct ChannelMoveClipboard { channel_id: ChannelId, - parent_id: Option, - intent: ClipboardIntent, -} - -#[derive(Debug, Copy, Clone, PartialEq, Eq)] -enum ClipboardIntent { - Move, - // Link, } const COLLABORATION_PANEL_KEY: &'static str = "CollaborationPanel"; @@ -217,19 +200,15 @@ pub fn init(cx: &mut AppContext) { _: &mut ViewContext| { panel.channel_clipboard = Some(ChannelMoveClipboard { channel_id: action.channel_id, - parent_id: action.parent_id, - intent: ClipboardIntent::Move, }); }, ); cx.add_action( |panel: &mut CollabPanel, _: &StartMoveChannel, _: &mut ViewContext| { - if let Some((_, path)) = panel.selected_channel() { + if let Some(channel) = panel.selected_channel() { panel.channel_clipboard = Some(ChannelMoveClipboard { - channel_id: path.channel_id(), - parent_id: path.parent_id(), - intent: ClipboardIntent::Move, + channel_id: channel.id, }) } }, @@ -237,29 +216,19 @@ pub fn init(cx: &mut AppContext) { cx.add_action( |panel: &mut CollabPanel, _: &MoveSelected, cx: &mut ViewContext| { - let clipboard = panel.channel_clipboard.take(); - if let Some(((selected_channel, _), clipboard)) = - panel.selected_channel().zip(clipboard) - { - match clipboard.intent { - ClipboardIntent::Move => panel.channel_store.update(cx, |channel_store, cx| { - match clipboard.parent_id { - Some(parent_id) => channel_store.move_channel( - clipboard.channel_id, - parent_id, - selected_channel.id, - cx, - ), - None => channel_store.link_channel( - clipboard.channel_id, - selected_channel.id, - cx, - ), - } - .detach_and_log_err(cx) - }), - } - } + let Some(clipboard) = panel.channel_clipboard.take() else { + return; + }; + let Some(selected_channel) = panel.selected_channel() else { + return; + }; + + panel + .channel_store + .update(cx, |channel_store, cx| { + channel_store.move_channel(clipboard.channel_id, Some(selected_channel.id), cx) + }) + .detach_and_log_err(cx) }, ); @@ -267,16 +236,9 @@ pub fn init(cx: &mut AppContext) { |panel: &mut CollabPanel, action: &MoveChannel, cx: &mut ViewContext| { if let Some(clipboard) = panel.channel_clipboard.take() { panel.channel_store.update(cx, |channel_store, cx| { - match clipboard.parent_id { - Some(parent_id) => channel_store.move_channel( - clipboard.channel_id, - parent_id, - action.to, - cx, - ), - None => channel_store.link_channel(clipboard.channel_id, action.to, cx), - } - .detach_and_log_err(cx) + channel_store + .move_channel(clipboard.channel_id, Some(action.to), cx) + .detach_and_log_err(cx) }) } }, @@ -286,11 +248,11 @@ pub fn init(cx: &mut AppContext) { #[derive(Debug)] pub enum ChannelEditingState { Create { - location: Option, + location: Option, pending_name: Option, }, Rename { - location: ChannelPath, + location: ChannelId, pending_name: Option, }, } @@ -324,16 +286,23 @@ pub struct CollabPanel { list_state: ListState, subscriptions: Vec, collapsed_sections: Vec
, - collapsed_channels: Vec, - drag_target_channel: Option, + collapsed_channels: Vec, + drag_target_channel: ChannelDragTarget, workspace: WeakViewHandle, context_menu_on_selected: bool, } +#[derive(PartialEq, Eq)] +enum ChannelDragTarget { + None, + Root, + Channel(ChannelId), +} + #[derive(Serialize, Deserialize)] struct SerializedCollabPanel { width: Option, - collapsed_channels: Option>, + collapsed_channels: Option>, } #[derive(Debug)] @@ -378,7 +347,7 @@ enum ListEntry { Channel { channel: Arc, depth: usize, - path: ChannelPath, + has_children: bool, }, ChannelNotes { channel_id: ChannelId, @@ -513,14 +482,14 @@ impl CollabPanel { ListEntry::Channel { channel, depth, - path, + has_children, } => { let channel_row = this.render_channel( &*channel, *depth, - path.to_owned(), &theme, is_selected, + *has_children, ix, cx, ); @@ -615,7 +584,7 @@ impl CollabPanel { workspace: workspace.weak_handle(), client: workspace.app_state().client.clone(), context_menu_on_selected: true, - drag_target_channel: None, + drag_target_channel: ChannelDragTarget::None, list_state, }; @@ -879,7 +848,7 @@ impl CollabPanel { if channel_store.channel_count() > 0 || self.channel_editing_state.is_some() { self.match_candidates.clear(); self.match_candidates - .extend(channel_store.channel_dag_entries().enumerate().map( + .extend(channel_store.ordered_channels().enumerate().map( |(ix, (_, channel))| StringMatchCandidate { id: ix, string: channel.name.clone(), @@ -901,48 +870,52 @@ impl CollabPanel { } let mut collapse_depth = None; for mat in matches { - let (channel, path) = channel_store - .channel_dag_entry_at(mat.candidate_id) - .unwrap(); - let depth = path.len() - 1; + let channel = channel_store.channel_at_index(mat.candidate_id).unwrap(); + let depth = channel.parent_path.len(); - if collapse_depth.is_none() && self.is_channel_collapsed(path) { + if collapse_depth.is_none() && self.is_channel_collapsed(channel.id) { collapse_depth = Some(depth); } else if let Some(collapsed_depth) = collapse_depth { if depth > collapsed_depth { continue; } - if self.is_channel_collapsed(path) { + if self.is_channel_collapsed(channel.id) { collapse_depth = Some(depth); } else { collapse_depth = None; } } + let has_children = channel_store + .channel_at_index(mat.candidate_id + 1) + .map_or(false, |next_channel| { + next_channel.parent_path.ends_with(&[channel.id]) + }); + match &self.channel_editing_state { Some(ChannelEditingState::Create { - location: parent_path, + location: parent_id, .. - }) if parent_path.as_ref() == Some(path) => { + }) if *parent_id == Some(channel.id) => { self.entries.push(ListEntry::Channel { channel: channel.clone(), depth, - path: path.clone(), + has_children: false, }); self.entries .push(ListEntry::ChannelEditor { depth: depth + 1 }); } Some(ChannelEditingState::Rename { - location: parent_path, + location: parent_id, .. - }) if parent_path == path => { + }) if parent_id == &channel.id => { self.entries.push(ListEntry::ChannelEditor { depth }); } _ => { self.entries.push(ListEntry::Channel { channel: channel.clone(), depth, - path: path.clone(), + has_children, }); } } @@ -1484,6 +1457,7 @@ impl CollabPanel { let mut channel_link = None; let mut channel_tooltip_text = None; let mut channel_icon = None; + let mut is_dragged_over = false; let text = match section { Section::ActiveCall => { @@ -1567,26 +1541,37 @@ impl CollabPanel { cx, ), ), - Section::Channels => Some( - MouseEventHandler::new::(0, cx, |state, _| { - render_icon_button( - theme - .collab_panel - .add_contact_button - .style_for(is_selected, state), - "icons/plus.svg", - ) - }) - .with_cursor_style(CursorStyle::PointingHand) - .on_click(MouseButton::Left, |_, this, cx| this.new_root_channel(cx)) - .with_tooltip::( - 0, - "Create a channel", - None, - tooltip_style.clone(), - cx, - ), - ), + Section::Channels => { + if cx + .global::>() + .currently_dragged::(cx.window()) + .is_some() + && self.drag_target_channel == ChannelDragTarget::Root + { + is_dragged_over = true; + } + + Some( + MouseEventHandler::new::(0, cx, |state, _| { + render_icon_button( + theme + .collab_panel + .add_contact_button + .style_for(is_selected, state), + "icons/plus.svg", + ) + }) + .with_cursor_style(CursorStyle::PointingHand) + .on_click(MouseButton::Left, |_, this, cx| this.new_root_channel(cx)) + .with_tooltip::( + 0, + "Create a channel", + None, + tooltip_style.clone(), + cx, + ), + ) + } _ => None, }; @@ -1657,9 +1642,37 @@ impl CollabPanel { .constrained() .with_height(theme.collab_panel.row_height) .contained() - .with_style(header_style.container) + .with_style(if is_dragged_over { + theme.collab_panel.dragged_over_header + } else { + header_style.container + }) }); + result = result + .on_move(move |_, this, cx| { + if cx + .global::>() + .currently_dragged::(cx.window()) + .is_some() + { + this.drag_target_channel = ChannelDragTarget::Root; + cx.notify() + } + }) + .on_up(MouseButton::Left, move |_, this, cx| { + if let Some((_, dragged_channel)) = cx + .global::>() + .currently_dragged::(cx.window()) + { + this.channel_store + .update(cx, |channel_store, cx| { + channel_store.move_channel(dragged_channel.id, None, cx) + }) + .detach_and_log_err(cx) + } + }); + if can_collapse { result = result .with_cursor_style(CursorStyle::PointingHand) @@ -1910,24 +1923,23 @@ impl CollabPanel { &self, channel: &Channel, depth: usize, - path: ChannelPath, theme: &theme::Theme, is_selected: bool, + has_children: bool, ix: usize, cx: &mut ViewContext, ) -> AnyElement { let channel_id = channel.id; let collab_theme = &theme.collab_panel; - let has_children = self.channel_store.read(cx).has_children(channel_id); let is_public = self .channel_store .read(cx) .channel_for_id(channel_id) .map(|channel| channel.visibility) == Some(proto::ChannelVisibility::Public); - let other_selected = - self.selected_channel().map(|channel| channel.0.id) == Some(channel.id); - let disclosed = has_children.then(|| !self.collapsed_channels.binary_search(&path).is_ok()); + let other_selected = self.selected_channel().map(|channel| channel.id) == Some(channel.id); + let disclosed = + has_children.then(|| !self.collapsed_channels.binary_search(&channel.id).is_ok()); let is_active = iife!({ let call_channel = ActiveCall::global(cx) @@ -1950,13 +1962,9 @@ impl CollabPanel { let mut is_dragged_over = false; if cx .global::>() - .currently_dragged::(cx.window()) + .currently_dragged::(cx.window()) .is_some() - && self - .drag_target_channel - .as_ref() - .filter(|(_, dragged_path)| path.starts_with(dragged_path)) - .is_some() + && self.drag_target_channel == ChannelDragTarget::Channel(channel_id) { is_dragged_over = true; } @@ -2139,7 +2147,7 @@ impl CollabPanel { .disclosable( disclosed, Box::new(ToggleCollapse { - location: path.clone(), + location: channel.id.clone(), }), ) .with_id(ix) @@ -2159,7 +2167,7 @@ impl CollabPanel { ) }) .on_click(MouseButton::Left, move |_, this, cx| { - if this.drag_target_channel.take().is_none() { + if this.drag_target_channel == ChannelDragTarget::None { if is_active { this.open_channel_notes(&OpenChannelNotes { channel_id }, cx) } else { @@ -2168,55 +2176,40 @@ impl CollabPanel { } }) .on_click(MouseButton::Right, { - let path = path.clone(); + let channel = channel.clone(); move |e, this, cx| { - this.deploy_channel_context_menu(Some(e.position), &path, ix, cx); + this.deploy_channel_context_menu(Some(e.position), &channel, ix, cx); } }) .on_up(MouseButton::Left, move |_, this, cx| { if let Some((_, dragged_channel)) = cx .global::>() - .currently_dragged::(cx.window()) + .currently_dragged::(cx.window()) { - this.channel_store.update(cx, |channel_store, cx| { - match dragged_channel.1 { - Some(parent_id) => channel_store.move_channel( - dragged_channel.0.id, - parent_id, - channel_id, - cx, - ), - None => channel_store.link_channel(dragged_channel.0.id, channel_id, cx), - } + this.channel_store + .update(cx, |channel_store, cx| { + channel_store.move_channel(dragged_channel.id, Some(channel_id), cx) + }) .detach_and_log_err(cx) - }) } }) .on_move({ let channel = channel.clone(); - let path = path.clone(); move |_, this, cx| { - if let Some((_, _dragged_channel)) = - cx.global::>() - .currently_dragged::(cx.window()) + if let Some((_, dragged_channel)) = cx + .global::>() + .currently_dragged::(cx.window()) { - match &this.drag_target_channel { - Some(current_target) - if current_target.0 == channel && current_target.1 == path => - { - return - } - _ => { - this.drag_target_channel = Some((channel.clone(), path.clone())); - cx.notify(); - } + if channel.id != dragged_channel.id { + this.drag_target_channel = ChannelDragTarget::Channel(channel.id); } + cx.notify() } } }) - .as_draggable( - (channel.clone(), path.parent_id()), - move |_, (channel, _), cx: &mut ViewContext| { + .as_draggable::<_, Channel>( + channel.clone(), + move |_, channel, cx: &mut ViewContext| { let theme = &theme::current(cx).collab_panel; Flex::::row() @@ -2551,39 +2544,29 @@ impl CollabPanel { } fn has_subchannels(&self, ix: usize) -> bool { - self.entries - .get(ix) - .zip(self.entries.get(ix + 1)) - .map(|entries| match entries { - ( - ListEntry::Channel { - path: this_path, .. - }, - ListEntry::Channel { - path: next_path, .. - }, - ) => next_path.starts_with(this_path), - _ => false, - }) - .unwrap_or(false) + self.entries.get(ix).map_or(false, |entry| { + if let ListEntry::Channel { has_children, .. } = entry { + *has_children + } else { + false + } + }) } fn deploy_channel_context_menu( &mut self, position: Option, - path: &ChannelPath, + channel: &Channel, ix: usize, cx: &mut ViewContext, ) { self.context_menu_on_selected = position.is_none(); - let channel_name = self.channel_clipboard.as_ref().and_then(|channel| { - let channel_name = self - .channel_store + let clipboard_channel_name = self.channel_clipboard.as_ref().and_then(|clipboard| { + self.channel_store .read(cx) - .channel_for_id(channel.channel_id) - .map(|channel| channel.name.clone())?; - Some(channel_name) + .channel_for_id(clipboard.channel_id) + .map(|channel| channel.name.clone()) }); self.context_menu.update(cx, |context_menu, cx| { @@ -2607,7 +2590,7 @@ impl CollabPanel { )); if self.has_subchannels(ix) { - let expand_action_name = if self.is_channel_collapsed(&path) { + let expand_action_name = if self.is_channel_collapsed(channel.id) { "Expand Subchannels" } else { "Collapse Subchannels" @@ -2615,7 +2598,7 @@ impl CollabPanel { items.push(ContextMenuItem::action( expand_action_name, ToggleCollapse { - location: path.clone(), + location: channel.id, }, )); } @@ -2623,61 +2606,52 @@ impl CollabPanel { items.push(ContextMenuItem::action( "Open Notes", OpenChannelNotes { - channel_id: path.channel_id(), + channel_id: channel.id, }, )); items.push(ContextMenuItem::action( "Open Chat", JoinChannelChat { - channel_id: path.channel_id(), + channel_id: channel.id, }, )); items.push(ContextMenuItem::action( "Copy Channel Link", CopyChannelLink { - channel_id: path.channel_id(), + channel_id: channel.id, }, )); - if self - .channel_store - .read(cx) - .is_channel_admin(path.channel_id()) - { - let parent_id = path.parent_id(); - + if self.channel_store.read(cx).is_channel_admin(channel.id) { items.extend([ ContextMenuItem::Separator, ContextMenuItem::action( "New Subchannel", NewChannel { - location: path.clone(), + location: channel.id, }, ), ContextMenuItem::action( "Rename", RenameChannel { - location: path.clone(), + channel_id: channel.id, }, ), ContextMenuItem::action( "Move this channel", StartMoveChannelFor { - channel_id: path.channel_id(), - parent_id, + channel_id: channel.id, }, ), ]); - if let Some(channel_name) = channel_name { + if let Some(channel_name) = clipboard_channel_name { items.push(ContextMenuItem::Separator); items.push(ContextMenuItem::action( format!("Move '#{}' here", channel_name), - MoveChannel { - to: path.channel_id(), - }, + MoveChannel { to: channel.id }, )); } @@ -2686,20 +2660,20 @@ impl CollabPanel { ContextMenuItem::action( "Invite Members", InviteMembers { - channel_id: path.channel_id(), + channel_id: channel.id, }, ), ContextMenuItem::action( "Manage Members", ManageMembers { - channel_id: path.channel_id(), + channel_id: channel.id, }, ), ContextMenuItem::Separator, ContextMenuItem::action( "Delete", RemoveChannel { - channel_id: path.channel_id(), + channel_id: channel.id, }, ), ]); @@ -2870,11 +2844,7 @@ impl CollabPanel { self.channel_store .update(cx, |channel_store, cx| { - channel_store.create_channel( - &channel_name, - location.as_ref().map(|location| location.channel_id()), - cx, - ) + channel_store.create_channel(&channel_name, *location, cx) }) .detach(); cx.notify(); @@ -2891,7 +2861,7 @@ impl CollabPanel { self.channel_store .update(cx, |channel_store, cx| { - channel_store.rename(location.channel_id(), &channel_name, cx) + channel_store.rename(*location, &channel_name, cx) }) .detach(); cx.notify(); @@ -2918,33 +2888,27 @@ impl CollabPanel { _: &CollapseSelectedChannel, cx: &mut ViewContext, ) { - let Some((_, path)) = self - .selected_channel() - .map(|(channel, parent)| (channel.id, parent)) - else { + let Some(channel_id) = self.selected_channel().map(|channel| channel.id) else { return; }; - if self.is_channel_collapsed(&path) { + if self.is_channel_collapsed(channel_id) { return; } - self.toggle_channel_collapsed(&path.clone(), cx); + self.toggle_channel_collapsed(channel_id, cx); } fn expand_selected_channel(&mut self, _: &ExpandSelectedChannel, cx: &mut ViewContext) { - let Some((_, path)) = self - .selected_channel() - .map(|(channel, parent)| (channel.id, parent)) - else { + let Some(id) = self.selected_channel().map(|channel| channel.id) else { return; }; - if !self.is_channel_collapsed(&path) { + if !self.is_channel_collapsed(id) { return; } - self.toggle_channel_collapsed(path.to_owned(), cx) + self.toggle_channel_collapsed(id, cx) } fn toggle_channel_collapsed_action( @@ -2952,21 +2916,16 @@ impl CollabPanel { action: &ToggleCollapse, cx: &mut ViewContext, ) { - self.toggle_channel_collapsed(&action.location, cx); + self.toggle_channel_collapsed(action.location, cx); } - fn toggle_channel_collapsed<'a>( - &mut self, - path: impl Into>, - cx: &mut ViewContext, - ) { - let path = path.into(); - match self.collapsed_channels.binary_search(&path) { + fn toggle_channel_collapsed<'a>(&mut self, channel_id: ChannelId, cx: &mut ViewContext) { + match self.collapsed_channels.binary_search(&channel_id) { Ok(ix) => { self.collapsed_channels.remove(ix); } Err(ix) => { - self.collapsed_channels.insert(ix, path.into_owned()); + self.collapsed_channels.insert(ix, channel_id); } }; self.serialize(cx); @@ -2975,8 +2934,8 @@ impl CollabPanel { cx.focus_self(); } - fn is_channel_collapsed(&self, path: &ChannelPath) -> bool { - self.collapsed_channels.binary_search(path).is_ok() + fn is_channel_collapsed(&self, channel_id: ChannelId) -> bool { + self.collapsed_channels.binary_search(&channel_id).is_ok() } fn leave_call(cx: &mut ViewContext) { @@ -3039,16 +2998,16 @@ impl CollabPanel { } fn remove(&mut self, _: &Remove, cx: &mut ViewContext) { - if let Some((channel, _)) = self.selected_channel() { + if let Some(channel) = self.selected_channel() { self.remove_channel(channel.id, cx) } } fn rename_selected_channel(&mut self, _: &menu::SecondaryConfirm, cx: &mut ViewContext) { - if let Some((_, parent)) = self.selected_channel() { + if let Some(channel) = self.selected_channel() { self.rename_channel( &RenameChannel { - location: parent.to_owned(), + channel_id: channel.id, }, cx, ); @@ -3057,15 +3016,12 @@ impl CollabPanel { fn rename_channel(&mut self, action: &RenameChannel, cx: &mut ViewContext) { let channel_store = self.channel_store.read(cx); - if !channel_store.is_channel_admin(action.location.channel_id()) { + if !channel_store.is_channel_admin(action.channel_id) { return; } - if let Some(channel) = channel_store - .channel_for_id(action.location.channel_id()) - .cloned() - { + if let Some(channel) = channel_store.channel_for_id(action.channel_id).cloned() { self.channel_editing_state = Some(ChannelEditingState::Rename { - location: action.location.to_owned(), + location: action.channel_id.to_owned(), pending_name: None, }); self.channel_name_editor.update(cx, |editor, cx| { @@ -3085,22 +3041,18 @@ impl CollabPanel { } fn show_inline_context_menu(&mut self, _: &menu::ShowContextMenu, cx: &mut ViewContext) { - let Some((_, path)) = self.selected_channel() else { + let Some(channel) = self.selected_channel() else { return; }; - self.deploy_channel_context_menu(None, &path.to_owned(), self.selection.unwrap(), cx); + self.deploy_channel_context_menu(None, &channel.clone(), self.selection.unwrap(), cx); } - fn selected_channel(&self) -> Option<(&Arc, &ChannelPath)> { + fn selected_channel(&self) -> Option<&Arc> { self.selection .and_then(|ix| self.entries.get(ix)) .and_then(|entry| match entry { - ListEntry::Channel { - channel, - path: parent, - .. - } => Some((channel, parent)), + ListEntry::Channel { channel, .. } => Some(channel), _ => None, }) } @@ -3517,19 +3469,13 @@ impl PartialEq for ListEntry { } } ListEntry::Channel { - channel: channel_1, - depth: depth_1, - path: parent_1, + channel: channel_1, .. } => { if let ListEntry::Channel { - channel: channel_2, - depth: depth_2, - path: parent_2, + channel: channel_2, .. } = other { - return channel_1.id == channel_2.id - && depth_1 == depth_2 - && parent_1 == parent_2; + return channel_1.id == channel_2.id; } } ListEntry::ChannelNotes { channel_id } => { diff --git a/crates/rpc/proto/zed.proto b/crates/rpc/proto/zed.proto index 9ecdbde66b..206777879b 100644 --- a/crates/rpc/proto/zed.proto +++ b/crates/rpc/proto/zed.proto @@ -171,8 +171,6 @@ message Envelope { AckChannelMessage ack_channel_message = 143; GetChannelMessagesById get_channel_messages_by_id = 144; - LinkChannel link_channel = 145; - UnlinkChannel unlink_channel = 146; MoveChannel move_channel = 147; SetChannelVisibility set_channel_visibility = 148; @@ -972,8 +970,6 @@ message LspDiskBasedDiagnosticsUpdated {} message UpdateChannels { repeated Channel channels = 1; - repeated ChannelEdge insert_edge = 2; - repeated ChannelEdge delete_edge = 3; repeated uint64 delete_channels = 4; repeated Channel channel_invitations = 5; repeated uint64 remove_channel_invitations = 6; @@ -993,11 +989,6 @@ message UnseenChannelBufferChange { repeated VectorClockEntry version = 3; } -message ChannelEdge { - uint64 channel_id = 1; - uint64 parent_id = 2; -} - message ChannelPermission { uint64 channel_id = 1; ChannelRole role = 3; @@ -1137,20 +1128,9 @@ message GetChannelMessagesById { repeated uint64 message_ids = 1; } -message LinkChannel { - uint64 channel_id = 1; - uint64 to = 2; -} - -message UnlinkChannel { - uint64 channel_id = 1; - uint64 from = 2; -} - message MoveChannel { uint64 channel_id = 1; - uint64 from = 2; - uint64 to = 3; + optional uint64 to = 2; } message JoinChannelBuffer { @@ -1586,6 +1566,7 @@ message Channel { string name = 2; ChannelVisibility visibility = 3; ChannelRole role = 4; + repeated uint64 parent_path = 5; } message Contact { diff --git a/crates/rpc/src/proto.rs b/crates/rpc/src/proto.rs index c501c85107..77a69122c2 100644 --- a/crates/rpc/src/proto.rs +++ b/crates/rpc/src/proto.rs @@ -210,7 +210,6 @@ messages!( (LeaveChannelChat, Foreground), (LeaveProject, Foreground), (LeaveRoom, Foreground), - (LinkChannel, Foreground), (MarkNotificationRead, Foreground), (MoveChannel, Foreground), (OnTypeFormatting, Background), @@ -263,7 +262,6 @@ messages!( (SynchronizeBuffersResponse, Foreground), (Test, Foreground), (Unfollow, Foreground), - (UnlinkChannel, Foreground), (UnshareProject, Foreground), (UpdateBuffer, Foreground), (UpdateBufferFile, Foreground), @@ -327,7 +325,6 @@ request_messages!( (JoinRoom, JoinRoomResponse), (LeaveChannelBuffer, Ack), (LeaveRoom, Ack), - (LinkChannel, Ack), (MarkNotificationRead, Ack), (MoveChannel, Ack), (OnTypeFormatting, OnTypeFormattingResponse), @@ -362,7 +359,6 @@ request_messages!( (ShareProject, ShareProjectResponse), (SynchronizeBuffers, SynchronizeBuffersResponse), (Test, Test), - (UnlinkChannel, Ack), (UpdateBuffer, Ack), (UpdateParticipantLocation, Ack), (UpdateProject, Ack), diff --git a/crates/theme/src/theme.rs b/crates/theme/src/theme.rs index 3f4264886f..e4b8c02eca 100644 --- a/crates/theme/src/theme.rs +++ b/crates/theme/src/theme.rs @@ -250,6 +250,7 @@ pub struct CollabPanel { pub add_contact_button: Toggleable>, pub add_channel_button: Toggleable>, pub header_row: ContainedText, + pub dragged_over_header: ContainerStyle, pub subheader_row: Toggleable>, pub leave_call: Interactive, pub contact_row: Toggleable>, diff --git a/styles/src/style_tree/collab_panel.ts b/styles/src/style_tree/collab_panel.ts index 2a7702842a..272b6055ed 100644 --- a/styles/src/style_tree/collab_panel.ts +++ b/styles/src/style_tree/collab_panel.ts @@ -210,6 +210,14 @@ export default function contacts_panel(): any { right: SPACING, }, }, + dragged_over_header: { + margin: { top: SPACING }, + padding: { + left: SPACING, + right: SPACING, + }, + background: background(layer, "hovered"), + }, subheader_row, leave_call: interactive({ base: { @@ -279,7 +287,7 @@ export default function contacts_panel(): any { margin: { left: CHANNEL_SPACING, }, - } + }, }, list_empty_label_container: { margin: { diff --git a/styles/src/style_tree/search.ts b/styles/src/style_tree/search.ts index b0ac023c09..2317108bde 100644 --- a/styles/src/style_tree/search.ts +++ b/styles/src/style_tree/search.ts @@ -2,7 +2,6 @@ import { with_opacity } from "../theme/color" import { background, border, foreground, text } from "./components" import { interactive, toggleable } from "../element" import { useTheme } from "../theme" -import { text_button } from "../component/text_button" const search_results = () => { const theme = useTheme() @@ -36,7 +35,7 @@ export default function search(): any { left: 10, right: 4, }, - margin: { right: SEARCH_ROW_SPACING } + margin: { right: SEARCH_ROW_SPACING }, } const include_exclude_editor = { @@ -378,7 +377,7 @@ export default function search(): any { modes_container: { padding: { right: SEARCH_ROW_SPACING, - } + }, }, replace_icon: { icon: {