feat: Make broadcast lists create their own chat (#4644)

feat: Make broadcast lists create their own chat - UIs need to ask for
the name when creating broadcast lists now (see
https://github.com/deltachat/deltachat-android/pull/2653)

That's quite a minimal approach: Add a List-ID header to outgoing
broadcast lists, so that the receiving Delta Chat shows them as a
separate chat, as talked about with @r10s and @hpk42.

Done:
- [x] Fix an existing bug that the chat name isn't updated when the
broadcast/mailing list name changes (I already started this locally)

To be done in other PRs:
- [ ] Right now the receiving side shows "Mailing list" in the subtitle
of such a chat, it would be nicer if it showed "Broadcast list" (or
alternatively, rename "Broadcast list" to "Mailing list", too)
- [ ] The UIs should probably ask for a name before creating the
broadcast list, since it will actually be sent over the wire. (Android
PR: https://github.com/deltachat/deltachat-android/pull/2653)

Fixes https://github.com/deltachat/deltachat-core-rust/issues/4597

BREAKING CHANGE: This means that UIs need to ask for the name when creating a broadcast list, similar to https://github.com/deltachat/deltachat-android/pull/2653.

deltachat-ffi/deltachat.h

@@ -1718,24 +1718,12 @@ uint32_t        dc_create_group_chat         (dc_context_t* context, int protect
  * Create a new broadcast list.
  *
  * Broadcast lists are similar to groups on the sending device,
- * however, recipients get the messages in normal one-to-one chats
- * and will not be aware of other members.
+ * however, recipients get the messages in a read-only chat
+ * and will see who the other members are.
  *
- * Replies to broadcasts go only to the sender
- * and not to all broadcast recipients.
- * Moreover, replies will not appear in the broadcast list
- * but in the one-to-one chat with the person answering.
- *
- * The name and the image of the broadcast list is set automatically
- * and is visible to the sender only.
- * Not asking for these data allows more focused creation
- * and we bypass the question who will get which data.
- * Also, many users will have at most one broadcast list
- * so, a generic name and image is sufficient at the first place.
- *
- * Later on, however, the name can be changed using dc_set_chat_name().
- * The image cannot be changed to have a unique, recognizable icon in the chat lists.
- * All in all, this is also what other messengers are doing here.
+ * For historical reasons, this function does not take a name directly,
+ * instead you have to set the name using dc_set_chat_name()
+ * after creating the broadcast list.
  *
  * @memberof dc_context_t
  * @param context The context object.

deltachat-jsonrpc/src/api/mod.rs

@@ -812,24 +812,12 @@ impl CommandApi {
     /// Create a new broadcast list.
     ///
     /// Broadcast lists are similar to groups on the sending device,
-    /// however, recipients get the messages in normal one-to-one chats
-    /// and will not be aware of other members.
+    /// however, recipients get the messages in a read-only chat
+    /// and will see who the other members are.
     ///
-    /// Replies to broadcasts go only to the sender
-    /// and not to all broadcast recipients.
-    /// Moreover, replies will not appear in the broadcast list
-    /// but in the one-to-one chat with the person answering.
-    ///
-    /// The name and the image of the broadcast list is set automatically
-    /// and is visible to the sender only.
-    /// Not asking for these data allows more focused creation
-    /// and we bypass the question who will get which data.
-    /// Also, many users will have at most one broadcast list
-    /// so, a generic name and image is sufficient at the first place.
-    ///
-    /// Later on, however, the name can be changed using dc_set_chat_name().
-    /// The image cannot be changed to have a unique, recognizable icon in the chat lists.
-    /// All in all, this is also what other messengers are doing here.
+    /// For historical reasons, this function does not take a name directly,
+    /// instead you have to set the name using dc_set_chat_name()
+    /// after creating the broadcast list.
     async fn create_broadcast_list(&self, account_id: u32) -> Result<u32> {
         let ctx = self.get_context(account_id).await?;
         chat::create_broadcast_list(&ctx)

src/chat.rs

@@ -6094,22 +6094,40 @@ mod tests {
             get_chat_contacts(&alice, chat_bob.id).await?.pop().unwrap(),
         )
         .await?;
-        let chat = Chat::load_from_db(&alice, broadcast_id).await?;
-        assert_eq!(chat.typ, Chattype::Broadcast);
-        assert_eq!(chat.name, stock_str::broadcast_list(&alice).await);
-        assert!(!chat.is_self_talk());
+        set_chat_name(&alice, broadcast_id, "Broadcast list").await?;
+        {
+            let chat = Chat::load_from_db(&alice, broadcast_id).await?;
+            assert_eq!(chat.typ, Chattype::Broadcast);
+            assert_eq!(chat.name, "Broadcast list");
+            assert!(!chat.is_self_talk());
 
-        send_text_msg(&alice, broadcast_id, "ola!".to_string()).await?;
-        let msg = alice.get_last_msg().await;
-        assert_eq!(msg.chat_id, chat.id);
+            send_text_msg(&alice, broadcast_id, "ola!".to_string()).await?;
+            let msg = alice.get_last_msg().await;
+            assert_eq!(msg.chat_id, chat.id);
+        }
 
-        let msg = bob.recv_msg(&alice.pop_sent_msg().await).await;
-        assert_eq!(msg.get_text(), "ola!");
-        assert!(!msg.get_showpadlock()); // avoid leaking recipients in encryption data
-        let chat = Chat::load_from_db(&bob, msg.chat_id).await?;
-        assert_eq!(chat.typ, Chattype::Single);
-        assert_eq!(chat.id, chat_bob.id);
-        assert!(!chat.is_self_talk());
+        {
+            let msg = bob.recv_msg(&alice.pop_sent_msg().await).await;
+            assert_eq!(msg.get_text(), "ola!");
+            assert_eq!(msg.subject, "Broadcast list");
+            assert!(!msg.get_showpadlock()); // avoid leaking recipients in encryption data
+            let chat = Chat::load_from_db(&bob, msg.chat_id).await?;
+            assert_eq!(chat.typ, Chattype::Mailinglist);
+            assert_ne!(chat.id, chat_bob.id);
+            assert_eq!(chat.name, "Broadcast list");
+            assert!(!chat.is_self_talk());
+        }
+
+        {
+            // Alice changes the name:
+            set_chat_name(&alice, broadcast_id, "My great broadcast").await?;
+            let sent = alice.send_text(broadcast_id, "I changed the title!").await;
+
+            let msg = bob.recv_msg(&sent).await;
+            assert_eq!(msg.subject, "Re: My great broadcast");
+            let bob_chat = Chat::load_from_db(&bob, msg.chat_id).await?;
+            assert_eq!(bob_chat.name, "My great broadcast");
+        }
 
         Ok(())
     }

src/mimefactory.rs

@@ -415,7 +415,9 @@ impl<'a> MimeFactory<'a> {
                     return Ok(self.msg.subject.clone());
                 }
 
-                if chat.typ == Chattype::Group && quoted_msg_subject.is_none_or_empty() {
+                if (chat.typ == Chattype::Group || chat.typ == Chattype::Broadcast)
+                    && quoted_msg_subject.is_none_or_empty()
+                {
                     let re = if self.in_reply_to.is_empty() {
                         ""
                     } else {
@@ -424,15 +426,13 @@ impl<'a> MimeFactory<'a> {
                     return Ok(format!("{}{}", re, chat.name));
                 }
 
-                if chat.typ != Chattype::Broadcast {
-                    let parent_subject = if quoted_msg_subject.is_none_or_empty() {
-                        chat.param.get(Param::LastSubject)
-                    } else {
-                        quoted_msg_subject.as_deref()
-                    };
-                    if let Some(last_subject) = parent_subject {
-                        return Ok(format!("Re: {}", remove_subject_prefix(last_subject)));
-                    }
+                let parent_subject = if quoted_msg_subject.is_none_or_empty() {
+                    chat.param.get(Param::LastSubject)
+                } else {
+                    quoted_msg_subject.as_deref()
+                };
+                if let Some(last_subject) = parent_subject {
+                    return Ok(format!("Re: {}", remove_subject_prefix(last_subject)));
                 }
 
                 let self_name = &match context.get_config(Config::Displayname).await? {
@@ -594,6 +594,15 @@ impl<'a> MimeFactory<'a> {
             ));
         }
 
+        if let Loaded::Message { chat } = &self.loaded {
+            if chat.typ == Chattype::Broadcast {
+                headers.protected.push(Header::new(
+                    "List-ID".into(),
+                    format!("{} <{}>", chat.name, chat.grpid),
+                ));
+            }
+        }
+
         // Non-standard headers.
         headers
             .unprotected

src/mimeparser.rs

@@ -123,23 +123,6 @@ pub(crate) enum AvatarAction {
     Change(String),
 }
 
-#[derive(Debug, PartialEq)]
-pub(crate) enum MailinglistType {
-    /// The message belongs to a mailing list and has a `ListId:`-header
-    /// that should be used to get a unique id.
-    ListIdBased,
-
-    /// The message belongs to a mailing list, but there is no `ListId:`-header;
-    /// `Sender:`-header should be used to get a unique id.
-    /// This method is used by implementations as Majordomo.
-    /// Note, that the `Sender:` header alone is not sufficient to detect these lists,
-    /// `get_mailinglist_type()` check additional conditions therefore.
-    SenderBased,
-
-    /// The message does not belong to a mailing list.
-    None,
-}
-
 /// System message type.
 #[derive(
     Debug, Default, Display, Clone, Copy, PartialEq, Eq, FromPrimitive, ToPrimitive, ToSql, FromSql,
@@ -1340,26 +1323,28 @@ impl MimeMessage {
         self.parts.push(part);
     }
 
-    pub(crate) fn get_mailinglist_type(&self) -> MailinglistType {
-        if self.get_header(HeaderDef::ListId).is_some() {
-            return MailinglistType::ListIdBased;
-        } else if self.get_header(HeaderDef::Sender).is_some() {
+    pub(crate) fn get_mailinglist_header(&self) -> Option<&str> {
+        if let Some(list_id) = self.get_header(HeaderDef::ListId) {
+            // The message belongs to a mailing list and has a `ListId:`-header
+            // that should be used to get a unique id.
+            return Some(list_id);
+        } else if let Some(sender) = self.get_header(HeaderDef::Sender) {
             // the `Sender:`-header alone is no indicator for mailing list
             // as also used for bot-impersonation via `set_override_sender_name()`
             if let Some(precedence) = self.get_header(HeaderDef::Precedence) {
                 if precedence == "list" || precedence == "bulk" {
-                    return MailinglistType::SenderBased;
+                    // The message belongs to a mailing list, but there is no `ListId:`-header;
+                    // `Sender:`-header is be used to get a unique id.
+                    // This method is used by implementations as Majordomo.
+                    return Some(sender);
                 }
             }
         }
-        MailinglistType::None
+        None
     }
 
     pub(crate) fn is_mailinglist_message(&self) -> bool {
-        match self.get_mailinglist_type() {
-            MailinglistType::ListIdBased | MailinglistType::SenderBased => true,
-            MailinglistType::None => false,
-        }
+        self.get_mailinglist_header().is_some()
     }
 
     pub fn repl_msg_by_error(&mut self, error_msg: &str) {

src/receive_imf.rs

@@ -28,9 +28,7 @@ use crate::log::LogExt;
 use crate::message::{
     self, rfc724_mid_exists, Message, MessageState, MessengerMessage, MsgId, Viewtype,
 };
-use crate::mimeparser::{
-    parse_message_ids, AvatarAction, MailinglistType, MimeMessage, SystemMessage,
-};
+use crate::mimeparser::{parse_message_ids, AvatarAction, MimeMessage, SystemMessage};
 use crate::param::{Param, Params};
 use crate::peerstate::{Peerstate, PeerstateKeyType, PeerstateVerifiedStatus};
 use crate::reaction::{set_msg_reaction, Reaction};
@@ -654,45 +652,23 @@ async fn add_parts(
 
         if chat_id.is_none() {
             // check if the message belongs to a mailing list
-            match mime_parser.get_mailinglist_type() {
-                MailinglistType::ListIdBased => {
-                    if let Some(list_id) = mime_parser.get_header(HeaderDef::ListId) {
-                        if let Some((new_chat_id, new_chat_id_blocked)) =
-                            create_or_lookup_mailinglist(
-                                context,
-                                allow_creation,
-                                list_id,
-                                mime_parser,
-                            )
-                            .await?
-                        {
-                            chat_id = Some(new_chat_id);
-                            chat_id_blocked = new_chat_id_blocked;
-                        }
-                    }
+            if let Some(mailinglist_header) = mime_parser.get_mailinglist_header() {
+                if let Some((new_chat_id, new_chat_id_blocked)) = create_or_lookup_mailinglist(
+                    context,
+                    allow_creation,
+                    mailinglist_header,
+                    mime_parser,
+                )
+                .await?
+                {
+                    chat_id = Some(new_chat_id);
+                    chat_id_blocked = new_chat_id_blocked;
                 }
-                MailinglistType::SenderBased => {
-                    if let Some(sender) = mime_parser.get_header(HeaderDef::Sender) {
-                        if let Some((new_chat_id, new_chat_id_blocked)) =
-                            create_or_lookup_mailinglist(
-                                context,
-                                allow_creation,
-                                sender,
-                                mime_parser,
-                            )
-                            .await?
-                        {
-                            chat_id = Some(new_chat_id);
-                            chat_id_blocked = new_chat_id_blocked;
-                        }
-                    }
-                }
-                MailinglistType::None => {}
             }
         }
 
         if let Some(chat_id) = chat_id {
-            apply_mailinglist_changes(context, mime_parser, chat_id).await?;
+            apply_mailinglist_changes(context, mime_parser, sent_timestamp, chat_id).await?;
         }
 
         // if contact renaming is prevented (for mailinglists and bots),
@@ -1921,6 +1897,8 @@ async fn apply_group_changes(
     Ok(better_msg)
 }
 
+static LIST_ID_REGEX: Lazy<Regex> = Lazy::new(|| Regex::new(r"^(.+)<(.+)>$").unwrap());
+
 /// Create or lookup a mailing list chat.
 ///
 /// `list_id_header` contains the Id that must be used for the mailing list
@@ -1937,23 +1915,71 @@ async fn create_or_lookup_mailinglist(
     list_id_header: &str,
     mime_parser: &MimeMessage,
 ) -> Result<Option<(ChatId, Blocked)>> {
-    static LIST_ID: Lazy<Regex> = Lazy::new(|| Regex::new(r"^(.+)<(.+)>$").unwrap());
-    let (mut name, listid) = match LIST_ID.captures(list_id_header) {
-        Some(cap) => (cap[1].trim().to_string(), cap[2].trim().to_string()),
-        None => (
-            "".to_string(),
-            list_id_header
-                .trim()
-                .trim_start_matches('<')
-                .trim_end_matches('>')
-                .to_string(),
-        ),
+    let listid = match LIST_ID_REGEX.captures(list_id_header) {
+        Some(cap) => cap[2].trim().to_string(),
+        None => list_id_header
+            .trim()
+            .trim_start_matches('<')
+            .trim_end_matches('>')
+            .to_string(),
     };
 
     if let Some((chat_id, _, blocked)) = chat::get_chat_id_by_grpid(context, &listid).await? {
         return Ok(Some((chat_id, blocked)));
     }
 
+    let name = compute_mailinglist_name(list_id_header, &listid, mime_parser);
+
+    if allow_creation {
+        // list does not exist but should be created
+        let param = mime_parser.list_post.as_ref().map(|list_post| {
+            let mut p = Params::new();
+            p.set(Param::ListPost, list_post);
+            p.to_string()
+        });
+
+        let is_bot = context.get_config_bool(Config::Bot).await?;
+        let blocked = if is_bot {
+            Blocked::Not
+        } else {
+            Blocked::Request
+        };
+        let chat_id = ChatId::create_multiuser_record(
+            context,
+            Chattype::Mailinglist,
+            &listid,
+            &name,
+            blocked,
+            ProtectionStatus::Unprotected,
+            param,
+        )
+        .await
+        .with_context(|| {
+            format!(
+                "failed to create mailinglist '{}' for grpid={}",
+                &name, &listid
+            )
+        })?;
+
+        chat::add_to_chat_contacts_table(context, chat_id, &[ContactId::SELF]).await?;
+        Ok(Some((chat_id, blocked)))
+    } else {
+        info!(context, "Creating list forbidden by caller.");
+        Ok(None)
+    }
+}
+
+#[allow(clippy::indexing_slicing)]
+fn compute_mailinglist_name(
+    list_id_header: &str,
+    listid: &str,
+    mime_parser: &MimeMessage,
+) -> String {
+    let mut name = match LIST_ID_REGEX.captures(list_id_header) {
+        Some(cap) => cap[1].trim().to_string(),
+        None => "".to_string(),
+    };
+
     // for mailchimp lists, the name in `ListId` is just a long number.
     // a usable name for these lists is in the `From` header
     // and we can detect these lists by a unique `ListId`-suffix.
@@ -1997,50 +2023,14 @@ async fn create_or_lookup_mailinglist(
         // 51231231231231231231231232869f58.xing.com -> xing.com
         static PREFIX_32_CHARS_HEX: Lazy<Regex> =
             Lazy::new(|| Regex::new(r"([0-9a-fA-F]{32})\.(.{6,})").unwrap());
-        if let Some(cap) = PREFIX_32_CHARS_HEX.captures(&listid) {
+        if let Some(cap) = PREFIX_32_CHARS_HEX.captures(listid) {
             name = cap[2].to_string();
         } else {
-            name = listid.clone();
+            name = listid.to_string();
         }
     }
 
-    if allow_creation {
-        // list does not exist but should be created
-        let param = mime_parser.list_post.as_ref().map(|list_post| {
-            let mut p = Params::new();
-            p.set(Param::ListPost, list_post);
-            p.to_string()
-        });
-
-        let is_bot = context.get_config_bool(Config::Bot).await?;
-        let blocked = if is_bot {
-            Blocked::Not
-        } else {
-            Blocked::Request
-        };
-        let chat_id = ChatId::create_multiuser_record(
-            context,
-            Chattype::Mailinglist,
-            &listid,
-            &name,
-            blocked,
-            ProtectionStatus::Unprotected,
-            param,
-        )
-        .await
-        .with_context(|| {
-            format!(
-                "failed to create mailinglist '{}' for grpid={}",
-                &name, &listid
-            )
-        })?;
-
-        chat::add_to_chat_contacts_table(context, chat_id, &[ContactId::SELF]).await?;
-        Ok(Some((chat_id, blocked)))
-    } else {
-        info!(context, "Creating list forbidden by caller.");
-        Ok(None)
-    }
+    strip_rtlo_characters(&name)
 }
 
 /// Set ListId param on the contact and ListPost param the chat.
@@ -2049,9 +2039,10 @@ async fn create_or_lookup_mailinglist(
 async fn apply_mailinglist_changes(
     context: &Context,
     mime_parser: &MimeMessage,
+    sent_timestamp: i64,
     chat_id: ChatId,
 ) -> Result<()> {
-    let Some(list_post) = &mime_parser.list_post else {
+    let Some(mailinglist_header) = mime_parser.get_mailinglist_header() else {
         return Ok(());
     };
 
@@ -2061,6 +2052,24 @@ async fn apply_mailinglist_changes(
     }
     let listid = &chat.grpid;
 
+    let new_name = compute_mailinglist_name(mailinglist_header, listid, mime_parser);
+    if chat.name != new_name
+        && chat_id
+            .update_timestamp(context, Param::GroupNameTimestamp, sent_timestamp)
+            .await?
+    {
+        info!(context, "Updating listname for chat {chat_id}.");
+        context
+            .sql
+            .execute("UPDATE chats SET name=? WHERE id=?;", (new_name, chat_id))
+            .await?;
+        context.emit_event(EventType::ChatModified(chat_id));
+    }
+
+    let Some(list_post) = &mime_parser.list_post else {
+        return Ok(());
+    };
+
     let list_post = match ContactAddress::new(list_post) {
         Ok(list_post) => list_post,
         Err(err) => {