fix: markseen_msgs(): Mark reactions to specified messages as seen too (#7884)

This allows to remove notifications for reactions from other devices. NB: UIs should pass all
messages to markseen_msgs(), incl. outgoing ones. markseen_msgs() should be called when a message
comes into view or when a reaction for a message being in view arrives.

Also don't emit `MsgsNoticed` from receive_imf_inner() if the chat still contains fresh hidden
messages, i.e. include reactions into this logic, to avoid removing notifications for reactions
until they are seen on another device.

Author: iequidoo

deltachat-ffi/deltachat.h

@@ -2104,9 +2104,12 @@ int             dc_resend_msgs               (dc_context_t* context, const uint3
 
 
 /**
- * Mark messages as presented to the user.
+ * Mark messages and reactions to them as presented to the user.
  * Typically, UIs call this function on scrolling through the message list,
  * when the messages are presented at least for a little moment.
+ * UIs should pass all messages to this function, incl. outgoing and info ones, as this is used also
+ * for synchronization and to track last position.
+ * This should also be called when a reaction for a message being in view arrives.
  * The concrete action depends on the type of the chat and on the users settings
  * (dc_msgs_presented() may be a better name therefore, but well. :)
  *

deltachat-jsonrpc/src/api.rs

@@ -1301,6 +1301,9 @@ impl CommandApi {
     /// Mark messages as presented to the user.
     /// Typically, UIs call this function on scrolling through the message list,
     /// when the messages are presented at least for a little moment.
+    /// UIs should pass all messages to this function, incl. outgoing and info ones, as this is used
+    /// also for synchronization and to track last position.
+    /// This should also be called when a reaction for a message being in view arrives.
     /// The concrete action depends on the type of the chat and on the users settings
     /// (dc_msgs_presented() may be a better name therefore, but well. :)
     ///

deltachat-rpc-client/tests/test_something.py

@@ -413,27 +413,32 @@ def test_dont_move_sync_msgs(acfactory, direct_imap):
         time.sleep(1)
 
 
-def test_reaction_seen_on_another_dev(acfactory) -> None:
+@pytest.mark.parametrize("chat_noticed", [False, True])
+def test_reaction_seen_on_another_dev(acfactory, chat_noticed) -> None:
     alice, bob = acfactory.get_online_accounts(2)
     alice2 = alice.clone()
     alice2.start_io()
 
     alice_contact_bob = alice.create_contact(bob, "Bob")
     alice_chat_bob = alice_contact_bob.create_chat()
-    alice_chat_bob.send_text("Hello!")
+    alice_msg = 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()
+    bob_msg = bob.get_message_by_id(msg_id)
+    snapshot = bob_msg.get_snapshot()
     snapshot.chat.accept()
-    message.send_reaction("😎")
+    bob_msg.send_reaction("😎")
     for a in [alice, alice2]:
         a.wait_for_event(EventType.INCOMING_REACTION)
 
     alice2.clear_all_events()
-    alice_chat_bob.mark_noticed()
+    if chat_noticed:
+        alice_chat_bob.mark_noticed()
+    else:
+        # UIs are supposed to mark messages being in view as seen, not reactions themselves.
+        alice_msg.mark_seen()
     chat_id = alice2.wait_for_event(EventType.MSGS_NOTICED).chat_id
     alice2_chat_bob = alice2.create_chat(bob)
     assert chat_id == alice2_chat_bob.id

src/message.rs

@@ -1829,7 +1829,10 @@ pub async fn delete_msgs_ex(
     Ok(())
 }
 
-/// Marks requested messages as seen.
+/// Marks requested messages and reactions to them as seen.
+/// NB: UIs should pass all messages to this function, incl. outgoing and info ones, as this is used
+/// also for synchronization and to track last position. This should be called when a message comes
+/// into view or when a reaction for a message being in view arrives.
 pub async fn markseen_msgs(context: &Context, msg_ids: Vec<MsgId>) -> Result<()> {
     if msg_ids.is_empty() {
         return Ok(());
@@ -1843,10 +1846,18 @@ pub async fn markseen_msgs(context: &Context, msg_ids: Vec<MsgId>) -> Result<()>
 
     let mut msgs = Vec::with_capacity(msg_ids.len());
     for &id in &msg_ids {
-        if let Some(msg) = context
+        let Some(rfc724_mid): Option<String> = context
             .sql
-            .query_row_optional(
+            .query_get_value("SELECT rfc724_mid FROM msgs WHERE id=?", (id,))
+            .await?
+        else {
+            continue;
+        };
+        context
+            .sql
+            .query_map(
                 "SELECT
+                    m.id AS id,
                     m.chat_id AS chat_id,
                     m.state AS state,
                     m.ephemeral_timer AS ephemeral_timer,
@@ -1857,9 +1868,11 @@ pub async fn markseen_msgs(context: &Context, msg_ids: Vec<MsgId>) -> Result<()>
                     c.archived AS archived,
                     c.blocked AS blocked
                  FROM msgs m LEFT JOIN chats c ON c.id=m.chat_id
-                 WHERE m.id=? AND m.chat_id>9",
-                (id,),
+                WHERE (m.id=? OR m.mime_in_reply_to=? AND m.hidden=1)
+                    AND m.chat_id>9 AND ?<=m.state AND m.state<?",
+                (id, rfc724_mid, MessageState::InFresh, MessageState::InSeen),
                 |row| {
+                    let id: MsgId = row.get("id")?;
                     let chat_id: ChatId = row.get("chat_id")?;
                     let state: MessageState = row.get("state")?;
                     let param: Params = row.get::<_, String>("param")?.parse().unwrap_or_default();
@@ -1884,11 +1897,14 @@ pub async fn markseen_msgs(context: &Context, msg_ids: Vec<MsgId>) -> Result<()>
                         ephemeral_timer,
                     ))
                 },
+                |rows| {
+                    for row in rows {
+                        msgs.push(row?);
+                    }
+                    Ok(())
+                },
             )
-            .await?
-        {
-            msgs.push(msg);
-        }
+            .await?;
     }
 
     if msgs
@@ -1917,48 +1933,45 @@ pub async fn markseen_msgs(context: &Context, msg_ids: Vec<MsgId>) -> Result<()>
         _curr_ephemeral_timer,
     ) in msgs
     {
-        if curr_state == MessageState::InFresh || curr_state == MessageState::InNoticed {
-            update_msg_state(context, id, MessageState::InSeen).await?;
-            info!(context, "Seen message {}.", id);
-
-            markseen_on_imap_table(context, &curr_rfc724_mid).await?;
-
-            // Read receipts for system messages are never sent to contacts.
-            // These messages have no place to display received read receipt
-            // anyway. And since their text is locally generated,
-            // quoting them is dangerous as it may contain contact names. E.g., for original message
-            // "Group left by me", a read receipt will quote "Group left by <name>", and the name can
-            // be a display name stored in address book rather than the name sent in the From field by
-            // the user.
-            //
-            // We also don't send read receipts for contact requests.
-            // Read receipts will not be sent even after accepting the chat.
-            let to_id = if curr_blocked == Blocked::Not
-                && !curr_hidden
-                && curr_param.get_bool(Param::WantsMdn).unwrap_or_default()
-                && curr_param.get_cmd() == SystemMessage::Unknown
-                && context.should_send_mdns().await?
-            {
-                Some(curr_from_id)
-            } else if context.get_config_bool(Config::BccSelf).await? {
-                Some(ContactId::SELF)
-            } else {
-                None
-            };
-            if let Some(to_id) = to_id {
-                context
-                    .sql
-                    .execute(
-                        "INSERT INTO smtp_mdns (msg_id, from_id, rfc724_mid) VALUES(?, ?, ?)",
-                        (id, to_id, curr_rfc724_mid),
-                    )
-                    .await
-                    .context("failed to insert into smtp_mdns")?;
-                context.scheduler.interrupt_smtp().await;
-            }
-            if !curr_hidden {
-                updated_chat_ids.insert(curr_chat_id);
-            }
+        update_msg_state(context, id, MessageState::InSeen).await?;
+        info!(context, "Seen message {}.", id);
+
+        markseen_on_imap_table(context, &curr_rfc724_mid).await?;
+
+        // Read receipts for system messages are never sent to contacts. These messages have no
+        // place to display received read receipt anyway. And since their text is locally generated,
+        // quoting them is dangerous as it may contain contact names. E.g., for original message
+        // "Group left by me", a read receipt will quote "Group left by <name>", and the name can be
+        // a display name stored in address book rather than the name sent in the From field by the
+        // user.
+        //
+        // We also don't send read receipts for contact requests. Read receipts will not be sent
+        // even after accepting the chat.
+        let to_id = if curr_blocked == Blocked::Not
+            && !curr_hidden
+            && curr_param.get_bool(Param::WantsMdn).unwrap_or_default()
+            && curr_param.get_cmd() == SystemMessage::Unknown
+            && context.should_send_mdns().await?
+        {
+            Some(curr_from_id)
+        } else if context.get_config_bool(Config::BccSelf).await? {
+            Some(ContactId::SELF)
+        } else {
+            None
+        };
+        if let Some(to_id) = to_id {
+            context
+                .sql
+                .execute(
+                    "INSERT INTO smtp_mdns (msg_id, from_id, rfc724_mid) VALUES(?, ?, ?)",
+                    (id, to_id, curr_rfc724_mid),
+                )
+                .await
+                .context("failed to insert into smtp_mdns")?;
+            context.scheduler.interrupt_smtp().await;
+        }
+        if !curr_hidden {
+            updated_chat_ids.insert(curr_chat_id);
         }
         archived_chats_maybe_noticed |= curr_state == MessageState::InFresh
             && !curr_hidden

src/reaction.rs

@@ -1108,4 +1108,37 @@ Content-Transfer-Encoding: 7bit\r
 
         Ok(())
     }
+
+    #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+    async fn test_markseen_referenced_msg() -> Result<()> {
+        let mut tcm = TestContextManager::new();
+        let alice = &tcm.alice().await;
+        let bob = &tcm.bob().await;
+        let chat_id = alice.create_chat(bob).await.id;
+
+        let alice_msg_id = send_text_msg(alice, chat_id, "foo".to_string()).await?;
+        let sent_msg = alice.pop_sent_msg().await;
+        let bob_msg = bob.recv_msg(&sent_msg).await;
+        bob_msg.chat_id.accept(bob).await?;
+
+        send_reaction(bob, bob_msg.id, "👀").await?;
+        let sent_msg = bob.pop_sent_msg().await;
+        let alice_reaction = alice.recv_msg_hidden(&sent_msg).await;
+        assert_eq!(alice_reaction.state, MessageState::InFresh);
+
+        markseen_msgs(alice, vec![alice_msg_id]).await?;
+        let alice_reaction = Message::load_from_db(alice, alice_reaction.id).await?;
+        assert_eq!(alice_reaction.state, MessageState::InSeen);
+        assert_eq!(
+            alice
+                .sql
+                .count(
+                    "SELECT COUNT(*) FROM smtp_mdns WHERE from_id=?",
+                    (ContactId::SELF,)
+                )
+                .await?,
+            1
+        );
+        Ok(())
+    }
 }

src/receive_imf.rs

@@ -35,7 +35,8 @@ use crate::message::{
     rfc724_mid_exists,
 };
 use crate::mimeparser::{
-    AvatarAction, GossipedKey, MimeMessage, PreMessageMode, SystemMessage, parse_message_ids,
+    AvatarAction, GossipedKey, MimeMessage, PreMessageMode, SystemMessage, parse_message_id,
+    parse_message_ids,
 };
 use crate::param::{Param, Params};
 use crate::peer_channels::{add_gossip_peer_from_header, insert_topic_stub, iroh_topic_from_str};
@@ -1023,7 +1024,7 @@ UPDATE config SET value=? WHERE keyname='configured_addr' AND value!=?1
                         "
 UPDATE msgs SET state=? WHERE
     state=? AND
-    hidden=0 AND
+    (hidden=0 OR hidden=1) AND
     chat_id=? AND
     timestamp<?",
                         (
@@ -1035,7 +1036,18 @@ UPDATE msgs SET state=? WHERE
                     )
                     .await
                     .context("UPDATE msgs.state")?;
-                if chat_id.get_fresh_msg_cnt(context).await? == 0 {
+                let n_fresh_msgs = context
+                    .sql
+                    .count(
+                        "
+SELECT COUNT(*) FROM msgs WHERE
+    state=? AND
+    (hidden=0 OR hidden=1) AND
+    chat_id=?",
+                        (MessageState::InFresh, chat_id),
+                    )
+                    .await?;
+                if n_fresh_msgs == 0 {
                     // Removes all notifications for the chat in UIs.
                     context.emit_event(EventType::MsgsNoticed(chat_id));
                 } else {
@@ -1993,9 +2005,13 @@ async fn add_parts(
     )
     .await?;
 
-    let mime_in_reply_to = mime_parser
-        .get_header(HeaderDef::InReplyTo)
-        .unwrap_or_default();
+    let mime_in_reply_to = match mime_parser.get_header(HeaderDef::InReplyTo) {
+        Some(in_reply_to) => parse_message_id(in_reply_to)
+            .log_err(context)
+            .ok()
+            .unwrap_or_default(),
+        None => "".to_string(),
+    };
     let mime_references = mime_parser
         .get_header(HeaderDef::References)
         .unwrap_or_default();
@@ -2122,7 +2138,7 @@ async fn add_parts(
             let is_incoming_fresh = mime_parser.incoming && !seen;
             set_msg_reaction(
                 context,
-                mime_in_reply_to,
+                &mime_in_reply_to,
                 chat_id,
                 from_id,
                 sort_timestamp,
@@ -2264,7 +2280,7 @@ RETURNING id
                     } else {
                         Vec::new()
                     },
-                    if trash { "" } else { mime_in_reply_to },
+                    if trash { "" } else { &mime_in_reply_to },
                     if trash { "" } else { mime_references },
                     !trash && save_mime_modified,
                     if trash { "" } else { part.error.as_deref().unwrap_or_default() },

src/sql/migrations.rs

@@ -2343,6 +2343,15 @@ ALTER TABLE contacts ADD COLUMN name_normalized TEXT;
         .await?;
     }
 
+    inc_and_check(&mut migration_version, 149)?;
+    if dbversion < migration_version {
+        sql.execute_migration(
+            "CREATE INDEX msgs_index10 ON msgs (mime_in_reply_to)",
+            migration_version,
+        )
+        .await?;
+    }
+
     let new_version = sql
         .get_raw_config_int(VERSION_CFG)
         .await?