Support MUC (XEP-0045) Room Discovery (§6.3)

RESOLVED FIXED in Instantbird 50

Status

Chat Core
XMPP
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Mook, Assigned: abdelrhman)

Tracking

trunk
Instantbird 50
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

2 years ago
Currently, once we find the feature stanza for the MUC, we record the service [1] and use it to populate the join chat dialog (the "server" input box).  It would be nice if we could do room discovery too and use it in the New Conversation tab.  I _think_ that means implementing prplIAccount::requestRoomInfo().

[1]: https://dxr.mozilla.org/comm-central/rev/b7472e239aa2/chat/protocols/xmpp/xmpp.jsm#1502

Updated

2 years ago
Blocks: 954959

Updated

2 years ago
Depends on: 1060891
(Assignee)

Updated

2 years ago
Assignee: nobody → ab
(Assignee)

Comment 1

2 years ago
Created attachment 8765614 [details] [diff] [review]
v1 -  Support Room Discovery
Attachment #8765614 - Flags: review?(aleth)
(Assignee)

Comment 2

2 years ago
Created attachment 8765616 [details] [diff] [review]
v1 - protocol-independant changes
Attachment #8765616 - Flags: review?(aleth)

Comment 3

2 years ago
Comment on attachment 8765616 [details] [diff] [review]
v1 - protocol-independant changes

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

::: im/components/ibConvStatsService.js
@@ +695,5 @@
>    },
>    get statusText() {
>      let roomInfo = this.account.prplAccount.getRoomInfo(this.displayName);
> +    let text = "";
> +    if (roomInfo.participantCount >= 0)

That's not going to work as is, participantCount is an unsigned long.
Attachment #8765616 - Flags: review?(aleth) → review-
(Assignee)

Comment 4

2 years ago
Created attachment 8765622 [details] [diff] [review]
v2 - protocol-independant changes
Attachment #8765616 - Attachment is obsolete: true
Attachment #8765622 - Flags: review?(aleth)

Comment 5

2 years ago
Comment on attachment 8765614 [details] [diff] [review]
v1 -  Support Room Discovery

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +1048,5 @@
> +  },
> +  get participantCount() {
> +    let jid = this._account._roomList.get(this.name);
> +    if (!this._account._mucs.has(jid))
> +      return -1;

unsigned long...

@@ +1708,5 @@
>      });
>    },
>  
> +  requestRoomInfo: function(aCallback) {
> +    this._roomList = new Map();

Add a mechanism to not request the list from the server again every time a new tab is opened, similar to https://dxr.mozilla.org/comm-central/source/chat/protocols/irc/irc.js#968

@@ +1711,5 @@
> +  requestRoomInfo: function(aCallback) {
> +    this._roomList = new Map();
> +    this._roomInfoCallback = aCallback;
> +
> +    // XEP-0045 (6.5): Querying for Room Items.

6.3, not room items, I think?

@@ +1733,5 @@
> +    // XEP-0059: Result Set Management.
> +    let set = query.getElement(["set"]);
> +    let last = set ? set.getElement(["last"]) : null;
> +    let isCompleted;
> +    if (last && items.length > 0) {

Can last && items.length == 0 happen?

@@ +1745,5 @@
> +    else
> +      isCompleted = true;
> +
> +    let rooms = items.map(item => {
> +      let name = item.attributes["name"];

Can it happen that this is undefined? e.g. 6.3 example 7
Attachment #8765614 - Flags: review?(aleth) → review-
Comment on attachment 8765614 [details] [diff] [review]
v1 -  Support Room Discovery

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +1048,5 @@
> +  },
> +  get participantCount() {
> +    let jid = this._account._roomList.get(this.name);
> +    if (!this._account._mucs.has(jid))
> +      return -1;

We probably want to add a constant for this (like what we did for typing notifications).
(Assignee)

Comment 7

2 years ago
(In reply to aleth [:aleth] from comment #5)
> 6.3, not room items, I think?
 
Yes, that was wrong.

> Can last && items.length == 0 happen?

I added it to avoid infinite recursive calls (I'm not sure if the last result will contain last element as I didn't see that in the spec XEP-0059). BTW, I'll check this again today.

> Can it happen that this is undefined? e.g. 6.3 example 7

Not sure, but I tested room discovery with a real server and returned jid and name, but I think this can happen.
(Assignee)

Comment 8

2 years ago
Created attachment 8766331 [details] [diff] [review]
v3 - protocol-independant changes
Attachment #8765622 - Attachment is obsolete: true
Attachment #8765622 - Flags: review?(aleth)
Attachment #8766331 - Flags: review?(aleth)
(Assignee)

Comment 9

2 years ago
Created attachment 8766417 [details] [diff] [review]
v2 - Support Room Discovery
Attachment #8765614 - Attachment is obsolete: true
Attachment #8766417 - Flags: review?(aleth)

Comment 10

2 years ago
Comment on attachment 8766331 [details] [diff] [review]
v3 - protocol-independant changes

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

::: chat/components/public/imIAccount.idl
@@ +54,1 @@
>    readonly attribute unsigned long participantCount;

Can't you just make this a signed long? ;)
(Assignee)

Comment 11

2 years ago
Created attachment 8766571 [details] [diff] [review]
v4 - protocol-independant changes
Attachment #8766331 - Attachment is obsolete: true
Attachment #8766331 - Flags: review?(aleth)
Attachment #8766571 - Flags: review?(aleth)

Comment 12

2 years ago
Comment on attachment 8766417 [details] [diff] [review]
v2 - Support Room Discovery

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

Overall this looks much better, but there are some problems.

There are important but slightly nonstandard servers. For example, on hipchat in response to the disco#info on the conf jid one receives (instead of?) a room list
<iq xmlns="jabber:client" from="conf.hipchat.com" type="result" id="14164f12-9889-7f43-a549-9a1cd6859da4" to="42_041@chat.hipchat.com/Instantbird||proxy|pubproxy-b300.hipchat.com|5242">
<query xmlns="http://jabber.org/protocol/disco#items">
<item xmlns="http://jabber.org/protocol/disco#items" jid="42_ibdev@conf.hipchat.com" name="Ibdev">
<x xmlns="http://hipchat.com/protocol/muc#room">
<id xmlns="http://hipchat.com/protocol/muc#room">
1875
</id>
<name xmlns="http://hipchat.com/protocol/muc#room">
Ibdev
</name>
<topic xmlns="http://hipchat.com/protocol/muc#room">
Welcome! Send this link to coworkers who need accounts: https://www.hipchat.com/invite/42/321a1be58d2d31826c29d62270a?utm_campaign=company_room_link
</topic>
<privacy xmlns="http://hipchat.com/protocol/muc#room">
public
</privacy>
<owner xmlns="http://hipchat.com/protocol/muc#room">
42_041@chat.hipchat.com
</owner>
<guest_url xmlns="http://hipchat.com/protocol/muc#room"/>
<version xmlns="http://hipchat.com/protocol/muc#room">
YUBD453G
</version>
<num_participants xmlns="http://hipchat.com/protocol/muc#room">
0
</num_participants>
</x>
</item>
</query>
</iq> 
The current code then does disco#info on the <item> and fails to parse the result properly. This looks so different that it can wait for a followup.

Slack works well up to the point where the room list is received, but in response to disco#info on individual rooms you get service-unavailable. The current code handles this error but in a way that means the rooms which we know to exist never show up in the newtab. The code should make use of the info we do have.

::: chat/protocols/xmpp/xmpp.jsm
@@ +1768,5 @@
> +
> +      // XEP-0045 (6.4): Querying for Room Information.
> +      let isLast = index == (items.length - 1);
> +      let iq = Stanza.iq("get", null, jid,
> +                         Stanza.node("query", Stanza.NS.disco_info));

This is a nice idea, but if the server has many rooms, we'll be flooding the server with requests and potentially get disconnected or throttled. So unfortunately it won't work.

So there are two options
- Decide you don't care about participant count (and sometimes name) and just use the room list data as before (parsing the jid for the room name when not available).
- Lazily request this info when getRoomInfo is called. But because getRoomInfo is called when a room needs to be displayed, it has to return data immediately and can't wait for a round trip to the server. So if you want to add this, you need to write some notification mechanism that sends some data immediately and follows up with more data if/when the results arrive from the server. This has to be done in a way that doesn't make scrolling in the newtab janky. Up to you if you think this effort is worth it just for the participant count.
- Maybe there is a third option I don't know about, some XEP similar to entity capabilities for presence...
Attachment #8766417 - Flags: review?(aleth) → review-
(Assignee)

Comment 13

2 years ago
(In reply to aleth [:aleth] from comment #12)
> So there are two options
> - Decide you don't care about participant count (and sometimes name) and
> just use the room list data as before (parsing the jid for the room name
> when not available).
> - Lazily request this info when getRoomInfo is called. But because
> getRoomInfo is called when a room needs to be displayed, it has to return
> data immediately and can't wait for a round trip to the server. So if you
> want to add this, you need to write some notification mechanism that sends
> some data immediately and follows up with more data if/when the results
> arrive from the server. This has to be done in a way that doesn't make
> scrolling in the newtab janky. Up to you if you think this effort is worth
> it just for the participant count.
> - Maybe there is a third option I don't know about, some XEP similar to
> entity capabilities for presence...

> This has to be done in a way that doesn't make scrolling in the newtab
> janky.

This needs to be tested well and I'm not sure about the behaviour, I think we can go with the first option and let the second one be an improvement.
(Assignee)

Updated

2 years ago
Flags: needinfo?(nhnt11)
(Assignee)

Comment 14

2 years ago
(In reply to aleth [:aleth] from comment #12)
> - Maybe there is a third option I don't know about, some XEP similar to
> entity capabilities for presence...

What about enqueuing the result of room list (jids) then request room info sequentially (when we get a response from room info, trigger the next request)?

Comment 15

2 years ago
(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #14)
> (In reply to aleth [:aleth] from comment #12)
> > - Maybe there is a third option I don't know about, some XEP similar to
> > entity capabilities for presence...
> 
> What about enqueuing the result of room list (jids) then request room info
> sequentially (when we get a response from room info, trigger the next
> request)?

To avoid blocking this bug, let's leave experimenting with detailed room infos for a followup.
(Assignee)

Comment 16

2 years ago
Created attachment 8767344 [details] [diff] [review]
v3 - Support Room Discovery

(In reply to aleth [:aleth] from comment #15)
> To avoid blocking this bug, let's leave experimenting with detailed room
> infos for a followup.

OK.
Attachment #8766417 - Attachment is obsolete: true
Attachment #8767344 - Flags: review?(aleth)

Comment 17

2 years ago
Comment on attachment 8766571 [details] [diff] [review]
v4 - protocol-independant changes

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

Typo in the commit message - please fix before checkin
Attachment #8766571 - Flags: review?(aleth) → review+

Comment 18

2 years ago
Comment on attachment 8767344 [details] [diff] [review]
v3 - Support Room Discovery

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +1082,5 @@
>    // Contains the domain of MUC service which is obtained using service
>    // discovery.
>    _mucService: null,
>  
> +   // Maps room names to {jid}.

room jid not {jid}

@@ +1773,5 @@
> +      let jid = item.attributes["jid"];
> +
> +      // We already have this muc.
> +      if (this._mucs.has(jid))
> +        return;

Not a good idea. The roomList should contain all mucs because you may leave the room before the next time the roomList is fetched from the server.

@@ +1777,5 @@
> +        return;
> +
> +      let name = item.attributes["name"];
> +      if (!name)
> +        name = jid;

Don't use the full jid, just the first part that isn't the same for all rooms. That's more readable.
Attachment #8767344 - Flags: review?(aleth) → review-
(Assignee)

Comment 19

2 years ago
Created attachment 8767401 [details] [diff] [review]
v4 - Support Room Discovery
Attachment #8767344 - Attachment is obsolete: true
Attachment #8767401 - Flags: review?(aleth)

Comment 20

2 years ago
Comment on attachment 8767401 [details] [diff] [review]
v4 - Support Room Discovery

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +1767,5 @@
> +    else
> +      this._pendingList = false;
> +
> +    let items = query.getElements(["item"]);
> +    let test = this._parseJID("test@test2.com").node;

?

@@ +1769,5 @@
> +
> +    let items = query.getElements(["item"]);
> +    let test = this._parseJID("test@test2.com").node;
> +    let rooms = items.map(item => {
> +      let jid = this._parseJID(item.attributes["jid"]);

Now you're parsing it, you can null check the result just in case
Attachment #8767401 - Flags: review?(aleth) → review-
(Assignee)

Comment 21

2 years ago
Created attachment 8767519 [details] [diff] [review]
v5 - Support Room Discovery
Attachment #8767401 - Attachment is obsolete: true
Attachment #8767519 - Flags: review?(aleth)

Comment 22

2 years ago
Comment on attachment 8767519 [details] [diff] [review]
v5 - Support Room Discovery

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

Thanks!

::: chat/protocols/xmpp/xmpp.jsm
@@ +1768,5 @@
> +      this._pendingList = false;
> +
> +    let rooms = [];
> +    let items = query.getElements(["item"]);
> +    items.forEach(item => {

You could shorten this to query.getElements(["item"]).forEach if you like
Attachment #8767519 - Flags: review?(aleth) → review+
(Assignee)

Comment 23

2 years ago
(In reply to aleth [:aleth] from comment #17)
> Typo in the commit message - please fix before checkin

Fixed
https://hg.mozilla.org/comm-central/rev/6f19fc2df369

(In reply to aleth [:aleth] from comment #22)
> You could shorten this to query.getElements(["item"]).forEach if you like

Done before checkin
https://hg.mozilla.org/comm-central/rev/28caf547300d
(Assignee)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(nhnt11)
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 50

Updated

a year ago
Depends on: 1292884
You need to log in before you can comment on or make changes to this bug.