Add support for XEP-0461: Message Replies#226
Conversation
- xmppo/go-xmpp#226 Drop this commit if/when that is merged.
Contains: - xmppo/go-xmpp#226 Drop this commit if/when those are merged upstream.
Contains: - xmppo/go-xmpp#226 Drop this commit if/when those are merged upstream.
|
Hello! Thanks for the contribution! The complexity is groupchat and direct chat use different logic. I'm curious about @mdosch's opinion but personally i prefer more typing. In your PR, I'd personally either enjoy different types (does not fit the current API design, or golang patterns very well), but i think helper methods can be good: func (m *xmpp.Chat) withMucReply(to StanzaID) {
...
}
func (m *xmpp.Chat) withDirectMessageReply(to originID) {
...
}
func (m *xmpp.Chat) withDirectMessageReplyFallback(to string) {
...
}This would avoid clients using the wrong ID unintentionally. It would require to make the OriginID public which i think is required anyway to enable direct message replies. About the compatibility fallback, it looks more complex to implement (checking indices within the body string), but i think it's definitely worth it at least on the receiving side. I'm not sure what's a good API design for that, but we don't want a go-xmpp client receiving a reply to consider the entire quoted fallback message, i believe. Opinions? |
|
Oh, thank you! I was admittedly not thinking of the direct message case here. Adding helper methods makes sense to me, and they have the side benefit of improving documentation. For fallback replies, this is what the fallback object would look like per XEP-0428: so for incoming messages, I think we just need:
edit: oh. I forgot about Go string slicing. perhaps we use runes here? Does that make sense? Are there any complexities I'm not considering? |
Not to pile on more work for the sake of more work for @sh4sh but go-xmpp should not strip the quote fully. It has to be up to the client to decide what to do, because only the client knows. I think go-xmpp should parse From the XEP:
Consider in the context of matterbridge as a client: some protocols don't support replies, so if go-xmpp strips the incoming quote before matterbridge sees the message, people on those protocols will be confused. |
|
I edited my message to be clearer that my solution is for parsing replies on incoming messages. Do we need logic to ensure the quote is not stripped if the client doesn't use our reply structure? Or maybe we can re-add it? Here is a full stanza with reply and reply fallback: I agree that for outgoing messages, it would be confusing to remove the fallback formatting while still including the quote in message body. Is there a case where go-xmpp would be formatting quotes into the outgoing message body, or should that be handled by the client? And do we want to store the quote somewhere, so the client/protocol receiving our messages can access it? |
|
These are all very good points, thank you! I'm not very familiar with unicode representations (or golang), but it looks like a rune is indeed a unicode codepoint as expected by XEP-0246. However, that spec also says when the offset (
The easiest would of course to be not handle any of that and leave it up to the client. However, since that's very specific i think a correct implementation in go-xmpp is actually better. I also believe we should make it easy for clients supporting message replies to access the stripped body (without the fallback quote), but in order to avoid a breaking change for clients not implementing it, it should not be the default, but maybe a helper method. Maybe something like: func (m *xmpp.Chat) BodyWithReply() (string, string, err)But it's a good point that the func (m *xmpp.Chat) AddMucReply(to StanzaId, fallback string)(where fallback would contain the original message text, which could then be formatted into the actual body by go-xmpp) Of course we're speculating here but the maintainers of go-xmpp probably have feedback and opinions about how to best handle those situations. Let's not put any pressure on them. I'll be happy to test/review your matterbridge PR before this pull-request is merged here :-) |
- xmppo/go-xmpp#226 Drop this commit if/when that is merged.
- xmppo/go-xmpp#226 Drop this commit if/when that is merged.
| var replytext string | ||
| if chat.Reply.ID != `` { | ||
| replytext = `<reply id='` + xmlEscape(chat.Reply.ID) + `'` | ||
| if chat.Reply.To != `` { |
There was a problem hiding this comment.
The spec says there SHOULD be a to field. I think we don't have a reason to escape this and we should produce an error when id or to is an empty string. Do you agree?
There was a problem hiding this comment.
oh yeah, nice! Makes sense to me, will fix
There was a problem hiding this comment.
The incoming side has to allow for missing tos because SHOULD means optional; I don't think it hurts anything to be symmetric on the sending side.
There was a problem hiding this comment.
I don't think it hurts anything to be symmetric on the sending side.
It doesn't hurt. But a SHOULD has a clear meaning in RFCs which means unless you have a very strong reason not to do it, you have to do it (a MUST accepts no exception). It's not a MAY so my opinion is we should not make it optional on the sending side.
In the end the maintainers of the library will decide whatever fits more the library as a whole. That was just my personal opinion :)
| } | ||
|
|
||
| chat.Text = validUTF8(chat.Text) | ||
| id := getUUID() |
There was a problem hiding this comment.
Is there a reason why you removed this part where go-xmpp generates a new ID automatically and ignores any previously set ID on the message? I think both are reasonable behavior, but if you have an argument for this, it would probably be better in a separate PR (personal opinion).
There was a problem hiding this comment.
Is that because in order to handle replies properly the client should keep track of sent origin-ids? And since the Send method for some reason returns the number of bytes written instead of the generated ID, we need to handle that outside of the function signature to avoid a breaking change?
There was a problem hiding this comment.
Yes, exactly. The client needs an ID that is referenced later on reply, when it turns into origin-id id. (see change in matterbridge#134 where it gets generated)
...it would be nice to just use the UUID that go-xmpp generates, but yeah it's not already returned in Send() so it felt out of reach.
This seemed like the cleanest way. open to suggestions!
|
Should we advertise support for XEP-0461 in disco info? By default, we only handle the fallback, so by default we should not advertise the feature. However, i don't think go-xmpp handles features in disco queries yet? Did i miss something? |
Co-authored-by: kousu <nick@kousu.ca>
<reply> is optional, so it should be optional in our datamodel too.
To be removed upon completion of xmppo/go-xmpp#226
To be removed upon completion of xmppo/go-xmpp#226
I've implemented send/receive in 1 2, specifically matterbridge-org/matterbridge@f8cb9ab and 9e62a00. I split the quote out to I haven't pushed it to Implementing correctly turned out to be pretty wack 😅 ; I had to pull in a library for handling interval arithmetic. It's one file and MIT licensed so we could vendor it if bloating the dependency tree is a worry. I don't think it's avoidable though, the way is defined implies doing interval subtraction to determine the final message text. I'm proud of it though, I think it's a complete basis for adding other fallbacks. (it's worth having a solid basis because Cheogram as least "uses it for many things") |
To be removed upon completion of xmppo/go-xmpp#226
XEP-0461: Message Replies
We now generate the reply object and correctly attribute author.
I didn't implement 3.1 Compatibility fallback, we're still compliant without but I can look into it soon.