From e71b17cc0ecab508424006217531eb602a67426f Mon Sep 17 00:00:00 2001 From: iequidoo Date: Fri, 13 Dec 2024 13:19:25 -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, 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 `InFresh` so that the existing `\Seen` flag synchronisation mechanism works for them, and mark the last fresh hidden incoming message (reaction) in the chat as seen in `chat::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. --- .../src/deltachat_rpc_client/const.py | 1 + deltachat-rpc-client/tests/test_something.py | 37 ++++++++ src/chat.rs | 84 ++++++++++++++----- src/reaction.rs | 18 +++- src/receive_imf.rs | 19 +++-- 5 files changed, 130 insertions(+), 29 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..bc0a642543 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; @@ -3247,18 +3248,20 @@ pub(crate) async fn marknoticed_chat_if_older_than( /// Marks all messages in the chat as noticed. /// If the given chat-id is the archive-link, marks all messages in all archived chats as noticed. pub async fn marknoticed_chat(context: &Context, chat_id: ChatId) -> Result<()> { - // "WHERE" below uses the index `(state, hidden, chat_id)`, see get_fresh_msg_cnt() for reasoning - // the additional SELECT statement may speed up things as no write-blocking is needed. + // `WHERE` statements below use the index `(state, hidden, chat_id)`, that's why we enumerate + // `hidden` values, see `get_fresh_msg_cnt()` for reasoning. + // The additional `SELECT` statement may speed up things as no write-blocking is needed. if chat_id.is_archived_link() { let chat_ids_in_archive = context .sql .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.hidden IN (0,1) 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 +3272,8 @@ 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 hidden IN (0,1) AND chat_id IN ({})", sql::repeat_vars(chat_ids_in_archive.len()) ), rusqlite::params_from_iter(&chat_ids_in_archive), @@ -3279,20 +3283,57 @@ 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); + let nr_msgs_noticed = conn.execute( + "UPDATE msgs + SET state=? + WHERE state=? AND hidden IN (0,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)); @@ -3333,11 +3374,12 @@ pub(crate) async fn mark_old_messages_as_noticed( .transaction(|transaction| { let mut changed_chats = Vec::new(); for (_, msg) in msgs_by_chat { + // NB: Enumerate `hidden` values to employ the index `(state, hidden, chat_id)`. let changed_rows = transaction.execute( "UPDATE msgs SET state=? WHERE state=? - AND hidden=0 + AND hidden IN (0,1) AND chat_id=? AND timestamp<=?;", ( diff --git a/src/reaction.rs b/src/reaction.rs index 527fec9269..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}; @@ -664,7 +664,7 @@ Here's my footer -- bob@example.net" let bob_reaction_msg = bob.pop_sent_msg().await; let alice_reaction_msg = alice.recv_msg_hidden(&bob_reaction_msg).await; - assert_eq!(alice_reaction_msg.state, MessageState::InSeen); + 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?; @@ -681,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 diff --git a/src/receive_imf.rs b/src/receive_imf.rs index c5562de662..0cd6932022 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(); @@ -1018,11 +1027,8 @@ 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 + // No check for `hidden` because only reactions are such and they should be `InFresh`. { MessageState::InSeen } else { @@ -1733,6 +1739,7 @@ RETURNING id Ok(ReceivedMsg { chat_id, state, + hidden, sort_timestamp, msg_ids: created_db_entries, needs_delete_job,