[CLOSED] Fix #2902: Externalize debug menu as an extension

[core-ai-bot]

Issue by TomMalbran
Monday Feb 25, 2013 at 03:51 GMT
Originally opened as adobe/brackets#2942

---

This request fixes #2902 by moving the DebugCommandHandlers, NodeDebugUtils and all the Debug menu creation to a new DebugCommands extension.

---

TomMalbran included the following code: https://github.com/adobe/brackets/pull/2942/commits

[core-ai-bot]

Comment by jasonsanjose
Monday Feb 25, 2013 at 20:46 GMT

---

Initial review complete

[core-ai-bot]

Comment by TomMalbran
Monday Feb 25, 2013 at 23:09 GMT

---

@jasonsanjose Thanks for the fast review!

I addressed all the comments, although I am not sure if moving _handleWindowGoingAway to Global.js was the best option since it added new dependencies on that File. Would be better to just leave it in DocumentCommandHandlers and export just that function and use it this extension? Anyway, is in Global.js now.

[core-ai-bot]

Comment by TomMalbran
Monday Feb 25, 2013 at 23:23 GMT

---

There is a problem with the dependencies now by moving _handleWindowGoingAway to Global.js.

Global.js requires ProjectManager.js to be loaded first to be able to register the trigger handler, but PreojectManager loads PrefUtils.js that requires Global.js to be defined first. So by requiring ProjectManager.js before Global.js brackets loads, but the tests don't (and maybe something else).

A solution would be what I mentioned first or make other file trigger the "beforeAppClose" event.

[core-ai-bot]

Comment by jasonsanjose
Tuesday Feb 26, 2013 at 18:05 GMT

---

Ah, should have seen that coming. Ok, let's back those changes out of Global.js. How about in DebugCommands/main.js...forgive any early morning copy paste errors:

function handleReload(commandData) {
    CommandManager.execute(Commands.FILE_CLOSE_ALL, { promptOnly: true }).done(function () {
        // Give everyone a chance to save their state - but don't let any problems block
        // us from quitting
        try {
            $(ProjectManager).triggerHandler("beforeAppClose");
        } catch (ex) {
            console.error(ex);
        }

        // Disable the cache to make reloads work
        _disableCache().always(function () {
            window.location.reload(true);
        });
    });
}

[core-ai-bot]

Comment by jasonsanjose
Tuesday Feb 26, 2013 at 21:36 GMT

---

Need to merge with master again to kick off a new build on travis. JSHint dependencies were updated, causing the build to fail...see #2956.

[core-ai-bot]

Comment by TomMalbran
Tuesday Feb 26, 2013 at 21:59 GMT

---

@jasonsanjose Now is everything working after backing the changes on Global.js and using your code in main.js!

Should we move the commands ids from Commands.js to the extension?

[core-ai-bot]

Comment by jasonsanjose
Tuesday Feb 26, 2013 at 22:27 GMT

---

Yes to moving the Commands. Just a few minor comments and this should be ready to go.

[core-ai-bot]

Comment by TomMalbran
Tuesday Feb 26, 2013 at 22:45 GMT

---

@jasonsanjose Done!

[core-ai-bot]

Comment by jasonsanjose
Tuesday Feb 26, 2013 at 23:13 GMT

---

Looks good. Merging.

[core-ai-bot]

Comment by jbalsas
Thursday Feb 28, 2013 at 04:47 GMT

---

@jasonsanjose I just noticed that this change may break extensions adding menu items to the debug menu since Menus.AppMenuBar.DEBUG_MENU is no longer available. I'm not sure if there are any out there, but just in case you'd want to note it somewhere.

On the other hand, should we discourage developers to add items to the debug menu, or even prevent it somehow?

[core-ai-bot]

Comment by TomMalbran
Thursday Feb 28, 2013 at 04:54 GMT

---

I noticed that this breaks the menu add in your extension. I don't think developers should add items to the debug menu since it might or not be present. Up until now it was available only in Brackets and not in Edge Code, so the extension would only work in Brackets.

It seems harder now to add to the debug menu since you need to look for the id and it will fail to add if your extension loads before the debug extension.

[core-ai-bot]

Comment by jbalsas
Thursday Feb 28, 2013 at 05:04 GMT

---

@TomMalbran I totally agree (I guess I put my extension there because I couldn't figure out where it fit ;)).

The wiki page for extensions says:

Add a menu item: Get a top-level menu by calling Menus.getMenu() with one of the AppMenuBar constants.

Just by reading that, I'd say people probably won't be trying to add items to the debug menu anymore, but maybe a explicit note in there about this wouldn't hurt in case someone goes looking for it.

[core-ai-bot]

Comment by jasonsanjose
Thursday Feb 28, 2013 at 05:43 GMT

---

Added to release notes https://github.com/adobe/brackets/wiki/Release-Notes:-Sprint-21