Skip to content

fix: markseen_msgs(): Mark reactions to specified messages as seen too (#7884) #7904

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

Open
iequidoo wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: markseen_msgs(): Mark reactions to specified messages as seen too (#7884) #7904

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 6 additions & 1 deletion deltachat-ffi/deltachat.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2127,9 +2127,14 @@ int dc_resend_msgs (dc_context_t* context, const uint3


/**
* Mark messages as presented to the user.
* Mark messages and reactions to them as presented to the user.
* Typically, UIs call this function on scrolling through the message list,
* when the messages are presented at least for a little moment.
* UIs should pass all messages to this function, incl. outgoing and info ones, as this is used also
* for synchronization and to track last position.
* This should also be called when a reaction for a message being in view arrives.
* If this is called for already presented messages, unless they have new reactions, nothing
* happens.
* The concrete action depends on the type of the chat and on the users settings
* (dc_msgs_presented() may be a better name therefore, but well. :)
*
Expand Down
5 changes: 5 additions & 0 deletions deltachat-jsonrpc/src/api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1327,6 +1327,11 @@ impl CommandApi {
/// Mark messages as presented to the user.
/// Typically, UIs call this function on scrolling through the message list,
/// when the messages are presented at least for a little moment.
/// UIs should pass all messages to this function, incl. outgoing and info ones, as this is used
/// also for synchronization and to track last position.
/// This should also be called when a reaction for a message being in view arrives.
Comment thread
iequidoo marked this conversation as resolved.
/// If this is called for already presented messages, unless they have new reactions, nothing
/// happens.
/// The concrete action depends on the type of the chat and on the users settings
/// (dc_msgs_presented() may be a better name therefore, but well. :)
///
Expand Down
17 changes: 11 additions & 6 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 @@ -416,27 +416,32 @@ def test_dont_move_sync_msgs(acfactory, direct_imap):
time.sleep(1)


def test_reaction_seen_on_another_dev(acfactory) -> None:
@pytest.mark.parametrize("chat_noticed", [False, True])
def test_reaction_seen_on_another_dev(acfactory, chat_noticed) -> None:
alice, bob = acfactory.get_online_accounts(2)
alice2 = alice.clone()
alice2.start_io()

alice_contact_bob = alice.create_contact(bob, "Bob")
alice_chat_bob = alice_contact_bob.create_chat()
alice_chat_bob.send_text("Hello!")
alice_msg = 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()
bob_msg = bob.get_message_by_id(msg_id)
snapshot = bob_msg.get_snapshot()
snapshot.chat.accept()
message.send_reaction("😎")
bob_msg.send_reaction("😎")
for a in [alice, alice2]:
a.wait_for_event(EventType.INCOMING_REACTION)

alice2.clear_all_events()
alice_chat_bob.mark_noticed()
if chat_noticed:
alice_chat_bob.mark_noticed()
else:
# UIs are supposed to mark messages being in view as seen, not reactions themselves.
alice_msg.mark_seen()
chat_id = alice2.wait_for_event(EventType.MSGS_NOTICED).chat_id
alice2_chat_bob = alice2.create_chat(bob)
assert chat_id == alice2_chat_bob.id
Expand Down
136 changes: 74 additions & 62 deletions src/message.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1800,7 +1800,9 @@ pub async fn delete_msgs_ex(
Ok(())
}

/// Marks requested messages as seen.
/// Marks requested messages and reactions to them as seen.
Comment thread
iequidoo marked this conversation as resolved.
/// This should be called when a message comes into view or when a reaction for a message being in
/// view arrives.
pub async fn markseen_msgs(context: &Context, msg_ids: Vec<MsgId>) -> Result<()> {
if msg_ids.is_empty() {
return Ok(());
Expand All @@ -1814,10 +1816,18 @@ pub async fn markseen_msgs(context: &Context, msg_ids: Vec<MsgId>) -> Result<()>

let mut msgs = Vec::with_capacity(msg_ids.len());
for &id in &msg_ids {
if let Some(msg) = context
let Some(rfc724_mid): Option<String> = context
.sql
.query_row_optional(
.query_get_value("SELECT rfc724_mid FROM msgs WHERE id=?", (id,))
.await?
else {
continue;
};
context
.sql
.query_map(
"SELECT
m.id AS id,
m.chat_id AS chat_id,
m.state AS state,
m.ephemeral_timer AS ephemeral_timer,
Expand All @@ -1828,9 +1838,11 @@ pub async fn markseen_msgs(context: &Context, msg_ids: Vec<MsgId>) -> Result<()>
c.archived AS archived,
c.blocked AS blocked
FROM msgs m LEFT JOIN chats c ON c.id=m.chat_id
WHERE m.id=? AND m.chat_id>9",
(id,),
WHERE (m.id=? OR m.mime_in_reply_to=? AND m.hidden=1)
AND m.chat_id>9 AND ?<=m.state AND m.state<?",
(id, rfc724_mid, MessageState::InFresh, MessageState::InSeen),
|row| {
let id: MsgId = row.get("id")?;
let chat_id: ChatId = row.get("chat_id")?;
let state: MessageState = row.get("state")?;
let param: Params = row.get::<_, String>("param")?.parse().unwrap_or_default();
Expand All @@ -1855,11 +1867,14 @@ pub async fn markseen_msgs(context: &Context, msg_ids: Vec<MsgId>) -> Result<()>
ephemeral_timer,
))
},
|rows| {
for row in rows {
msgs.push(row?);
Copy link
Copy Markdown
Collaborator

@link2xt link2xt Mar 26, 2026

Choose a reason for hiding this comment

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

Can query_map_vec be used?

Copy link
Copy Markdown
Collaborator Author

@iequidoo iequidoo Mar 26, 2026

Choose a reason for hiding this comment

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

Unfortunately, we need many SQL queries here because different id and rfc724_mid are passed in each query, so it won't help a lot, we'll still need to join the returned vectors.

}
Ok(())
},
)
.await?
{
msgs.push(msg);
}
.await?;
}

if msgs
Expand Down Expand Up @@ -1888,60 +1903,57 @@ pub async fn markseen_msgs(context: &Context, msg_ids: Vec<MsgId>) -> Result<()>
_curr_ephemeral_timer,
) in msgs
{
if curr_state == MessageState::InFresh || curr_state == MessageState::InNoticed {
update_msg_state(context, id, MessageState::InSeen).await?;
info!(context, "Seen message {}.", id);

markseen_on_imap_table(context, &curr_rfc724_mid).await?;

// Read receipts for system messages are never sent to contacts.
// These messages have no place to display received read receipt
// anyway. And since their text is locally generated,
// quoting them is dangerous as it may contain contact names. E.g., for original message
// "Group left by me", a read receipt will quote "Group left by <name>", and the name can
// be a display name stored in address book rather than the name sent in the From field by
// the user.
//
// We also don't send read receipts for contact requests.
// Read receipts will not be sent even after accepting the chat.
let to_id = if curr_blocked == Blocked::Not
&& !curr_hidden
&& curr_param.get_bool(Param::WantsMdn).unwrap_or_default()
&& curr_param.get_cmd() == SystemMessage::Unknown
&& context.should_send_mdns().await?
{
// Clear WantsMdn to not handle a MDN twice
// if the state later is InFresh again as markfresh_chat() was called.
// BccSelf MDN messages in the next branch may be sent twice for syncing.
context
.sql
.execute(
"UPDATE msgs SET param=? WHERE id=?",
(curr_param.clone().remove(Param::WantsMdn).to_string(), id),
)
.await
.context("failed to clear WantsMdn")?;
Some(curr_from_id)
} else if context.get_config_bool(Config::BccSelf).await? {
Some(ContactId::SELF)
} else {
None
};
if let Some(to_id) = to_id {
context
.sql
.execute(
"INSERT INTO smtp_mdns (msg_id, from_id, rfc724_mid) VALUES(?, ?, ?)",
(id, to_id, curr_rfc724_mid),
)
.await
.context("failed to insert into smtp_mdns")?;
context.scheduler.interrupt_smtp().await;
}
update_msg_state(context, id, MessageState::InSeen).await?;
info!(context, "Seen message {}.", id);

if !curr_hidden {
updated_chat_ids.insert(curr_chat_id);
}
markseen_on_imap_table(context, &curr_rfc724_mid).await?;

// Read receipts for system messages are never sent to contacts. These messages have no
// place to display received read receipt anyway. And since their text is locally generated,
// quoting them is dangerous as it may contain contact names. E.g., for original message
// "Group left by me", a read receipt will quote "Group left by <name>", and the name can be
// a display name stored in address book rather than the name sent in the From field by the
// user.
//
// We also don't send read receipts for contact requests. Read receipts will not be sent
// even after accepting the chat.
let to_id = if curr_blocked == Blocked::Not
&& !curr_hidden
&& curr_param.get_bool(Param::WantsMdn).unwrap_or_default()
&& curr_param.get_cmd() == SystemMessage::Unknown
&& context.should_send_mdns().await?
{
// Clear WantsMdn to not handle a MDN twice
// if the state later is InFresh again as markfresh_chat() was called.
// BccSelf MDN messages in the next branch may be sent twice for syncing.
context
.sql
.execute(
"UPDATE msgs SET param=? WHERE id=?",
(curr_param.clone().remove(Param::WantsMdn).to_string(), id),
)
.await
.context("failed to clear WantsMdn")?;
Some(curr_from_id)
} else if context.get_config_bool(Config::BccSelf).await? {
Some(ContactId::SELF)
} else {
None
};
if let Some(to_id) = to_id {
context
.sql
.execute(
"INSERT INTO smtp_mdns (msg_id, from_id, rfc724_mid) VALUES(?, ?, ?)",
(id, to_id, curr_rfc724_mid),
)
.await
.context("failed to insert into smtp_mdns")?;
context.scheduler.interrupt_smtp().await;
}

if !curr_hidden {
updated_chat_ids.insert(curr_chat_id);
}
archived_chats_maybe_noticed |= curr_state == MessageState::InFresh
&& !curr_hidden
Expand Down
33 changes: 33 additions & 0 deletions src/reaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1105,4 +1105,37 @@ Content-Transfer-Encoding: 7bit\r

Ok(())
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_markseen_referenced_msg() -> Result<()> {
let mut tcm = TestContextManager::new();
let alice = &tcm.alice().await;
let bob = &tcm.bob().await;
let chat_id = alice.create_chat(bob).await.id;

let alice_msg_id = send_text_msg(alice, chat_id, "foo".to_string()).await?;
let sent_msg = alice.pop_sent_msg().await;
let bob_msg = bob.recv_msg(&sent_msg).await;
bob_msg.chat_id.accept(bob).await?;

send_reaction(bob, bob_msg.id, "👀").await?;
let sent_msg = bob.pop_sent_msg().await;
let alice_reaction = alice.recv_msg_hidden(&sent_msg).await;
assert_eq!(alice_reaction.state, MessageState::InFresh);

markseen_msgs(alice, vec![alice_msg_id]).await?;
let alice_reaction = Message::load_from_db(alice, alice_reaction.id).await?;
assert_eq!(alice_reaction.state, MessageState::InSeen);
assert_eq!(
alice
.sql
.count(
"SELECT COUNT(*) FROM smtp_mdns WHERE from_id=?",
(ContactId::SELF,)
)
.await?,
1
);
Ok(())
}
}
36 changes: 28 additions & 8 deletions src/receive_imf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ use crate::message::{
rfc724_mid_exists,
};
use crate::mimeparser::{
AvatarAction, GossipedKey, MimeMessage, PreMessageMode, SystemMessage, parse_message_ids,
AvatarAction, GossipedKey, MimeMessage, PreMessageMode, SystemMessage, parse_message_id,
parse_message_ids,
};
use crate::param::{Param, Params};
use crate::peer_channels::{add_gossip_peer_from_header, insert_topic_stub, iroh_topic_from_str};
Expand Down Expand Up @@ -1018,10 +1019,14 @@ UPDATE config SET value=? WHERE keyname='configured_addr' AND value!=?1
context
.sql
.execute(
// Employ `msgs_index7 ON msgs (state, hidden, chat_id)` by explicit
// enumeration of `hidden` values, otherwise SQLite would iterate over all
// messages having `state` specified before, i.e. it would use the index
// only partly.
"
UPDATE msgs SET state=? WHERE
state=? AND
hidden=0 AND
hidden IN (0,1) AND
chat_id=? AND
timestamp<?",
(
Expand All @@ -1033,7 +1038,18 @@ UPDATE msgs SET state=? WHERE
)
.await
.context("UPDATE msgs.state")?;
if chat_id.get_fresh_msg_cnt(context).await? == 0 {
let n_fresh_msgs = context
.sql
.count(
"
SELECT COUNT(*) FROM msgs WHERE
state=? AND
(hidden=0 OR hidden=1) AND
Copy link
Copy Markdown
Collaborator

@link2xt link2xt Mar 26, 2026

Choose a reason for hiding this comment

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

I don't understand what hidden=0 OR hidden=1 means. Can this be removed? Is it a way to check for NULL? Does NULL ever get inserted?

Copy link
Copy Markdown
Collaborator Author

@iequidoo iequidoo Mar 26, 2026

Choose a reason for hiding this comment

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

It's a way to employ msgs_index7 ON msgs (state, hidden, chat_id). Unfortunately, SQLite doesn't track all values used for a particular column, so it can't use the index because it thinks that it should iterate over all messages having state specified before. I made some experiments earlier that confirm this behavior, though i didn't repeat them for this PR.

But it seems that i made a mistake here, OR should be the final operator connecting AND-terms. The relevant docs are here: https://sqlite.org/queryplanner.html#_or_connected_terms_in_the_where_clause. Better to use hidden IN (0,1), it defenitely works well with the index because i recently measured it in 677a799.

Copy link
Copy Markdown
Collaborator

@link2xt link2xt Mar 26, 2026

Choose a reason for hiding this comment

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

I have used EXPLAIN QUERY PLAN in #8040, it is useful for showing which index is actually used. As long as you never run ANALYZE sqlite is stable (#6585)

Copy link
Copy Markdown
Collaborator Author

@iequidoo iequidoo Mar 26, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

EXPLAIN QUERY PLAN UPDATE msgs SET state=? WHERE state=? AND chat_id=? AND timestamp<?;
QUERY PLAN
`--SEARCH msgs USING INDEX msgs_index7 (state=?)
EXPLAIN QUERY PLAN UPDATE msgs SET state=? WHERE state=? AND hidden IN (0,1) AND chat_id=? AND timestamp<?;
QUERY PLAN
`--SEARCH msgs USING INDEX msgs_index7 (state=? AND hidden=? AND chat_id=? AND timestamp<?)

Copy link
Copy Markdown
Collaborator Author

@iequidoo iequidoo Mar 26, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I ran it on my db, so it employs also timestamp because i'm testing some locally added db migrations. But anyway, w/o hidden IN (0,1) it can't even use chat_id.

Copy link
Copy Markdown
Collaborator Author

@iequidoo iequidoo Mar 26, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe we already add timestamp to msgs_index7? I see many places where it would be useful, e.g. calc_sort_timestamp(), building chatlists etc.

chat_id=?",
(MessageState::InFresh, chat_id),
)
.await?;
if n_fresh_msgs == 0 {
// Removes all notifications for the chat in UIs.
context.emit_event(EventType::MsgsNoticed(chat_id));
} else {
Expand Down Expand Up @@ -2011,9 +2027,13 @@ async fn add_parts(
)
.await?;

let mime_in_reply_to = mime_parser
.get_header(HeaderDef::InReplyTo)
.unwrap_or_default();
let mime_in_reply_to = match mime_parser.get_header(HeaderDef::InReplyTo) {
Some(in_reply_to) => parse_message_id(in_reply_to)
.log_err(context)
.ok()
.unwrap_or_default(),
None => "".to_string(),
};
let mime_references = mime_parser
.get_header(HeaderDef::References)
.unwrap_or_default();
Expand Down Expand Up @@ -2140,7 +2160,7 @@ async fn add_parts(
let is_incoming_fresh = mime_parser.incoming && !seen;
set_msg_reaction(
context,
mime_in_reply_to,
&mime_in_reply_to,
chat_id,
from_id,
sort_timestamp,
Expand Down Expand Up @@ -2282,7 +2302,7 @@ RETURNING id
} else {
Vec::new()
},
if trash { "" } else { mime_in_reply_to },
if trash { "" } else { &mime_in_reply_to },
if trash { "" } else { mime_references },
!trash && save_mime_modified,
if trash { "" } else { part.error.as_deref().unwrap_or_default() },
Expand Down
9 changes: 9 additions & 0 deletions src/sql/migrations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2363,6 +2363,15 @@ ALTER TABLE contacts ADD COLUMN name_normalized TEXT;
.await?;
}

inc_and_check(&mut migration_version, 150)?;
if dbversion < migration_version {
sql.execute_migration(
"CREATE INDEX msgs_index10 ON msgs (mime_in_reply_to)",
migration_version,
)
.await?;
}

let new_version = sql
.get_raw_config_int(VERSION_CFG)
.await?
Expand Down
Loading