-
-
Notifications
You must be signed in to change notification settings - Fork 118
Send and apply MDNs to self (#7005) #7738
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
Conversation
link2xt
left a comment
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.
Second commit should not be merged, seems it is only for testing.
src/receive_imf.rs
Outdated
| .start_ephemeral_timer(context) | ||
| .await | ||
| .with_context(|| format!("start_ephemeral_timer for {msg_id}")) | ||
| .log_err(context) |
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.
Why ignore start_ephemeral_timer() errors? We did not ignore it in mark_seen_by_uid() which is used by sync_seen_flags, it should not fail without a good reason.
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.
Such functions may become more complex over time and start throwing logic errors e.g. if a message has already disappeared. There's a loop over items in the MDN, it's not necessary to fail the whole loop then or the whole receive_imf_inner(). I even was thinking about ignoring update_msg_state() errors (i.e. doing continue;), but it's a simple enough function.
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.
It just hides the problems, if the function becomes complex and buggy instead of an error with a device message that users will report, they will have silently non-starting ephemeral timer.
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.
Still no need to add a device message about failed-to-receive messages if just an MDN failed to be handled. I replaced log_err() with error! here in order to have an error toast.
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.
There is no toast for error! on Android anymore. They are out of fashion because of accessibility problems: https://primer.style/accessibility/patterns/accessible-notifications-and-messages/#toasts
src/receive_imf.rs
Outdated
| ) | ||
| .await | ||
| .context("UPDATE msgs.state")?; | ||
| if chat_id.get_fresh_msg_cnt(context).await? == 0 { |
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.
This is a change in the logic mentioned in #7659 (comment)
Previously we always emitted MsgsNoticed. Probably fine, because nothing should visually change in the chat, but maybe Android or iOS will not update the badge now as "chatlist item changed" event is likely only used on desktop.
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.
Yes, i decided to check for get_fresh_msg_cnt() here, otherwise if the user reads messages being offline and then the device comes online, sent MDNs will remove all notifications from other devices even if new messages have arrived. Notification not removed at all and badge counters not updated look more acceptable to me. CC @r10s
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.
I've tested this PR (with removed sync_seen_flags() additionally) in Desktop and it doesn't update the badge counter on the profile icon on another device until i read the last message in the chat, unfortunately. I.e. despite ChatlistItemChanged is emitted. CC @Simon-Laux
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.
But the badge counter on the chatlist item is updated. So the two numbers differ.
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.
Other platforms are even more likely to have problems if they don't use chatlist events.
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.
I'd move this change out of this PR into a separate PR so we have old bug with removed notifications but no new badge bug that needs fixes on all platforms.
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.
} else {
context.emit_event(EventType::MsgsNoticed(DC_CHAT_ID_TRASH));
}works as a w/a here. Then the profile icon badge counter is updated as well. But can also emit it for the self-chat so that it's less weird.
EDIT: Ok, let's move this to a separate PR, no need to make this decision here.
src/receive_imf.rs
Outdated
| .start_ephemeral_timer(context) | ||
| .await | ||
| .with_context(|| format!("start_ephemeral_timer for {msg_id}")) | ||
| .log_err(context) |
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.
Such functions may become more complex over time and start throwing logic errors e.g. if a message has already disappeared. There's a loop over items in the MDN, it's not necessary to fail the whole loop then or the whole receive_imf_inner(). I even was thinking about ignoring update_msg_state() errors (i.e. doing continue;), but it's a simple enough function.
src/receive_imf.rs
Outdated
| ) | ||
| .await | ||
| .context("UPDATE msgs.state")?; | ||
| if chat_id.get_fresh_msg_cnt(context).await? == 0 { |
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.
Yes, i decided to check for get_fresh_msg_cnt() here, otherwise if the user reads messages being offline and then the device comes online, sent MDNs will remove all notifications from other devices even if new messages have arrived. Notification not removed at all and badge counters not updated look more acceptable to me. CC @r10s
| ) | ||
| .await | ||
| .context("failed to insert into smtp_mdns")?; | ||
| context.scheduler.interrupt_smtp().await; |
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.
We may consider not waking up SMTP here if created a self-only MDN to avoid exhausting ratelimit, but then it's possible that messages' state isn't synced across devices on time
5401bd1 to
2bd3ffa
Compare
|
So the existing tests work with and w/o |
We currently synchronize "seen" status of messages by setting `\Seen` flag on IMAP and then looking for new `\Seen` flags using `CONDSTORE` IMAP extension. This approach has multiple disadvantages: - It requires that the server supports `CONDSTORE` extension. For example Maddy does not support CONDSTORE yet: foxcpp/maddy#727 - It leaks the seen status to the server without any encryption. - It requires more than just store-and-forward queues and prevents replacing IMAP with simpler protocols like POP3 or UUCP or some HTTP-based API for queue polling. A simpler approach is to send MDNs to self when `Config::BccSelf` (aka multidevice) is enabled, regardless of whether the message requested and MDN. If MDN was requested and we have MDNs enabled, then also send to the message sender, but MDN to self is sent regardless of whether read receipts are actually enabled. `sync_seen_flags()` and `CONDSTORE` check is better completely removed, maybe after one release. `store_seen_flags_on_imap()` can be kept for unencrypted non-chat messages. One potential problem with sending MDNs is that it may trigger ratelimits on some providers and count as another recipient.
3ed9022 to
2048901
Compare
We currently synchronize "seen" status of messages by setting
\Seenflag on IMAP and then lookingfor new
\Seenflags usingCONDSTOREIMAP extension. This approach has multiple disadvantages:CONDSTOREextension. For example Maddy does not supportCONDSTORE yet: Feature request: Quotas and CONDSTORE for Delta Chat foxcpp/maddy#727
protocols like POP3 or UUCP or some HTTP-based API for queue polling.
A simpler approach is to send MDNs to self when
Config::BccSelf(aka multidevice) is enabled,regardless of whether the message requested and MDN. If MDN was requested and we have MDNs enabled,
then also send to the message sender, but MDN to self is sent regardless of whether read receipts
are actually enabled.
sync_seen_flags()andCONDSTOREcheck is better completely removed, maybe after onerelease.
store_seen_flags_on_imap()can be kept for unencrypted non-chat messages.One potential problem with sending MDNs is that it may trigger ratelimits on some providers and
count as another recipient.
Close #7005