Fix #2902: Externalize debug menu as an extension #2942

Merged
merged 9 commits into from Feb 26, 2013

Conversation

Projects
None yet
3 participants
@TomMalbran
Contributor

TomMalbran commented Feb 25, 2013

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

@ghost ghost assigned jasonsanjose Feb 25, 2013

@@ -36815,7 +36815,7 @@ define('editor/EditorCommandHandlers',['require','exports','module','command/Com
/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */

This comment has been minimized.

Show comment Hide comment
@jasonsanjose

jasonsanjose Feb 25, 2013

Member

Please revert this file. This is only for performance testing and we need this snapshot to stay intact. Thanks!

@jasonsanjose

jasonsanjose Feb 25, 2013

Member

Please revert this file. This is only for performance testing and we need this snapshot to stay intact. Thanks!

+ /*
+ * Debug menu
+ */
+ var menu = Menus.addMenu(Strings.DEBUG_MENU, DEBUG_MENU, Menus.BEFORE, Menus.AppMenuBar.HELP_MENU);

This comment has been minimized.

Show comment Hide comment
@jasonsanjose

jasonsanjose Feb 25, 2013

Member

Need to refactor the 2 debug keyboard shortcuts out of src/base-config/keyboard.json into this extension.

@jasonsanjose

jasonsanjose Feb 25, 2013

Member

Need to refactor the 2 debug keyboard shortcuts out of src/base-config/keyboard.json into this extension.

src/document/DocumentCommandHandlers.js
@@ -911,5 +909,7 @@ define(function (require, exports, module) {
// Listen for changes that require updating the editor titlebar
$(DocumentManager).on("dirtyFlagChange", handleDirtyChange);
$(DocumentManager).on("currentDocumentChange fileNameChange", updateDocumentTitle);
+
+ exports.handleFileReload = handleFileReload;

This comment has been minimized.

Show comment Hide comment
@jasonsanjose

jasonsanjose Feb 25, 2013

Member

It seems like a cleaner way to do this would be to refactor _handleWindowGoingAway to some other module like Global.js and move handleFileReload to the debug extension.

@jasonsanjose

jasonsanjose Feb 25, 2013

Member

It seems like a cleaner way to do this would be to refactor _handleWindowGoingAway to some other module like Global.js and move handleFileReload to the debug extension.

@jasonsanjose

This comment has been minimized.

Show comment Hide comment
@jasonsanjose

jasonsanjose Feb 25, 2013

Member

Initial review complete

Member

jasonsanjose commented Feb 25, 2013

Initial review complete

@TomMalbran

This comment has been minimized.

Show comment Hide comment
@TomMalbran

TomMalbran Feb 25, 2013

Contributor

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

Contributor

TomMalbran commented Feb 25, 2013

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

@TomMalbran

This comment has been minimized.

Show comment Hide comment
@TomMalbran

TomMalbran Feb 25, 2013

Contributor

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.

Contributor

TomMalbran commented Feb 25, 2013

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.

@jasonsanjose

This comment has been minimized.

Show comment Hide comment
@jasonsanjose

jasonsanjose Feb 26, 2013

Member

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

jasonsanjose commented Feb 26, 2013

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);
        });
    });
}
@jasonsanjose

This comment has been minimized.

Show comment Hide comment
@jasonsanjose

jasonsanjose Feb 26, 2013

Member

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.

Member

jasonsanjose commented Feb 26, 2013

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.

@TomMalbran

This comment has been minimized.

Show comment Hide comment
@TomMalbran

TomMalbran Feb 26, 2013

Contributor

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

Contributor

TomMalbran commented Feb 26, 2013

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

src/utils/Global.js
@@ -23,7 +23,7 @@
/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
-/*global define */
+/*global define, $ */

This comment has been minimized.

Show comment Hide comment
@jasonsanjose

jasonsanjose Feb 26, 2013

Member

Looks like there are some leftover edits in this file Global.js. It should be completely clean.

@jasonsanjose

jasonsanjose Feb 26, 2013

Member

Looks like there are some leftover edits in this file Global.js. It should be completely clean.

+ */
+ var menu = Menus.addMenu(Strings.DEBUG_MENU, DEBUG_MENU, Menus.BEFORE, Menus.AppMenuBar.HELP_MENU);
+ menu.addMenuItem(Commands.DEBUG_SHOW_DEVELOPER_TOOLS);
+ menu.addMenuItem(Commands.DEBUG_REFRESH_WINDOW);

This comment has been minimized.

Show comment Hide comment
@jasonsanjose

jasonsanjose Feb 26, 2013

Member

Can remove the KeyBindingManager calls above and pass keybindings to addMenuItem for both:

menu.addMenuItem(Commands. DEBUG_SHOW_DEVELOPER_TOOLS, KeyboardPrefs.showDeveloperTools);
menu.addMenuItem(Commands.DEBUG_REFRESH_WINDOW, KeyboardPrefs.refreshWindow);
@jasonsanjose

jasonsanjose Feb 26, 2013

Member

Can remove the KeyBindingManager calls above and pass keybindings to addMenuItem for both:

menu.addMenuItem(Commands. DEBUG_SHOW_DEVELOPER_TOOLS, KeyboardPrefs.showDeveloperTools);
menu.addMenuItem(Commands.DEBUG_REFRESH_WINDOW, KeyboardPrefs.refreshWindow);
@jasonsanjose

This comment has been minimized.

Show comment Hide comment
@jasonsanjose

jasonsanjose Feb 26, 2013

Member

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

Member

jasonsanjose commented Feb 26, 2013

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

@TomMalbran

This comment has been minimized.

Show comment Hide comment
@TomMalbran

TomMalbran Feb 26, 2013

Contributor
Contributor

TomMalbran commented Feb 26, 2013

@jasonsanjose

This comment has been minimized.

Show comment Hide comment
@jasonsanjose

jasonsanjose Feb 26, 2013

Member

Looks good. Merging.

Member

jasonsanjose commented Feb 26, 2013

Looks good. Merging.

jasonsanjose added a commit that referenced this pull request Feb 26, 2013

Merge pull request #2942 from TomMalbran/tom/debug-extension
Fix #2902: Externalize debug menu as an extension

@jasonsanjose jasonsanjose merged commit 30c2751 into adobe:master Feb 26, 2013

1 check passed

default The Travis build passed
Details

@TomMalbran TomMalbran deleted the TomMalbran:tom/debug-extension branch Feb 26, 2013

@jbalsas

This comment has been minimized.

Show comment Hide comment
@jbalsas

jbalsas Feb 28, 2013

Contributor

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

Contributor

jbalsas commented Feb 28, 2013

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

@TomMalbran

This comment has been minimized.

Show comment Hide comment
@TomMalbran

TomMalbran Feb 28, 2013

Contributor

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.

Contributor

TomMalbran commented Feb 28, 2013

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.

@jbalsas

This comment has been minimized.

Show comment Hide comment
@jbalsas

jbalsas Feb 28, 2013

Contributor

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

Contributor

jbalsas commented Feb 28, 2013

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

@jasonsanjose

This comment has been minimized.

Show comment Hide comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment