[OPEN] Fix for Issue 4213: Remove strings for commands that are not user facing #4306
Conversation
| @@ -1110,7 +1110,7 @@ define(function (require, exports, module) { | |||
|
|
|||
| CommandManager.register(Strings.CMD_FILE_CLOSE, Commands.FILE_CLOSE, handleFileClose); | |||
| CommandManager.register(Strings.CMD_FILE_CLOSE_ALL, Commands.FILE_CLOSE_ALL, handleFileCloseAll); | |||
| CommandManager.register(Strings.CMD_CLOSE_WINDOW, Commands.FILE_CLOSE_WINDOW, handleFileCloseWindow); | |||
| CommandManager.register(null, Commands.FILE_CLOSE_WINDOW, handleFileCloseWindow); | |||
There was a problem hiding this comment.
Re-align arguments with above lines. Same with the next change below.
|
Thanks @WebsiteDeveloper. Changes look good except for the whitespace changes. Initial review complete. |
ghost
assigned 
|
I don't think we should make it quite so easy to omit the name of a command. E.g. extensions that add a command with a keyboard shortcut but no menu representation should be strongly encouraged (if not forced) to provide a name, since we'll need it for any future shortcut preferences UI (not to mention current uses such as Brackets Commands Guide). There aren't that many cases of purely internal-only commands. Perhaps we should have a different API for them (or a flag argument?) since it's also very useful for future UI to know explicitly which commands can be safely assigned to custom shortcuts, menu items, etc. and which should never be invoked directly by the user. |
|
@peterflynn we could set the name parameter explicitly to boolean "false" and check for that. |
|
@jasonsanjose @peterflynn i pushed the whitespace fixes. |
|
@jasonsanjose @peterflynn by the way, the travis error seems due to a failing unit test, i will fix that once we decide, which api to use. |
|
@peterflynn It would be easy to detect the case where a menu item does not have a name in Menus.addMenuItem(). I suggest that the command id string could be used for a name -- this would prevent any empty names, be descriptive enough to know what it's for, and ugly enough to clearly indicate that something's missing. How does that sound? |
|
@redmunds I think I got a little lost in this thread as to what the current proposal is :-) It seems like there are a couple valid use cases for internal-only commands. Do commands neatly separate into internal-only (don't need a name, should never be assignable to a keyboard shortcut, should never be listed in my Command Search extension. etc.) and "user-accessible" (name is required, always assignable to a shortcut, listed in Command Search, etc.)? If so I'd argue we should have two different APIs to register them -- seems safer than overloading one API with optional arguments. Also, since there are some valid use cases for these internal-only commands (as illustrated in this PR), we wouldn't want the official way to register them to always spam us with console warnings... |
|
@peterflynn i will just add an API like CommandManager.registerInternal(...) and maybe an isInternal flag in the Command itself for easy checking. |
|
The "valid use cases for these internal-only commands" are only registered as Commands. They should be never added as MenuItems. The check I suggested would warn if that was ever inadvertantly done. |
|
@WebsiteDeveloper Please add a new @peterflynn Currently, if code attempts to add one of these "internal" commands a menu it fails (i.e. no item is added to the menu), the console gets a message like this: Above I [suggested](https://github.com/adobe/brackets/pull/4306#issuecomment-22802792] that we could also insert the menu item using the command id, but I can live without that (and the effort & risk involved). Please let us know if you care one way or the other. |
| @@ -1200,7 +1200,7 @@ define(function (require, exports, module) { | |||
|
|
|||
| CommandManager.register(Strings.CMD_FILE_CLOSE, Commands.FILE_CLOSE, handleFileClose); | |||
| CommandManager.register(Strings.CMD_FILE_CLOSE_ALL, Commands.FILE_CLOSE_ALL, handleFileCloseAll); | |||
| CommandManager.register(Strings.CMD_CLOSE_WINDOW, Commands.FILE_CLOSE_WINDOW, handleFileCloseWindow); | |||
| CommandManager.register(null, Commands.FILE_CLOSE_WINDOW, handleFileCloseWindow); | |||
There was a problem hiding this comment.
These "internal" commands should all be moved to a new group at the end (with an appropriate comment).
|
@redmunds changes pushed. |
| @@ -193,6 +193,36 @@ define(function (require, exports, module) { | |||
| } | |||
|
|
|||
| /** | |||
| * Registers a global internal only command. | |||
| * @param {string} id - unique identifier for command. | |||
| * Core commands in Brackets use a simple command title as an id, for example "open.file". | |||
There was a problem hiding this comment.
While "open.file" is a fictional example, when I first read it, I thought of "file.open", which is not an internal id. To avoid confusion, change this to an actual internal id such as "app.abort_quit".
|
Done with review. I like this much better! Just 1 minor nit. |
|
@redmunds changes pushed. |
|
Looks good. Merging. |
[OPEN] Fix for Issue 4213: Remove strings for commands that are not user facing
#4213