Skip to content

Commit

Permalink
feat: Mark reactions as IMAP-seen in marknoticed_chat() (#6210)
Browse files Browse the repository at this point in the history
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 incoming 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 and `InFresh`, so that the existing `\Seen` flag synchronisation mechanism works for them,
and mark the last fresh hidden incoming message in the chat as seen in `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.

It's interesting that sent out reactions are already hidden messages, so this change mostly just
unifies things.
  • Loading branch information
iequidoo committed Dec 8, 2024
1 parent 39be591 commit 6c90599
Show file tree
Hide file tree
Showing 6 changed files with 154 additions and 35 deletions.
1 change: 1 addition & 0 deletions deltachat-rpc-client/src/deltachat_rpc_client/const.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ class EventType(str, Enum):
ERROR_SELF_NOT_IN_GROUP = "ErrorSelfNotInGroup"
MSGS_CHANGED = "MsgsChanged"
REACTIONS_CHANGED = "ReactionsChanged"
INCOMING_REACTION = "IncomingReaction"
INCOMING_MSG = "IncomingMsg"
INCOMING_MSG_BUNCH = "IncomingMsgBunch"
MSGS_NOTICED = "MsgsNoticed"
Expand Down
37 changes: 37 additions & 0 deletions deltachat-rpc-client/tests/test_something.py
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
79 changes: 60 additions & 19 deletions src/chat.rs
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 @@ -3308,10 +3309,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::<Result<Vec<_>, _>>().map_err(Into::into)
|ids| ids.collect::<Result<Vec<_>, _>>().map_err(Into::into),
)
.await?;
if chat_ids_in_archive.is_empty() {
Expand All @@ -3322,7 +3323,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),
Expand All @@ -3332,20 +3333,61 @@ 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| {
// 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);

// NB: Enumerate `hidden` values to employ `msgs_index7`.
let nr_msgs_noticed = conn.execute(
"UPDATE msgs
SET state=?
WHERE state=?
AND (hidden=0 OR hidden=1)
AND chat_id=?",
(MessageState::InNoticed, MessageState::InFresh, chat_id),
)?;
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 @@ -3390,7 +3432,6 @@ pub(crate) async fn mark_old_messages_as_noticed(
"UPDATE msgs
SET state=?
WHERE state=?
AND hidden=0
AND chat_id=?
AND timestamp<=?;",
(
Expand Down
27 changes: 21 additions & 6 deletions src/reaction.rs
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 -- [email protected]"
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 -- [email protected]"
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 -- [email protected]"
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 -- [email protected]"
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 -- [email protected]"
{
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 -- [email protected]"
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
32 changes: 22 additions & 10 deletions src/receive_imf.rs
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 @@ -760,7 +769,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,7 +1030,9 @@ async fn add_parts(
state = if seen
|| fetching_existing_messages
|| is_mdn
|| is_reaction
// Currently only reactions are hidden, so this check is just in case we have other
// hidden messages in the future to make `chat::marknoticed_chat()` no-op for them.
|| (hidden && !is_reaction)
|| chat_id_blocked == Blocked::Yes
{
MessageState::InSeen
Expand Down Expand Up @@ -1232,7 +1243,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(|| {
Expand Down Expand Up @@ -1407,7 +1418,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() {
Expand Down Expand Up @@ -1599,10 +1610,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 @@ -1618,7 +1629,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() {
Expand All @@ -1628,7 +1639,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)?;
Expand Down Expand Up @@ -1735,6 +1746,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
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

0 comments on commit 6c90599

Please sign in to comment.