Skip to content

feat: Mark reactions as IMAP-seen in marknoticed_chat(), second try #6480

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.

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

Merged
merged 7 commits into from
Feb 22, 2025

Conversation

Hocuri
Copy link
Collaborator

@Hocuri Hocuri commented Jan 27, 2025

Instead of being trashed, the message containing a reaction remains in the chat, hidden and InFresh. When the chat is opened, it will be marked as Seen on the server, so that a second device removes the notifications for the reaction.

This is a more minimal alternative to #6213, trying to keep things simpler.

Close #6210.

Also, this adds a benchmark.

@Hocuri Hocuri force-pushed the hoc/seen-reactions branch from 82b4dfa to 74ab452 Compare January 27, 2025 15:51
src/chat.rs Outdated
Comment on lines 3349 to 3368
let hidden_messages = context
.sql
.query_map(
"SELECT id, rfc724_mid FROM msgs
WHERE state=?
AND hidden=1
AND chat_id=?
ORDER BY id DESC LIMIT 100", // LIMIT to 100 in order to avoid blocking the UI too long, usually there will be less than 100 messages anyway
(MessageState::InFresh, chat_id), // No need to check for InNoticed messages, because reactions are never InNoticed
|row| {
let msg_id: MsgId = row.get(0)?;
let rfc724_mid: String = row.get(1)?;
Ok((msg_id, rfc724_mid))
},
|rows| {
rows.collect::<std::result::Result<Vec<_>, _>>()
.map_err(Into::into)
},
)
.await?;
for (msg_id, rfc724_mid) in &hidden_messages {
message::update_msg_state(context, *msg_id, MessageState::InSeen).await?;
imap::markseen_on_imap_table(context, rfc724_mid).await?;
}
Copy link
Collaborator Author

@Hocuri Hocuri Jan 28, 2025

Choose a reason for hiding this comment

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

Performance matters here because this is done in the "hot path" of the user opening a chat. This could be replaced by:

        context
            .sql
            .transaction(|trans| {
                trans.execute(
                    "UPDATE msgs SET state=? WHERE state=? AND hidden=1 AND chat_id=?",
                    (MessageState::InSeen, MessageState::InNoticed, chat_id),
                )?;
                trans.execute(
                    "INSERT OR IGNORE INTO imap_markseen (id)
                    SELECT id FROM imap WHERE rfc724_mid IN
                    (SELECT rfc724_mid FROM msgs WHERE state=? AND hidden=1 AND chat_id=?)",
                    (MessageState::InNoticed, chat_id),
                )?;
                Ok(())
            })
            .await?;
        context.scheduler.interrupt_inbox().await;

However, in my benchmark, this actually has worse performance:

image

Probably because the messages have to be loaded twice now. Also, this always opens a write connection, not only when messages actually have to be marked as noticed.

If you know of a possibly more performant version of this, tell me & I'll benchmark it. But the most important part is the SELECT statement anyway, since it's the only thing that will be executed even if no messages are fresh.

Maybe I'll make update_msg_state() and markseen_on_imap_table() take a transaction in a subsequent PR, maybe this improves the (benchmark) performance.

@Hocuri Hocuri changed the title [WIP] feat: Mark reactions as IMAP-seen in marknoticed_chat(), second try feat: Mark reactions as IMAP-seen in marknoticed_chat(), second try Feb 16, 2025
@Hocuri Hocuri force-pushed the hoc/seen-reactions branch from 09bf947 to 359b208 Compare February 16, 2025 15:22
@Hocuri Hocuri requested review from iequidoo and link2xt February 16, 2025 15:22
@Hocuri Hocuri enabled auto-merge (squash) February 22, 2025 09:46
@Hocuri Hocuri disabled auto-merge February 22, 2025 10:06
@Hocuri Hocuri merged commit 38b08fa into main Feb 22, 2025
29 of 30 checks passed
@Hocuri Hocuri deleted the hoc/seen-reactions branch February 22, 2025 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it possible to mark reactions as "seen"
2 participants