Closed Bug 277588 Opened 20 years ago Closed 20 years ago

the removeCommands method should accept the same array as used in defineCommands

Categories

(Other Applications :: ChatZilla, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: glenjamin+bmo, Assigned: rginda)

Details

(Whiteboard: [cz-0.9.68])

Attachments

(1 file, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a4) Gecko/20040927
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a4) Gecko/20040927

The command manager accepts an 3d array for defineCommands but requires an array
of objects all with {name: command} which is superflous.

Reproducible: Always

Steps to Reproduce:
Attached patch Fix for removeCommands (obsolete) — Splinter Review
Attachment #170697 - Flags: review?(silver)
Attachment #170697 - Flags: review?(silver) → review?(samuel)
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
Attachment #170697 - Attachment is obsolete: true
Attached patch corrected coding pedantics (obsolete) — Splinter Review
Attachment #170699 - Flags: review?(samuel)
Attachment #170697 - Flags: review?(samuel)
Attachment #170699 - Flags: review?(samuel) → review+
actually, if we're being pedantic...

+        var command = isinstance(cmdary[i], Array) ?
+                      {name: cmdary[i][0]} : cmdary[i];

should be...

+        var command = isinstance(cmdary[i], Array) ?
+            {name: cmdary[i][0]} : cmdary[i];

Because the previous line has no unclosed paren or brace, the continuing line
gets a single indent level (4 spaces.)  Not a big offense though, nice patch.

I assume you figured out (or someone told you) that removeCommands was
originally meant to take an array of command objects, which happens to be the
return value of defineCommands.
(In reply to comment #3)
> actually, if we're being pedantic...
> 
> +        var command = isinstance(cmdary[i], Array) ?
> +                      {name: cmdary[i][0]} : cmdary[i];
> 
> should be...
> 
> +        var command = isinstance(cmdary[i], Array) ?
> +            {name: cmdary[i][0]} : cmdary[i];
> 
> Because the previous line has no unclosed paren or brace, the continuing line
> gets a single indent level (4 spaces.)  Not a big offense though, nice patch.
> 
> I assume you figured out (or someone told you) that removeCommands was
> originally meant to take an array of command objects, which happens to be the
> return value of defineCommands.

Heh, couldnt seem to find any examples of ternary indentation to go by.

Never really noticed about the returned array from defineCommands, is there any
point in using this path now then?
It's actually the same rule that determines what to do when you want to break
right before the right-hand-side of an assignment, as in...

http://lxr.mozilla.org/mozilla/source/extensions/irc/xul/content/static.js

I don't see any harm in keeping the old path around, probably someone is already
using it in a plugin.
aha, here's the other example I was looking for...

http://lxr.mozilla.org/mozilla/source/extensions/irc/xul/content/static.js#2082
Comment on attachment 170699 [details] [diff] [review]
corrected coding pedantics

>Index: mozilla/extensions/irc/js/lib/command-manager.js
>-    for (var c in commands)
>-        this.removeCommand(commands[c]);
>+    for (var i = 0; i < cmdary.length; ++i)

This wont work when called using the previous style, which you very carefully
tried to cope with inside the loop.

http://lxr.mozilla.org/mozilla/source/extensions/irc/js/lib/command-manager.js#
179 sets a property for each command defined, using the command's name as the
property name. It is not an array - so it wont have a length or numeric
properties.

You should keep the |in| version of the loop, since arrays only have their
numeric indicies set as enumerable in normal circumstances, or somehow branch
the codepaths outside of the loop constructs.
Attached patch Fix v3 (obsolete) — Splinter Review
ok, finally got around to correcting that.
Attachment #170699 - Attachment is obsolete: true
Attachment #174847 - Flags: review?
Attached patch fix v4Splinter Review
I really should double check my patches before i upload, missed a minor
pedantics fix.
Attachment #174910 - Flags: review?
Attachment #174847 - Attachment is obsolete: true
Attachment #174847 - Flags: review?
Attachment #174910 - Flags: review? → review?(silver)
Comment on attachment 174910 [details] [diff] [review]
fix v4

r=silver@warwickcompsoc.co.uk
Attachment #174910 - Flags: review?(silver) → review+
Checked in --> FIXED.

In approximately 6 minutes a build with this fix will be available from:
  http://twpol.dyndns.org/mozilla/chatzilla/nightly/
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Whiteboard: [cz-0.9.68]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: