Closed Bug 1010342 Opened 6 years ago Closed 5 years ago

Join chat dialog should prepopulate the server field for XMPP accounts

Categories

(Chat Core :: XMPP, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: h0ckeysk8er, Assigned: abdelrahman)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Priority: -- → P3
Blocks: 954959
Component: Instant Messaging → XMPP
OS: Windows 7 → All
Product: Thunderbird → Chat Core
Hardware: x86_64 → All
Version: 24 → trunk
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.
I can split it into 3 if you like.
(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!
Blocks: 1172360
Blocks: 1142142
Confirming, this is a part of Service Discovery (XEP-0030) [1]

[1] http://xmpp.org/extensions/xep-0030.html#info-basic
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → a.ahmed1026
Status: NEW → ASSIGNED
Attachment #8625402 - Flags: review?(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).
Attachment #8625402 - Flags: review?(aleth) → review-
Attachment #8625402 - Attachment is obsolete: true
Attachment #8625502 - Flags: review?(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.
Attachment #8625502 - Flags: review?(aleth) → review-
Attachment #8625502 - Attachment is obsolete: true
Attachment #8625529 - Flags: review?(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
Attachment #8625529 - Flags: review?(aleth) → review-
(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 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!
Attachment #8625529 - Flags: review- → review+
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6
You need to log in before you can comment on or make changes to this bug.