From b3447ada275b4005e6bab70242f827e3b3dc39ce Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Wed, 9 Aug 2023 17:11:52 -0700 Subject: [PATCH] Dial in the channel creating/renaming UI * Ensure channel list is in a consistent state with no flicker while the channel creation / rename request is outstanding. * Maintain selection properly when renaming and creating channels. * Style the channel name editor more consistently with the non-editable channel names. Co-authored-by: Mikayla --- crates/client/src/channel_store.rs | 62 +++++- crates/collab/src/rpc.rs | 28 +-- crates/collab/src/tests.rs | 4 +- crates/collab/src/tests/channel_tests.rs | 16 +- crates/collab_ui/src/collab_panel.rs | 234 ++++++++++++++++------- crates/rpc/proto/zed.proto | 18 +- crates/rpc/src/proto.rs | 6 +- styles/src/style_tree/collab_panel.ts | 2 +- 8 files changed, 260 insertions(+), 110 deletions(-) diff --git a/crates/client/src/channel_store.rs b/crates/client/src/channel_store.rs index 6325bc1a30..206423579a 100644 --- a/crates/client/src/channel_store.rs +++ b/crates/client/src/channel_store.rs @@ -39,8 +39,13 @@ pub struct ChannelMembership { pub admin: bool, } +pub enum ChannelEvent { + ChannelCreated(ChannelId), + ChannelRenamed(ChannelId), +} + impl Entity for ChannelStore { - type Event = (); + type Event = ChannelEvent; } pub enum ChannelMemberStatus { @@ -127,15 +132,37 @@ impl ChannelStore { &self, name: &str, parent_id: Option, - ) -> impl Future> { + cx: &mut ModelContext, + ) -> Task> { let client = self.client.clone(); let name = name.trim_start_matches("#").to_owned(); - async move { - Ok(client + cx.spawn(|this, mut cx| async move { + let channel = client .request(proto::CreateChannel { name, parent_id }) .await? - .channel_id) - } + .channel + .ok_or_else(|| anyhow!("missing channel in response"))?; + + let channel_id = channel.id; + + this.update(&mut cx, |this, cx| { + this.update_channels( + proto::UpdateChannels { + channels: vec![channel], + ..Default::default() + }, + cx, + ); + + // This event is emitted because the collab panel wants to clear the pending edit state + // before this frame is rendered. But we can't guarantee that the collab panel's future + // will resolve before this flush_effects finishes. Synchronously emitting this event + // ensures that the collab panel will observe this creation before the frame completes + cx.emit(ChannelEvent::ChannelCreated(channel_id)); + }); + + Ok(channel_id) + }) } pub fn invite_member( @@ -240,10 +267,27 @@ impl ChannelStore { ) -> Task> { let client = self.client.clone(); let name = new_name.to_string(); - cx.spawn(|_this, _cx| async move { - client + cx.spawn(|this, mut cx| async move { + let channel = client .request(proto::RenameChannel { channel_id, name }) - .await?; + .await? + .channel + .ok_or_else(|| anyhow!("missing channel in response"))?; + this.update(&mut cx, |this, cx| { + this.update_channels( + proto::UpdateChannels { + channels: vec![channel], + ..Default::default() + }, + cx, + ); + + // This event is emitted because the collab panel wants to clear the pending edit state + // before this frame is rendered. But we can't guarantee that the collab panel's future + // will resolve before this flush_effects finishes. Synchronously emitting this event + // ensures that the collab panel will observe this creation before the frame complete + cx.emit(ChannelEvent::ChannelRenamed(channel_id)) + }); Ok(()) }) } diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index c2f0d31f90..f9f2d4a2e2 100644 --- a/crates/collab/src/rpc.rs +++ b/crates/collab/src/rpc.rs @@ -2146,16 +2146,18 @@ async fn create_channel( .create_channel(&request.name, parent_id, &live_kit_room, session.user_id) .await?; - response.send(proto::CreateChannelResponse { - channel_id: id.to_proto(), - })?; - - let mut update = proto::UpdateChannels::default(); - update.channels.push(proto::Channel { + let channel = proto::Channel { id: id.to_proto(), name: request.name, parent_id: request.parent_id, - }); + }; + + response.send(proto::ChannelResponse { + channel: Some(channel.clone()), + })?; + + let mut update = proto::UpdateChannels::default(); + update.channels.push(channel); let user_ids_to_notify = if let Some(parent_id) = parent_id { db.get_channel_members(parent_id).await? @@ -2317,14 +2319,16 @@ async fn rename_channel( .rename_channel(channel_id, session.user_id, &request.name) .await?; - response.send(proto::Ack {})?; - - let mut update = proto::UpdateChannels::default(); - update.channels.push(proto::Channel { + let channel = proto::Channel { id: request.channel_id, name: new_name, parent_id: None, - }); + }; + response.send(proto::ChannelResponse { + channel: Some(channel.clone()), + })?; + let mut update = proto::UpdateChannels::default(); + update.channels.push(channel); let member_ids = db.get_channel_members(channel_id).await?; diff --git a/crates/collab/src/tests.rs b/crates/collab/src/tests.rs index 31d7b629f8..46cbcb0213 100644 --- a/crates/collab/src/tests.rs +++ b/crates/collab/src/tests.rs @@ -278,8 +278,8 @@ impl TestServer { let channel_id = admin_client .app_state .channel_store - .update(admin_cx, |channel_store, _| { - channel_store.create_channel(channel, None) + .update(admin_cx, |channel_store, cx| { + channel_store.create_channel(channel, None, cx) }) .await .unwrap(); diff --git a/crates/collab/src/tests/channel_tests.rs b/crates/collab/src/tests/channel_tests.rs index 63fab0d5f8..0dc6d478d1 100644 --- a/crates/collab/src/tests/channel_tests.rs +++ b/crates/collab/src/tests/channel_tests.rs @@ -21,15 +21,15 @@ async fn test_core_channels( let channel_a_id = client_a .channel_store() - .update(cx_a, |channel_store, _| { - channel_store.create_channel("channel-a", None) + .update(cx_a, |channel_store, cx| { + channel_store.create_channel("channel-a", None, cx) }) .await .unwrap(); let channel_b_id = client_a .channel_store() - .update(cx_a, |channel_store, _| { - channel_store.create_channel("channel-b", Some(channel_a_id)) + .update(cx_a, |channel_store, cx| { + channel_store.create_channel("channel-b", Some(channel_a_id), cx) }) .await .unwrap(); @@ -150,8 +150,8 @@ async fn test_core_channels( let channel_c_id = client_a .channel_store() - .update(cx_a, |channel_store, _| { - channel_store.create_channel("channel-c", Some(channel_b_id)) + .update(cx_a, |channel_store, cx| { + channel_store.create_channel("channel-c", Some(channel_b_id), cx) }) .await .unwrap(); @@ -351,8 +351,8 @@ async fn test_joining_channel_ancestor_member( let sub_id = client_a .channel_store() - .update(cx_a, |channel_store, _| { - channel_store.create_channel("sub_channel", Some(parent_id)) + .update(cx_a, |channel_store, cx| { + channel_store.create_channel("sub_channel", Some(parent_id), cx) }) .await .unwrap(); diff --git a/crates/collab_ui/src/collab_panel.rs b/crates/collab_ui/src/collab_panel.rs index 7bf2290622..cb40d496b6 100644 --- a/crates/collab_ui/src/collab_panel.rs +++ b/crates/collab_ui/src/collab_panel.rs @@ -4,7 +4,9 @@ mod panel_settings; use anyhow::Result; use call::ActiveCall; -use client::{proto::PeerId, Channel, ChannelId, ChannelStore, Client, Contact, User, UserStore}; +use client::{ + proto::PeerId, Channel, ChannelEvent, ChannelId, ChannelStore, Client, Contact, User, UserStore, +}; use contact_finder::build_contact_finder; use context_menu::{ContextMenu, ContextMenuItem}; use db::kvp::KEY_VALUE_STORE; @@ -105,8 +107,23 @@ pub fn init(_client: Arc, cx: &mut AppContext) { #[derive(Debug)] pub enum ChannelEditingState { - Create { parent_id: Option }, - Rename { channel_id: u64 }, + Create { + parent_id: Option, + pending_name: Option, + }, + Rename { + channel_id: u64, + pending_name: Option, + }, +} + +impl ChannelEditingState { + fn pending_name(&self) -> Option<&str> { + match self { + ChannelEditingState::Create { pending_name, .. } => pending_name.as_deref(), + ChannelEditingState::Rename { pending_name, .. } => pending_name.as_deref(), + } + } } pub struct CollabPanel { @@ -211,7 +228,7 @@ impl CollabPanel { if !query.is_empty() { this.selection.take(); } - this.update_entries(false, cx); + this.update_entries(true, cx); if !query.is_empty() { this.selection = this .entries @@ -233,6 +250,11 @@ impl CollabPanel { cx.subscribe(&channel_name_editor, |this, _, event, cx| { if let editor::Event::Blurred = event { + if let Some(state) = &this.channel_editing_state { + if state.pending_name().is_some() { + return; + } + } this.take_editing_state(cx); this.update_entries(false, cx); cx.notify(); @@ -391,17 +413,35 @@ impl CollabPanel { let active_call = ActiveCall::global(cx); this.subscriptions .push(cx.observe(&this.user_store, |this, _, cx| { - this.update_entries(false, cx) + this.update_entries(true, cx) })); this.subscriptions .push(cx.observe(&this.channel_store, |this, _, cx| { - this.update_entries(false, cx) + this.update_entries(true, cx) })); this.subscriptions - .push(cx.observe(&active_call, |this, _, cx| this.update_entries(false, cx))); + .push(cx.observe(&active_call, |this, _, cx| this.update_entries(true, cx))); this.subscriptions.push( - cx.observe_global::(move |this, cx| this.update_entries(false, cx)), + cx.observe_global::(move |this, cx| this.update_entries(true, cx)), ); + this.subscriptions.push(cx.subscribe( + &this.channel_store, + |this, _channel_store, e, cx| match e { + ChannelEvent::ChannelCreated(channel_id) + | ChannelEvent::ChannelRenamed(channel_id) => { + if this.take_editing_state(cx) { + this.update_entries(false, cx); + this.selection = this.entries.iter().position(|entry| { + if let ListEntry::Channel(channel) = entry { + channel.id == *channel_id + } else { + false + } + }); + } + } + }, + )); this }) @@ -453,7 +493,7 @@ impl CollabPanel { ); } - fn update_entries(&mut self, select_editor: bool, cx: &mut ViewContext) { + fn update_entries(&mut self, select_same_item: bool, cx: &mut ViewContext) { let channel_store = self.channel_store.read(cx); let user_store = self.user_store.read(cx); let query = self.filter_editor.read(cx).text(cx); @@ -595,7 +635,13 @@ impl CollabPanel { executor.clone(), )); if let Some(state) = &self.channel_editing_state { - if matches!(state, ChannelEditingState::Create { parent_id: None }) { + if matches!( + state, + ChannelEditingState::Create { + parent_id: None, + .. + } + ) { self.entries.push(ListEntry::ChannelEditor { depth: 0 }); } } @@ -603,7 +649,7 @@ impl CollabPanel { let channel = &channels[mat.candidate_id]; match &self.channel_editing_state { - Some(ChannelEditingState::Create { parent_id }) + Some(ChannelEditingState::Create { parent_id, .. }) if *parent_id == Some(channel.id) => { self.entries.push(ListEntry::Channel(channel.clone())); @@ -611,11 +657,11 @@ impl CollabPanel { depth: channel.depth + 1, }); } - Some(ChannelEditingState::Rename { channel_id }) + Some(ChannelEditingState::Rename { channel_id, .. }) if *channel_id == channel.id => { self.entries.push(ListEntry::ChannelEditor { - depth: channel.depth + 1, + depth: channel.depth, }); } _ => { @@ -775,14 +821,7 @@ impl CollabPanel { } } - if select_editor { - for (ix, entry) in self.entries.iter().enumerate() { - if matches!(*entry, ListEntry::ChannelEditor { .. }) { - self.selection = Some(ix); - break; - } - } - } else { + if select_same_item { if let Some(prev_selected_entry) = prev_selected_entry { self.selection.take(); for (ix, entry) in self.entries.iter().enumerate() { @@ -792,6 +831,14 @@ impl CollabPanel { } } } + } else { + self.selection = self.selection.and_then(|prev_selection| { + if self.entries.is_empty() { + None + } else { + Some(prev_selection.min(self.entries.len() - 1)) + } + }); } let old_scroll_top = self.list_state.logical_scroll_top(); @@ -1088,18 +1135,14 @@ impl CollabPanel { .into_any() } - fn take_editing_state( - &mut self, - cx: &mut ViewContext, - ) -> Option<(ChannelEditingState, String)> { - if let Some(state) = self.channel_editing_state.take() { + fn take_editing_state(&mut self, cx: &mut ViewContext) -> bool { + if let Some(_) = self.channel_editing_state.take() { self.channel_name_editor.update(cx, |editor, cx| { - let name = editor.text(cx); editor.set_text("", cx); - Some((state, name)) - }) + }); + true } else { - None + false } } @@ -1367,22 +1410,43 @@ impl CollabPanel { .left(), ) .with_child( - ChildView::new(&self.channel_name_editor, cx) + if let Some(pending_name) = self + .channel_editing_state + .as_ref() + .and_then(|state| state.pending_name()) + { + Label::new( + pending_name.to_string(), + theme.collab_panel.contact_username.text.clone(), + ) .contained() - .with_style(theme.collab_panel.channel_editor) - .flex(1.0, true), + .with_style(theme.collab_panel.contact_username.container) + .aligned() + .left() + .flex(1., true) + .into_any() + } else { + ChildView::new(&self.channel_name_editor, cx) + .aligned() + .left() + .contained() + .with_style(theme.collab_panel.channel_editor) + .flex(1.0, true) + .into_any() + }, ) .align_children_center() + .constrained() + .with_height(theme.collab_panel.row_height) .contained() + .with_style(gpui::elements::ContainerStyle { + background_color: Some(theme.editor.background), + ..*theme.collab_panel.contact_row.default_style() + }) .with_padding_left( theme.collab_panel.contact_row.default_style().padding.left + theme.collab_panel.channel_indent * depth as f32, ) - .contained() - .with_style(gpui::elements::ContainerStyle { - background_color: Some(theme.editor.background), - ..Default::default() - }) .into_any() } @@ -1684,7 +1748,7 @@ impl CollabPanel { } fn cancel(&mut self, _: &Cancel, cx: &mut ViewContext) { - if self.take_editing_state(cx).is_some() { + if self.take_editing_state(cx) { cx.focus(&self.filter_editor); } else { self.filter_editor.update(cx, |editor, cx| { @@ -1785,29 +1849,47 @@ impl CollabPanel { } } - fn confirm_channel_edit(&mut self, cx: &mut ViewContext<'_, '_, CollabPanel>) -> bool { - if let Some((editing_state, channel_name)) = self.take_editing_state(cx) { + fn confirm_channel_edit(&mut self, cx: &mut ViewContext) -> bool { + if let Some(editing_state) = &mut self.channel_editing_state { match editing_state { - ChannelEditingState::Create { parent_id } => { - let request = self.channel_store.update(cx, |channel_store, _| { - channel_store.create_channel(&channel_name, parent_id) - }); - cx.foreground() - .spawn(async move { - request.await?; - anyhow::Ok(()) - }) - .detach(); - } - ChannelEditingState::Rename { channel_id } => { + ChannelEditingState::Create { + parent_id, + pending_name, + .. + } => { + if pending_name.is_some() { + return false; + } + let channel_name = self.channel_name_editor.read(cx).text(cx); + + *pending_name = Some(channel_name.clone()); + self.channel_store .update(cx, |channel_store, cx| { - channel_store.rename(channel_id, &channel_name, cx) + channel_store.create_channel(&channel_name, *parent_id, cx) }) .detach(); + cx.notify(); + } + ChannelEditingState::Rename { + channel_id, + pending_name, + } => { + if pending_name.is_some() { + return false; + } + let channel_name = self.channel_name_editor.read(cx).text(cx); + *pending_name = Some(channel_name.clone()); + + self.channel_store + .update(cx, |channel_store, cx| { + channel_store.rename(*channel_id, &channel_name, cx) + }) + .detach(); + cx.notify(); } } - self.update_entries(false, cx); + cx.focus_self(); true } else { false @@ -1844,17 +1926,30 @@ impl CollabPanel { } fn new_root_channel(&mut self, cx: &mut ViewContext) { - self.channel_editing_state = Some(ChannelEditingState::Create { parent_id: None }); - self.update_entries(true, cx); + self.channel_editing_state = Some(ChannelEditingState::Create { + parent_id: None, + pending_name: None, + }); + self.update_entries(false, cx); + self.select_channel_editor(); cx.focus(self.channel_name_editor.as_any()); cx.notify(); } + fn select_channel_editor(&mut self) { + self.selection = self.entries.iter().position(|entry| match entry { + ListEntry::ChannelEditor { .. } => true, + _ => false, + }); + } + fn new_subchannel(&mut self, action: &NewChannel, cx: &mut ViewContext) { self.channel_editing_state = Some(ChannelEditingState::Create { parent_id: Some(action.channel_id), + pending_name: None, }); - self.update_entries(true, cx); + self.update_entries(false, cx); + self.select_channel_editor(); cx.focus(self.channel_name_editor.as_any()); cx.notify(); } @@ -1887,19 +1982,22 @@ impl CollabPanel { } fn rename_channel(&mut self, action: &RenameChannel, cx: &mut ViewContext) { - if let Some(channel) = self - .channel_store - .read(cx) - .channel_for_id(action.channel_id) - { + let channel_store = self.channel_store.read(cx); + if !channel_store.is_user_admin(action.channel_id) { + return; + } + if let Some(channel) = channel_store.channel_for_id(action.channel_id) { self.channel_editing_state = Some(ChannelEditingState::Rename { channel_id: action.channel_id, + pending_name: None, }); self.channel_name_editor.update(cx, |editor, cx| { editor.set_text(channel.name.clone(), cx); editor.select_all(&Default::default(), cx); }); - self.update_entries(true, cx); + cx.focus(self.channel_name_editor.as_any()); + self.update_entries(false, cx); + self.select_channel_editor(); } } @@ -2069,8 +2167,12 @@ impl View for CollabPanel { if !self.has_focus { self.has_focus = true; if !self.context_menu.is_focused(cx) { - if self.channel_editing_state.is_some() { - cx.focus(&self.channel_name_editor); + if let Some(editing_state) = &self.channel_editing_state { + if editing_state.pending_name().is_none() { + cx.focus(&self.channel_name_editor); + } else { + cx.focus(&self.filter_editor); + } } else { cx.focus(&self.filter_editor); } diff --git a/crates/rpc/proto/zed.proto b/crates/rpc/proto/zed.proto index 13b4c60aad..fc9a66753c 100644 --- a/crates/rpc/proto/zed.proto +++ b/crates/rpc/proto/zed.proto @@ -131,17 +131,17 @@ message Envelope { RefreshInlayHints refresh_inlay_hints = 118; CreateChannel create_channel = 119; - CreateChannelResponse create_channel_response = 120; + ChannelResponse channel_response = 120; InviteChannelMember invite_channel_member = 121; RemoveChannelMember remove_channel_member = 122; RespondToChannelInvite respond_to_channel_invite = 123; UpdateChannels update_channels = 124; - JoinChannel join_channel = 126; - RemoveChannel remove_channel = 127; - GetChannelMembers get_channel_members = 128; - GetChannelMembersResponse get_channel_members_response = 129; - SetChannelMemberAdmin set_channel_member_admin = 130; - RenameChannel rename_channel = 131; + JoinChannel join_channel = 125; + RemoveChannel remove_channel = 126; + GetChannelMembers get_channel_members = 127; + GetChannelMembersResponse get_channel_members_response = 128; + SetChannelMemberAdmin set_channel_member_admin = 129; + RenameChannel rename_channel = 130; } } @@ -921,8 +921,8 @@ message CreateChannel { optional uint64 parent_id = 2; } -message CreateChannelResponse { - uint64 channel_id = 1; +message ChannelResponse { + Channel channel = 1; } message InviteChannelMember { diff --git a/crates/rpc/src/proto.rs b/crates/rpc/src/proto.rs index d3a3091131..92732b00b5 100644 --- a/crates/rpc/src/proto.rs +++ b/crates/rpc/src/proto.rs @@ -146,7 +146,7 @@ messages!( (CopyProjectEntry, Foreground), (CreateBufferForPeer, Foreground), (CreateChannel, Foreground), - (CreateChannelResponse, Foreground), + (ChannelResponse, Foreground), (CreateProjectEntry, Foreground), (CreateRoom, Foreground), (CreateRoomResponse, Foreground), @@ -262,7 +262,7 @@ request_messages!( (CopyProjectEntry, ProjectEntryResponse), (CreateProjectEntry, ProjectEntryResponse), (CreateRoom, CreateRoomResponse), - (CreateChannel, CreateChannelResponse), + (CreateChannel, ChannelResponse), (DeclineCall, Ack), (DeleteProjectEntry, ProjectEntryResponse), (ExpandProjectEntry, ExpandProjectEntryResponse), @@ -305,7 +305,7 @@ request_messages!( (JoinChannel, JoinRoomResponse), (RemoveChannel, Ack), (RenameProjectEntry, ProjectEntryResponse), - (RenameChannel, Ack), + (RenameChannel, ChannelResponse), (SaveBuffer, BufferSaved), (SearchProject, SearchProjectResponse), (ShareProject, ShareProjectResponse), diff --git a/styles/src/style_tree/collab_panel.ts b/styles/src/style_tree/collab_panel.ts index 0979760b88..6c10da7482 100644 --- a/styles/src/style_tree/collab_panel.ts +++ b/styles/src/style_tree/collab_panel.ts @@ -358,7 +358,7 @@ export default function contacts_panel(): any { face_overlap: 8, channel_editor: { padding: { - left: 8, + left: name_margin, } } }