qxmpp-project / qxmpp

Cross-platform C++ XMPP client and server library
408 stars 197 forks source link

Implement XEP-0444: Message Reactions #492

Closed melvo closed 1 year ago

melvo commented 1 year ago

PR check list:

melvo commented 1 year ago

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

lnjX commented 1 year ago

5.9 and windows builds are broken

lnjX commented 1 year ago

@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 commented 1 year ago

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)

melvo commented 1 year ago

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[bot] commented 1 year ago

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 ```diff @@ 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](https://codecov.io/gh/qxmpp-project/qxmpp/pull/492?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qxmpp-project) | Coverage Δ | | |---|---|---| | [src/base/QXmppMessage.h](https://codecov.io/gh/qxmpp-project/qxmpp/pull/492/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qxmpp-project#diff-c3JjL2Jhc2UvUVhtcHBNZXNzYWdlLmg=) | `100.00% <ø> (ø)` | | | [src/client/QXmppClient.cpp](https://codecov.io/gh/qxmpp-project/qxmpp/pull/492/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qxmpp-project#diff-c3JjL2NsaWVudC9RWG1wcENsaWVudC5jcHA=) | `59.80% <ø> (ø)` | | | [src/base/QXmppMessage.cpp](https://codecov.io/gh/qxmpp-project/qxmpp/pull/492/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qxmpp-project#diff-c3JjL2Jhc2UvUVhtcHBNZXNzYWdlLmNwcA==) | `98.35% <100.00%> (+0.03%)` | :arrow_up: | | [src/base/QXmppMessageReaction.cpp](https://codecov.io/gh/qxmpp-project/qxmpp/pull/492/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qxmpp-project#diff-c3JjL2Jhc2UvUVhtcHBNZXNzYWdlUmVhY3Rpb24uY3Bw) | `100.00% <100.00%> (ø)` | | | [src/base/QXmppMessageReaction.h](https://codecov.io/gh/qxmpp-project/qxmpp/pull/492/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qxmpp-project#diff-c3JjL2Jhc2UvUVhtcHBNZXNzYWdlUmVhY3Rpb24uaA==) | `100.00% <100.00%> (ø)` | | | [tests/qxmppmessage/tst\_qxmppmessage.cpp](https://codecov.io/gh/qxmpp-project/qxmpp/pull/492/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qxmpp-project#diff-dGVzdHMvcXhtcHBtZXNzYWdlL3RzdF9xeG1wcG1lc3NhZ2UuY3Bw) | `94.92% <100.00%> (+0.09%)` | :arrow_up: | | [.../qxmppmessagereaction/tst\_qxmppmessagereaction.cpp](https://codecov.io/gh/qxmpp-project/qxmpp/pull/492/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qxmpp-project#diff-dGVzdHMvcXhtcHBtZXNzYWdlcmVhY3Rpb24vdHN0X3F4bXBwbWVzc2FnZXJlYWN0aW9uLmNwcA==) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qxmpp-project). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qxmpp-project)

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

mar-v-in commented 1 year ago

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.

lnjX commented 1 year ago

@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.

mar-v-in commented 1 year ago
lnjX commented 1 year ago

@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.