Closed Bug 954126 Opened 10 years ago Closed 10 years ago

Implement /help command to get information on registered commands

Categories

(Instantbird Graveyard :: Conversation, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

References

Details

Attachments

(2 files, 3 obsolete files)

*** Original post on bio 691 at 2011-02-11 14:46:00 UTC ***

From discussion on IRC w/ flo, it'd be good to be able to list all available commands, etc. One way this could be done is with a /help command that does the following:

/help would provide the list of commands available to the current conversation (this protocol + global commands)
/help <command name> would provide the help of that command

This message would be displayed as a system message in the current conversation (and should not be logged).

This could easily be done in an extension, but depends on each command having a help string when registered.
*** Original post on bio 691 at 2011-02-11 14:53:56 UTC ***

As this is unlikely to get in the way, we could also include it by default it a clean patch is proposed :).
*** Original post on bio 691 at 2011-02-15 02:04:26 UTC ***

(In reply to comment #1)
> As this is unlikely to get in the way, we could also include it by default it a
> clean patch is proposed :).

I've worked on this a little bit, it's approx. 50 lines right now, but I'm sure it can be compressed. Working a bug or two out though.
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Attached file Command, not working (obsolete) —
*** Original post on bio 691 as attmnt 524 at 2011-02-17 22:57:00 UTC ***

I'm attaching the command as I have it so far, it doesn't work though since I can't add messages to conversations. I'm kind of just putting it here to save off my work.
Attached patch v1.0 (obsolete) — Splinter Review
*** Original post on bio 691 as attmnt 633 at 2011-05-20 22:59:00 UTC ***

Working version, this will log the help call -- we could change that by making another method, but I wanted to get a review first to see if this even seems like a reasonable patch.
Attachment #8352376 - Flags: review?(florian)
Comment on attachment 8352265 [details]
Command, not working

*** Original change on bio 691 attmnt 524 at 2011-05-20 22:59:58 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352265 - Attachment is obsolete: true
Comment on attachment 8352376 [details] [diff] [review]
v1.0

*** Original change on bio 691 attmnt 633 at 2011-05-20 23:31:50 UTC ***

>diff --git a/purple/purplexpcom/src/imCommands.js b/purple/purplexpcom/src/imCommands.js
>@@ -52,16 +53,71 @@ CommandsService.prototype = {
>+    this.registerCommand({
>+      name:"help",

If you add here something like:
        cmdSrv: this,
you should be able to access the unwrapped command service instance.

>+      priority: Ci.imICommand.PRIORITY_DEFAULT,
>+      run: function(aMsg, aConv) {
>+        let commands = Services.cmd.listCommands(aConv);

This will iterate over all the registered commands. You probably need this only in the case where no command name was specified.

>+        let conv = Services.conversations.getUIConversation(aConv);
>+
>+        if (!conv || !commands)

commands cannot be null here. If there's no result, you will have an empty array, which doesn't evaluate to false.

>+        if (!aMsg.length) {
An empty string will evaluate to false. You don't need the .length here.

>+          // No command given, list all possible commands available for this
>+          // conversation
>+          let message = "Registered commands: ";
Needs to be translated.


>+          let commandNames = commands.map(function(aCommand) {
>+            return aCommand.name;
>+          });
Shorter and more readable.
          let commandNames = commands.map(function(aCommand) aCommand.name);
If this turns out to be > 80 columns, you can use only "c" as the parameter name instead of the full "aCommand" for such a trivial function.


>+          if (commandNames.length > 1) {
>+            message += commandNames.slice(0, -1).join(", ") + ", and " +
>+                       commandNames.slice(-1);
>+          } else
>+            message += commandNames

You sentence isn't really a sentence here (doesn't even end with a point). Is it really worth all this complication to add the "and" after the last comma? Is this even localizable?
If you want to keep the "and", something like this may be more readable:

let message = commandNames.pop();
let others = commandNames.join(", ");
if (others)
  message = others + " and " + message; (this is a formatted localizable string)
and then you finish with a formatted localizable string using the value of message.

>+          conv.systemMessage("Use /help &lt;command&gt; for more information.");
Another translatable string.

>+        // A command name was given
>+        // Find the command name that matches
>+        let commandName = aMsg.toLowerCase();
>+
>+        for each (let command in commands) {
>+          if (commandName == command.name.toLowerCase()) {
>+            let text;
>+            if (command.helpString) {
>+              text = "Help topic for " + commandName + " :" +
>+                     command.helpString;
>+            } else
>+              text = "No help topic available for " + commandName + ".";
>+
>+            // Display the message
>+            conv.systemMessage(text);
>+            return true;
>+          }
>+        }

This code may not always display the help for the right command, as it's possible to have 2 commands registered with the same name, in which case the priority field should be taken into account.
Take inspiration in the code for executeCommand.
Attachment #8352376 - Flags: review?(florian) → review-
*** Original post on bio 691 at 2011-05-21 00:42:23 UTC ***

(In reply to comment #5)
> (From update of attachment 8352376 [details] [diff] [review] (bio-attmnt 633) [details])
> >diff --git a/purple/purplexpcom/src/imCommands.js b/purple/purplexpcom/src/imCommands.js
> >@@ -52,16 +53,71 @@ CommandsService.prototype = {
> >+    this.registerCommand({
> >+      name:"help",
> 
> If you add here something like:
>         cmdSrv: this,
> you should be able to access the unwrapped command service instance.
I don't think I need this though.

> 
> >+      priority: Ci.imICommand.PRIORITY_DEFAULT,
> >+      run: function(aMsg, aConv) {
> >+        let commands = Services.cmd.listCommands(aConv);
> 
> This will iterate over all the registered commands. You probably need this only
> in the case where no command name was specified.
Did you want me to use this with the above so I don't need to loop over all of them?


> >+          if (commandNames.length > 1) {
> >+            message += commandNames.slice(0, -1).join(", ") + ", and " +
> >+                       commandNames.slice(-1);
> >+          } else
> >+            message += commandNames
> 
> You sentence isn't really a sentence here (doesn't even end with a point). Is
> it really worth all this complication to add the "and" after the last comma? Is
> this even localizable?
> If you want to keep the "and", something like this may be more readable:
I simplified this done a lot to just join them with a comma (I also sorted them though).


The string translations I'm going to need a bit of help with, is this sort of thing usually done w/ some sort of wildcard to replace parts of the strings or as two separate strings (the beginning and end), etc? We can probably talk about this on IRC.


The other mostly trivial things I've taken care of or at least know how to. :)
*** Original post on bio 691 at 2011-05-21 09:26:19 UTC ***

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 8352376 [details] [diff] [review] (bio-attmnt 633) [details] [details])
> > >diff --git a/purple/purplexpcom/src/imCommands.js b/purple/purplexpcom/src/imCommands.js
> > >@@ -52,16 +53,71 @@ CommandsService.prototype = {
> > >+    this.registerCommand({
> > >+      name:"help",
> > 
> > If you add here something like:
> >         cmdSrv: this,
> > you should be able to access the unwrapped command service instance.
> I don't think I need this though.

I think you will need it to do what executeCommand does.

> > >+        let commands = Services.cmd.listCommands(aConv);
> > 
> > This will iterate over all the registered commands. You probably need this only
> > in the case where no command name was specified.
> Did you want me to use this with the above so I don't need to loop over all of
> them?

I'm not sure I understand the question, but to be clear: don't use this when trying to generate the help message for a single command.

> The string translations I'm going to need a bit of help with, is this sort of
> thing usually done w/ some sort of wildcard to replace parts of the strings or
> as two separate strings (the beginning and end), etc? We can probably talk
> about this on IRC.

Using a begin and end part as 2 different strings for the same sentence is usually confusing for localizers, and in some cases even impossible to translate correctly (if the same begin happens to be used with several different possible ends, and the ends imply a change in the wording of the begin for some languages).
Attached patch v2.0 (obsolete) — Splinter Review
*** Original post on bio 691 as attmnt 634 at 2011-05-21 16:07:00 UTC ***

Now with strings that can be translated, fixed the other review comments.
Attachment #8352377 - Flags: review?(florian)
Comment on attachment 8352376 [details] [diff] [review]
v1.0

*** Original change on bio 691 attmnt 633 at 2011-05-21 16:07:39 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352376 - Attachment is obsolete: true
Comment on attachment 8352377 [details] [diff] [review]
v2.0

*** Original change on bio 691 attmnt 634 at 2011-05-21 21:34:16 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352377 - Flags: review?(florian) → review-
*** Original post on bio 691 at 2011-05-21 21:34:16 UTC ***

Comment on attachment 8352377 [details] [diff] [review] (bio-attmnt 634)
v2.0

The approach seems right, and if you have tested it (I haven't yet) I believe this works as expected :). The following comments are mostly nits.

>diff --git a/purple/locales/en-US/commands.properties b/purple/locales/en-US/commands.properties
>new file mode 100644
>--- /dev/null
>+++ b/purple/locales/en-US/commands.properties
>@@ -0,0 +1,7 @@
>+# LOCALIZATION NOTE (statusChangedWithStatusText):
>+#  %S is the display name of the contact.

Fix that to hide the shameless copy paste of a file from which you kept nothing else? ;)

>+registeredCommands=Registered commands: %S.
>+noCommand=No command '%S' registered.

I believe the word 'registered' here is rather confusing for users.
"Commands: %S" and "No '%S' command." seem simpler to me.

>+helpCommandMessage=Use /help &lt;command&gt; for more information.

Shouldn't this be part of the "registeredCommands" string, separated with a \n ?

>+helpTopic=Help topic for %S:\r\n%S

How is the word "topic" meaningful here? Is that message even needed? Why the \r?
The string could probably just be "%S:\n%S" (I'm not even sure the line break is actually needed, maybe just a space would be as good). And such a string really needs a localization note :).

>+noHelpTopic=No help topic available for %S.

"No help message for the "%S" command, sorry."

>diff --git a/purple/locales/jar.mn b/purple/locales/jar.mn

> @AB_CD@.jar:
> % locale purple @AB_CD@ %locale/@AB_CD@/purple/
>+  locale/@AB_CD@/purple/commands.properties (%commands.properties)
> 	locale/@AB_CD@/purple/conversations.properties (%conversations.properties)

This is one of the very rare places in the code (the only other is Makefile rules) where tabs should be used instead of spaces :).

>diff --git a/purple/purplexpcom/public/imIConversationsService.idl b/purple/purplexpcom/public

>+  // Display a message in the conversation
I would prefer the word "write" here as the message will actually be logged with the current implementation. With "Display" in the comment, I would expect the message to not be logged.
And even though it's pretty obvious with the name of the method, the command should probably specify too that it's a system message, so that nobody can wonder whether the method is just a poorly named method for inserting a standard message.

So what about "// Write a system message into the conversation."?
>+  void systemMessage(in AUTF8String aMessage);
>+

>diff --git a/purple/purplexpcom/src/imCommands.js b/purple/purplexpcom/src/imCommands.js

>+    // Reference the command service so we can use the internal properties
>+    // directly
>+    let cmdSrv = this;

The way I proposed in comment 7 seemed cleaner to me.

>+    this.registerCommand({
>+      name:"help",
>+      priority: Ci.imICommand.PRIORITY_DEFAULT,
>+      run: function(aMsg, aConv) {
>+        let conv = Services.conversations.getUIConversation(aConv);
>+        if (!conv)
>+          return false;
>+
>+        // Handle when no command is given, list all possible commands that are
>+        // available for this conversation (alphabetically)

When a comment is a real sentence, finish it with a point. (This comment is applicable throughout the patch :))

>+        if (!aMsg) {
>+          let commands = Services.cmd.listCommands(aConv);

Services.cmd of come on! You just kept a reference to an unwrapped version of the service :).

>+          if (!commands.length)
>+            return false;
>+
>+          let message =  bundle.formatStringFromName(
One space would be enough between '=' and 'bundle'.

>+            "registeredCommands",
>+            [commands.map(function(aCmd) aCmd.name).sort().join(", ")],
>+            1);

And the indent is a bit strange. I think I would prefer something like:
          let cmds = commands.map(function(aCmd) aCmd.name).sort().join(", ");
          let message = bundle.formatStringFromName("registeredCommands",
                                                    [cmds], 1);

>+          // Display the message
>+          conv.systemMessage(message);
>+          conv.systemMessage(bundle.GetStringFromName("helpCommandMessage"));

I'm not sure this should be displayed as 2 separate system messages rather than just separated by a line break.

>+        // A command name was given, find the command name that matches

Wouldn't it be better to add a method (not exposed in the idl) in CommandsService doing just that, rather than duplicating the code? The method would take a name and a conversation as parameter, and return the sorted array.

>+        // Get the 2 possible commands (the global and the proto specific)
>+        let cmdArray = [];
>+        if (!cmdSrv._commands.hasOwnProperty(aMsg)) {
>+          // No command that matches.
>+          let message =  bundle.formatStringFromName("noCommand", [aMsg], 1);

You have 2 spaces again after '='.
Would it be nice to fall back to displaying the list of possible commands after this error message?


>+          let command = cmdArray.shift();
Why .shift and not just [0]? Do you care to modify the array?

>+
>+          if (command.helpString) {
>+            text = bundle.formatStringFromName(
>+              "helpTopic", [command.name, command.helpString], 2);

I prefer this indentation (which requires shortening the variable name to fit in 80 columns):
            text = bundle.formatStringFromName("helpTopic",
                                               [cmd.name, cmd.helpString], 2);


>+          } else
The |else| on a separate line.
Attached patch v2.1Splinter Review
*** Original post on bio 691 as attmnt 641 at 2011-05-23 02:00:00 UTC ***

Fixes the review comments.
Attachment #8352384 - Flags: review?(florian)
Comment on attachment 8352377 [details] [diff] [review]
v2.0

*** Original change on bio 691 attmnt 634 at 2011-05-23 02:00:20 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352377 - Attachment is obsolete: true
Comment on attachment 8352384 [details] [diff] [review]
v2.1

*** Original change on bio 691 attmnt 641 at 2011-05-23 13:42:40 UTC ***

r=me with additional changes discussed on IRC that I'm going to attach.
Attachment #8352384 - Flags: review?(florian) → review+
*** Original post on bio 691 as attmnt 642 at 2011-05-23 13:43:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Blocks: 954231
*** Original post on bio 691 at 2011-05-23 14:16:12 UTC ***

Pushed attachment 8352384 [details] [diff] [review] (bio-attmnt 641) and 642 together:
https://hg.instantbird.org/instantbird/rev/fc27e39cc4d0
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.3b1
Blocks: 954265
You need to log in before you can comment on or make changes to this bug.