feat: When reactions are seen, remove notification from second device (#6480)

Instead of being trashed, the message containing a reaction remains in the chat, hidden and InFresh. When the chat is opened, it will be marked as Seen on the server, so that a second device removes the notifications for the reaction.

Close https://github.com/deltachat/deltachat-core-rust/issues/6210.

Also, this adds a benchmark.

Cargo.toml

@@ -158,6 +158,10 @@ harness = false
 name = "get_chat_msgs"
 harness = false
 
+[[bench]]
+name = "marknoticed_chat"
+harness = false
+
 [[bench]]
 name = "get_chatlist"
 harness = false

benches/marknoticed_chat.rs

@@ -0,0 +1,94 @@
+#![recursion_limit = "256"]
+use std::path::Path;
+
+use criterion::{black_box, criterion_group, criterion_main, BatchSize, Criterion};
+use deltachat::chat::{self, ChatId};
+use deltachat::chatlist::Chatlist;
+use deltachat::context::Context;
+use deltachat::stock_str::StockStrings;
+use deltachat::Events;
+use futures_lite::future::block_on;
+use tempfile::tempdir;
+
+async fn marknoticed_chat_benchmark(context: &Context, chats: &[ChatId]) {
+    for c in chats.iter().take(20) {
+        chat::marknoticed_chat(context, *c).await.unwrap();
+    }
+}
+
+fn criterion_benchmark(c: &mut Criterion) {
+    // To enable this benchmark, set `DELTACHAT_BENCHMARK_DATABASE` to some large database with many
+    // messages, such as your primary account.
+    if let Ok(path) = std::env::var("DELTACHAT_BENCHMARK_DATABASE") {
+        let rt = tokio::runtime::Runtime::new().unwrap();
+
+        let chats: Vec<_> = rt.block_on(async {
+            let context = Context::new(Path::new(&path), 100, Events::new(), StockStrings::new())
+                .await
+                .unwrap();
+            let chatlist = Chatlist::try_load(&context, 0, None, None).await.unwrap();
+            let len = chatlist.len();
+            (1..len).map(|i| chatlist.get_chat_id(i).unwrap()).collect()
+        });
+
+        // This mainly tests the performance of marknoticed_chat()
+        // when nothing has to be done
+        c.bench_function(
+            "chat::marknoticed_chat (mark 20 chats as noticed repeatedly)",
+            |b| {
+                let dir = tempdir().unwrap();
+                let dir = dir.path();
+                let new_db = dir.join("dc.db");
+                std::fs::copy(&path, &new_db).unwrap();
+
+                let context = block_on(async {
+                    Context::new(Path::new(&new_db), 100, Events::new(), StockStrings::new())
+                        .await
+                        .unwrap()
+                });
+
+                b.to_async(&rt)
+                    .iter(|| marknoticed_chat_benchmark(&context, black_box(&chats)))
+            },
+        );
+
+        // If the first 20 chats contain fresh messages or reactions,
+        // this tests the performance of marking them as noticed.
+        c.bench_function(
+            "chat::marknoticed_chat (mark 20 chats as noticed, resetting after every iteration)",
+            |b| {
+                b.to_async(&rt).iter_batched(
+                    || {
+                        let dir = tempdir().unwrap();
+                        let new_db = dir.path().join("dc.db");
+                        std::fs::copy(&path, &new_db).unwrap();
+
+                        let context = block_on(async {
+                            Context::new(
+                                Path::new(&new_db),
+                                100,
+                                Events::new(),
+                                StockStrings::new(),
+                            )
+                            .await
+                            .unwrap()
+                        });
+                        (dir, context)
+                    },
+                    |(_dir, context)| {
+                        let chats = &chats;
+                        async move {
+                            marknoticed_chat_benchmark(black_box(&context), black_box(chats)).await
+                        }
+                    },
+                    BatchSize::PerIteration,
+                );
+            },
+        );
+    } else {
+        println!("env var not set: DELTACHAT_BENCHMARK_DATABASE");
+    }
+}
+
+criterion_group!(benches, criterion_benchmark);
+criterion_main!(benches);

deltachat-rpc-client/tests/test_something.py

@@ -287,6 +287,44 @@ 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
+
+    alice2.clear_all_events()
+    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

@@ -20,7 +20,6 @@ use tokio::task;
 use crate::aheader::EncryptPreference;
 use crate::blob::BlobObject;
 use crate::chatlist::Chatlist;
-use crate::chatlist_events;
 use crate::color::str_to_color;
 use crate::config::Config;
 use crate::constants::{
@@ -51,6 +50,7 @@ use crate::tools::{
     truncate_msg_text, IsNoneOrEmpty, SystemTime,
 };
 use crate::webxdc::StatusUpdateSerial;
+use crate::{chatlist_events, imap};
 
 /// An chat item, such as a message or a marker.
 #[derive(Debug, Copy, Clone, PartialEq, Eq)]
@@ -3396,7 +3396,7 @@ pub async fn marknoticed_chat(context: &Context, chat_id: ChatId) -> Result<()>
     } else {
         start_chat_ephemeral_timers(context, chat_id).await?;
 
-        if context
+        let noticed_msgs_count = context
             .sql
             .execute(
                 "UPDATE msgs
@@ -3406,9 +3406,36 @@ pub async fn marknoticed_chat(context: &Context, chat_id: ChatId) -> Result<()>
             AND chat_id=?;",
                 (MessageState::InNoticed, MessageState::InFresh, chat_id),
             )
-            .await?
+            .await?;
-            == 0
+
-        {
+        // This is to trigger emitting `MsgsNoticed` on other devices when reactions are noticed
+        // locally (i.e. when the chat was opened locally).
+        let hidden_messages = context
+            .sql
+            .query_map(
+                "SELECT id, rfc724_mid FROM msgs
+                    WHERE state=?
+                      AND hidden=1
+                      AND chat_id=?
+                    ORDER BY id LIMIT 100", // LIMIT to 100 in order to avoid blocking the UI too long, usually there will be less than 100 messages anyway
+                (MessageState::InFresh, chat_id), // No need to check for InNoticed messages, because reactions are never InNoticed
+                |row| {
+                    let msg_id: MsgId = row.get(0)?;
+                    let rfc724_mid: String = row.get(1)?;
+                    Ok((msg_id, rfc724_mid))
+                },
+                |rows| {
+                    rows.collect::<std::result::Result<Vec<_>, _>>()
+                        .map_err(Into::into)
+                },
+            )
+            .await?;
+        for (msg_id, rfc724_mid) in &hidden_messages {
+            message::update_msg_state(context, *msg_id, MessageState::InSeen).await?;
+            imap::markseen_on_imap_table(context, rfc724_mid).await?;
+        }
+
+        if noticed_msgs_count == 0 {
             return Ok(());
         }
     }

src/reaction.rs

@@ -398,7 +398,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};
@@ -623,7 +623,9 @@ Here's my footer -- bob@example.net"
             .get_matching_opt(t, |evt| {
                 matches!(
                     evt,
-                    EventType::IncomingReaction { .. } | EventType::IncomingMsg { .. }
+                    EventType::IncomingReaction { .. }
+                        | EventType::IncomingMsg { .. }
+                        | EventType::MsgsChanged { .. }
                 )
             })
             .await;
@@ -667,7 +669,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?;
@@ -691,6 +694,20 @@ Here's my footer -- bob@example.net"
         .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
@@ -730,7 +747,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_chat.id,
@@ -899,7 +916,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");
@@ -951,7 +968,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.
@@ -973,7 +990,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

@@ -54,6 +54,9 @@ pub struct ReceivedMsg {
     /// Received message state.
     pub state: MessageState,
 
+    /// Whether the message is hidden.
+    pub hidden: bool,
+
     /// Message timestamp for sorting.
     pub sort_timestamp: i64,
 
@@ -192,6 +195,7 @@ pub(crate) async fn receive_imf_inner(
             return Ok(Some(ReceivedMsg {
                 chat_id: DC_CHAT_ID_TRASH,
                 state: MessageState::Undefined,
+                hidden: false,
                 sort_timestamp: 0,
                 msg_ids,
                 needs_delete_job: false,
@@ -385,6 +389,7 @@ pub(crate) async fn receive_imf_inner(
                 received_msg = Some(ReceivedMsg {
                     chat_id: DC_CHAT_ID_TRASH,
                     state: MessageState::InSeen,
+                    hidden: false,
                     sort_timestamp: mime_parser.timestamp_sent,
                     msg_ids: vec![msg_id],
                     needs_delete_job: res == securejoin::HandshakeMessage::Done,
@@ -619,7 +624,9 @@ pub(crate) async fn receive_imf_inner(
         }
     }
 
-    if let Some(replace_chat_id) = replace_chat_id {
+    if received_msg.hidden {
+        // No need to emit an event about the changed message
+    } else if let Some(replace_chat_id) = replace_chat_id {
         context.emit_msgs_changed_without_msg_id(replace_chat_id);
     } else if !chat_id.is_trash() {
         let fresh = received_msg.state == MessageState::InFresh;
@@ -778,7 +785,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;
 
@@ -1033,11 +1040,8 @@ async fn add_parts(
             }
         }
 
-        state = if seen
+        state = if seen || fetching_existing_messages || is_mdn || chat_id_blocked == Blocked::Yes
-            || fetching_existing_messages
+        // No check for `hidden` because only reactions are such and they should be `InFresh`.
-            || is_mdn
-            || is_reaction
-            || chat_id_blocked == Blocked::Yes
         {
             MessageState::InSeen
         } else {
@@ -1246,14 +1250,10 @@ async fn add_parts(
     }
 
     let orig_chat_id = chat_id;
-    let mut chat_id = if is_reaction {
+    let mut chat_id = chat_id.unwrap_or_else(|| {
+        info!(context, "No chat id for message (TRASH).");
         DC_CHAT_ID_TRASH
-    } else {
+    });
-        chat_id.unwrap_or_else(|| {
-            info!(context, "No chat id for message (TRASH).");
-            DC_CHAT_ID_TRASH
-        })
-    };
 
     // Extract ephemeral timer from the message or use the existing timer if the message is not fully downloaded.
     let mut ephemeral_timer = if is_partial_download.is_some() {
@@ -1655,11 +1655,11 @@ RETURNING id
                     typ,
                     state,
                     is_dc_message,
-                    if trash { "" } else { msg },
+                    if trash || hidden { "" } else { msg },
-                    if trash { None } else { message::normalize_text(msg) },
+                    if trash || hidden { None } else { message::normalize_text(msg) },
-                    if trash { "" } else { &subject },
+                    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 {
@@ -1667,7 +1667,7 @@ RETURNING id
                     },
                     hidden,
                     part.bytes as isize,
-                    if (save_mime_headers || save_mime_modified) && !trash {
+                    if (save_mime_headers || save_mime_modified) && !(trash || hidden) {
                         mime_headers.clone()
                     } else {
                         Vec::new()
@@ -1756,7 +1756,7 @@ RETURNING id
         "Message has {icnt} parts and is assigned to chat #{chat_id}."
     );
 
-    if !chat_id.is_trash() {
+    if !chat_id.is_trash() && !hidden {
         let mut chat = Chat::load_from_db(context, chat_id).await?;
 
         // In contrast to most other update-timestamps,
@@ -1794,6 +1794,7 @@ RETURNING id
     Ok(ReceivedMsg {
         chat_id,
         state,
+        hidden,
         sort_timestamp,
         msg_ids: created_db_entries,
         needs_delete_job,

src/test_utils.rs

@@ -607,6 +607,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(