Save subject for messages 2: Outgoing messages (#2283)

* Save subject for sent-out messages

* Test that subject is saved (outgoing)

* Correctly set subject when forwarding messages, add test for this

src/chat.rs

@@ -28,7 +28,7 @@ use crate::context::Context;
 use crate::dc_tools::{
     dc_create_id, dc_create_outgoing_rfc724_mid, dc_create_smeared_timestamp,
     dc_create_smeared_timestamps, dc_get_abs_path, dc_gm2local_offset, improve_single_line_input,
-    time, IsNoneOrEmpty,
+    remove_subject_prefix, time, IsNoneOrEmpty,
 };
 use crate::ephemeral::{delete_expired_messages, schedule_ephemeral_task, Timer as EphemeralTimer};
 use crate::events::EventType;
@@ -1143,6 +1143,7 @@ impl Chat {
                         type,
                         state,
                         txt,
+                        subject,
                         param,
                         hidden,
                         mime_in_reply_to,
@@ -1152,7 +1153,7 @@ impl Chat {
                         location_id,
                         ephemeral_timer,
                         ephemeral_timestamp)
-                        VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?);",
+                        VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?);",
                 paramsv![
                     new_rfc724_mid,
                     self.id,
@@ -1162,6 +1163,7 @@ impl Chat {
                     msg.viewtype,
                     msg.state,
                     msg.text.as_ref().cloned().unwrap_or_default(),
+                    &msg.subject,
                     msg.param.to_string(),
                     msg.hidden,
                     msg.in_reply_to.as_deref().unwrap_or_default(),
@@ -2779,6 +2781,8 @@ pub async fn forward_msgs(
             msg.param.remove(Param::Cmd);
             msg.param.remove(Param::OverrideSenderDisplayname);
 
+            msg.subject = format!("Fwd: {}", remove_subject_prefix(&msg.subject));
+
             let new_msg_id: MsgId;
             if msg.state == MessageState::OutPreparing {
                 let fresh9 = curr_timestamp;

src/dc_tools.rs

@@ -662,6 +662,26 @@ where
     }
 }
 
+pub fn remove_subject_prefix(last_subject: &str) -> String {
+    let subject_start = if last_subject.starts_with("Chat:") {
+        0
+    } else {
+        // "Antw:" is the longest abbreviation in
+        // https://en.wikipedia.org/wiki/List_of_email_subject_abbreviations#Abbreviations_in_other_languages,
+        // so look at the first _5_ characters:
+        match last_subject.chars().take(5).position(|c| c == ':') {
+            Some(prefix_end) => prefix_end + 1,
+            None => 0,
+        }
+    };
+    last_subject
+        .chars()
+        .skip(subject_start)
+        .collect::<String>()
+        .trim()
+        .to_string()
+}
+
 #[cfg(test)]
 mod tests {
     #![allow(clippy::indexing_slicing)]

src/job.rs

@@ -1019,6 +1019,9 @@ pub async fn send_msg_job(context: &Context, msg_id: MsgId) -> Result<Option<Job
     param.set(Param::File, blob.as_name());
     param.set(Param::Recipients, &recipients);
 
+    msg.subject = rendered_msg.subject.clone();
+    msg.update_subject(context).await;
+
     let job = create(Action::SendMsgToSmtp, msg_id.to_u32() as i32, param, 0)?;
 
     Ok(Some(job))

src/message.rs

@@ -22,6 +22,7 @@ use crate::dc_tools::{
 use crate::ephemeral::Timer as EphemeralTimer;
 use crate::events::EventType;
 use crate::job::{self, Action};
+use crate::log::LogExt;
 use crate::lot::{Lot, LotState, Meaning};
 use crate::mimeparser::{FailureReport, SystemMessage};
 use crate::param::{Param, Params};
@@ -307,7 +308,6 @@ pub struct Message {
     pub(crate) ephemeral_timer: EphemeralTimer,
     pub(crate) ephemeral_timestamp: i64,
     pub(crate) text: Option<String>,
-    /// The value of the Subject header. Not set for messages that we sent ourselves.
     pub(crate) subject: String,
     pub(crate) rfc724_mid: String,
     pub(crate) in_reply_to: Option<String>,
@@ -912,7 +912,7 @@ impl Message {
         Ok(None)
     }
 
-    pub async fn update_param(&mut self, context: &Context) -> bool {
+    pub async fn update_param(&self, context: &Context) {
         context
             .sql
             .execute(
@@ -920,7 +920,18 @@ impl Message {
                 paramsv![self.param.to_string(), self.id],
             )
             .await
-            .is_ok()
+            .ok_or_log(context);
+    }
+
+    pub(crate) async fn update_subject(&self, context: &Context) {
+        context
+            .sql
+            .execute(
+                "UPDATE msgs SET subject=? WHERE id=?;",
+                paramsv![self.subject, self.id],
+            )
+            .await
+            .ok_or_log(context);
     }
 
     /// Gets the error status of the message.

src/mimefactory.rs

@@ -6,7 +6,8 @@ use crate::contact::Contact;
 use crate::context::{get_version_str, Context};
 use crate::dc_tools::IsNoneOrEmpty;
 use crate::dc_tools::{
-    dc_create_outgoing_rfc724_mid, dc_create_smeared_timestamp, dc_get_filebytes, time,
+    dc_create_outgoing_rfc724_mid, dc_create_smeared_timestamp, dc_get_filebytes,
+    remove_subject_prefix, time,
 };
 use crate::e2ee::EncryptHelper;
 use crate::ephemeral::Timer as EphemeralTimer;
@@ -65,7 +66,6 @@ pub struct MimeFactory<'a> {
     req_mdn: bool,
     last_added_location_id: u32,
     attach_selfavatar: bool,
-    quoted_msg_subject: Option<String>,
 }
 
 /// Result of rendering a message, ready to be submitted to a send job.
@@ -79,6 +79,7 @@ pub struct RenderedEmail {
 
     /// Message ID (Message in the sense of Email)
     pub rfc724_mid: String,
+    pub subject: String,
 }
 
 impl<'a> MimeFactory<'a> {
@@ -176,7 +177,6 @@ impl<'a> MimeFactory<'a> {
             req_mdn,
             last_added_location_id: 0,
             attach_selfavatar,
-            quoted_msg_subject: msg.quoted_message(context).await?.map(|m| m.subject),
         };
         Ok(factory)
     }
@@ -221,7 +221,6 @@ impl<'a> MimeFactory<'a> {
             req_mdn: false,
             last_added_location_id: 0,
             attach_selfavatar: false,
-            quoted_msg_subject: None,
         };
 
         Ok(res)
@@ -356,13 +355,19 @@ impl<'a> MimeFactory<'a> {
     }
 
     async fn subject_str(&self, context: &Context) -> anyhow::Result<String> {
+        let quoted_msg_subject = self.msg.quoted_message(context).await?.map(|m| m.subject);
+
         Ok(match self.loaded {
             Loaded::Message { ref chat } => {
                 if self.msg.param.get_cmd() == SystemMessage::AutocryptSetupMessage {
                     return Ok(stock_str::ac_setup_msg_subject(context).await);
                 }
 
-                if chat.typ == Chattype::Group && self.quoted_msg_subject.is_none_or_empty() {
+                if !self.msg.subject.is_empty() {
+                    return Ok(self.msg.subject.clone());
+                }
+
+                if chat.typ == Chattype::Group && quoted_msg_subject.is_none_or_empty() {
                     // If we have a `quoted_msg_subject`, we use the subject of the quoted message
                     // instead of the group name
                     let re = if self.in_reply_to.is_empty() {
@@ -373,42 +378,21 @@ impl<'a> MimeFactory<'a> {
                     return Ok(format!("{}{}", re, chat.name));
                 }
 
-                let parent_subject = if self.quoted_msg_subject.is_none_or_empty() {
+                let parent_subject = if quoted_msg_subject.is_none_or_empty() {
                     chat.param.get(Param::LastSubject)
                 } else {
-                    self.quoted_msg_subject.as_deref()
+                    quoted_msg_subject.as_deref()
                 };
 
-                match parent_subject {
+                if let Some(last_subject) = parent_subject {
-                    Some(last_subject) => {
+                    format!("Re: {}", remove_subject_prefix(last_subject))
-                        let subject_start = if last_subject.starts_with("Chat:") {
+                } else {
-                            0
+                    let self_name = match context.get_config(Config::Displayname).await {
-                        } else {
+                        Some(name) => name,
-                            // "Antw:" is the longest abbreviation in
+                        None => context.get_config(Config::Addr).await.unwrap_or_default(),
-                            // https://en.wikipedia.org/wiki/List_of_email_subject_abbreviations#Abbreviations_in_other_languages,
+                    };
-                            // so look at the first _5_ characters:
-                            match last_subject.chars().take(5).position(|c| c == ':') {
-                                Some(prefix_end) => prefix_end + 1,
-                                None => 0,
-                            }
-                        };
-                        format!(
-                            "Re: {}",
-                            last_subject
-                                .chars()
-                                .skip(subject_start)
-                                .collect::<String>()
-                                .trim()
-                        )
-                    }
-                    None => {
-                        let self_name = match context.get_config(Config::Displayname).await {
-                            Some(name) => name,
-                            None => context.get_config(Config::Addr).await.unwrap_or_default(),
-                        };
 
-                        stock_str::subject_for_new_contact(context, self_name).await
+                    stock_str::subject_for_new_contact(context, self_name).await
-                    }
                 }
             }
             Loaded::MDN { .. } => stock_str::read_rcpt(context).await,
@@ -497,13 +481,13 @@ impl<'a> MimeFactory<'a> {
         let e2ee_guaranteed = self.is_e2ee_guaranteed();
         let encrypt_helper = EncryptHelper::new(context).await?;
 
-        let subject = if subject_str
+        let encoded_subject = if subject_str
             .chars()
             .all(|c| c.is_ascii_alphanumeric() || c == ' ')
         // We do not use needs_encoding() here because needs_encoding() returns true if the string contains a space
         // but we do not want to encode all subjects just because they contain a space.
         {
-            subject_str
+            subject_str.clone()
         } else {
             encode_words(&subject_str)
         };
@@ -514,7 +498,7 @@ impl<'a> MimeFactory<'a> {
             unprotected_headers.push(Header::new("Autocrypt".into(), aheader));
         }
 
-        protected_headers.push(Header::new("Subject".into(), subject));
+        protected_headers.push(Header::new("Subject".into(), encoded_subject));
 
         let rfc724_mid = match self.loaded {
             Loaded::Message { .. } => self.msg.rfc724_mid.clone(),
@@ -671,6 +655,7 @@ impl<'a> MimeFactory<'a> {
             is_gossiped,
             last_added_location_id,
             rfc724_mid,
+            subject: subject_str,
         })
     }
 
@@ -1551,15 +1536,6 @@ mod tests {
 
     #[async_std::test]
     async fn test_subject_in_group() {
-        // 6. Test that in a group, replies also take the quoted message's subject, while non-replies use the group title as subject
-        let t = TestContext::new_alice().await;
-        let group_id =
-            chat::create_group_chat(&t, chat::ProtectionStatus::Unprotected, "groupname")
-                .await
-                .unwrap();
-        let bob = Contact::create(&t, "", "bob@example.org").await.unwrap();
-        chat::add_contact_to_chat(&t, group_id, bob).await;
-
         async fn send_msg_get_subject(
             t: &TestContext,
             group_id: ChatId,
@@ -1571,8 +1547,25 @@ mod tests {
                 new_msg.set_quote(t, q).await.unwrap();
             }
             let sent = t.send_msg(group_id, &mut new_msg).await;
-            t.parse_msg(&sent).await.get_subject().unwrap()
+            get_subject(t, sent).await
         }
+        async fn get_subject(t: &TestContext, sent: crate::test_utils::SentMessage) -> String {
+            let parsed_subject = t.parse_msg(&sent).await.get_subject().unwrap();
+
+            let sent_msg = Message::load_from_db(t, sent.sender_msg_id).await.unwrap();
+            assert_eq!(parsed_subject, sent_msg.subject);
+
+            parsed_subject
+        }
+
+        // 6. Test that in a group, replies also take the quoted message's subject, while non-replies use the group title as subject
+        let t = TestContext::new_alice().await;
+        let group_id =
+            chat::create_group_chat(&t, chat::ProtectionStatus::Unprotected, "groupname") // TODO encodings, ä
+                .await
+                .unwrap();
+        let bob = Contact::create(&t, "", "bob@example.org").await.unwrap();
+        chat::add_contact_to_chat(&t, group_id, bob).await;
 
         let subject = send_msg_get_subject(&t, group_id, None).await;
         assert_eq!(subject, "groupname");
@@ -1607,10 +1600,20 @@ mod tests {
         assert_eq!(subject, "Re: groupname");
 
         let subject = send_msg_get_subject(&t, group_id, Some(&message_from_bob)).await;
+        let outgoing_quoting_msg = t.get_last_msg().await;
         assert_eq!(subject, "Re: Different subject");
 
         let subject = send_msg_get_subject(&t, group_id, None).await;
         assert_eq!(subject, "Re: groupname");
+
+        let subject = send_msg_get_subject(&t, group_id, Some(&outgoing_quoting_msg)).await;
+        assert_eq!(subject, "Re: Different subject");
+
+        chat::forward_msgs(&t, &[message_from_bob.id], group_id)
+            .await
+            .unwrap();
+        let subject = get_subject(&t, t.pop_sent_msg().await).await;
+        assert_eq!(subject, "Fwd: Different subject");
     }
 
     async fn first_subject_str(t: TestContext) -> String {