Remove old contact request notification mechanism, use notification instead

Max Brunsfeld created

Change summary

crates/client/src/user.rs                | 35 ++++------------
crates/collab/src/db.rs                  | 15 +-----
crates/collab/src/db/queries/contacts.rs |  5 --
crates/collab/src/db/tests/db_tests.rs   | 19 --------
crates/collab/src/rpc.rs                 | 54 +++++++++++--------------
crates/rpc/proto/zed.proto               |  2 
crates/rpc/src/notification.rs           | 12 ++++
crates/zed/src/zed.rs                    |  1 
8 files changed, 49 insertions(+), 94 deletions(-)

Detailed changes

crates/client/src/user.rs 🔗

@@ -293,21 +293,19 @@ impl UserStore {
                     // No need to paralellize here
                     let mut updated_contacts = Vec::new();
                     for contact in message.contacts {
-                        let should_notify = contact.should_notify;
-                        updated_contacts.push((
-                            Arc::new(Contact::from_proto(contact, &this, &mut cx).await?),
-                            should_notify,
+                        updated_contacts.push(Arc::new(
+                            Contact::from_proto(contact, &this, &mut cx).await?,
                         ));
                     }
 
                     let mut incoming_requests = Vec::new();
                     for request in message.incoming_requests {
-                        incoming_requests.push({
-                            let user = this
-                                .update(&mut cx, |this, cx| this.get_user(request.requester_id, cx))
-                                .await?;
-                            (user, request.should_notify)
-                        });
+                        incoming_requests.push(
+                            this.update(&mut cx, |this, cx| {
+                                this.get_user(request.requester_id, cx)
+                            })
+                            .await?,
+                        );
                     }
 
                     let mut outgoing_requests = Vec::new();
@@ -330,13 +328,7 @@ impl UserStore {
                         this.contacts
                             .retain(|contact| !removed_contacts.contains(&contact.user.id));
                         // Update existing contacts and insert new ones
-                        for (updated_contact, should_notify) in updated_contacts {
-                            if should_notify {
-                                cx.emit(Event::Contact {
-                                    user: updated_contact.user.clone(),
-                                    kind: ContactEventKind::Accepted,
-                                });
-                            }
+                        for updated_contact in updated_contacts {
                             match this.contacts.binary_search_by_key(
                                 &&updated_contact.user.github_login,
                                 |contact| &contact.user.github_login,
@@ -359,14 +351,7 @@ impl UserStore {
                             }
                         });
                         // Update existing incoming requests and insert new ones
-                        for (user, should_notify) in incoming_requests {
-                            if should_notify {
-                                cx.emit(Event::Contact {
-                                    user: user.clone(),
-                                    kind: ContactEventKind::Requested,
-                                });
-                            }
-
+                        for user in incoming_requests {
                             match this
                                 .incoming_contact_requests
                                 .binary_search_by_key(&&user.github_login, |contact| {

crates/collab/src/db.rs 🔗

@@ -370,18 +370,9 @@ impl<T> RoomGuard<T> {
 
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub enum Contact {
-    Accepted {
-        user_id: UserId,
-        should_notify: bool,
-        busy: bool,
-    },
-    Outgoing {
-        user_id: UserId,
-    },
-    Incoming {
-        user_id: UserId,
-        should_notify: bool,
-    },
+    Accepted { user_id: UserId, busy: bool },
+    Outgoing { user_id: UserId },
+    Incoming { user_id: UserId },
 }
 
 impl Contact {

crates/collab/src/db/queries/contacts.rs 🔗

@@ -8,7 +8,6 @@ impl Database {
             user_id_b: UserId,
             a_to_b: bool,
             accepted: bool,
-            should_notify: bool,
             user_a_busy: bool,
             user_b_busy: bool,
         }
@@ -53,7 +52,6 @@ impl Database {
                     if db_contact.accepted {
                         contacts.push(Contact::Accepted {
                             user_id: db_contact.user_id_b,
-                            should_notify: db_contact.should_notify && db_contact.a_to_b,
                             busy: db_contact.user_b_busy,
                         });
                     } else if db_contact.a_to_b {
@@ -63,19 +61,16 @@ impl Database {
                     } else {
                         contacts.push(Contact::Incoming {
                             user_id: db_contact.user_id_b,
-                            should_notify: db_contact.should_notify,
                         });
                     }
                 } else if db_contact.accepted {
                     contacts.push(Contact::Accepted {
                         user_id: db_contact.user_id_a,
-                        should_notify: db_contact.should_notify && !db_contact.a_to_b,
                         busy: db_contact.user_a_busy,
                     });
                 } else if db_contact.a_to_b {
                     contacts.push(Contact::Incoming {
                         user_id: db_contact.user_id_a,
-                        should_notify: db_contact.should_notify,
                     });
                 } else {
                     contacts.push(Contact::Outgoing {

crates/collab/src/db/tests/db_tests.rs 🔗

@@ -264,10 +264,7 @@ async fn test_add_contacts(db: &Arc<Database>) {
     );
     assert_eq!(
         db.get_contacts(user_2).await.unwrap(),
-        &[Contact::Incoming {
-            user_id: user_1,
-            should_notify: true
-        }]
+        &[Contact::Incoming { user_id: user_1 }]
     );
 
     // User 2 dismisses the contact request notification without accepting or rejecting.
@@ -280,10 +277,7 @@ async fn test_add_contacts(db: &Arc<Database>) {
         .unwrap();
     assert_eq!(
         db.get_contacts(user_2).await.unwrap(),
-        &[Contact::Incoming {
-            user_id: user_1,
-            should_notify: false
-        }]
+        &[Contact::Incoming { user_id: user_1 }]
     );
 
     // User can't accept their own contact request
@@ -299,7 +293,6 @@ async fn test_add_contacts(db: &Arc<Database>) {
         db.get_contacts(user_1).await.unwrap(),
         &[Contact::Accepted {
             user_id: user_2,
-            should_notify: true,
             busy: false,
         }],
     );
@@ -309,7 +302,6 @@ async fn test_add_contacts(db: &Arc<Database>) {
         db.get_contacts(user_2).await.unwrap(),
         &[Contact::Accepted {
             user_id: user_1,
-            should_notify: false,
             busy: false,
         }]
     );
@@ -326,7 +318,6 @@ async fn test_add_contacts(db: &Arc<Database>) {
         db.get_contacts(user_1).await.unwrap(),
         &[Contact::Accepted {
             user_id: user_2,
-            should_notify: true,
             busy: false,
         }]
     );
@@ -339,7 +330,6 @@ async fn test_add_contacts(db: &Arc<Database>) {
         db.get_contacts(user_1).await.unwrap(),
         &[Contact::Accepted {
             user_id: user_2,
-            should_notify: false,
             busy: false,
         }]
     );
@@ -353,12 +343,10 @@ async fn test_add_contacts(db: &Arc<Database>) {
         &[
             Contact::Accepted {
                 user_id: user_2,
-                should_notify: false,
                 busy: false,
             },
             Contact::Accepted {
                 user_id: user_3,
-                should_notify: false,
                 busy: false,
             }
         ]
@@ -367,7 +355,6 @@ async fn test_add_contacts(db: &Arc<Database>) {
         db.get_contacts(user_3).await.unwrap(),
         &[Contact::Accepted {
             user_id: user_1,
-            should_notify: false,
             busy: false,
         }],
     );
@@ -383,7 +370,6 @@ async fn test_add_contacts(db: &Arc<Database>) {
         db.get_contacts(user_2).await.unwrap(),
         &[Contact::Accepted {
             user_id: user_1,
-            should_notify: false,
             busy: false,
         }]
     );
@@ -391,7 +377,6 @@ async fn test_add_contacts(db: &Arc<Database>) {
         db.get_contacts(user_3).await.unwrap(),
         &[Contact::Accepted {
             user_id: user_1,
-            should_notify: false,
             busy: false,
         }],
     );

crates/collab/src/rpc.rs 🔗

@@ -388,7 +388,7 @@ impl Server {
                             let contacts = app_state.db.get_contacts(user_id).await.trace_err();
                             if let Some((busy, contacts)) = busy.zip(contacts) {
                                 let pool = pool.lock();
-                                let updated_contact = contact_for_user(user_id, false, busy, &pool);
+                                let updated_contact = contact_for_user(user_id, busy, &pool);
                                 for contact in contacts {
                                     if let db::Contact::Accepted {
                                         user_id: contact_user_id,
@@ -690,7 +690,7 @@ impl Server {
         if let Some(user) = self.app_state.db.get_user_by_id(inviter_id).await? {
             if let Some(code) = &user.invite_code {
                 let pool = self.connection_pool.lock();
-                let invitee_contact = contact_for_user(invitee_id, true, false, &pool);
+                let invitee_contact = contact_for_user(invitee_id, false, &pool);
                 for connection_id in pool.user_connection_ids(inviter_id) {
                     self.peer.send(
                         connection_id,
@@ -2090,7 +2090,6 @@ async fn request_contact(
         .incoming_requests
         .push(proto::IncomingContactRequest {
             requester_id: requester_id.to_proto(),
-            should_notify: true,
         });
     for connection_id in session
         .connection_pool()
@@ -2124,7 +2123,8 @@ async fn respond_to_contact_request(
     } else {
         let accept = request.response == proto::ContactRequestResponse::Accept as i32;
 
-        db.respond_to_contact_request(responder_id, requester_id, accept)
+        let notification = db
+            .respond_to_contact_request(responder_id, requester_id, accept)
             .await?;
         let requester_busy = db.is_user_busy(requester_id).await?;
         let responder_busy = db.is_user_busy(responder_id).await?;
@@ -2135,7 +2135,7 @@ async fn respond_to_contact_request(
         if accept {
             update
                 .contacts
-                .push(contact_for_user(requester_id, false, requester_busy, &pool));
+                .push(contact_for_user(requester_id, requester_busy, &pool));
         }
         update
             .remove_incoming_requests
@@ -2149,13 +2149,19 @@ async fn respond_to_contact_request(
         if accept {
             update
                 .contacts
-                .push(contact_for_user(responder_id, true, responder_busy, &pool));
+                .push(contact_for_user(responder_id, responder_busy, &pool));
         }
         update
             .remove_outgoing_requests
             .push(responder_id.to_proto());
         for connection_id in pool.user_connection_ids(requester_id) {
             session.peer.send(connection_id, update.clone())?;
+            session.peer.send(
+                connection_id,
+                proto::AddNotifications {
+                    notifications: vec![notification.clone()],
+                },
+            )?;
         }
     }
 
@@ -3127,42 +3133,28 @@ fn build_initial_contacts_update(
 
     for contact in contacts {
         match contact {
-            db::Contact::Accepted {
-                user_id,
-                should_notify,
-                busy,
-            } => {
-                update
-                    .contacts
-                    .push(contact_for_user(user_id, should_notify, busy, &pool));
+            db::Contact::Accepted { user_id, busy } => {
+                update.contacts.push(contact_for_user(user_id, busy, &pool));
             }
             db::Contact::Outgoing { user_id } => update.outgoing_requests.push(user_id.to_proto()),
-            db::Contact::Incoming {
-                user_id,
-                should_notify,
-            } => update
-                .incoming_requests
-                .push(proto::IncomingContactRequest {
-                    requester_id: user_id.to_proto(),
-                    should_notify,
-                }),
+            db::Contact::Incoming { user_id } => {
+                update
+                    .incoming_requests
+                    .push(proto::IncomingContactRequest {
+                        requester_id: user_id.to_proto(),
+                    })
+            }
         }
     }
 
     update
 }
 
-fn contact_for_user(
-    user_id: UserId,
-    should_notify: bool,
-    busy: bool,
-    pool: &ConnectionPool,
-) -> proto::Contact {
+fn contact_for_user(user_id: UserId, busy: bool, pool: &ConnectionPool) -> proto::Contact {
     proto::Contact {
         user_id: user_id.to_proto(),
         online: pool.is_user_online(user_id),
         busy,
-        should_notify,
     }
 }
 
@@ -3223,7 +3215,7 @@ async fn update_user_contacts(user_id: UserId, session: &Session) -> Result<()>
     let busy = db.is_user_busy(user_id).await?;
 
     let pool = session.connection_pool().await;
-    let updated_contact = contact_for_user(user_id, false, busy, &pool);
+    let updated_contact = contact_for_user(user_id, busy, &pool);
     for contact in contacts {
         if let db::Contact::Accepted {
             user_id: contact_user_id,

crates/rpc/proto/zed.proto 🔗

@@ -1223,7 +1223,6 @@ message ShowContacts {}
 
 message IncomingContactRequest {
     uint64 requester_id = 1;
-    bool should_notify = 2;
 }
 
 message UpdateDiagnostics {
@@ -1549,7 +1548,6 @@ message Contact {
     uint64 user_id = 1;
     bool online = 2;
     bool busy = 3;
-    bool should_notify = 4;
 }
 
 message WorktreeMetadata {

crates/rpc/src/notification.rs 🔗

@@ -6,6 +6,12 @@ use strum::{EnumVariantNames, IntoStaticStr, VariantNames as _};
 const KIND: &'static str = "kind";
 const ACTOR_ID: &'static str = "actor_id";
 
+/// A notification that can be stored, associated with a given user.
+///
+/// This struct is stored in the collab database as JSON, so it shouldn't be
+/// changed in a backward-incompatible way.
+///
+/// For example, when renaming a variant, add a serde alias for the old name.
 #[derive(Debug, Clone, PartialEq, Eq, EnumVariantNames, IntoStaticStr, Serialize, Deserialize)]
 #[serde(tag = "kind")]
 pub enum Notification {
@@ -26,6 +32,8 @@ pub enum Notification {
     },
 }
 
+/// The representation of a notification that is stored in the database and
+/// sent over the wire.
 #[derive(Debug)]
 pub struct AnyNotification {
     pub kind: Cow<'static, str>,
@@ -87,8 +95,8 @@ fn test_notification() {
         assert_eq!(deserialized, notification);
     }
 
-    // When notifications are serialized, redundant data is not stored
-    // in the JSON.
+    // When notifications are serialized, the `kind` and `actor_id` fields are
+    // stored separately, and do not appear redundantly in the JSON.
     let notification = Notification::ContactRequest { actor_id: 1 };
     assert_eq!(notification.to_any().content, "{}");
 }

crates/zed/src/zed.rs 🔗

@@ -2445,6 +2445,7 @@ mod tests {
             audio::init((), cx);
             channel::init(&app_state.client, app_state.user_store.clone(), cx);
             call::init(app_state.client.clone(), app_state.user_store.clone(), cx);
+            notifications::init(app_state.client.clone(), app_state.user_store.clone(), cx);
             workspace::init(app_state.clone(), cx);
             Project::init_settings(cx);
             language::init(cx);