Reload Without User Extensions #6334

lkcampbell · 2013-12-29T19:10:10Z

This PR fixes issue #5078 by adding a new Debug Command: Reload Without User Extensions.

lkcampbell · 2013-12-29T19:16:53Z

FYI, one of the big chunks of code, the function _disableCache() in main.js, has not been changed, it has only been moved to the top of the file to avoid a JSLint error, so disregard that part of the diff.

lkcampbell · 2013-12-29T20:14:40Z

This PR replaces PR #5620.

redmunds · 2013-12-30T00:58:06Z

I notice that extensions in the src/extensions/dev folder are still loaded in "Reload Without User Extensions" mode. Seems like they should not be loaded either.

lkcampbell · 2013-12-30T02:50:22Z

@redmunds, updated code committed.

redmunds · 2013-12-30T17:21:52Z

I notice that Menus.js is using CollectionUtils (which is deprecated in favor of lodash) when removing MenuItems. You can see warnings in console log. This should be cleaned up either in this pull request or split into a separate one.

redmunds · 2013-12-30T17:32:38Z

src/extensions/default/DebugCommands/main.js

-    CommandManager.register(Strings.CMD_NEW_BRACKETS_WINDOW,  DEBUG_NEW_BRACKETS_WINDOW,    _handleNewBracketsWindow);
+    CommandManager.register(Strings.CMD_REFRESH_WINDOW,             DEBUG_REFRESH_WINDOW,           handleFileReload);
+    CommandManager.register(Strings.CMD_RELOAD_WITHOUT_USER_EXTS,   DEBUG_RELOAD_WITHOUT_USER_EXTS, _handleReloadWithoutUserExts);
+    CommandManager.register(Strings.CMD_NEW_BRACKETS_WINDOW,        DEBUG_NEW_BRACKETS_WINDOW,      _handleNewBracketsWindow);


I can see that this existed before you made any changes to this file, but functions are not named consistently -- some are preceded with underscores and some are not. Since extensions can not (yet, anyway) export functions, all functions are, by definition, private -- so there's no reason to precede names with underscores. I can go either way as long as it's consistent.

Also, handleFileReload doesn't seem quite right since all files are being reloaded, so this should just be handleReload (or _handleReload).

It does export _runUnitTests(), which I imagine is used by Jasmine, so should I remove all underscores from functions except for _runUnitTests()? Or should I set exports._runUnitTests to runUnitTests?

I missed that one. It should be _runUnitTests() because it's only for unit tests.

Done. Removed underscores from all methods except _runUnitTests().

redmunds · 2013-12-30T17:50:33Z

This functionality should be mentioned in the documentation. Here are a couple places that come to mind:

The How to Use Brackets Unofficial Features section
Troubleshooting Guide Disable All Extensions section

lkcampbell · 2013-12-30T20:34:47Z

Updating Menus.js to use lodash instead of CollectionUtils: Done.

lkcampbell · 2013-12-30T20:40:15Z

Another piece of documentation that needs to be updated:

The How to Report an Issue Extension bugs section

Do you want me to make these changes now or after the next Sprint release?

lkcampbell · 2013-12-30T20:47:00Z

@redmunds, I reviewed your review. Made some changes but have not yet done the commit because I have some additional questions. See above.

redmunds · 2013-12-30T23:34:23Z

Do you want me to make these (documentation) changes now or after the next Sprint release?

Good point. These changes should be done right after next Sprint Release.

lkcampbell · 2013-12-31T03:58:19Z

@redmunds, code review updates committed.

redmunds · 2014-01-02T16:47:41Z

src/utils/ExtensionLoader.js

+
+        if (params.get("reloadWithoutUserExts") === "true") {
+            paths = ["default"];
+        }


This block of code assumes that the only possible values for the paths array is ["default", "dev", getUserExtensionPath()] or ["default"]. This seems to currently be the case, but this code would be more robust if it were to remove the "dev" and getUserExtensionPath() elements instead.

Other than unit tests, the paths argument is always null -- so the standard list is already hardcoded right here on line 323. So one alternative would be to just move this if up into the !paths case (i.e. if a list of paths is explicitly provided by unit tests, we ignore the reloadWithoutUserExts flag and use the list verbatim; the flag would only be respected when using the default set of paths.

I like that a little better since it's actually hard to guess what the correct response is when the array contains unexpected additional elements -- are those elements more similar to "default" (i.e. we should still load them even in safe mode) or are they more similar to "dev"/"user" (we shouldn't load them)? Impossible to say if we don't know anything about them...

I agree that is good enough for now, and it's a simpler fix.

redmunds · 2014-01-02T16:48:48Z

Done with final pass through the code. 1 more comment.

redmunds · 2014-01-02T18:44:53Z

Also, I just submitted pull request adobe/brackets-shell#407 to fix menu drawing performance on Windows. Not sure if you noticed this, but on Windows you can watch each menu being removed one at a time because the menu is redrawn for every menu item removed (which really slows down performance).

This is not a common or time-critical operation, so it's not required for this pull request, but I'd like to get it in the same Sprint. Let me know if you want to review that one.

lkcampbell · 2014-01-03T07:37:57Z

@redmunds, sounds good. I will look at this pull request and your pull request this weekend.

lkcampbell · 2014-01-04T15:03:10Z

@redmunds and @peterflynn, code review changes committed.

redmunds · 2014-01-04T18:10:23Z

Looks good. Thanks for sticking with this complicated issue! Merging.

Reload Without User Extensions

ishan1608 · 2014-01-06T20:33:53Z

"Extension Manager" is disabled when reloaded for NO Extensions, but the menu item "Reload Without User Extensions" is still there and not disabled. Doesn't it makes more sense to disable that too ?
I mean the user can't install a new extension when in No Extensions mode, so re-executing the command "Reload Without User Extensions" doesn't makes any sense. It will just remove all the core brackets menus, and reload them. There will be no change.

redmunds · 2014-01-06T20:37:29Z

@ishanatmuz Thanks for trying out this new feature. Installing and uninstalling extensions is not the only reason for wanting to Reload Brackets (with or without user extensions), so we need to keep this enabled for now.

ishan1608 · 2014-01-06T20:44:54Z

@redmunds What I was trying to imply is this: If there are no extensions left to disable after the first use of "Reload Without User Extensions", then what's the point of keeping it there ?
Am I missing some key point here, some key functionality that this command provides and I am not seeing it.

redmunds · 2014-01-06T20:55:32Z

@ishanatmuz "Reload Without User Extensions" does not change you to a different mode. It's a quick way to Reload Brackets to try to isolate whether a problem you are seeing is caused by an extension (or not). If you then "Reload Brackets" or shutdown and restart Brackets, user extensions are once again loaded.

So, if you use "Reload Without User Extensions" and then do something that requires a reload (such as make an update to your system externally from Brackets), then you may want to "Reload Without User Extensions" again to refresh your system, but not load user extensions.

Hope that makes sense. If you have any suggestions on how to improve this, then your feedback is welcome.

njx · 2014-01-06T21:13:22Z

Part of the problem might be that it's not obvious that "Reload Brackets" will turn extensions back on again. Should we consider making it so that menu item changes its name to "Reload With Extensions" when we're in safe mode?

ishan1608 · 2014-01-06T21:41:23Z

@redmunds As its mentioned here #5078 (comment) I was expecting shutdown/restart to be the only way to get the extensions enabled back.
I was hoping that when the extensions are disabled, "Reload Brackets (F5)" doesn't re-enables extension.

@njx I think that better solution is not just to change the menu, but change it's behavior to re-load without extensions.
I think this implementation is better because, once the user has already mentioned that he/she doesn't wants extensions to be active temporarily, then why does he/she has to carefully choose the right option from the menu. The user can accidentally click on Reload Brackets (F5) instead of Reload Without User Extensions, and then be frustrated with what happened.
I think that making the shutdown/restart the ONLY WAY to re-enable extensions makes more sense, as it complies with the more general idea of SAFE MODE in the browsers or windows systems.
Plus it also sticks with the original goal ( to quote @lkcampbell ) Yes, I would expect shutdown/restart to be the only way to get out of the mode which was an answer to a query by @redmunds .

I am just pitching the previously stated idea here. I was expecting this to be a feature since I started using brackets. And the original idea seemed more practical to me as most of us use keyboard shortcuts instead of going to menus.
I never imagined a solution like the current situation, and I was following the conversation from the minute I found the issue mentioned.

redmunds · 2014-01-07T01:34:00Z

"Reload Brackets" and shutdown/restart are only slightly different, so I am not sure that they were intended to be handled differently. I, personally, considered them to be synonymous in this context.

One missing piece to this puzzle is that we plan to provide a UI in the Extension Manager for explicitly enabling and disabling individual extensions. These settings will persist across sessions.

Do you agree that this will be even better than the behavior that you are describing? If so, is the current behavior ok as it is?

If you think a SAFE MODE is important, then it seems like it should persist across shutdown/restart.

ishan1608 · 2014-01-07T08:39:28Z

@redmunds Obviously having the ability to disable extensions individually which persists, over more than one session is a good feature to have. The point here to note is that even if there is a single option to Disable All extensions. One problem can still arise there. As you yourself mentioned here #5078 (comment) that A user could leave brackets open for long enough to forget that they're in this mode. It gets expanded to multiple sessions when the Extension Manager persists the behavior across sessions and disables the extensions.
What I think is that having Extension Manager to have the ability to disable/enable extensions with persistence across sessions is good. But the general purpose use of disabling all extensions at once is to have the user test whether some bug is caused by an extension or core brackets, when he/she has no idea, regarding whether the bug is caused by an extension and the core brackets is in the clear.

What I suggest is to have the feature of enabling/disabling extensions with persistence across different sessions. And we should also have the "Reload Without User Extensions" feature. Just the modification in this feature is that when once in the No User Extension mode, the menu item Reload Without User Extensions should be disabled. The Reload Brackets (F5) menu item should be modified to behave as Reload Without User Extensions. And when the user shutdown/restart the behavior of Reload Brackets (F5) goes back to the norm of reloading with extensions.
What I am trying to say is that Reload Brackets (F5) should be smart enough to detect whether the user has explicitly expressed that he/she doesn't want the extensions to be active for now and then act accordingly.
I think this should be easy enough for @lkcampbell to do this little modification.
And this also leaves the option of the user wanting all the extensions to be disabled for more than one session through the Disable All option in Extension Manager.
Of-course there is no Disable All option in Extension Manager, I am advocating it to be there.

peterflynn · 2014-01-07T09:40:05Z

@ishanatmuz What you're proposing doesn't sound very different from what's already in this pull request:

In the pull request: after choosing to reload without extensions, the user can reload again and keep extensions turned off (by reselecting the same menu item). The user can re-enable extensions either by quitting Brackets completely (then relaunching), or by reloading using the normal menu item.
In your proposal: after choosing to reload without extensions, the user can reload again and keep extensions turned off (by selecting the normal reload menu item). The user can re-enable extensions only by quitting Brackets completely (then relaunching).

You mentioned the risk that users might forget that they're in the mode where extensions are disabled -- but that actually seems to argue more for the behavior in the pull request, where it's easier to turn extensions back on. (I'm not terribly worried about users having this issue though).

It seems simpler technically to leave this as is, and also less confusing to avoid having the menu items' meanings juggle around after disabling extensions. @njx's suggestion to relabel the menu items while extensions are disabled wold make it even more explicit -- I can't see any way users would get confused at that point. @ishanatmuz does that labeling change sound ok to you?

peterflynn · 2014-01-07T09:44:45Z

src/nls/root/strings.js

@@ -436,6 +437,7 @@ define({
    "DEBUG_MENU"                                : "Debug",
    "CMD_SHOW_DEV_TOOLS"                        : "Show Developer Tools",
    "CMD_REFRESH_WINDOW"                        : "Reload {APP_NAME}",
+    "CMD_RELOAD_WITHOUT_USER_EXTS"              : "Reload Without User Extensions",


@redmunds @lkcampbell
Independent of the thread above, I wonder if we should consider relabeling this. "User Extensions" seems easy to misinterpret as referring to the "extensions/user" folder, which is wrong since this command also affects the "src/extensions/dev" folder.

I think we should simply say "Extensions" in these strings -- as far as most users are concerned, the only "extensions" that exist are the ones that show up in Extension Manager. The average end user has no idea that, as an implementation detail, certain non-optional core functionality happens to be structured as extensions too. So I think we may be trying to parse hairs more than needed in the labels here.

I support @peterflynn in this.

I like this idea as well mainly because it is a shorter length and fits in the menu better, while retaining the important semantics.

@SAplayer, @WebsiteDeveloper I know you guys were discussing this as part of the really long German translation (pull request #6366). Since I can't read German, what was the final translation, in English, that you guys agreed on?

@lkcampbell
Yes, we were talking about the same thing.

We first had this:
Reload without custom (meaning installed/User) extensions

Then I changed it to this:
Reload without extensions

But our reason to change this wasn't because it hadn't included the dev folder, it was just too long.

ishan1608 · 2014-01-07T10:07:50Z

@peterflynn Of-course label change is a better scenario, and of-course it is technically simpler to leave it as it is. But what I am concerned more about is adopting to the way users behave. Users generally reload using the shortcut-key instead of going to the menus.
So let's say they reloaded without extensions, and then made some changes and need to reload again (without the extensions), they are going to have to go to the menu and do this again. They can't use keyboard shortcut for this. Since we have a habit of using the shortcuts, accidents can happen where the user (out of habit) used the shortcut-key to reload instead of going to the menu. It will be easier for the user instead of going the menus time and again.
Don't you think that once the user has mentioned that extensions aren't required temporarily, then mentioning it time and again is frustrating ? Keyboard shortcuts have this advantage of quick development flow. Forcing user to access this only via menus doesn't seem justifiable to me.

lkcampbell · 2014-01-07T15:25:39Z

@ishanatmuz, so, are you saying that you want a shortcut key assigned to the command? Because we could probably do that as well, assuming a good option is still available.

marcelgerber · 2014-01-07T16:12:12Z

My opinion:
After the safe-mode is activated, change the menu items:
Reload without extensions (F5)
Reload with extensions

ishan1608 · 2014-01-07T18:32:54Z

I thought that I was clear enough on what I think is better. Anyways....
What I was stating is this:

Reload Without User Extensions should be persistent for one session, and one session only.
The menu Reload Without User Extensions should be disabled once the user has opted to disable the extensions for that session.
The current behavior of Reload Brackets (F5) is to bring back the extensions. It should be changed to NOT bring back the extensions when the user enters the No Extensions mode. It should be noted that I expect it to reload WITH the extensions in normal case scenario where the user has not used Reload Without User Extensions, thus is not in No Extensions mode and is just normally reloading brackets.
The above point will make sure that the only way to get extensions enabled back is to shutdown/restart brackets.

I think this time it's clear enough to not have any confusions regarding what I am trying to say.

As @lkcampbell suggested we can have another shortcut-key associated with Reload Without User Extensions say F6. And this is almost a good idea, but it adds the burden of remembering one more shortcut, and it will be a deviation from the normal work-flow of the user.

I tend to agree more with the suggestion made by @SAplayer . This will not be a deviation from the normal work-flow. But I have just one tiny little comment to make on this. I have also mentioned it earlier. It just doesn't sticks with what @lkcampbell mentioned earlier #5078 (comment) shutdown/restart to be the only way to get out of the _mode_.

Now I do realize that design decisions change with the development. And if everybody wants that there should be some option to get the extensions back on without shutdown/restart, then I think the suggestion made by @SAplayer is solid. But I can't help but notice that there isn't any discussion regarding this change in the original goal.
Please don't think that I am vehemently against it, I just didn't found any discussion regarding this change, and I wanted to point that out.

lkcampbell · 2014-01-07T19:14:23Z

@ishanatmuz, the change of the goal that I stated is in the next comment.

That's the point that it went from "Restart" to "Reload". It was in response to some implementation questions that @redmunds was posing and I was investigating at the time.

marcelgerber · 2014-01-07T20:05:43Z

My final suggestion:

Use the menu entries as mentioned in Reload Without User Extensions #6334 (comment)
Add a tooltip to the StatusBar entry Use "Debug -> Restart with Extensions" to exit the "No extensions" mode.

This would hopefully avoid confusion in case someone doesn't know how to exit this mode and doesn't want to search through all menu entries.

redmunds · 2014-01-07T22:38:40Z

@ishanatmuz To solve the "user could leave brackets open for long enough to forget that they're in this mode" issue, "User Extensions Disabled" is displayed in the status bar when extensions are not loaded.

Thank you everyone for the input. The core team discussed this offline and decided that we should change the wording of the menu items and add a shortcut for the new command to:

Debug > Reload With Extensions with a shortcut of F5 on Windows/Linux and Cmd-R on Mac
Debug > Reload Without Extensions with a shortcut of Shift-F5 on Windows/Linux and Cmd-Shift-R on Mac

This keeps these menu items and shortcuts always the same and always enabled.

lkcampbell · 2014-01-07T23:52:21Z

@redmunds, sounds good, I will submit the PR tonight.

redmunds · 2014-01-08T02:00:22Z

@lkcampbell Thanks! FYI, this feature is already coming in handy -- it helped me isolate this one: #6401

ishan1608 · 2014-01-08T04:23:34Z

@lkcampbell I thought that #5078 (comment) was only meant to change the name, and not change the exit behavior. Sorry for misinterpreting that.

@redmunds That is a good decision. I wonder why no-one came up with the idea of using Shift-F5 for disabling extensions earlier. It would have saved a lot of trouble.

P.S. Seeing that issue #5078 is marked as Low Priority, I apologize for wasting so much time of the team over this matter.

lkcampbell · 2014-01-08T15:33:03Z

@redmunds, update committed.

lkcampbell · 2014-01-08T15:38:12Z

@ishanatmuz, it's always been high priority for me :). You never need to apologize for helping to make the Brackets better.

Add Debug Menu command: Reload Without User Extensions

9dff969

ghost assigned redmunds Dec 29, 2013

Small code cleanup

3e335ad

Reload should not load extensions in dev directory

dacd9bb

redmunds reviewed Dec 30, 2013
View reviewed changes

Code cleanup. Replace CollectionUtils with lodash. Add console log.

7b40ba8

Small comment update

8182f60

redmunds reviewed Jan 2, 2014
View reviewed changes

redmunds mentioned this pull request Jan 2, 2014

Fix remove menu performance on Windows adobe/brackets-shell#407

Merged

Make ExtensionLoader paths code more robust

d85f302

redmunds added a commit that referenced this pull request Jan 4, 2014

Merge pull request #6334 from lkcampbell/fix-5078

d1ece15

Reload Without User Extensions

redmunds merged commit d1ece15 into adobe:master Jan 4, 2014

lkcampbell deleted the fix-5078 branch January 4, 2014 19:51

redmunds mentioned this pull request Jan 5, 2014

Brackets calls deprecated function internally: CollectionUtils.forEach #6368

Closed

peterflynn reviewed Jan 7, 2014
View reviewed changes

lkcampbell mentioned this pull request Jan 8, 2014

Reload Without Extensions update #6411

Merged

core-ai-bot mentioned this pull request Aug 17, 2021

[CLOSED] Fix remove menu performance on Windows brackets-archive/brackets-shell#262

Open

core-ai-bot mentioned this pull request Aug 30, 2021

[CLOSED] Reload Without User Extensions brackets-archive/bracketsIssues#5764

Open

Reload Without User Extensions #6334

Reload Without User Extensions #6334

Conversation

lkcampbell commented Dec 29, 2013

lkcampbell commented Dec 29, 2013

lkcampbell commented Dec 29, 2013

redmunds commented Dec 30, 2013

lkcampbell commented Dec 30, 2013

redmunds commented Dec 30, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redmunds commented Dec 30, 2013

lkcampbell commented Dec 30, 2013

lkcampbell commented Dec 30, 2013

lkcampbell commented Dec 30, 2013

redmunds commented Dec 30, 2013

lkcampbell commented Dec 31, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redmunds commented Jan 2, 2014

redmunds commented Jan 2, 2014

lkcampbell commented Jan 3, 2014

lkcampbell commented Jan 4, 2014

redmunds commented Jan 4, 2014

ishan1608 commented Jan 6, 2014

redmunds commented Jan 6, 2014

ishan1608 commented Jan 6, 2014

redmunds commented Jan 6, 2014

njx commented Jan 6, 2014

ishan1608 commented Jan 6, 2014

redmunds commented Jan 7, 2014

ishan1608 commented Jan 7, 2014

peterflynn commented Jan 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishan1608 commented Jan 7, 2014

lkcampbell commented Jan 7, 2014

marcelgerber commented Jan 7, 2014

ishan1608 commented Jan 7, 2014

lkcampbell commented Jan 7, 2014

marcelgerber commented Jan 7, 2014

redmunds commented Jan 7, 2014

lkcampbell commented Jan 7, 2014

redmunds commented Jan 8, 2014

ishan1608 commented Jan 8, 2014

lkcampbell commented Jan 8, 2014

lkcampbell commented Jan 8, 2014