From 97b6a0380126f390a30d37714cfe7a9034e64508 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Fri, 13 Dec 2024 13:13:35 -0300 Subject: [PATCH 1/2] feat: Make received reactions hidden chat messages as well Before, received reactions went to the trash chat. Make them hidden chat messages as already done for sent reactions, just for unification. Incoming received reactions remain `InSeen`, so no changes are needed to `chat::marknoticed_chat()` et al. --- src/reaction.rs | 11 ++++++----- src/receive_imf.rs | 22 +++++++++------------- src/test_utils.rs | 13 +++++++++++++ 3 files changed, 28 insertions(+), 18 deletions(-) diff --git a/src/reaction.rs b/src/reaction.rs index f0f18b2598..527fec9269 100644 --- a/src/reaction.rs +++ b/src/reaction.rs @@ -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::InSeen); assert_eq!(get_chat_msgs(&alice, chat_alice.id).await?.len(), 2); let reactions = get_msg_reactions(&alice, alice_msg.sender_msg_id).await?; @@ -719,7 +720,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 +883,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 +935,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 +957,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 9fb741edcc..cea8751386 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -764,7 +764,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; @@ -1235,14 +1235,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() { @@ -1600,10 +1596,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 { @@ -1619,7 +1615,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() { @@ -1629,7 +1625,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)?; 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( From 6dc7f5064a866663790b0a03f17879a28b9fa414 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Fri, 13 Dec 2024 13:19:25 -0300 Subject: [PATCH 2/2] 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. --- deltachat-rpc-client/tests/test_something.py | 37 ++++++++++ src/chat.rs | 71 +++++++++++++++----- src/reaction.rs | 18 ++++- src/receive_imf.rs | 19 ++++-- 4 files changed, 120 insertions(+), 25 deletions(-) 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 99769fbcf0..3468035d59 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; @@ -3243,15 +3244,16 @@ pub async fn get_chat_msgs_ex( /// 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.archived=1", + WHERE m.state=10 AND m.hidden IN (0,1) AND m.chat_id>9 AND c.archived=1", (), |row| row.get::<_, ChatId>(0), |ids| ids.collect::, _>>().map_err(Into::into), @@ -3265,7 +3267,7 @@ pub async fn marknoticed_chat(context: &Context, chat_id: ChatId) -> Result<()> .sql .transaction(|transaction| { let mut stmt = transaction.prepare( - "UPDATE msgs SET state=13 WHERE state=10 AND hidden=0 AND chat_id = ?", + "UPDATE msgs SET state=13 WHERE state=10 AND hidden IN (0,1) AND chat_id=?", )?; for chat_id_in_archive in &chat_ids_in_archive { stmt.execute((chat_id_in_archive,))?; @@ -3281,22 +3283,56 @@ pub async fn marknoticed_chat(context: &Context, chat_id: ChatId) -> Result<()> } } else { start_chat_ephemeral_timers(context, chat_id).await?; - - if context - .sql - .execute( + 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=0 - AND chat_id=?;", + SET state=? + WHERE state=? AND hidden IN (0,1) AND chat_id=?", (MessageState::InNoticed, MessageState::InFresh, chat_id), - ) - .await? - == 0 - { + )?; + 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,11 +3373,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 cea8751386..9137b2eab8 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(); @@ -1021,11 +1030,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 { @@ -1727,6 +1733,7 @@ RETURNING id Ok(ReceivedMsg { chat_id, state, + hidden, sort_timestamp, msg_ids: created_db_entries, needs_delete_job,