-
-
Notifications
You must be signed in to change notification settings - Fork 85
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
fix: markseen_msgs: Limit not yet downloaded messages state to InNoticed (#2970) #5999
base: main
Are you sure you want to change the base?
Conversation
So the question is if it's doable in the apps to mark a fully downloaded message as seen again (when the user sees it) so that it's |
5a15031
to
be59fd4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, the new logic is:
- When fully downloading a big message, set
state
toInNoticed
(if it's\Seen
on IMAP) orInFresh
. The SQL code then calculatesmax(?,13)
, i.e. if it wasInFresh
, it's also changed toInNoticed
. - The state of the existing message is then changed to
InNoticed
if it wasInSeen
.
I think the logic could be simplified a bit, see #6047 (maybe it's possible to do it in an even simpler way, idk).
But, I'm concerned that we will send duplicate MDNs by doing this: Right now, even if the message is marked as \Seen
on IMAP, it's changed to InNoticed
and a read receipt is sent. So, every device downloading the message is going to send a read receipt.
Also, what I don't understand in the existing code: Why isn't a read receipt sent when the user sees the partially downloaded message? markseen_msgs()
looks like it unconditionally sends an MDN.
Yes, but currently there are no MDNs for big encrypted messages at all. Probably the correct solution is to set the IMAP
Because if it's an encrypted message, it "doesn't want" MDN until it's fully downloaded :) EDIT: So, if a big message isn't encrypted, with this change there will be two MDNs even in a single-device setup. Looks not good. |
be59fd4
to
f2abfd5
Compare
At least |
f2abfd5
to
1a7f108
Compare
Fixed it in another way, now should work correctly even for multi-device |
src/message.rs
Outdated
if curr_state == MessageState::InFresh | ||
&& matches!( | ||
curr_download_state, | ||
DownloadState::Available | DownloadState::InProgress |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about DownloadState::Failed
? I'm not sure when DownloadState::Failed
is set, but IIUC it's e.g. when the network vanishes while trying to download a message, and in this case, we still shouldn't mark it as \Seen
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed this and extended the test accordingly. Only DownloadedState::Done
messages must be upgraded to InSeen
, the rest -- only to InNoticed
src/message.rs
Outdated
if curr_state == MessageState::InFresh || curr_state == MessageState::InNoticed { | ||
if curr_state == MessageState::InFresh | ||
&& matches!( | ||
curr_download_state, | ||
DownloadState::Available | DownloadState::InProgress | ||
) | ||
{ | ||
update_msg_state(context, id, MessageState::InNoticed).await?; | ||
updated_chat_ids.insert(curr_chat_id); | ||
} else if curr_state == MessageState::InFresh || curr_state == MessageState::InNoticed { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this code, InNoticed
partially downloaded messages won't go into the first if
(because they're not InFresh
), so they will go into the else if
and marked as \Seen
, which they shouldn't.
Needs to be rewritten as sth like (pseudocode):
if partially_downloaded {
// Would be nice to have a comment here, e.g.:
// Don't mark partially downloaded messages as Seen or send a read receipt since they are not really seen by the user.
if curr_state == InFresh {
mark as InNoticed
}
} else if ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, rewrote the code this way and extended the test accordingly
1a7f108
to
b142273
Compare
Fixed one more bug, see Overall the logic seems complicated, so probably more tests are needed. This has already failed several review iterations, so let it be draft for now. EDIT: At least there must be a test that the message state isn't downgraded to |
b142273
to
7184237
Compare
In fact this bug was a little hidden, |
@iequidoo just re-request a review from & mark the PR as ready for review (= not a draft) once it's ready |
Add a test on what happens currently when apps call `markseen_msgs()` for not downloaded encrypted messages. Such messages are marked as seen, but MDNs aren't sent for them. Also currently when such a message is downloaded, it remains `InSeen` despite the full content hasn't yet been seen by the user.
…ced (#2970) This fixes sending MDNs for big messages when they are downloaded and really seen. Otherwise MDNs are not sent for big encrypted messages because they "don't want MDN" until downloaded.
If a message partially downloaded before is already IMAP-seen upon a full download, it should be updated to `InSeen`. OTOH if it's not IMAP-seen, but already `InNoticed` locally, its state should be preserved. So we take the maximum of two states.
7184237
to
c4dd912
Compare
Closes #2970