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 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.
  • Loading branch information
@iequidoo
iequidoo committed Nov 22, 2024
1 parent 8a0c913 commit de2b5a1
Show file tree
Hide file tree
Showing 5 changed files with 117 additions and 39 deletions.
37 changes: 37 additions & 0 deletions deltachat-rpc-client/tests/test_something.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,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
56 changes: 37 additions & 19 deletions src/chat.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3290,10 +3290,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 @@ -3304,7 +3304,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 @@ -3314,20 +3314,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<?
AND hidden=1
AND chat_id=?
ORDER BY id DESC LIMIT 100",
)?;
let hidden_msgs = stmt
.query_map(
(MessageState::InFresh, MessageState::InSeen, chat_id),
|row| {
let id: MsgId = row.get(0)?;
Ok(id)
},
)?
.collect::<std::result::Result<Vec<_>, _>>()?;
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));
Expand Down Expand Up @@ -3372,7 +3391,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
View file
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 @@ -641,7 +641,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 @@ -657,6 +658,20 @@ Here's my footer -- [email protected]"
.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
Expand Down Expand Up @@ -695,7 +710,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;
assert!(has_incoming_reactions_event(&alice).await);

let chatlist = Chatlist::try_load(&bob, 0, None, None).await?;
Expand Down Expand Up @@ -855,7 +870,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 @@ -907,7 +922,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 @@ -929,7 +944,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
23 changes: 9 additions & 14 deletions src/receive_imf.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -760,7 +760,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 @@ -1018,12 +1018,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
Expand Down Expand Up @@ -1232,7 +1227,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 +1402,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 +1594,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 +1613,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 +1623,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
13 changes: 13 additions & 0 deletions src/test_utils.rs
View file
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 de2b5a1

Please sign in to comment.