feat: Mark reactions as IMAP-seen in marknoticed_chat() (#6210) #6213
Conversation
|
Since you marked it as a draft, I take it that it's not ready to be reviewed yet? |
iequidoo
force-pushed
the
iequidoo/seen-reactions
branch
from
a089199
to
b71c681
Compare
Now it's ready and seems working, but probably needs more tests. Was a draft because i needed to optimise out saving unnecessary columns to db. It's interesting that sent out reactions were already hidden messages, didn't know about that. |
|
Ideally there will be a test that marks a reaction as seen on one device and checks that a MsgsNoticed event is emitted on another device (and check that the test fails without this PR here), in addition to the unit tests. If this is not feasible, would be good to manually test that this event is sent. Edit: If, during testing, it turns out that it's harder to do than expected, it would be fine to just ignore the issue, see deltachat/deltachat-android#3377 (comment) |
I think it's not so difficult to write a jsonrpc Python test for that and it would be good to have incoming reactions in the |
iequidoo
force-pushed
the
iequidoo/seen-reactions
branch
3 times, most recently
from
de2b5a1
to
d9482dc
Compare
src/chat.rs
Outdated
| )?; | ||
| let mut stmt = conn.prepare( | ||
| "SELECT id FROM msgs | ||
| WHERE state>=? AND state<? |
There was a problem hiding this comment.
Performance: Right now, this PR slows down opening a chat by over 1ms (measured by writing let t = std::time::Instant::now(); and info!(context, "dbg it took {:?}", t.elapsed()); into the code). This can be fixed by writing WHERE state=? here and replacing MessageState::InFresh, MessageState::InSeen with MessageState::InNoticed below.
There was a problem hiding this comment.
I find it strange that marknoticed_chat() then will transfer visible messages from InFresh to InNoticed, but hidden messages -- only from InNoticed to InSeen, but not from InFresh (InSeen looks correct for hidden messages as there's nothing to see). Looks like smth conceptually broken.
So it seems the slowness is caused by existing hidden InFresh messages?
There was a problem hiding this comment.
If we don't want other hidden messages to be returned by SELECT here and marked as seen on IMAP, we should probably create hidden messages other than reactions as InSeen initially. And as for existing hidden messages, add a db migration for them
src/receive_imf.rs
Outdated
|
|
||
| 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 { |
There was a problem hiding this comment.
This change leads to IncomingMsg instead of MsgsChanged being emitted, so that the UI shows a notification "Hocuri: ❤️" if I reacted with a heart emoji. Which we don't want, since we instead to show the "Hocuri reacted ❤️ to "I like your avatar!"" message in the UI.
So, this needs to be:
state = if seen || fetching_existing_messages || is_mdn || chat_id_blocked == Blocked::Yes {
MessageState::InSeen
} else if is_reaction {
MessageState::InNoticed
} else {
MessageState::InFresh
};There was a problem hiding this comment.
Ok, this is a bug which requires a separate test it seems.
But again, making reactions InNoticed to only make marknoticed_chat() faster doesn't look conceptually correct. Reactions are still a user-noticeable info, so there's smth to "notice". And wrt the bug you found, rather we shouldn't emit IncomingMsg for hidden messages. But not sure, need to try and see if the tests doesn't break at least. EDIT: Tried, the Rust tests continue to work at least.
There was a problem hiding this comment.
A PR extending the existing tests to catch this bug: #6257. Checked that the modified tests fail for this PR
There was a problem hiding this comment.
Incoming reactions aren't really InFresh either, because InFresh means that the message is counted in the unread-counter and is shown in notifications. And info messages are also immediately added as InNoticed, because that's what they semantically are.
There was a problem hiding this comment.
Not necessarily, get_fresh_msg_cnt() includes the and m.hidden=0 SQL condition, so if we need, we can make incoming reactions hidden InFresh messages. As for "is shown in notifications", now this is true for incoming reactions, that's also why i think they should be InFresh. If we add them as InNoticed and show notifications for them, that would be different from incoming messages. So, my suggestion is:
- Add incoming reactions as as hidden InFresh messages.
- Add other incoming hidden messages as InSeen. But looking at
receive_imf.rsi don't see such messages even exist. But then i don't understand why we haveand m.hidden=0checks for incoming messages in SQL everywhere. So this needs investigation. - Add a db migration for existing hidden InFresh messages. At least just in case.
There was a problem hiding this comment.
From a code perspective, we can make them InFresh, it requires one more special case (in order not to send an IncomingMsg), but that doesn't make any difference, really.
What does make a difference is whether we introduce 1ms of delay (1ms on quite a new phone, that is - older phones may easily need 3 times as long). Doing this just once isn't noticeable, but if we do it often (and without necessarily needing to) then we will end up with slow code (and markseen is called every time a chat is opened). Measuring this isn't hard, you can also do it - if you find a way that is just as fast, I'm fine with that, too. You may need to measure on a phone, not on Desktop, because on a fast PC the difference may not be noticeable.
Add a db migration for existing hidden InFresh messages. At least just in case.
We shouldn't do database migrations "just in case", since there is always some chance that they create problems, and we don't have any indication that it will solve problems.
There was a problem hiding this comment.
From a code perspective, we can make them InFresh, it requires one more special case (in order not to send an IncomingMsg)
Done in 110f0d3, doesn't look complex.
What does make a difference is whether we introduce 1ms of delay
I'm going to profile that, but anyway, there's a new SQL statement selecting hidden messages (actually reactions), so i'm afraid it will always add some delay (NB: message::markseen_msgs() is a no-op if msg_ids is empty, so it's not a source of the problem). In #6213 (comment) you suggested a solution:
This can be fixed by writing
WHERE state=?here and replacingMessageState::InFresh, MessageState::InSeenwithMessageState::InNoticedbelow.
but i don't understand how it works -- just because you simplify the SQL statement (which is unlikely) or because you don't select other unnecessary messages and don't pass them to message::markseen_msgs()? But there should be no other hidden InFresh messages currently (NB: Only looking at current receive_imf.rs, i'm not sure they were never created in the past).
We shouldn't do database migrations "just in case", since there is always some chance that they create problems
See "NB" above -- if hidden InFresh messages were created in the past, they will be selected and passed to message::markseen_msgs() which we should avoid. So, probably this db migration will be a no-op, but as we can't be sure (we can't inspect all the previously released versions, really), better to add it.
There was a problem hiding this comment.
but i don't understand how it works -- just because you simplify the SQL statement (which is unlikely) or because you don't select other unnecessary messages and don't pass them to
message::markseen_msgs()?
I don't know, my guess would be that it's because the simplification in the SQL statement enabled some optimization.
There was a problem hiding this comment.
Maybe it works because we have msgs_index7 ON msgs (state, hidden, chat_id). If so, i should replace state>=? AND state<? with an OR condition containing just InFresh and InNoticed.
EDIT: Maybe just with state=<InFresh> because InNoticed reactions are normally a result of mark_old_messages_as_noticed() which is called from imap when new messages arrive, but that doesn't need synchronisation using the \Seen flag because that happens on all devices anyway.
EDIT: This will lead to not all reactions marked as seen on IMAP, but i think it's ok, better to optimise here.
|
BTW, this should be changed now: deltachat-core-rust/src/chat.rs Lines 1411 to 1413 in e121fc1 We have hidden messages now, but they shouldn't affect how other messages are ordered, so |
This is a code path for protection messages. Protection messages should be sorted under any hidden messages, incl. reactions, and we already have outgoing reactions as hidden messages. The comment just says why we don't add As for the |
iequidoo
force-pushed
the
iequidoo/seen-reactions
branch
from
d9482dc
to
110f0d3
Compare
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 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.
iequidoo
force-pushed
the
iequidoo/seen-reactions
branch
from
110f0d3
to
3e8b050
Compare
| let mut stmt = conn.prepare( | ||
| "SELECT id FROM msgs | ||
| WHERE state=? AND hidden=1 AND chat_id=? | ||
| ORDER BY id DESC LIMIT 100", |
There was a problem hiding this comment.
Now this statement should be fast as we have msgs_index7 ON msgs (state, hidden, chat_id), but maybe we want LIMIT 1 here because the effect is the same anyway. This is what i don't understand:
- Suppose one device received reaction A.
- And another device -- A and B.
- The user clicks on "Mark as read" on the first device.
- On the second device
MsgsNoticedis emitted which results in removing a notification for reaction B. - Then the user has a chance to see the notification for reaction B on the first device only.
CC @r10s
Seems the same problem exists for "usual" messages because MsgsNoticed only contains ChatId. Didn't know about that. I hope this is just my misunderstanding.
See commit messages.
Close #6210
For
DC_CHAT_ID_ARCHIVED_LINKthis does nothing, marking all reactions as seen in the archive doesn't seem useful.mark_old_messages_as_noticed()only marks reactions asInNoticed, this function is called fromimapwhen new messages arrive, so it is anyway called on all devices, no synchronisation is needed.DONE: