998609 - Support topics in XMPP MUCs

Status: RESOLVED FIXED

(Chat Core :: XMPP, enhancement)

Description

[Patrick Cloke [:clokep]]

This is a pretty straightforward part of RFC 6121 part 5.2.4.

Side note: I hate our XMPP XML API.

Comment 1

[Patrick Cloke [:clokep]]

Attachment: Patch

Comment 2

[Florian Quèze [:florian]]

Comment on attachment 8409314 [details] [diff] [review]
Patch

Review of attachment 8409314 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for looking at our XMPP MUCs! :-)

The specification for MUC topics is http://xmpp.org/extensions/xep-0045.html#enter-subject

It says "In accordance with the core definition of XML stanzas, any message can contain a <subject/> element; only a message that contains a <subject/> but no <body/> element shall be considered a subject change for MUC purposes."

Unless I missed something, I don't think your patch implements the "but no <body/>" part.

Comment 3

[Patrick Cloke [:clokep]]

Unfortunately the server I've been using to test this doesn't seem to send the message with only a subject, it also contains a buddy (that says something like "foobar has set the topic to xyz".

Comment 4

[Florian Quèze [:florian]]

(In reply to Patrick Cloke [:clokep] from comment #3)
> Unfortunately the server I've been using to test this doesn't seem to send
> the message with only a subject, it also contains a buddy (that says
> something like "foobar has set the topic to xyz".

You meant body, not buddy, right?

Comment 5

[Patrick Cloke [:clokep]]

(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> (In reply to Patrick Cloke [:clokep] from comment #3)
> > Unfortunately the server I've been using to test this doesn't seem to send
> > the message with only a subject, it also contains a buddy (that says
> > something like "foobar has set the topic to xyz".
> 
> You meant body, not buddy, right?

Yes, sorry. :)

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8409314 [details] [diff] [review]
Patch

I changed my mind, it makes sense to match the behavior libpurple has, as doing something different could be perceived as a regression once we switch JS-XMPP on.

Here is what libpurple does:
http://lxr.instantbird.org/instantbird/source/purple/libpurple/protocols/jabber/message.c#219

I'm wondering if to exactly match we shouldn't move the "muc.incomingMessage(body, aStanza, date);" line after the topic handling.

Comment 7

[Patrick Cloke [:clokep]]

(In reply to Florian Quèze [:florian] [:flo] from comment #6)
> Comment on attachment 8409314 [details] [diff] [review]
> Patch
> 
> I changed my mind, it makes sense to match the behavior libpurple has, as
> doing something different could be perceived as a regression once we switch
> JS-XMPP on.
> 
> Here is what libpurple does:
> http://lxr.instantbird.org/instantbird/source/purple/libpurple/protocols/
> jabber/message.c#219
> 
> I'm wondering if to exactly match we shouldn't move the
> "muc.incomingMessage(body, aStanza, date);" line after the topic handling.

I can take a look at doing that.

Comment 8

[Patrick Cloke [:clokep]]

sawrubh, can you look at making the changes Florian requested here? Review can go back to me or him. Thanks!

Comment 9

[Saurabh Anand [:sawrubh]]

Attachment: Patch(v1)

So there can be multiple subject elements with different xml:lang's according to the RFC while there is no such mention in the XEP. What should be abide by?

Comment 10

[Patrick Cloke [:clokep]]

Thanks for fixing this up!

https://hg.mozilla.org/comm-central/rev/23794caa2fd4

Could you please file any follow up bugs you found?