Closed
Bug 277588
Opened 20 years ago
Closed 19 years ago
the removeCommands method should accept the same array as used in defineCommands
Categories
(Other Applications :: ChatZilla, defect)
Other Applications
ChatZilla
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: glenjamin+bmo, Assigned: rginda)
Details
(Whiteboard: [cz-0.9.68])
Attachments
(1 file, 3 obsolete files)
1.02 KB,
patch
|
bugzilla-mozilla-20000923
:
review+
|
Details | Diff | Splinter Review |
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•20 years ago
|
||
Reporter | ||
Updated•20 years ago
|
Attachment #170697 -
Flags: review?(silver)
Reporter | ||
Updated•20 years ago
|
Attachment #170697 -
Flags: review?(silver) → review?(samuel)
Updated•20 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
Reporter | ||
Updated•20 years ago
|
Attachment #170697 -
Attachment is obsolete: true
Reporter | ||
Comment 2•20 years ago
|
||
Attachment #170699 -
Flags: review?(samuel)
Updated•20 years ago
|
Attachment #170697 -
Flags: review?(samuel)
Updated•20 years ago
|
Attachment #170699 -
Flags: review?(samuel) → review+
Assignee | ||
Comment 3•20 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•20 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•20 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•20 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•20 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•19 years ago
|
||
ok, finally got around to correcting that.
Attachment #170699 -
Attachment is obsolete: true
Attachment #174847 -
Flags: review?
Reporter | ||
Comment 9•19 years ago
|
||
I really should double check my patches before i upload, missed a minor pedantics fix.
Reporter | ||
Updated•19 years ago
|
Attachment #174910 -
Flags: review?
Reporter | ||
Updated•19 years ago
|
Attachment #174847 -
Attachment is obsolete: true
Reporter | ||
Updated•19 years ago
|
Attachment #174847 -
Flags: review?
Reporter | ||
Updated•19 years ago
|
Attachment #174910 -
Flags: review? → review?(silver)
Comment 10•19 years ago
|
||
Comment on attachment 174910 [details] [diff] [review] fix v4 r=silver@warwickcompsoc.co.uk
Attachment #174910 -
Flags: review?(silver) → review+
Comment 11•19 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
Closed: 19 years ago
Resolution: --- → FIXED
Updated•18 years ago
|
Whiteboard: [cz-0.9.68]
You need to log in
before you can comment on or make changes to this bug.
Description
•