Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Fix #3412: Cryptic error message when addMenuItem() is passed bad args #3611

Merged
merged 2 commits into from
May 4, 2013
Merged

Fix #3412: Cryptic error message when addMenuItem() is passed bad args #3611

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4895a76
Better error messages for AddMenu and AddMenuItem
TomMalbran Apr 25, 2013
d3abaa4
Added NO_ERROR and default cases. Merged some vars.
TomMalbran May 3, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 46 additions & 22 deletions src/command/Menus.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,23 +91,32 @@ define(function (require, exports, module) {


/**
* Insertion position constants
* Used by addMenu(), addMenuItem(), and addSubMenu() to
* specify the relative position of a newly created menu object
* @enum {string}
*/
var BEFORE = "before";
var AFTER = "after";
var FIRST = "first";
var LAST = "last";
var FIRST_IN_SECTION = "firstInSection";
var LAST_IN_SECTION = "lastInSection";

/**
* Other constants
*/
var DIVIDER = "---";
* Insertion position constants
* Used by addMenu(), addMenuItem(), and addSubMenu() to
* specify the relative position of a newly created menu object
* @enum {string}
*/
var BEFORE = "before";
var AFTER = "after";
var FIRST = "first";
var LAST = "last";
var FIRST_IN_SECTION = "firstInSection";
var LAST_IN_SECTION = "lastInSection";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're cleaning up this code, could you make this a single var statement with comma-separated declarations?

Same for Error Codes below.


/**
* Other constants
*/
var DIVIDER = "---";

/**
* Error Codes from Brackets Shell
* @enum {number}
*/
var NO_ERROR = 0;
var ERR_UNKNOWN = 1;
var ERR_INVALID_PARAMS = 2;
var ERR_NOT_FOUND = 3;

/**
* Maps menuID's to Menu objects
* @type {Object.<string, Menu>}
Expand Down Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that makes sense. Would it make sense to change all other this.id references to menuID ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

id,
$menuItem,
$link,
menuItem,
Expand Down Expand Up @@ -547,8 +557,13 @@ define(function (require, exports, module) {
}

brackets.app.addMenuItem(this.id, name, commandID, bindingStr, displayStr, position, relativeID, function (err) {
if (err) {
console.error("addMenuItem() -- error: " + err + " when adding command: " + commandID);
switch (err) {
case ERR_INVALID_PARAMS:
console.error("addMenuItem(): Invalid Parameters when adding the command " + commandID);
break;
case ERR_NOT_FOUND:
console.error("_getRelativeMenuItem(): MenuItem with Command id " + relativeID + " not found in the Menu " + menuID);
break;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switch staement should have a default: statement with a generic error message something like the original:

    console.error("addMenuItem() -- unknown error: " + err + " when adding command: " + commandID);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also need a case NO_ERROR: statement (that does nothing) so this doesn't fall into default: case.

});
menuItem.isNative = true;
Expand Down Expand Up @@ -800,15 +815,24 @@ define(function (require, exports, module) {

if (!_isHTMLMenu(id)) {
brackets.app.addMenu(name, id, position, relativeID, function (err) {
if (err) {
console.error("addMenu() -- error: " + err + " when adding menu with ID: " + id);
} else {
switch (err) {
case ERR_UNKNOWN:
console.error("addMenu(): Unknown Error when adding the menu " + id);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also need a case NO_ERROR: statement (that does nothing) so this doesn't fall into default: case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one does have a NO_ERROR case that does what the else branch did previously.

break;
case ERR_INVALID_PARAMS:
console.error("addMenu(): Invalid Parameters when adding the menu " + id);
break;
case ERR_NOT_FOUND:
console.error("addMenu(): Menu with command " + relativeID + " could not be found when adding the menu " + id);
break;
case NO_ERROR:
// Make sure name is up to date
brackets.app.setMenuTitle(id, name, function (err) {
if (err) {
console.error("setMenuTitle() -- error: " + err);
}
});
break;
}
});
return menu;
Expand Down