1284165 - support version command

Status: RESOLVED FIXED

(Chat Core :: XMPP, defect)

Description

[Abdelrahman Ahmed [:abdelrahman]]

Send query and handle response according to (XEP-0092: Software Version)

Comment 1

[Patrick Cloke [:clokep]]

To be clear, this isn't about a command that a user would actually run, right?

Comment 2

[aleth [:aleth]]

(In reply to Patrick Cloke [:clokep] from comment #1)
> To be clear, this isn't about a command that a user would actually run,
> right?

It's the equivalent of /version on IRC.

Comment 3

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v1 - support version command

Comment 4

[aleth [:aleth]]

Comment on attachment 8772565 [details] [diff] [review]
v1 - support version command

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

::: chat/locales/en-US/xmpp.properties
@@ +86,5 @@
>  #   %S is the jid that is invalid.
>  conversation.error.invalidJID=%S is an invalid jid (Jabber identifiers must be of the form user@domain).
>  conversation.error.commandFailedNotInRoom=You have to rejoin the room to be able to use this command.
> +#   %S is the name of the recipient.
> +conversation.error.resourceNotAvailable=You have talk first as %S could be connected with various different clients.

You must talk first as %S could be connected with more than one client.

@@ +269,5 @@
>  command.inviteto=%S <room jid>[<password>]: Invite your conversation partner to join a room, together with its password if required.
>  command.me=%S <action to perform>: Perform an action.
>  command.nick=%S <new nickname>: Change your nickname.
>  command.msg=%S <nick> <message>: Send a private message to a participant in the room.
> +command.version=%S: Request the version of your conversation partner.
\ No newline at end of file

Request information about the client your conversation partner is using.

::: chat/protocols/xmpp/xmpp.jsm
@@ +675,5 @@
>    },
>  
> +  // Query the user for its Software Version.
> +  getVersion: function() {
> +    // XEP-0092: Software Version.

At least add TODO to use service discovery first here.

@@ +691,5 @@
> +            break;
> +        }
> +        this.writeMessage(this.name, message, {system: true, error: true});
> +        return;
> +      }

This seems overly complicated. How about using handleError? If that doesn't support a "default" condition yet you can add that ;)

@@ +700,5 @@
> +        let version = query.getElement(["version"]);
> +        if (!name || !version) {
> +          // XEP-0092: name and version elements are REQUIRED.
> +          this.ERROR("Received a response of version query does not contain " +
> +                    "name or version.");

this.WARN - as this is not something that should only happen if there is an error in our code somewhere.

@@ +712,5 @@
> +          os = os.innerText;
> +          messageID += "WithOS";
> +        }
> +
> +        let params = [this.shortName, client, os].filter(s => s);

shortName and client are never empty.
So it seems what you really want is 
let params = [this.shortName, client];
if (os) {params.push(...) ...}

@@ +717,5 @@
> +        this.writeMessage(this.name, _(messageID, ...params), {system: true});
> +      }
> +      else
> +        this.ERROR("Received a response of version query does not contain " +
> +                  "query element or 'jabber:iq:version' namespace.");

Turn this around to avoid indentation, ie.
if (!query || ...)
  this.WARN("Received an incorrect version response.")

Comment 5

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v2 - support version command

Comment 6

[aleth [:aleth]]

Comment on attachment 8773276 [details] [diff] [review]
v2 - support version command

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +689,5 @@
> +          // user for its software version.
> +          default: _("conversation.error.version.unknown")
> +        }, this);
> +        return errorHandler(aStanza);
> +      }

The errorHandler will return false if it's not an error. As you have a default handler you can use this here:
if ((handleErrors(...))(aStanza)) return;

@@ +693,5 @@
> +      }
> +
> +      let query = aStanza.getElement(["query"]);
> +      if (!query || query.uri != Stanza.NS.version) {
> +        this.WARN("Received a response of version query does not contain " +

English nit: "Received a response to version query which does not..."

@@ +708,5 @@
> +        return;
> +      }
> +
> +      let messageID = "conversation.message.version";
> +      let client = name.innerText + " " + version.innerText;

Let's make name and version separate parameters, it doesn't add any complexity and might help localizers.

@@ +1581,5 @@
>          return uncapitalize(aStr.split("-").map(capitalize).join(""));
>        }
>        let condition = toCamelCase(error.condition);
>        // Check if we have a handler property for this kind of error.
>        if (!(condition in aHandlers))

&& !("default" in aHandlers)

Comment 7

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v3 - support version command

Comment 8

[aleth [:aleth]]

Comment on attachment 8773318 [details] [diff] [review]
v3 - support version command

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

Thanks! Please fix the issues before checkin.

::: chat/protocols/xmpp/xmpp.jsm
@@ +685,5 @@
> +    this._account.sendStanza(s, aStanza => {
> +      // TODO: handle other errors that can result from querying
> +      // user for its software version.
> +      if (this._account.handleErrors({
> +            default: _("conversation.error.version.unknown"

closing bracket?

@@ +692,5 @@
> +      }
> +
> +      let query = aStanza.getElement(["query"]);
> +      if (!query || query.uri != Stanza.NS.version) {
> +        this.WARN("Received a response to version query does not contain " +

nit: see previous review

@@ +701,5 @@
> +      let name = query.getElement(["name"]);
> +      let version = query.getElement(["version"]);
> +      if (!name || !version) {
> +        // XEP-0092: name and version elements are REQUIRED.
> +        this.WARN("Received a response to version query does not contain " +

same here

Comment 9

[Abdelrahman Ahmed [:abdelrahman]]

Issues in comment 8 are fixed.
https://hg.mozilla.org/comm-central/rev/a3d15a0e03cf