From b71c68185ee11ee084cb874f6f0c74b192c527e1 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 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, so that the existing `\Seen` flag synchronisation mechanism works for them, and mark all incoming hidden messages in the chat as seen in `marknoticed_chat()`. It's interesting that sent out reactions are already hidden messages, so this change mostly just unifies things. --- src/chat.rs | 56 ++++++++++++++++++++++++++++++---------------- src/reaction.rs | 27 +++++++++++++++++----- src/receive_imf.rs | 23 ++++++++----------- src/test_utils.rs | 13 +++++++++++ 4 files changed, 80 insertions(+), 39 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index 641791c92c..c94b0e2366 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -3298,10 +3298,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() { @@ -3312,7 +3312,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), @@ -3322,20 +3322,39 @@ 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| { + let nr_msgs_noticed = conn.execute( + "UPDATE msgs + SET state=? + WHERE state=? + AND hidden=0 + AND chat_id=?", + (MessageState::InNoticed, MessageState::InFresh, chat_id), + )?; + let mut stmt = conn.prepare( + "SELECT id FROM msgs + WHERE state>=? AND state, _>>()?; + Ok((nr_msgs_noticed, hidden_msgs)) + }; + let (nr_msgs_noticed, hidden_msgs) = context.sql.call_write(conn_fn).await?; + if nr_msgs_noticed == 0 && hidden_msgs.is_empty() { + return Ok(()); + } + message::markseen_msgs(context, hidden_msgs).await?; } context.emit_event(EventType::MsgsNoticed(chat_id)); @@ -3380,7 +3399,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 6bb71f2a49..c9477b6a93 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}; @@ -641,7 +641,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?; @@ -657,6 +658,20 @@ Here's my footer -- bob@example.net" .await?; expect_incoming_reactions_event(&alice, alice_msg.sender_msg_id, *bob_id, "👍").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 @@ -695,7 +710,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; assert!(has_incoming_reactions_event(&alice).await); let chatlist = Chatlist::try_load(&bob, 0, None, None).await?; @@ -855,7 +870,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"); @@ -907,7 +922,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. @@ -929,7 +944,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 e4bbeac892..44cea8bacf 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -758,7 +758,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; @@ -1016,12 +1016,7 @@ 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 { MessageState::InSeen } else { MessageState::InFresh @@ -1230,7 +1225,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(|| { @@ -1405,7 +1400,7 @@ async fn add_parts( // that the ui should show button to display the full message. // a flag used to avoid adding "show full message" button to multiple parts of the message. - let mut save_mime_modified = mime_parser.is_mime_modified; + let mut save_mime_modified = mime_parser.is_mime_modified && !hidden; let mime_headers = if save_mime_headers || save_mime_modified { let headers = if !mime_parser.decoded_data.is_empty() { @@ -1597,10 +1592,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 +1611,7 @@ RETURNING id mime_in_reply_to, mime_references, 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 +1621,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 4a8473bc05..4cb794a43a 100644 --- a/src/test_utils.rs +++ b/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(