From 313a20043d66282500328d40fd86a89f3bd3cea1 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Thu, 19 Dec 2024 23:35:07 -0300 Subject: [PATCH] fix: Emit MsgsNoticed on receipt of an IMAP-seen message `imap::Session::sync_seen_flags()` emits `MsgsNoticed` for existing messages seen on other devices, so `receive_imf` should do the same when it receives a seen message. Otherwise a multi-device user may see a new message notification on device A, just swipe it, then see another new message notification and mark it as read, and when a device B goes online, it will show a notification for the first message, and it won't be removed because `MsgsNoticed` isn't emitted. I have checked this with my DC Android and Desktop. With this fix the notification can be removed if UIs handle `MsgsNoticed` accordingly, though currently i can't achieve this behaviour in Desktop. Also add blocked chat's messages and silent group changes messages as `InNoticed` instead of `InSeen`, they don't need to cause `MsgsNoticed` events. This doesn't change any UX, checked this in Desktop. --- src/chat/chat_tests.rs | 2 +- src/imap.rs | 3 ++ src/reaction.rs | 33 ++++++++++++++++- src/receive_imf.rs | 7 +++- src/receive_imf/receive_imf_tests.rs | 54 ++++++++++++++++++++++++++++ 5 files changed, 96 insertions(+), 3 deletions(-) diff --git a/src/chat/chat_tests.rs b/src/chat/chat_tests.rs index 64651f583..9ab610a2b 100644 --- a/src/chat/chat_tests.rs +++ b/src/chat/chat_tests.rs @@ -747,7 +747,7 @@ async fn test_leave_group() -> Result<()> { assert_eq!(get_chat_contacts(&alice, alice_chat_id).await?.len(), 1); - assert_eq!(rcvd_leave_msg.state, MessageState::InSeen); + assert_eq!(rcvd_leave_msg.state, MessageState::InNoticed); alice.emit_event(EventType::Test); alice diff --git a/src/imap.rs b/src/imap.rs index ea6d90736..855ae6362 100644 --- a/src/imap.rs +++ b/src/imap.rs @@ -1281,6 +1281,9 @@ impl Session { context.on_archived_chats_maybe_noticed(); } for updated_chat_id in updated_chat_ids { + // NB: There are no other events that remove notifications at least for messages seen on + // other devices, so while we may also remove useful notifications for newer messages, + // we have no other choice. context.emit_event(EventType::MsgsNoticed(updated_chat_id)); chatlist_events::emit_chatlist_item_changed(context, updated_chat_id); } diff --git a/src/reaction.rs b/src/reaction.rs index 6b90e0947..ee430d4a4 100644 --- a/src/reaction.rs +++ b/src/reaction.rs @@ -525,6 +525,7 @@ Content-Disposition: reaction\n\ assert_eq!(bob_reaction.as_str(), "👍"); // Alice receives reaction to her message from Bob with a footer. + alice.evtracker.clear_events(); receive_imf( &alice, "To: alice@example.org\n\ @@ -546,7 +547,11 @@ Here's my footer -- bob@example.net" false, ) .await?; - + let ev = alice + .evtracker + .get_matching_opt(&alice, |e| matches!(e, EventType::MsgsNoticed { .. })) + .await; + assert!(ev.is_none()); let reactions = get_msg_reactions(&alice, msg.id).await?; assert_eq!(reactions.to_string(), "😀1"); @@ -590,6 +595,32 @@ Content-Disposition: reaction\n\ let reactions = get_msg_reactions(&alice, msg.id).await?; assert_eq!(reactions.to_string(), "👍1"); + // Alice receives a seen reaction to her message from Bob. If a reaction is seen, that's a + // result of an explicit user action and also means that the user has noticed notifications + // for earlier messages. + alice.evtracker.clear_events(); + let seen = true; + receive_imf( + &alice, + "To: alice@example.org\n\ +From: bob@example.net\n\ +Date: Today, 29 February 2021 00:00:10 -800\n\ +Message-ID: 56792@example.net\n\ +In-Reply-To: 12345@example.org\n\ +Subject: Meeting\n\ +Mime-Version: 1.0 (1.0)\n\ +Content-Type: text/plain; charset=utf-8\n\ +Content-Disposition: reaction\n\ +\n\ +\u{1F44D}" + .as_bytes(), + seen, + ) + .await?; + let _ev = alice + .evtracker + .get_matching(|e| matches!(e, EventType::MsgsNoticed { .. })) + .await; Ok(()) } diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 4e7bc7826..5d362ef4d 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -1014,6 +1014,9 @@ pub(crate) async fn receive_imf_inner( chat_id.emit_msg_event(context, *msg_id, mime_parser.incoming && fresh); } } + if !chat_id.is_trash() && received_msg.state == MessageState::InSeen { + context.emit_event(EventType::MsgsNoticed(chat_id)); + } context.new_msgs_notify.notify_one(); mime_parser @@ -1751,10 +1754,12 @@ async fn add_parts( let state = if !mime_parser.incoming { MessageState::OutDelivered - } else if seen || is_mdn || chat_id_blocked == Blocked::Yes || group_changes.silent + } else if seen || is_mdn // No check for `hidden` because only reactions are such and they should be `InFresh`. { MessageState::InSeen + } else if chat_id_blocked == Blocked::Yes || group_changes.silent { + MessageState::InNoticed } else { MessageState::InFresh }; diff --git a/src/receive_imf/receive_imf_tests.rs b/src/receive_imf/receive_imf_tests.rs index 3feb8183f..155930b1d 100644 --- a/src/receive_imf/receive_imf_tests.rs +++ b/src/receive_imf/receive_imf_tests.rs @@ -271,6 +271,31 @@ async fn test_adhoc_groups_merge() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_msgs_noticed_on_seen_msg() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + let seen = true; + let rcvd_msg = receive_imf( + alice, + b"From: bob@example.net\n\ + To: alice@example.org\n\ + Message-ID: <3333@example.net>\n\ + Date: Sun, 22 Mar 2020 22:37:57 +0000\n\ + \n\ + This is a seen message.\n", + seen, + ) + .await? + .unwrap(); + let ev = alice + .evtracker + .get_matching(|e| matches!(e, EventType::MsgsNoticed { .. })) + .await; + assert_eq!(ev, EventType::MsgsNoticed(rcvd_msg.chat_id)); + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_mdn_and_alias() -> Result<()> { let mut tcm = TestContextManager::new(); @@ -2687,6 +2712,35 @@ async fn test_read_receipts_dont_unmark_bots() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_no_msgs_noticed_on_seen_mdn() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + let bob = &tcm.bob().await; + + let received_msg = tcm.send_recv_accept(alice, bob, "hi").await; + + let mdn_mimefactory = crate::mimefactory::MimeFactory::from_mdn( + bob, + received_msg.from_id, + received_msg.rfc724_mid, + vec![], + ) + .await?; + let rendered_mdn = mdn_mimefactory.render(bob).await?; + let mdn_body = rendered_mdn.message; + + let seen = true; + alice.evtracker.clear_events(); + receive_imf(alice, mdn_body.as_bytes(), seen).await?; + let ev = alice + .evtracker + .get_matching_opt(alice, |e| matches!(e, EventType::MsgsNoticed { .. })) + .await; + assert!(ev.is_none()); + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_gmx_forwarded_msg() -> Result<()> { let t = TestContext::new_alice().await;