feat: a way to add a device message as InNoticed#8400
Conversation
WofWca
force-pushed
the
wofwca/add-device-message-seen
branch
from
57487a6 to
d87ba39
Compare
|
Maybe the more proper thing to do would be to move all this "mark as read" logic to Core instead, but I decided to start with one small step. |
iequidoo
left a comment
iequidoo
left a comment
There was a problem hiding this comment.
Overall the idea looks good, if i got it correctly: since you know that you're adding the same message to all accounts, you show a notification on your own
| #[serde(flatten)] | ||
| pub message_data: MessageData, | ||
| #[serde(default)] | ||
| pub message_state: Option<DeviceMessageState>, |
There was a problem hiding this comment.
Can't this be replaced with a bool flag noticed? Why do we need a new enum with 3 variants (InSeen also)? If adding a message as InNoticed doesn't work, i.e. Core still emits IncomingMsg, that should be fixed.
There was a problem hiding this comment.
TBH I still haven't fully grasped the concept of noticed vs. seen.
If what we actually want is noticed, then I can remove "seen". But I would keep it as an enum, for clarity, consistency with the Message struct, and flexibility.
There was a problem hiding this comment.
InSeen means "actually seen/read", so an MDN can be sent (this isn't relevant to device messages though). InNoticed means "the user has opened the chat or read some later chat messages", so the user has had a chance to see the message on this or another device and no new notification is needed. As we don't want to trigger a notification, InNoticed should already work.
As for enum, this is minor, but anyway DeviceMessageState is inconsistent with enum MessageState because it doesn't list the same variants and with a bool flag there will be less code i guess.
There was a problem hiding this comment.
OK, I removed the InSeen variant.
This is useful for adding the same device message on multiple accounts, which is what we do at least on Desktop currently: https://github.com/deltachat/deltachat-desktop/blob/c50896f1d529b09ea584f8fccca3f5fca7fe6ed7/packages/frontend/src/deviceMessages.ts#L35-L44. The problem on desktop is that we then have to `rpc.marknoticedChat()` so that the user isn't forced to read the same message on all their accounts: deltachat/deltachat-desktop#3934. However, that, in turn, is now causing problems in our notification clearing logic: marking a chat as noticed is supposed to remove the per-account "N messages in M chats" notification, but we only want to do that when the user actually reads a message on the account and, whereas we do this `marknoticedChat()` for device messages on app startup for all accounts. See the TODO in deltachat/deltachat-desktop#6154.
WofWca
force-pushed
the
wofwca/add-device-message-seen
branch
from
d87ba39 to
d1f8da3
Compare
WofWca
changed the title
InSeenInNoticed
iequidoo
left a comment
iequidoo
left a comment
There was a problem hiding this comment.
The idea LGTM, but not sure about the API (new enum, etc.), so i leave it up to other reviewers.
I also see that MessageState variants should be documented more clearly, there are always questions about InNoticed vs InSeen. Here's my interpretation, maybe we should document it this way:
InNoticedmeans that the user should already be aware of the new message and which chat it belongs to, but probably hasn't read it yet. So, no event is needed, but we can't send an MDN yet.InSeenmeans that the user has probably already read the message and an MDN can be sent.
| assert_eq!(msg1.text, "first message"); | ||
| assert_eq!(msg1.from_id, ContactId::DEVICE); | ||
| assert_eq!(msg1.to_id, ContactId::SELF); | ||
| assert_eq!(msg1.get_state(), MessageState::InNoticed); |
There was a problem hiding this comment.
Would be good to also check that no IncomingMsg event is emitted
2 participants
This is useful for adding the same device message on multiple accounts,
which is what we do at least on Desktop currently:
https://github.com/deltachat/deltachat-desktop/blob/c50896f1d529b09ea584f8fccca3f5fca7fe6ed7/packages/frontend/src/deviceMessages.ts#L35-L44.
The problem on desktop is that we then have to
rpc.marknoticedChat()so that the user isn't forced to read the same message
on all their accounts:
deltachat/deltachat-desktop#3934.
However, that, in turn, is now causing problems
in our notification clearing logic: marking a chat as noticed
is supposed to remove the per-account "N messages in M chats"
notification, but we only want to do that when the user
actually reads a message on the account and,
whereas we do this
marknoticedChat()for device messageson app startup for all accounts. See the TODO in
deltachat/deltachat-desktop#6154.