notify adding reactions #6072
notify adding reactions #6072
Conversation
|
I thought Android relies on Wonder if we should deprecate |
|
at least on android, notifications are driven by DC_EVENT_INCOMING_MSG - and with this PR also DC_EVENT_INCOMING_REACTION. but yes, it seems at least on android only getFreshMsg().length is used - having that as getFreshMsgsCount(0) would be nicely fitting to other places where the API uses this pattern. i did not check other OS yet, however |
r10s
force-pushed
the
r10s/notify-reactions
branch
from
932e1d6
to
e5e239f
Compare
src/receive_imf.rs
Outdated
| @@ -1482,6 +1482,15 @@ async fn add_parts( | |||
| Reaction::from(reaction_str.as_str()), | |||
| ) | |||
| .await?; | |||
| if mime_parser.incoming && in_fresh { | |||
| if let Some((msg_id, _)) = rfc724_mid_exists(context, mime_in_reply_to).await? { | |||
| context.emit_event(EventType::IncomingReaction { | |||
There was a problem hiding this comment.
But this emits an event for any reaction, not only to one's own sent message
There was a problem hiding this comment.
you're right, the condition is not sufficient (in_fresh was also wrong, fixed that and added a little test)
There was a problem hiding this comment.
But this emits an event for any reaction, not only to one's own sent message
should be be fixed by the last commit.
not sure, however, how to test the absence of an event - the event loop should be empty, maybe test for that? @iequidoo do you know, how that can be done or even have better ideas?
There was a problem hiding this comment.
You can use EventType::Test as a fence, just emit it and check that the next event is Test. That means there are no other events in the queue. Almost all the core code emits events "synchronously", there should be no parallel tasks continuing emitting events, so i think such a check is fine
Co-authored-by: iequidoo <[email protected]>
Co-authored-by: iequidoo <[email protected]>
| ) | ||
| .await?; | ||
| let is_fresh_reaction = !seen && !fetching_existing_messages; | ||
| if mime_parser.incoming && is_fresh_reaction && !reaction.is_empty() { | ||
| if let Some((msg_id, _)) = rfc724_mid_exists(context, mime_in_reply_to).await? { |
There was a problem hiding this comment.
I suggest to move this code to set_msg_reaction() somehow, at least there's already a call to rfc724_mid_exists(), though still the msgs table isn't accessed there, so Message::load_from_db() may be needed, but i think the check whether the message is outgoing can be merged into some existing SQL statement there
There was a problem hiding this comment.
hm, not sure adding complexity to the function and if it is worth optimising here too much, at the level of SQL.
but set_msg_reaction() could return MsgId, so that the additional call to rfc724_mid_exists() is not needed
| @@ -1484,7 +1484,7 @@ async fn add_parts( | |||
| ) | |||
| .await?; | |||
| let is_fresh_reaction = !seen && !fetching_existing_messages; | |||
| if mime_parser.incoming && is_fresh_reaction { | |||
| if mime_parser.incoming && is_fresh_reaction && !reaction.is_empty() { | |||
There was a problem hiding this comment.
Why not emit an event when a reaction is removed? The user will continue thinking there's a reaction that doesn't exist anymore. But that would require a translation for "unreacted" or smth like this, have no better idea
There was a problem hiding this comment.
yes, maybe. not sure if it is worth going that path, seems a cornercase, sort of "edit" - would we eg. notify all edits in case we have an edit function at some point? also, the removal is not even shown in-app. i see these notification more as "sth. was happening", not that the user tracks all notifications and then knows the final state :) eg. on iOS these things may even be accumulated as "2 messages, 3 reactions".
so, let's focus in this PR on notify adding reactions, it is a big inprovement already. we can always iterate, maybe let's see if that is really a real-world problem :)
do whatsapp/signal/telegram show a notification for a removed reaction?
this PR adds an event for reactions received for one's own messages.
this will allow UIs to add notification for these reactions.
this is how it looks like on iOS' lockscreen using deltachat/deltachat-ios#2331: