Fix #3412: Cryptic error message when addMenuItem() is passed bad args #3611
Conversation
| } else { | ||
| switch (err) { | ||
| case ERR_UNKNOWN: | ||
| console.error("addMenu(): Unknown Error when adding the menu " + id); |
There was a problem hiding this comment.
I found this error in appshell_extensions_mac.mm line 718, but I am not sure for what it is, so I could use some suggestions for what to replace it with. Or I could go back to the previous error message in the default branch of the switch.
There was a problem hiding this comment.
This seems to be a catchall value passed from brackets-shell, so this is ok.
This switch statement should also have a default: statement. Text sent to console is basically the same as ERR_UNKNOWN case, but add actual value to make it slightly different, which may help us isolate certain problems:
console.error("addMenu(): Unknown Error (" + err + ") when adding the menu " + id);
There was a problem hiding this comment.
Also need a case NO_ERROR: statement (that does nothing) so this doesn't fall into default: case.
There was a problem hiding this comment.
This one does have a NO_ERROR case that does what the else branch did previously.
ghost
assigned 
| var FIRST = "first"; | ||
| var LAST = "last"; | ||
| var FIRST_IN_SECTION = "firstInSection"; | ||
| var LAST_IN_SECTION = "lastInSection"; |
There was a problem hiding this comment.
Since you're cleaning up this code, could you make this a single var statement with comma-separated declarations?
Same for Error Codes below.
|
Done with initial review. |
|
@redmunds Fixed pushed :) |
| @@ -462,7 +471,8 @@ define(function (require, exports, module) { | |||
| * @return {MenuItem} the newly created MenuItem | |||
| */ | |||
| Menu.prototype.addMenuItem = function (command, keyBindings, position, relativeID) { | |||
| var id, | |||
| var menuID = this.id, | |||
There was a problem hiding this comment.
The use of this.id seems inconsistent in this function. It's stored in menuID which is only used in 1 place, and there are 3 other references to this.id. I think you can get rid of this var and just use this.id everywhere.
There was a problem hiding this comment.
Not really. I added it to be able to access the menu ID in the brckets.add.addMenuItem callback function, since using this there will not refer to the Menu instance. Since I was going to use it once I saved the id there directly instead of doing something like that = this or self = this and then using that.id in the callback. But if this is a better solution I'll change it.
There was a problem hiding this comment.
Ah, that makes sense. Would it make sense to change all other this.id references to menuID ?
There was a problem hiding this comment.
All the other reference don't need to be changed since those can use the this object to access the menu id, but they could be changed if required.
|
Done with review. One more minor comment. |
|
Merging. |
Fix #3412: Cryptic error message when addMenuItem() is passed bad args
Fix for the issue #3412 by graving the resulting error code from the callback and providing a better error message for each code. I added a similar approach to
addMenu()since it had a similar problem.I searched through the Brackets Shell code to find which where the possible error codes for each function to provide a specific error message for each, but let me know if I missed one, since I mainly went through the Mac implementation since in the Win one was harder to find the error codes, and the list isn't complete in the JavaScript file.