qxmpp icon indicating copy to clipboard operation
qxmpp copied to clipboard

Implement XEP-0444: Message Reactions

Open melvo opened this issue 2 years ago • 4 comments

PR check list:

  • [x] Document your code
  • [x] Add \since QXmpp 1.X, QXMPP_EXPORT
  • [ ] Fix doxygen warnings (see log when building with -DBUILD_DOCUMENTATION=ON)
  • [x] Update doc/doap.xml
  • [x] Add unit tests
  • [x] Format the code: Run clang-format -i src/<edited-file(s)> tests/<edited-file(s)>

melvo avatar Oct 04 '22 18:10 melvo

@lnjX I think that reactions would fit better than reaction because it is a list of emojis. What do you think?

melvo avatar Oct 04 '22 18:10 melvo

5.9 and windows builds are broken

lnjX avatar Oct 10 '22 14:10 lnjX

@lnjX I think that reactions would fit better than reaction because it is a list of emojis. What do you think?

yes, reactions would be better, it makes more sense + the XML element is also called reactions.

lnjX avatar Oct 10 '22 15:10 lnjX

I think it's unfortunate that the XEP doesn't use message attaching (yes, I've read the note about it in the XEP), because you can't easily fetch all the reactions via MAM but you need to receive all later MAM reaction messages to display reactions correctly.


Edit: I actually meant Message Fastening (https://xmpp.org/extensions/xep-0422.html)

lnjX avatar Oct 10 '22 15:10 lnjX

I think it's unfortunate that the XEP doesn't use message attaching (yes, I've read the note about it in the XEP), because you can't easily fetch all the reactions via MAM but you need to receive all later MAM reaction messages to display reactions correctly.

Edit: I actually meant Message Fastening (https://xmpp.org/extensions/xep-0422.html)

@mar-v-in What do you think about that?

melvo avatar Oct 15 '22 14:10 melvo

Codecov Report

Base: 68.50% // Head: 68.60% // Increases project coverage by +0.09% :tada:

Coverage data is based on head (b3e8ea0) compared to base (19c9d12). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #492      +/-   ##
==========================================
+ Coverage   68.50%   68.60%   +0.09%     
==========================================
  Files         292      295       +3     
  Lines       25673    25752      +79     
==========================================
+ Hits        17587    17666      +79     
  Misses       8086     8086              
Impacted Files Coverage Δ
src/base/QXmppMessage.h 100.00% <ø> (ø)
src/client/QXmppClient.cpp 59.80% <ø> (ø)
src/base/QXmppMessage.cpp 98.35% <100.00%> (+0.03%) :arrow_up:
src/base/QXmppMessageReaction.cpp 100.00% <100.00%> (ø)
src/base/QXmppMessageReaction.h 100.00% <100.00%> (ø)
tests/qxmppmessage/tst_qxmppmessage.cpp 94.92% <100.00%> (+0.09%) :arrow_up:
.../qxmppmessagereaction/tst_qxmppmessagereaction.cpp 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar Oct 15 '22 14:10 codecov[bot]

As of now, Fastening (XEP-0422) does not solve any issues here. MAM-FC (XEP-0427) - even if implemented by a server - would not be suitable for reactions as it doesn't preserve the information of the sender of the fastening in collate mode (simplified mode would only remove the reactions from MAM, full mode is what we have now). Sensible summarizing/collating of reactions would only be possible with server directly supporting reactions (and its processing rules), which means using fastening would not help realizing that.

mar-v-in avatar Oct 15 '22 15:10 mar-v-in

@mar-v-in I see it's not ideal. I think what would still make sense is to fetch old messages in collate mode and to load all fastenings/all reaction messages with a fastenings query.

Currently I see a problem in the following scenario:

  1. you join a group chat
  2. you load the message history
  3. you receive messages with reactions, but you can't find a message with the id => your client ignores the message
  4. you load another old message which people have reacted to, but your client doesn't display them

There're (at least) two ways to workaround this a) to load all messages in chronological order or b) to store the reactions messages even if the message id is unknown when receiving the reaction.

I haven't thought about fastenings in detail, maybe message fastenings (or mam fc) still need some adjustments or changes in the architecture, but in general it sounds like the "right way" to do this.

lnjX avatar Oct 15 '22 17:10 lnjX

  • Dino stores the reactions even for unknown messages. I wouldn't consider this a workaround though.
  • The problem is that you need to know who sent which reaction because you need to be able to process the live reactions when they come in (which means to replace what was there before from the same sender). Also you need to identity the user by occupant-id in anonymous MUCs.
  • I disagree that fastening / MAM-FC is the right way to do it. It tries to generify over all possible things that can be attached to a message. I doubt this is possible. I'd rather think that it makes sense to include processing and summarizing rules for MAM in each XEP where it makes sense to have those.
  • In practice, the summarized message isn't much smaller than the original message plus all its attachment, so the only advantage is that things are retrieved together. This indeed could be done in a generic way - one would probably want to have this even two-way (for replies, that would allow to instruct the server to also provide the message you replied to). I once drafted a specification for that, so if you think this is something that makes sense, we could build this in a XEP

mar-v-in avatar Oct 15 '22 17:10 mar-v-in

@mar-v-in I see your point. My main goal would be to be able to receive all attached/related messages at once. Your draft sounds interesting and having it work both ways (also for replys) sounds very useful.

lnjX avatar Oct 16 '22 18:10 lnjX