diff --git a/Cargo.toml b/Cargo.toml index 81794de31..64819b8f7 100644 --- a/Cargo.toml +++ b/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 diff --git a/benches/marknoticed_chat.rs b/benches/marknoticed_chat.rs new file mode 100644 index 000000000..8cdc0f96d --- /dev/null +++ b/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); diff --git a/deltachat-rpc-client/tests/test_something.py b/deltachat-rpc-client/tests/test_something.py index f91ce9076..787f29338 100644 --- a/deltachat-rpc-client/tests/test_something.py +++ b/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) diff --git a/src/chat.rs b/src/chat.rs index 198487382..a0eb70e90 100644 --- a/src/chat.rs +++ b/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? - == 0 - { + .await?; + + // 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::, _>>() + .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(()); } } diff --git a/src/reaction.rs b/src/reaction.rs index a54d0eb4d..4f62bf955 100644 --- a/src/reaction.rs +++ b/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) diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 517dabf38..dee7e1b27 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, @@ -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 - || 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 { @@ -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 { None } else { message::normalize_text(msg) }, - if trash { "" } else { &subject }, + if trash || hidden { "" } else { msg }, + 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 { @@ -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, diff --git a/src/test_utils.rs b/src/test_utils.rs index 8beba6633..657493edf 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(