[CLOSED] [OPEN] Fix for Issue 4213: Remove strings for commands that are not user facing

[core-ai-bot]

Issue by WebsiteDeveloper
Friday Jun 21, 2013 at 14:28 GMT
Originally opened as adobe/brackets#4306

---

#4213

---

WebsiteDeveloper included the following code: https://github.com/adobe/brackets/pull/4306/commits

[core-ai-bot]

Comment by jasonsanjose
Tuesday Jul 09, 2013 at 20:51 GMT

---

Thanks@WebsiteDeveloper. Changes look good except for the whitespace changes. Initial review complete.

[core-ai-bot]

Comment by peterflynn
Tuesday Jul 09, 2013 at 21:31 GMT

---

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.

[core-ai-bot]

Comment by WebsiteDeveloper
Tuesday Jul 16, 2013 at 16:33 GMT

---

@peterflynn we could set the name parameter explicitly to boolean "false" and check for that.

[core-ai-bot]

Comment by WebsiteDeveloper
Wednesday Jul 31, 2013 at 18:26 GMT

---

@jasonsanjose@peterflynn i pushed the whitespace fixes.
Any opinions on how to handle non-userfacing commands in general or via a more intuitive api?

[core-ai-bot]

Comment by WebsiteDeveloper
Monday Aug 12, 2013 at 07:34 GMT

---

@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.

[core-ai-bot]

Comment by redmunds
Saturday Aug 17, 2013 at 01:05 GMT

---

@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?

[core-ai-bot]

Comment by peterflynn
Thursday Aug 22, 2013 at 01:01 GMT

---

@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...

[core-ai-bot]

Comment by WebsiteDeveloper
Thursday Aug 22, 2013 at 08:02 GMT

---

@peterflynn i will just add an API like CommandManager.registerInternal(...) and maybe an isInternal flag in the Command itself for easy checking.

[core-ai-bot]

Comment by redmunds
Thursday Aug 22, 2013 at 16:42 GMT

---

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.

[core-ai-bot]

Comment by redmunds
Friday Sep 06, 2013 at 15:19 GMT

---

@WebsiteDeveloper Please add a new CommandManager.registerInternal(...) API call. This will make the intent much more clear.

@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:

addMenuItem(): Invalid Parameters when adding the command file.close_window Menus.js:571

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.

[core-ai-bot]

Comment by WebsiteDeveloper
Saturday Sep 07, 2013 at 15:48 GMT

---

@redmunds changes pushed.

[core-ai-bot]

Comment by redmunds
Sunday Sep 08, 2013 at 16:32 GMT

---

Done with review. I like this much better! Just 1 minor nit.

[core-ai-bot]

Comment by WebsiteDeveloper
Sunday Sep 08, 2013 at 16:39 GMT

---

@redmunds changes pushed.

[core-ai-bot]

Comment by redmunds
Tuesday Sep 10, 2013 at 00:42 GMT

---

Looks good. Merging.