/ba doesn't work as a command, even though it is unambiguous for /back

RESOLVED FIXED in 1.6

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: florian, Assigned: aleth)

Tracking

trunk
x86
Other

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

5 years ago
*** Original post on bio 1923 at 2013-04-11 20:22:00 UTC ***

Steps to reproduce:
Type "/ba<enter>" in a conversation.

Expected result:
/ba should behave as an alias of /back

Actual result:
22:20:44 - /ba is not a supported command. Type /help to see the list of commands.
(Reporter)

Updated

5 years ago
Blocks: 954873
(Assignee)

Comment 1

5 years ago
Posted patch 955360.patch (obsolete) — Splinter Review
The issue is that the commandname search should ignore commands that won't apply in the given conversation. I had to remove the recursion in the existing code to avoid duplicated code.
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Attachment #8364300 - Flags: review?(clokep)
Comment on attachment 8364300 [details] [diff] [review]
955360.patch

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

I have a couple of questions, I'm going to mark this as r- for now.

::: chat/components/src/imCommands.js
@@ +192,4 @@
>      let cmdArray = [];
> +    for (let commandName of commandNames) {
> +      if (!commandName.startsWith(aName))
> +        continue;

This is probably more efficient, but I did wonder if it'd be clearer to run Object.keys(this._commands).filter(c => !c.startsWith(aName)); on line 187. I'd like a comment above this saying "Check if this command is a partial match."

@@ +193,5 @@
> +    for (let commandName of commandNames) {
> +      if (!commandName.startsWith(aName))
> +        continue;
> +      if (cmdArray.length)
> +        return [];

What is this checking?

@@ +204,5 @@
> +        cmdArray.push(commands[prplId]);
> +
> +      // Remove the commands that can't apply in this context.
> +      if (aConversation)
> +        cmdArray = cmdArray.filter(this._usageContextFilter(aConversation));

Can this be done outside of the loop?
Attachment #8364300 - Flags: review?(clokep) → review-
(Assignee)

Comment 3

5 years ago
Posted patch 955360.patch 2 (obsolete) — Splinter Review
>What is this checking?
See the preceding comment.

>Can this be done outside of the loop?
No. The whole point of the patch is that anything that potentially disqualifies a command has to be done before we decide we have too many command matches.
Attachment #8364300 - Attachment is obsolete: true
Attachment #8364327 - Flags: review?(clokep)
Comment on attachment 8364327 [details] [diff] [review]
955360.patch 2

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

I'm still a bit confused about the early return.

::: chat/components/src/imCommands.js
@@ +194,5 @@
> +    // return an empty array (don't assume a certain command).
> +    let cmdArray = [];
> +    for (let commandName of commandNames) {
> +      if (cmdArray.length)
> +        return [];

Doesn't this check need to be done after the usageContextFilter for the current command? E.g. there's no guarantee that the current commandName will actually add anything to cmdArray.
(Assignee)

Comment 5

5 years ago
Posted patch 955360.patch 3 (obsolete) — Splinter Review
Good catch!
Attachment #8364327 - Attachment is obsolete: true
Attachment #8364327 - Flags: review?(clokep)
Attachment #8364335 - Flags: review?(clokep)
(Reporter)

Comment 6

5 years ago
Comment on attachment 8364335 [details] [diff] [review]
955360.patch 3

How difficult would it be to convince you to write a test for this code?
Comment on attachment 8364335 [details] [diff] [review]
955360.patch 3

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

::: chat/components/src/imCommands.js
@@ +210,5 @@
> +      // If we have found a second matching command name, return the empty array.
> +      if (cmdArray.length && matches.length)
> +        return [];
> +
> +      cmdArray = cmdArray.concat(matches);

I think I'd prefer else if matches.length, cmdArray = matches; unless there isn't a good reason to do this? (I find the concat confusing since one or both arrays can be empty.) r+ with this change.
Attachment #8364335 - Flags: review?(clokep) → review-
(Assignee)

Comment 8

5 years ago
Posted patch 955360.patch 4 (obsolete) — Splinter Review
I'll look into tests.
Attachment #8364335 - Attachment is obsolete: true
Attachment #8364382 - Flags: review?(clokep)
Attachment #8364382 - Flags: review?(clokep) → review+
(Assignee)

Comment 9

5 years ago
Posted patch 955360.patch 5 (obsolete) — Splinter Review
Attachment #8364382 - Attachment is obsolete: true
Attachment #8365911 - Flags: review?(clokep)
(Assignee)

Comment 10

5 years ago
Posted patch 955360-test.patch (obsolete) — Splinter Review
Attachment #8365912 - Flags: review?(clokep)
Comment on attachment 8365912 [details] [diff] [review]
955360-test.patch

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

This looks great overall! I hope you're enjoying writing tests! I have a few comments though!

::: chat/components/src/test/test_commands.js
@@ +39,5 @@
> +};
> +var fakeChat2 = {
> +  account: fakeAccount2,
> +  isChat: true
> +};

These can all be const if they don't change.

@@ +41,5 @@
> +  account: fakeAccount2,
> +  isChat: true
> +};
> +
> +function run_test() {

Would it make sense to separate this into different tests so it's easier to tell when one fails? (It doesn't look like it is, but I want to ensure you thought about it at least. :)) http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/test/test_ircMessage.js#119 is an example of how to do this.

@@ +104,5 @@
> +  // particular, the name and isPrpl value of the first (i.e. preferred)
> +  // entry in the returned array of commands is given. (If the isPrpl
> +  // value is not given, false is assumed, if the name is not given, that
> +  // corresponds to no commmands being returned.)
> +  let testData = [

I looked over this, but not super intensely and it looks OK. I think it'd be clearer if the conversations were defined in line below instead of above (unless you can't do that for some reason that I missed).

@@ +139,5 @@
> +  ];
> +
> +  let getCmdlist = function(aConv) {
> +    let commands = cmdserv.listCommandsForConversation(aConv, {});
> +    return commands.map(aCmd => aCmd.name).sort().join(", ");

Wouldn't it make more sense to keep this as an array?

@@ +144,5 @@
> +  };
> +
> +  for (let test of testData) {
> +    // Check which commands are available in which context.
> +    do_print(test.cmdList);

I'm unsure whether these should be checked in.

@@ +149,5 @@
> +    do_check_eq(test.cmdlist, getCmdlist(test.conv));
> +
> +    // Check which command is found, if any.
> +    for (let testCmd of testCmds) {
> +      let result = test.results.shift();

This is the EXPECTED result, correct? Can you add a comment saying this? I usually try to use variable names that make this clear by prefixing one with "expected".

@@ +152,5 @@
> +    for (let testCmd of testCmds) {
> +      let result = test.results.shift();
> +      let cmdArray = cmdserv._findCommands(test.conv, testCmd);
> +      do_print(testCmd);
> +      do_check_eq(!!cmdArray.length, !!result.length);

What is this testing? This needs a comment.

@@ +155,5 @@
> +      do_print(testCmd);
> +      do_check_eq(!!cmdArray.length, !!result.length);
> +      if (cmdArray.length) {
> +        do_check_eq(cmdArray[0].name, result[0]);
> +        do_check_eq(!!cmdArray[0].isPrpl, !!result[1]);

I find the !!s here pretty confusing. It shouldn't be necessary on isPrpl, I'd think.
Attachment #8365912 - Flags: review?(clokep)
Attachment #8365912 - Flags: review-
Attachment #8365912 - Flags: feedback?(florian)
Comment on attachment 8365911 [details] [diff] [review]
955360.patch 5

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

This looks fine. I'd like Florian to look at the usageContextFilter change to see if he notices anything "wrong" about it.

::: chat/components/src/imCommands.js
@@ +166,5 @@
>      return result;
>    },
>    _usageContextFilter: function(aConversation) {
> +    if (!aConversation)
> +      return function(c) true;

I know you explained this to me on IRC, but can you please explain this change in the bug for the record? Thanks!
Attachment #8365911 - Flags: review?(clokep)
Attachment #8365911 - Flags: review+
Attachment #8365911 - Flags: feedback?(florian)
(Reporter)

Comment 13

5 years ago
Comment on attachment 8365911 [details] [diff] [review]
955360.patch 5

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

::: chat/components/src/imCommands.js
@@ +166,5 @@
>      return result;
>    },
>    _usageContextFilter: function(aConversation) {
> +    if (!aConversation)
> +      return function(c) true;

I don't understand why you are doing this here.

@@ +205,5 @@
> +      if (prplId && commands.hasOwnProperty(prplId))
> +        matches.push(commands[prplId]);
> +
> +      // Remove the commands that can't apply in this context.
> +      matches = matches.filter(this._usageContextFilter(aConversation));

Wouldn't it make more sense to have an |if (aConversation)| line before this? (And yes, it would be duplicated in listCommandsForConversation, but that's just one line, and avoids the .filter dummy calls).
Attachment #8365911 - Flags: feedback?(florian) → review-
(Reporter)

Comment 14

5 years ago
Comment on attachment 8365912 [details] [diff] [review]
955360-test.patch

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

::: chat/components/src/test/test_commands.js
@@ +5,5 @@
> +const {interfaces: Ci, utils: Cu} = Components;
> +
> +Cu.import("resource:///modules/imServices.jsm");
> +let imCommands = {};
> +Services.scriptloader.loadSubScript("resource:///components/imCommands.js", imCommands);

The reason for doing this instead of using Services.cmd is only so that you can access _findCommands, right? Maybe there should be a comment explaining it?

@@ +11,5 @@
> +const kPrplId = "green";
> +const kPrplId2 = "red";
> +
> +var fakeAccount = {
> +    connected: true,

Why the 4 chars indent here?

@@ +39,5 @@
> +};
> +var fakeChat2 = {
> +  account: fakeAccount2,
> +  isChat: true
> +};

Do we usually use const for objects that aren't a single value? (I see no problem with using 'var' here.)

@@ +47,5 @@
> +  cmdserv.initCommands();
> +
> +  // Some commands providing multiple possible completions.
> +  cmdserv.registerCommand({
> +    isPrpl: true,

All commands here seem to have isPrpl: true :-(.

@@ +54,5 @@
> +    usageContext: Ci.imICommand.CMD_CONTEXT_ALL,
> +    priority: Ci.imICommand.CMD_PRIORITY_PRPL,
> +    run: (aMsg, aConv) => true
> +  }, kPrplId2);
> +  cmdserv.registerCommand({

There's a lot of duplication here.

What about a helper object, and then just doing registerCommand(new FakeCommand("baloney")); ? (FakeCommand could have an optional second parameter taking an object with the non default values of the command)

@@ +104,5 @@
> +  // particular, the name and isPrpl value of the first (i.e. preferred)
> +  // entry in the returned array of commands is given. (If the isPrpl
> +  // value is not given, false is assumed, if the name is not given, that
> +  // corresponds to no commmands being returned.)
> +  let testData = [

I also think inlining conversations would simplify things.

@@ +144,5 @@
> +  };
> +
> +  for (let test of testData) {
> +    // Check which commands are available in which context.
> +    do_print(test.cmdList);

If you want to print info, make it actually useful by adding a "desc"[ription] field to each test data entry, with a human readable description of what it is testing.

@@ +145,5 @@
> +
> +  for (let test of testData) {
> +    // Check which commands are available in which context.
> +    do_print(test.cmdList);
> +    do_check_eq(test.cmdlist, getCmdlist(test.conv));

Can't the getCmdlist function easily be inlined?

@@ +152,5 @@
> +    for (let testCmd of testCmds) {
> +      let result = test.results.shift();
> +      let cmdArray = cmdserv._findCommands(test.conv, testCmd);
> +      do_print(testCmd);
> +      do_check_eq(!!cmdArray.length, !!result.length);

length > 0 may be less confusing than !! here.
Attachment #8365912 - Flags: feedback?(florian) → feedback+
(Assignee)

Comment 15

5 years ago
Trades useless filter calls for duplication as requested ;)
Attachment #8365911 - Attachment is obsolete: true
Attachment #8367237 - Flags: review?(clokep)
(Assignee)

Comment 16

5 years ago
Thanks for all the good ideas in the commments! I'd been paying more attention to the tests themselves than to coding style (and wasn't sure about the style for tests anyway...)

(In reply to Patrick Cloke [:clokep] from comment #11)
> > +function run_test() {
> 
> Would it make sense to separate this into different tests so it's easier to
> tell when one fails? (It doesn't look like it is, but I want to ensure you
> thought about it at least. :))
> http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/test/
> test_ircMessage.js#119 is an example of how to do this.

Thanks, yes, I saw that but I don't think it simplifies things here. Adding do_prints seems a better way to make the test output legible.

> > +  let getCmdlist = function(aConv) {
> > +    let commands = cmdserv.listCommandsForConversation(aConv, {});
> > +    return commands.map(aCmd => aCmd.name).sort().join(", ");
> 
> Wouldn't it make more sense to keep this as an array?

It would make everything harder to read.
Attachment #8365912 - Attachment is obsolete: true
Attachment #8367239 - Flags: review?(clokep)
Attachment #8367237 - Flags: review?(clokep) → review+
Attachment #8367239 - Flags: review?(clokep) → review+
(Assignee)

Updated

5 years ago
Whiteboard: checkin-needed
https://hg.mozilla.org/comm-central/rev/acb28933aaf4
https://hg.mozilla.org/comm-central/rev/41da0af4c3a9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: checkin-needed
Target Milestone: --- → 1.6
Duplicate of this bug: 955733
You need to log in before you can comment on or make changes to this bug.