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

RESOLVED FIXED

Status

Other Applications
ChatZilla
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: Glen Mailer, Assigned: Robert Ginda)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [cz-0.9.68])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

13 years ago
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:
(Reporter)

Comment 1

13 years ago
Created attachment 170697 [details] [diff] [review]
Fix for removeCommands
(Reporter)

Updated

13 years ago
Attachment #170697 - Flags: review?(silver)
(Reporter)

Updated

13 years ago
Attachment #170697 - Flags: review?(silver) → review?(samuel)

Updated

13 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
(Reporter)

Updated

13 years ago
Attachment #170697 - Attachment is obsolete: true
(Reporter)

Comment 2

13 years ago
Created attachment 170699 [details] [diff] [review]
corrected coding pedantics
Attachment #170699 - Flags: review?(samuel)

Updated

13 years ago
Attachment #170697 - Flags: review?(samuel)

Updated

13 years ago
Attachment #170699 - Flags: review?(samuel) → review+
(Assignee)

Comment 3

13 years ago
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.
(Reporter)

Comment 4

13 years ago
(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?
(Assignee)

Comment 5

13 years ago
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.
(Assignee)

Comment 6

13 years ago
aha, here's the other example I was looking for...

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

Comment 7

13 years ago
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.
(Reporter)

Comment 8

13 years ago
Created attachment 174847 [details] [diff] [review]
Fix v3

ok, finally got around to correcting that.
Attachment #170699 - Attachment is obsolete: true
Attachment #174847 - Flags: review?
(Reporter)

Comment 9

13 years ago
Created attachment 174910 [details] [diff] [review]
fix v4

I really should double check my patches before i upload, missed a minor
pedantics fix.
(Reporter)

Updated

13 years ago
Attachment #174910 - Flags: review?
(Reporter)

Updated

13 years ago
Attachment #174847 - Attachment is obsolete: true
(Reporter)

Updated

13 years ago
Attachment #174847 - Flags: review?
(Reporter)

Updated

13 years ago
Attachment #174910 - Flags: review? → review?(silver)

Comment 10

13 years ago
Comment on attachment 174910 [details] [diff] [review]
fix v4

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

Comment 11

13 years ago
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
Last Resolved: 13 years ago
Resolution: --- → FIXED

Updated

12 years ago
Whiteboard: [cz-0.9.68]
You need to log in before you can comment on or make changes to this bug.