Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CLOSED] Reload Without User Extensions #5764

Open
core-ai-bot opened this issue Aug 30, 2021 · 30 comments
Open

[CLOSED] Reload Without User Extensions #5764

core-ai-bot opened this issue Aug 30, 2021 · 30 comments

Comments

@core-ai-bot
Copy link
Member

Issue by lkcampbell
Sunday Dec 29, 2013 at 19:10 GMT
Originally opened as adobe/brackets#6334


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


lkcampbell included the following code: https://github.com/adobe/brackets/pull/6334/commits

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Sunday Dec 29, 2013 at 19:16 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Sunday Dec 29, 2013 at 20:14 GMT


This PR replaces PR #5620.

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Monday Dec 30, 2013 at 00:58 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Monday Dec 30, 2013 at 02:50 GMT


@redmunds, updated code committed.

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Monday Dec 30, 2013 at 17:21 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Monday Dec 30, 2013 at 17:50 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Monday Dec 30, 2013 at 20:34 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Monday Dec 30, 2013 at 20:40 GMT


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?

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Monday Dec 30, 2013 at 20:47 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Monday Dec 30, 2013 at 23:34 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Tuesday Dec 31, 2013 at 03:58 GMT


@redmunds, code review updates committed.

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Thursday Jan 02, 2014 at 16:48 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Thursday Jan 02, 2014 at 18:44 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Friday Jan 03, 2014 at 07:37 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Saturday Jan 04, 2014 at 15:03 GMT


@redmunds and@peterflynn, code review changes committed.

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Saturday Jan 04, 2014 at 18:10 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by ishan1608
Monday Jan 06, 2014 at 20:33 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Monday Jan 06, 2014 at 20:37 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by ishan1608
Monday Jan 06, 2014 at 20:44 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Monday Jan 06, 2014 at 20:55 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by njx
Monday Jan 06, 2014 at 21:13 GMT


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?

@core-ai-bot
Copy link
Member Author

Comment by ishan1608
Monday Jan 06, 2014 at 21:41 GMT


@redmunds As its mentioned here adobe/brackets#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.

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Tuesday Jan 07, 2014 at 01:34 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by ishan1608
Tuesday Jan 07, 2014 at 08:39 GMT


@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 adobe/brackets#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.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Tuesday Jan 07, 2014 at 09:40 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by ishan1608
Tuesday Jan 07, 2014 at 10:07 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Tuesday Jan 07, 2014 at 15:25 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by MarcelGerber
Tuesday Jan 07, 2014 at 16:12 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by ishan1608
Tuesday Jan 07, 2014 at 18:32 GMT


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 adobe/brackets#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.

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Tuesday Jan 07, 2014 at 19:14 GMT


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant