feat: Mark reactions as IMAP-seen in marknoticed_chat() (#6210)

When a reaction notification is shown in the UIs, there's an option "Mark Read", but the UIs are
unaware of reactions message ids, moreover there are no `msgs` rows for incoming reactions in the
core, so the UIs just call `marknoticed_chat()` in this case. We don't want to introduce reactions
message ids to the UIs (at least currently), but let's make received reactions usual messages, just
hidden and `InFresh`, so that the existing `\Seen` flag synchronisation mechanism works for them,
and mark the last fresh hidden incoming message in the chat as seen in `marknoticed_chat()` to
trigger emitting `MsgsNoticed` on other devices.

There's a problem though that another device may have more reactions received and not yet seen
notifications are removed from it when handling `MsgsNoticed`, but the same problem already exists
for "usual" messages, so let's not solve it for now.

It's interesting that sent out reactions are already hidden messages, so this change mostly just
unifies things.

Author: iequidoo

deltachat-rpc-client/src/deltachat_rpc_client/const.py

@@ -39,6 +39,7 @@ class EventType(str, Enum):
     ERROR_SELF_NOT_IN_GROUP = "ErrorSelfNotInGroup"
     MSGS_CHANGED = "MsgsChanged"
     REACTIONS_CHANGED = "ReactionsChanged"
+    INCOMING_REACTION = "IncomingReaction"
     INCOMING_MSG = "IncomingMsg"
     INCOMING_MSG_BUNCH = "IncomingMsgBunch"
     MSGS_NOTICED = "MsgsNoticed"

deltachat-rpc-client/tests/test_something.py

@@ -285,6 +285,43 @@ def test_message(acfactory) -> None:
     assert reactions == snapshot.reactions
 
 
+def test_reaction_seen_on_another_dev(acfactory, tmp_path) -> None:
+    alice, bob = acfactory.get_online_accounts(2)
+    alice.export_backup(tmp_path)
+    files = list(tmp_path.glob("*.tar"))
+    alice2 = acfactory.get_unconfigured_account()
+    alice2.import_backup(files[0])
+    alice2.start_io()
+
+    bob_addr = bob.get_config("addr")
+    alice_contact_bob = alice.create_contact(bob_addr, "Bob")
+    alice_chat_bob = alice_contact_bob.create_chat()
+    alice_chat_bob.send_text("Hello!")
+
+    event = bob.wait_for_incoming_msg_event()
+    msg_id = event.msg_id
+
+    message = bob.get_message_by_id(msg_id)
+    snapshot = message.get_snapshot()
+    snapshot.chat.accept()
+    message.send_reaction("😎")
+    for a in [alice, alice2]:
+        while True:
+            event = a.wait_for_event()
+            if event.kind == EventType.INCOMING_REACTION:
+                break
+
+    alice_chat_bob.mark_noticed()
+    while True:
+        event = alice2.wait_for_event()
+        if event.kind == EventType.MSGS_NOTICED:
+            chat_id = event.chat_id
+            break
+    alice2_contact_bob = alice2.get_contact_by_addr(bob_addr)
+    alice2_chat_bob = alice2_contact_bob.create_chat()
+    assert chat_id == alice2_chat_bob.id
+
+
 def test_is_bot(acfactory) -> None:
     """Test that we can recognize messages submitted by bots."""
     alice, bob = acfactory.get_online_accounts(2)

src/chat.rs

@@ -3290,10 +3290,10 @@ pub async fn marknoticed_chat(context: &Context, chat_id: ChatId) -> Result<()>
             .query_map(
                 "SELECT DISTINCT(m.chat_id) FROM msgs m
                     LEFT JOIN chats c ON m.chat_id=c.id
-                    WHERE m.state=10 AND m.hidden=0 AND m.chat_id>9 AND c.blocked=0 AND c.archived=1",
-                    (),
+                    WHERE m.state=10 AND m.chat_id>9 AND c.blocked=0 AND c.archived=1",
+                (),
                 |row| row.get::<_, ChatId>(0),
-                |ids| ids.collect::<Result<Vec<_>, _>>().map_err(Into::into)
+                |ids| ids.collect::<Result<Vec<_>, _>>().map_err(Into::into),
             )
             .await?;
         if chat_ids_in_archive.is_empty() {
@@ -3304,7 +3304,7 @@ pub async fn marknoticed_chat(context: &Context, chat_id: ChatId) -> Result<()>
             .sql
             .execute(
                 &format!(
-                    "UPDATE msgs SET state=13 WHERE state=10 AND hidden=0 AND chat_id IN ({});",
+                    "UPDATE msgs SET state=13 WHERE state=10 AND chat_id IN ({});",
                     sql::repeat_vars(chat_ids_in_archive.len())
                 ),
                 rusqlite::params_from_iter(&chat_ids_in_archive),
@@ -3314,20 +3314,39 @@ pub async fn marknoticed_chat(context: &Context, chat_id: ChatId) -> Result<()>
             context.emit_event(EventType::MsgsNoticed(chat_id_in_archive));
             chatlist_events::emit_chatlist_item_changed(context, chat_id_in_archive);
         }
-    } else if context
-        .sql
-        .execute(
-            "UPDATE msgs
-            SET state=?
-          WHERE state=?
-            AND hidden=0
-            AND chat_id=?;",
-            (MessageState::InNoticed, MessageState::InFresh, chat_id),
-        )
-        .await?
-        == 0
-    {
-        return Ok(());
+    } else {
+        let conn_fn = |conn: &mut rusqlite::Connection| {
+            let nr_msgs_noticed = conn.execute(
+                "UPDATE msgs
+                    SET state=?
+                    WHERE state=?
+                    AND hidden=0
+                    AND chat_id=?",
+                (MessageState::InNoticed, MessageState::InFresh, chat_id),
+            )?;
+            let mut stmt = conn.prepare(
+                "SELECT id FROM msgs
+                    WHERE state>=? AND state<?
+                    AND hidden=1
+                    AND chat_id=?
+                    ORDER BY id DESC LIMIT 100",
+            )?;
+            let hidden_msgs = stmt
+                .query_map(
+                    (MessageState::InFresh, MessageState::InSeen, chat_id),
+                    |row| {
+                        let id: MsgId = row.get(0)?;
+                        Ok(id)
+                    },
+                )?
+                .collect::<std::result::Result<Vec<_>, _>>()?;
+            Ok((nr_msgs_noticed, hidden_msgs))
+        };
+        let (nr_msgs_noticed, hidden_msgs) = context.sql.call_write(conn_fn).await?;
+        if nr_msgs_noticed == 0 && hidden_msgs.is_empty() {
+            return Ok(());
+        }
+        message::markseen_msgs(context, hidden_msgs).await?;
     }
 
     context.emit_event(EventType::MsgsNoticed(chat_id));
@@ -3372,7 +3391,6 @@ pub(crate) async fn mark_old_messages_as_noticed(
                     "UPDATE msgs
             SET state=?
           WHERE state=?
-            AND hidden=0
             AND chat_id=?
             AND timestamp<=?;",
                     (

src/reaction.rs

@@ -397,7 +397,7 @@ mod tests {
     use deltachat_contact_tools::ContactAddress;
 
     use super::*;
-    use crate::chat::{forward_msgs, get_chat_msgs, send_text_msg};
+    use crate::chat::{forward_msgs, get_chat_msgs, marknoticed_chat, send_text_msg};
     use crate::chatlist::Chatlist;
     use crate::config::Config;
     use crate::contact::{Contact, Origin};
@@ -663,7 +663,8 @@ Here's my footer -- bob@example.net"
         assert_eq!(get_chat_msgs(&bob, bob_msg.chat_id).await?.len(), 2);
 
         let bob_reaction_msg = bob.pop_sent_msg().await;
-        alice.recv_msg_trash(&bob_reaction_msg).await;
+        let alice_reaction_msg = alice.recv_msg_hidden(&bob_reaction_msg).await;
+        assert_eq!(alice_reaction_msg.state, MessageState::InFresh);
         assert_eq!(get_chat_msgs(&alice, chat_alice.id).await?.len(), 2);
 
         let reactions = get_msg_reactions(&alice, alice_msg.sender_msg_id).await?;
@@ -680,6 +681,20 @@ Here's my footer -- bob@example.net"
         expect_incoming_reactions_event(&alice, alice_msg.sender_msg_id, *bob_id, "👍").await?;
         expect_no_unwanted_events(&alice).await;
 
+        marknoticed_chat(&alice, chat_alice.id).await?;
+        assert_eq!(
+            alice_reaction_msg.id.get_state(&alice).await?,
+            MessageState::InSeen
+        );
+        // Reactions don't request MDNs.
+        assert_eq!(
+            alice
+                .sql
+                .count("SELECT COUNT(*) FROM smtp_mdns", ())
+                .await?,
+            0
+        );
+
         // Alice reacts to own message.
         send_reaction(&alice, alice_msg.sender_msg_id, "👍 😀")
             .await
@@ -719,7 +734,7 @@ Here's my footer -- bob@example.net"
         bob_msg1.chat_id.accept(&bob).await?;
         send_reaction(&bob, bob_msg1.id, "👍").await?;
         let bob_send_reaction = bob.pop_sent_msg().await;
-        alice.recv_msg_trash(&bob_send_reaction).await;
+        alice.recv_msg_hidden(&bob_send_reaction).await;
         expect_incoming_reactions_event(&alice, alice_msg1.sender_msg_id, alice_bob_id, "👍")
             .await?;
         expect_no_unwanted_events(&alice).await;
@@ -882,7 +897,7 @@ Here's my footer -- bob@example.net"
         let bob_reaction_msg = bob.pop_sent_msg().await;
 
         // Alice receives a reaction.
-        alice.recv_msg_trash(&bob_reaction_msg).await;
+        alice.recv_msg_hidden(&bob_reaction_msg).await;
 
         let reactions = get_msg_reactions(&alice, alice_msg_id).await?;
         assert_eq!(reactions.to_string(), "👍1");
@@ -934,7 +949,7 @@ Here's my footer -- bob@example.net"
         {
             send_reaction(&alice2, alice2_msg.id, "👍").await?;
             let msg = alice2.pop_sent_msg().await;
-            alice1.recv_msg_trash(&msg).await;
+            alice1.recv_msg_hidden(&msg).await;
         }
 
         // Check that the status is still the same.
@@ -956,7 +971,7 @@ Here's my footer -- bob@example.net"
         let alice1_msg = alice1.recv_msg(&alice0.pop_sent_msg().await).await;
 
         send_reaction(&alice0, alice0_msg_id, "👀").await?;
-        alice1.recv_msg_trash(&alice0.pop_sent_msg().await).await;
+        alice1.recv_msg_hidden(&alice0.pop_sent_msg().await).await;
 
         expect_reactions_changed_event(&alice0, chat_id, alice0_msg_id, ContactId::SELF).await?;
         expect_reactions_changed_event(&alice1, alice1_msg.chat_id, alice1_msg.id, ContactId::SELF)

src/receive_imf.rs

@@ -760,7 +760,7 @@ async fn add_parts(
     // (of course, the user can add other chats manually later)
     let to_id: ContactId;
     let state: MessageState;
-    let mut hidden = false;
+    let mut hidden = is_reaction;
     let mut needs_delete_job = false;
     let mut restore_protection = false;
 
@@ -1018,12 +1018,7 @@ async fn add_parts(
             }
         }
 
-        state = if seen
-            || fetching_existing_messages
-            || is_mdn
-            || is_reaction
-            || chat_id_blocked == Blocked::Yes
-        {
+        state = if seen || fetching_existing_messages || is_mdn || chat_id_blocked == Blocked::Yes {
             MessageState::InSeen
         } else {
             MessageState::InFresh
@@ -1232,7 +1227,7 @@ async fn add_parts(
     }
 
     let orig_chat_id = chat_id;
-    let mut chat_id = if is_mdn || is_reaction {
+    let mut chat_id = if is_mdn {
         DC_CHAT_ID_TRASH
     } else {
         chat_id.unwrap_or_else(|| {
@@ -1407,7 +1402,7 @@ async fn add_parts(
     // that the ui should show button to display the full message.
 
     // a flag used to avoid adding "show full message" button to multiple parts of the message.
-    let mut save_mime_modified = mime_parser.is_mime_modified;
+    let mut save_mime_modified = mime_parser.is_mime_modified && !hidden;
 
     let mime_headers = if save_mime_headers || save_mime_modified {
         let headers = if !mime_parser.decoded_data.is_empty() {
@@ -1599,10 +1594,10 @@ RETURNING id
                     state,
                     is_dc_message,
                     if trash { "" } else { msg },
-                    if trash { None } else { message::normalize_text(msg) },
-                    if trash { "" } else { &subject },
+                    if trash || hidden { None } else { message::normalize_text(msg) },
+                    if trash || hidden { "" } else { &subject },
                     // txt_raw might contain invalid utf8
-                    if trash { "" } else { &txt_raw },
+                    if trash || hidden { "" } else { &txt_raw },
                     if trash {
                         "".to_string()
                     } else {
@@ -1618,7 +1613,7 @@ RETURNING id
                     mime_in_reply_to,
                     mime_references,
                     mime_modified,
-                    part.error.as_deref().unwrap_or_default(),
+                    if trash || hidden { "" } else { part.error.as_deref().unwrap_or_default() },
                     ephemeral_timer,
                     ephemeral_timestamp,
                     if is_partial_download.is_some() {
@@ -1628,7 +1623,7 @@ RETURNING id
                     } else {
                         DownloadState::Done
                     },
-                    mime_parser.hop_info
+                    if trash || hidden { "" } else { &mime_parser.hop_info },
                 ],
                 |row| {
                     let msg_id: MsgId = row.get(0)?;

src/test_utils.rs

@@ -606,6 +606,19 @@ impl TestContext {
         msg
     }
 
+    /// Receive a message using the `receive_imf()` pipeline. Panics if it's not hidden.
+    pub async fn recv_msg_hidden(&self, msg: &SentMessage<'_>) -> Message {
+        let received = self
+            .recv_msg_opt(msg)
+            .await
+            .expect("receive_imf() seems not to have added a new message to the db");
+        let msg = Message::load_from_db(self, *received.msg_ids.last().unwrap())
+            .await
+            .unwrap();
+        assert!(msg.hidden);
+        msg
+    }
+
     /// Receive a message using the `receive_imf()` pipeline. This is similar
     /// to `recv_msg()`, but doesn't assume that the message is shown in the chat.
     pub async fn recv_msg_opt(