Reactions send notifications while chat is open on different device

[Amzd]

• Operating System (Linux/Mac/Windows/iOS/Android): iOS / desktop
• Core Version: v2.35
• Client Version: v2.35

Expected behavior

DC_EVENT_INCOMING_MSG is not fired when a message is not fresh so it would make sense if DC_EVENT_INCOMING_REACTION (and maybe DC_EVENT_INCOMING_WEBXDC_NOTIFY, didn't test) would also not be fired when those messages are not fresh.

Actual behavior

• DC_EVENT_INCOMING_MSG: only fired if message is fresh
• DC_EVENT_INCOMING_REACTION: always fired

Steps to reproduce the problem

1. open a chat on desktop
2. receive a message in that chat
3. get no notification for it on iOS because DC_EVENT_INCOMING_MSG is not fired (this is good)
4. receive a reaction in that chat
5. get a notification for it on iOS because DC_EVENT_INCOMING_REACTION is fired (this is not consistent with messages)

report on iOS repo: deltachat/deltachat-ios#3011

[iequidoo]

Do UIs pass reaction messages to markseen_msgs() currently? If not, and there are no such plans, i think the fix is to make all reactions InSeen when the chat is noticed. Apart from this, reactions are usual chat messages, just hidden, and handling their state shall not be different from visible messages, need to check this throughout the code.

Another problem here is that we want to remove \Seen IMAP flag synchronization, but if we do this now, we will emit DC_EVENT_INCOMING_MSG for seen messages (even non-reactions) because an MDN arrives later than the message. This can be postponed for this bug, but apparently we shouldn't emit events early (from receive_imf), we should accumulate them and discard events for messages having MDNs. This looks easy though, can be done in the imap receiving loop.

NB: At least in Android (haven't checked other clients) you can "MARK READ" a notification for reaction. Looks good to me, but also need to check that this is handled in Core the same way as for visible messages.
...

the fix is to make all reactions InSeen when the chat is noticed.

This is already done in marknoticed_chat(). So the question is whether iOS calls dc_marknoticed_chat() constantly, i.e. when the chat is open and a new message or reaction arrives. @Amzd
...

Tested how it works in Android if i keep the chat open in Desktop: If i receive a reaction, a notification arrives in Android and it's not removed even if i scroll the chat forth and back. But if i click on the "scroll to bottom" button, notification is removed on Android (because Desktop calls marknoticed_chat() i guess). For usual messages however i don't need to scroll the chat, notification appear and then disappear quickly -- it seems that Desktop constantly calls marknoticed_chat() (EDIT: it calls markseen_msgs() actually) in this case. Looks like we need to verify the current behavior first, so cc-ing @WofWca

[Amzd]

dc_marknoticed_chat() is not called when a new message comes in, only when the view is shown initially. I assumed that dc_markseen_msgs would do that under the hood but we can add that on the client side.

iOS already supports removing notifications as soon as DC_EVENT_MSGS_NOTICED comes in, it's just that it rarely comes in probably because other clients don't call dc_marknoticed_chat() either?

it seems that Desktop constantly calls marknoticed_chat() in this case.

I think it is because it marks the message as read which means the DC_EVENT_INCOMING_MSG event doesn't fire?.. like what I mentioned in the issue

[iequidoo]

dc_marknoticed_chat() is not called when a new message comes in, only when the view is shown initially. I assumed that dc_markseen_msgs would do that under the hood but we can add that on the client side.

Sure, at least for usual messages dc_markseen_msgs() must be called if the chat is open, for reactions dc_marknoticed_chat() is also fine however because in this case Core marks hidden messages as seen.

iOS already supports removing notifications as soon as DC_EVENT_MSGS_NOTICED comes in, it's just that it rarely comes in probably because other clients don't call dc_marknoticed_chat() either?

Yes, at least Desktop doesn't call it AFAIU because i need to manually click on "scroll to bottom" or reopen the chat so that reaction notifications disappear on Android.

EDIT: Core doesn't report the reaction message id however (see IncomingReaction), but only the referenced message id, so there's no way for UIs currently to call dc_markseen_msgs() for reactions. dc_marknoticed_chat() should be called.

[WofWca]

it seems that Desktop constantly calls marknoticed_chat() (EDIT: it calls markseen_msgs() actually) in this case. Looks like we need to verify the current behavior first

We mark messages as seen when they are in direct view.
We only mark Chat as noticed when we switch to it, or, and I think when pressing jump to bottom, but never automatically.

dc_marknoticed_chat() should be called.

Are you saying that we are supposed to call it every time we receive a reaction in a chat that is currently open?
To me, this sound like this logic should be internal to core.

I would say that it is logical to mark a reaction message as read when a message that this reaction belongs to is in view.

But if we are supposed to mark chat as noticed for every time we read a message from there, then it sounds like this should be happening in Core.

[iequidoo]

I would say that it is logical to mark a reaction message as read when a message that this reaction belongs to is in view.

This looks like a real solution. Then UIs should call dc_markseen_msgs() when a message comes into view (it doesn't matter if it's already seen or not) or it's already in view and a reaction arrives for it (Core reports the message id referenced by a reaction, so this looks easy). On the Core side, we must mark reactions as seen even if the referenced message is already seen, this is the actionable item to close the bug. Further fixes needed to remove the \Seen flag synchronization may be done later.

EDIT: However, if UIs aren't going to call dc_marknoticed_chat() when a reaction in an open chat is received, the behavior reported here as a bug isn't always "fixed", but only if the referenced message is in view. @Amzd are you fine with this?

[Amzd]

only if the referenced message is in view

yes that is fine, I think that's a good fix at least for reactions. However for DC_EVENT_INCOMING_WEBXDC_NOTIFY that doesn't work as you are not very likely to scroll to the webxdc message right? or are the info messages also marked as seen?

I still think there is more to this because the DC_EVENT_MSGS_NOTICED event should clear the notifications on iOS and android, and this should be fired when a message is marked as read on a different device right? But I don't fully grasp how and when that doesn't work as expected so lets do the proposed fix, and iterate later.

[WofWca]

However for DC_EVENT_INCOMING_WEBXDC_NOTIFY

You're suggesting that a WebXDC info message is similar to a reaction message, in that it has a "parent message"?

or are the info messages also marked as seen?

I would say yes. We treat them as regular messages. Not different from "someone left the group" and other ones.
Maybe it makes sense to mark such info messages as read when the WebXDC app gets opened or is open, but that's for another day I'd say.

[Amzd]

Wait I read too hastily, why can't core just mark all the reactions as seen when client reports it has seen the message they belong to? This seems like it would save work on all clients?

[WofWca]

Isn't this what #7884 (comment) suggested?

[Amzd]

Oh I am losing it sry

[iequidoo]

However for DC_EVENT_INCOMING_WEBXDC_NOTIFY that doesn't work as you are not very likely to scroll to the webxdc message right? or are the info messages also marked as seen?

Unfortunately, the state of webxdc info messages can't be synced across devices using the current logic because we use MDNs referencing Message-Id for that and webxdc info messages don't have it. Anyway, let's start with reactions. For webxdc info messages we can introduce a new sync message type and use it instead of self-sent MDNs.

I still think there is more to this because the DC_EVENT_MSGS_NOTICED event should clear the notifications on iOS and android, and this should be fired when a message is marked as read on a different device right?

DC_EVENT_MSGS_NOTICED is already fired when all visible (non-hidden) messages are noticed in the chat, probably we should modify this logic and count all messages, including hidden (reactions are such). Anyway, this logic doesn't work well already now because it either removes all notifications for the chat or none of them. Core should report which messages were seen exactly, we need a new event for this.