From 3aa1e03a7b827a5537ac518fc94df2738fcd4989 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Fri, 15 Nov 2024 19:23:49 -0300 Subject: [PATCH] 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. --- .../src/deltachat_rpc_client/const.py | 1 + deltachat-rpc-client/tests/test_something.py | 37 +++++++++ src/chat.rs | 79 ++++++++++++++----- src/reaction.rs | 27 +++++-- src/receive_imf.rs | 30 ++++--- src/test_utils.rs | 13 +++ 6 files changed, 153 insertions(+), 34 deletions(-) diff --git a/deltachat-rpc-client/src/deltachat_rpc_client/const.py b/deltachat-rpc-client/src/deltachat_rpc_client/const.py index f2542f0eef..e06ba7f72f 100644 --- a/deltachat-rpc-client/src/deltachat_rpc_client/const.py +++ b/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" diff --git a/deltachat-rpc-client/tests/test_something.py b/deltachat-rpc-client/tests/test_something.py index cf1d422f29..f394e30c3e 100644 --- a/deltachat-rpc-client/tests/test_something.py +++ b/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) diff --git a/src/chat.rs b/src/chat.rs index 320fffb854..433b19f470 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -39,6 +39,7 @@ use crate::mimeparser::SystemMessage; use crate::param::{Param, Params}; use crate::peerstate::Peerstate; use crate::receive_imf::ReceivedMsg; +use crate::rusqlite::OptionalExtension; use crate::securejoin::BobState; use crate::smtp::send_msg_to_smtp; use crate::sql; @@ -3255,10 +3256,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::, _>>().map_err(Into::into) + |ids| ids.collect::, _>>().map_err(Into::into), ) .await?; if chat_ids_in_archive.is_empty() { @@ -3269,7 +3270,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), @@ -3279,20 +3280,61 @@ 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| { + // This is to trigger emitting `MsgsNoticed` on other devices when reactions are noticed + // locally. We filter out `InNoticed` messages because they are normally a result of + // `mark_old_messages_as_noticed()` which happens on all devices anyway. Also we limit + // this to one message because the effect is the same anyway. + // + // Even if `message::markseen_msgs()` fails then, in the worst case other devices won't + // emit `MsgsNoticed` and app notifications won't be removed. The bigger problem is that + // another device may have more reactions received and not yet seen notifications are + // removed from it, but the same problem already exists for "usual" messages, so let's + // not solve it for now. + let mut stmt = conn.prepare( + "SELECT id, state FROM msgs + WHERE (state=? OR state=? OR state=?) + AND hidden=1 + AND chat_id=? + ORDER BY id DESC LIMIT 1", + )?; + let id_to_markseen = stmt + .query_row( + ( + MessageState::InFresh, + MessageState::InNoticed, + MessageState::InSeen, + chat_id, + ), + |row| { + let id: MsgId = row.get(0)?; + let state: MessageState = row.get(1)?; + Ok((id, state)) + }, + ) + .optional()? + .filter(|&(_, state)| state == MessageState::InFresh) + .map(|(id, _)| id); + + // NB: Enumerate `hidden` values to employ `msgs_index7`. + let nr_msgs_noticed = conn.execute( + "UPDATE msgs + SET state=? + WHERE state=? + AND (hidden=0 OR hidden=1) + AND chat_id=?", + (MessageState::InNoticed, MessageState::InFresh, chat_id), + )?; + Ok((nr_msgs_noticed, id_to_markseen)) + }; + let (nr_msgs_noticed, id_to_markseen) = context.sql.call_write(conn_fn).await?; + if nr_msgs_noticed == 0 { + return Ok(()); + } + if let Some(id) = id_to_markseen { + message::markseen_msgs(context, vec![id]).await?; + } } context.emit_event(EventType::MsgsNoticed(chat_id)); @@ -3337,7 +3379,6 @@ pub(crate) async fn mark_old_messages_as_noticed( "UPDATE msgs SET state=? WHERE state=? - AND hidden=0 AND chat_id=? AND timestamp<=?;", ( diff --git a/src/reaction.rs b/src/reaction.rs index f0f18b2598..464dcb5794 100644 --- a/src/reaction.rs +++ b/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) diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 439731e9b5..37525e5197 100644 --- a/src/receive_imf.rs +++ b/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, @@ -373,6 +377,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, @@ -611,7 +616,11 @@ pub(crate) async fn receive_imf_inner( } else if !chat_id.is_trash() { let fresh = received_msg.state == MessageState::InFresh; for msg_id in &received_msg.msg_ids { - chat_id.emit_msg_event(context, *msg_id, mime_parser.incoming && fresh); + chat_id.emit_msg_event( + context, + *msg_id, + mime_parser.incoming && fresh && !received_msg.hidden, + ); } } context.new_msgs_notify.notify_one(); @@ -760,7 +769,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; @@ -1021,7 +1030,9 @@ async fn add_parts( state = if seen || fetching_existing_messages || is_mdn - || is_reaction + // Currently only reactions are hidden, so this check is just in case we have other + // hidden messages in the future to make `chat::marknoticed_chat()` no-op for them. + || (hidden && !is_reaction) || chat_id_blocked == Blocked::Yes { MessageState::InSeen @@ -1232,7 +1243,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(|| { @@ -1597,10 +1608,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 { @@ -1616,7 +1627,7 @@ RETURNING id mime_in_reply_to, mime_references, save_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() { @@ -1626,7 +1637,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)?; @@ -1733,6 +1744,7 @@ RETURNING id Ok(ReceivedMsg { chat_id, state, + hidden, sort_timestamp, msg_ids: created_db_entries, needs_delete_job, diff --git a/src/test_utils.rs b/src/test_utils.rs index e680d2025a..df42ca87ff 100644 --- a/src/test_utils.rs +++ b/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(