Skip to content

feat: Mark reactions as IMAP-seen in marknoticed_chat() (#6210) #6213

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

feat: Mark reactions as IMAP-seen in marknoticed_chat() (#6210) #6213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
97b6a03
feat: Make received reactions hidden chat messages as well
iequidoo Dec 13, 2024
6dc7f50
feat: Mark reactions as IMAP-seen in marknoticed_chat() (#6210)
iequidoo Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions deltachat-rpc-client/tests/test_something.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
71 changes: 54 additions & 17 deletions src/chat.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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::<Result<Vec<_>, _>>().map_err(Into::into),
Expand All @@ -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,))?;
Expand All @@ -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.
Comment on lines +3289 to +3290
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have assumed the previous limit of 100 to be more efficient, because all reactions need to be marked as seen eventually, and it's more efficient to mark all of them at once (although I do like having a limit at all, because otherwise the UI may become unresponsive when too many messages are marked as seen at once)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to mark all reactions as seen on IMAP, we only want to trigger MsgsNoticed on other devices, so we only select the last hidden message (reaction) and check if it's InFresh. Anyway we don't send MDNs for reactions. If a reactions is already InNoticed or InSeen, it normally has the same state on other devices, so there's nothing to do, that's why i added filter() below.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to mark all reactions as seen on IMAP

It's true that we don't need to, but this PR here as-is will mark all reactions as seen eventually after the user opened the chat repeatedly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, i added LIMIT 1, but i select InFresh, InNoticed and InSeen hidden messages (reactions) and then check that the selected reaction is InFresh (the filter() call below).

For uniformity it would be better to mark all reactions as seen eventually, but it's an extra work on IMAP that i suggest to avoid at least for now.

//
// 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));
Expand Down Expand Up @@ -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<=?;",
(
Expand Down
27 changes: 21 additions & 6 deletions src/reaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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::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?;
Expand All @@ -680,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
Expand Down Expand Up @@ -719,7 +734,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;
Expand Down Expand Up @@ -882,7 +897,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");
Expand Down Expand Up @@ -934,7 +949,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.
Expand All @@ -956,7 +971,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)
Expand Down
41 changes: 22 additions & 19 deletions src/receive_imf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -764,7 +773,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;

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -1235,14 +1241,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() {
Expand Down Expand Up @@ -1600,10 +1602,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 {
Expand All @@ -1619,7 +1621,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() {
Expand All @@ -1629,7 +1631,7 @@ RETURNING id
} else {
DownloadState::Done
},
mime_parser.hop_info
if trash || hidden { "" } else { &mime_parser.hop_info },
Comment on lines -1632 to +1634
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you can pull this improvement out into a separate PR that also updates MsgId::trash() and delete_expired_messages()?

],
|row| {
let msg_id: MsgId = row.get(0)?;
Expand Down Expand Up @@ -1731,6 +1733,7 @@ RETURNING id
Ok(ReceivedMsg {
chat_id,
state,
hidden,
sort_timestamp,
msg_ids: created_db_entries,
needs_delete_job,
Expand Down
13 changes: 13 additions & 0 deletions src/test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Loading