1048678 - Implement CTCP Client Info command

Status: RESOLVED FIXED

(Chat Core :: IRC, defect)

Description

[Patrick Cloke [:clokep]]

We should implement [1] CLIENTINFO [2] so that we can advertise / query other clients for information. Note that CLIENTINFO has some weird thing where it can take a parameter and is supposed to return a plaintext description. For now I'd be OK with either ignoring parameters or sending back an error in that situation. (Check what other clients actually do.)

Additionally...this needs to be dynamic. You should be able to look at the contents of _ctcpHandlers [3] and create a Set() of the keys of each handler's commands [4]. This would get a unique list of known CTCP handlers. (Although a lot of our current handlers just "return false" which means we don't actually handle anything in that case...)

We'll probably need some way to query and store this of other clients in the future too, but I wouldn't worry about that yet.

[1] https://mxr.mozilla.org/comm-central/source/chat/protocols/irc/ircCTCP.jsm#142
[2] From http://www.irchelp.org/irchelp/rfc/ctcpspec.html:
> KNOWN REQUEST/REPLY PAIRS:
> 
> CLIENTINFO  - Dynamic master index of what a client knows.

And details from father on:
> CLIENTINFO
> ==========
> This is for client developers use to make it easier to show other
> client hackers what a certain client knows when it comes to CTCP. The
> replies should be fairly verbose explaining what CTCP commands are
> understood, what arguments are expected of what type, and what replies
> might be expected from the client.
> 
> The query is the word CLIENTINFO in a "privmsg" optionally followed by
> a colon and one or more specifying words delimited by spaces, where
> the word CLIENTINFO by itself,
> 
>     \001CLIENTINFO\001
> 
> should be replied to by giving a list of known tags (see above in
> section TAGGED DATA). This is only intended to be read by humans.
> 
> With one argument, the reply should be a description of how to use
> that tag. With two arguments, a description of how to use that
> tag's subcommand. And so on.

And some examples:
> Sending
> 
>     PRIVMSG victim :\001CLIENTINFO\001
> 
> might return
> 
>     :victim NOTICE actor :\001CLIENTINFO :You can request help of the
>     commands CLIENTINFO ERRMSG FINGER USERINFO VERSION by giving
>     an argument to CLIENTINFO.\001
> 
> Sending
> 
>     PRIVMSG victim :\001CLIENTINFO CLIENTINFO\001
> 
> might return
> 
>     :victim NOTICE actor :\001CLIENTINFO :CLIENTINFO with 0
>     arguments gives a list of known client query keywords. With 1
>     argument, a description of the client query keyword is
>     returned.\001
> 
> while sending
> 
>     PRIVMSG victim :\001clientinfo clientinfo\001
> 
> probably will return something like
> 
>     :victim NOTICE actor :\001ERRMSG clientinfo clientinfo :Query is
>     unknown\001
> 
> as tag "clientinfo" isn't known.
> 
> Sending
> 
>     PRIVMSG victim :\001CLIENTINFO ERRMSG\001
> 
> might return
> 
>     :victim NOTICE actor :\001CLIENTINFO :ERRMSG is the given answer
>     on seeing an unknown keyword. When seeing the keyword ERRMSG,
>     it works like an echo.\001

[3] https://mxr.mozilla.org/comm-central/source/chat/protocols/irc/ircHandlers.jsm#32
[4] https://mxr.mozilla.org/comm-central/source/chat/protocols/irc/ircCTCP.jsm#120

Comment 1

[Patrick Cloke [:clokep]]

Attachment: Patch v1

Comment 2

[aleth [:aleth]]

Comment on attachment 8474104 [details] [diff] [review]
Patch v1

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

Thanks for getting started on this!

::: chat/protocols/irc/ircCTCP.jsm
@@ +149,5 @@
> +          for (let command in handler.commands)
> +            info.add(command);
> +        }
> +
> +        let supportedCtcp = [i for (i of info)].join(" ");

[...info].join(" "); is shorter

@@ +150,5 @@
> +            info.add(command);
> +        }
> +
> +        let supportedCtcp = [i for (i of info)].join(" ");
> +        this.LOG("Reporting support CTCP messages: " + supportedCtcp);

I wouldn't understand this string. How about "Reporting support for the following CTCP messages:"

@@ +154,5 @@
> +        this.LOG("Reporting support CTCP messages: " + supportedCtcp);
> +        this.sendCTCPMessage(aMessage.nickname, true, "CLIENTINFO",
> +                             supportedCtcp);
> +      }
> +      else {

Shouldn't this check aMessage.command?

@@ +157,5 @@
> +      }
> +      else {
> +        // Store the data for future use.
> +        let info = aMessage.ctcp.param.split(" ");
> +        this.setWhois(aMessage.nickname, {clientInfo: info})

We'll have to consider when to request this info, as existing whois info is deleted on every refresh.

@@ +178,5 @@
>          return this.handlePingReply(aMessage.nickname, aMessage.ctcp.param);
>      },
>  
>      // An encryption protocol between clients without any known reference.
> +    //"SED": function(aMessage) false,

Needs a comment either here or somewhere in the file to explain why returning false isn't enough here.

@@ +183,3 @@
>  
>      // Where to obtain a copy of a client.
> +    //"SOURCE": function(aMessage) false,

ditto

@@ +207,5 @@
>        return true;
>      },
>  
>      // A string set by the user (never the client coder)
> +    //"USERINFO": function(aMessage) false,

And here

Comment 3

[Patrick Cloke [:clokep]]

(In reply to aleth [:aleth] from comment #2)
> @@ +150,5 @@
> > +            info.add(command);
> > +        }
> > +
> > +        let supportedCtcp = [i for (i of info)].join(" ");
> > +        this.LOG("Reporting support CTCP messages: " + supportedCtcp);
> 
> I wouldn't understand this string. How about "Reporting support for the
> following CTCP messages:"

I don't understand it either! Wonder how I came up with that...

> @@ +154,5 @@
> > +        this.LOG("Reporting support CTCP messages: " + supportedCtcp);
> > +        this.sendCTCPMessage(aMessage.nickname, true, "CLIENTINFO",
> > +                             supportedCtcp);
> > +      }
> > +      else {
> 
> Shouldn't this check aMessage.command?

To do what? command matches the function name, why would we check it again?

> @@ +157,5 @@
> > +      }
> > +      else {
> > +        // Store the data for future use.
> > +        let info = aMessage.ctcp.param.split(" ");
> > +        this.setWhois(aMessage.nickname, {clientInfo: info})
> 
> We'll have to consider when to request this info, as existing whois info is
> deleted on every refresh.

I agree, I'm not sure this is the best place to put it (but it was the easiest...) Frankly, I wanted to get the responding part in ASAP and didn't care about the requesting part as much.

> @@ +178,5 @@
> >          return this.handlePingReply(aMessage.nickname, aMessage.ctcp.param);
> >      },
> >  
> >      // An encryption protocol between clients without any known reference.
> > +    //"SED": function(aMessage) false,
> 
> Needs a comment either here or somewhere in the file to explain why
> returning false isn't enough here.

I also did think of adding a manual array people had to fill in, which do you prefer?

Comment 4

[aleth [:aleth]]

(In reply to Patrick Cloke [:clokep] from comment #3)
> > Shouldn't this check aMessage.command?
> 
> To do what? command matches the function name, why would we check it again?

It's confusing because you check it in the if clause, "if (aMessage.command == "PRIVMSG")". It certainly needs a comment then.

> > We'll have to consider when to request this info, as existing whois info is
> > deleted on every refresh.
> 
> I agree, I'm not sure this is the best place to put it (but it was the
> easiest...) Frankly, I wanted to get the responding part in ASAP and didn't
> care about the requesting part as much.

I'm OK with this, just wanted to flag it up to consider for the next patch ;)

> > Needs a comment either here or somewhere in the file to explain why
> > returning false isn't enough here.
> 
> I also did think of adding a manual array people had to fill in, which do
> you prefer?

I like what you did, it just needs a comment as it differs from our usual IRC practice.

Comment 5

[Patrick Cloke [:clokep]]

(In reply to aleth [:aleth] from comment #4)
> (In reply to Patrick Cloke [:clokep] from comment #3)
> > > Shouldn't this check aMessage.command?
> > 
> > To do what? command matches the function name, why would we check it again?
> 
> It's confusing because you check it in the if clause, "if (aMessage.command
> == "PRIVMSG")". It certainly needs a comment then.

Sorry, I read that as the CTCP command, not the IRC command. There's no need to check the IRC command again. If it is not PRIVMSG, it must be NOTICE. http://mxr.mozilla.org/comm-central/source/chat/protocols/irc/ircCTCP.jsm#49

Comment 6

[Patrick Cloke [:clokep]]

Attachment: Patch v2

Comment 7

[Patrick Cloke [:clokep]]

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