1010342 - Join chat dialog should prepopulate the server field for XMPP accounts

Status: RESOLVED FIXED

(Chat Core :: XMPP, defect, P3)

Description

[h0ckeysk8er]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140506152807

Steps to reproduce:

Clicking on the Join Chat button opens the "Join chat" dialog for the XMPP account.


Actual results:

The dialog opens with the account name and nickname fields prepopulated.  For my XMPP server, multi-user chat is located at rooms.server.domain.com instead of server.domain.com.  You must manually input the server name and if it differs from your account server, you get no warning or indication that it is incorrect when the dialog is submitted to join the chat.


Expected results:

The server field of the dialog should be prepopulated with the server name for the multi-user chat service similar to the behavior of other IM clients such as Pidgin.  TB should perform a service discovery query to find the MUC server per http://xmpp.org/extensions/xep-0045.html#disco-service

There should also be some error or warning when the join chat dialog is submitted and it is an invalid request to the server.

Additionally, it would be ideal if there would also be a separate button to click to list the available rooms/channels on that server per http://xmpp.org/extensions/xep-0045.html#disco-rooms with the ability to select a room/channel to join.

Comment 1

[Florian Quèze [:florian]]

Thanks for your report. Bugs / enhancement requests are usually more actionable if you request only one thing per bug. In your description I can see at least 3 things:
- do service discovery to know the name of the conference server of your XMPP server and have a sane default value
- display somewhere a list of the channels available.
- giving the user clear feedback when joining a MUC failed for some reason.

Comment 2

[h0ckeysk8er]

I can split it into 3 if you like.

Comment 3

[Patrick Cloke [:clokep]]

(In reply to h0ckeysk8er from comment #2)
> I can split it into 3 if you like.

Let's keep this one as the service discovery, please file the other two as separate issues. Thanks!

Comment 4

[Abdelrahman Ahmed [:abdelrahman]]

Confirming, this is a part of Service Discovery (XEP-0030) [1]

[1] http://xmpp.org/extensions/xep-0030.html#info-basic

Comment 5

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 1 - service discovery for a muc service

Comment 6

[aleth [:aleth]]

Comment on attachment 8625402 [details] [diff] [review]
rev 1 - service discovery for a muc service

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

Thanks for tackling this!

::: chat/protocols/xmpp/xmpp.jsm
@@ +1136,4 @@
>  
>    /* Called when the XMPP session is started */
>    onConnection: function() {
>      this.reportConnecting(_("connection.downloadingRoster"));

Add a comment above this:
// Request the roster. The account will be marked as connected when this is complete.

@@ +1138,5 @@
>    onConnection: function() {
>      this.reportConnecting(_("connection.downloadingRoster"));
>      let s = Stanza.iq("get", null, null, Stanza.node("query", Stanza.NS.roster));
>  
>      /* Set the call back onRoster */

Remove this comment and join the line below with the previous block.

@@ +1145,5 @@
> +    // XEP-0030 and XEP-0045 (6): Service Discovery.
> +    // Queries Server for Associated Services.
> +    let iq = Stanza.iq("get", null, this._jid.domain,
> +                       Stanza.node("query", Stanza.NS.disco_items));
> +    this.sendStanza(iq, aStanza => {

This callback is quite long and will probably get longer as more features are added later, so please move it to its own method (e.g. onServiceDiscovery).

Comment 7

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 2 - service discovery for a muc service

Comment 8

[aleth [:aleth]]

Comment on attachment 8625502 [details] [diff] [review]
rev 2 - service discovery for a muc service

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +1135,5 @@
>    /* XMPPSession events */
>  
> +  // Called when the XMPP session is started.
> +  // Request the roster. The account will be marked as connected when this is
> +  // complete.

Move the "request the roster" comment inside (just above this.reportConnecting) as it describes what the first block of code does.

@@ +1148,5 @@
> +                       Stanza.node("query", Stanza.NS.disco_items));
> +    this.sendStanza(iq, aStanza => {
> +      this.onServiceDiscovery(aStanza);
> +      return true;
> +    });

this.sendStanza(iq, this.onServiceDiscovery, this);

@@ +1211,5 @@
>        this.WARN("Unhandled presence stanza.");
>    },
>  
> +  // XEP-0030: Discovering services and their features that are supported by
> +  // the server. 

Trailing whitespace.

Comment 9

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 3 - service discovery for a muc service

Comment 10

[aleth [:aleth]]

Comment on attachment 8625529 [details] [diff] [review]
rev 3 - service discovery for a muc service

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

Can you change the join command accordingly?

Also, when I tested this patch with "Join chat" from the menu, it didn't work:
Timestamp: 23/06/2015 23:55:44
Error: uncaught exception: Some required fields are empty!
Source File: chrome://instantbird/content/menus.js
Line: 140

Comment 11

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #10)
> Can you change the join command accordingly?

There is a separate bug for that (bug 1172360).

Comment 12

[aleth [:aleth]]

Comment on attachment 8625529 [details] [diff] [review]
rev 3 - service discovery for a muc service

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

Thanks for adding this!

Comment 13

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/b68eb761cf82