qxmpp
qxmpp copied to clipboard
Implement XEP-0444: Message Reactions
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)>
@lnjX I think that reactions
would fit better than reaction
because it is a list of emojis. What do you think?
5.9 and windows builds are broken
@lnjX I think that
reactions
would fit better thanreaction
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.
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)
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?
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.
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 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:
- you join a group chat
- you load the message history
- you receive messages with reactions, but you can't find a message with the id => your client ignores the message
- 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.
- 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 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.