Reload Without Extensions update

[lkcampbell]

This PR updates PR #6334. Changes the menu strings and adds a shortcut key.

[lkcampbell]

@redmunds, do you want me to update the status bar string also? Are we changing all instances of "User Extensions" to "Extensions"?

[redmunds]

@lkcampbell I was wondering about that myself. I have not heard it mentioned. If anyone thinks this should change, please speak up!

cc: @njx @peterflynn since they suggested wording changes for menu items

[redmunds]

@lkcampbell Yes, also change the status bar string.

[lkcampbell]

@redmunds, updates committed.

[redmunds]

I'm seeing a bug on Mac: after using Cmd-Shift-R shortcut, the "Debug" menu stays highlighted. Works correctly for Cmd-R.

The only difference seems to be that Cmd-Shift-R reloads entire menu, but Cmd-R does not reload menu. I'm not sure why this causes a difference.

On a related note, I think we'll need to change Cmd-R to also reload menu to handle the "extension uninstalled" case so any menu items effectively get removed, correct?

[redmunds]

FYI, Windows looks good, so that's a Mac-only problem.

[lkcampbell]

@redmunds, I'm not seeing the bug on Mac OS Mavericks. But, to answer your question, yes, the entire menu is removed just before the reload when Reload Without Extensions is executed (Cmd-Shift-R), and it is not removed when reloaded normally (Cmd-R), so it is probably caused by that difference. Could it be fixed with a menu focus change or something like that?

Yes, changing the extension remove and update code to use reload instead of restart will require the same menu removal code. Did you want to change it so all reload code removes the entire menu in anticipation of this next change?

[redmunds]

I'm on Mac 10.8, so I play around with trying to fix it.

Did you to change it so all reload code removes the entire menu?

No I haven't made any change. I thought you signed up for that one. :)

[redmunds]

@RaymondLim noticed that shortcuts are not getting cleaned up in RemoveMenuItem() on Mac. We're hoping fixing that fixes this problem. There might also be a memory leak here.

If this is not an easy fix, we might want to take all string changes (i.e. everything except adding shortcut to "Reload Without Extensions") to get those in.

[lkcampbell]

@redmunds, oops, typo, I meant to say it like "is this the change you want?"

I think, at this point, we should keep it as simple as possible. This feature has an uncanny knack for finding other issues and "memory leak" is one of my least favorite phrases. So, I can roll back the shortcuts and file an issue to add them later, after the shortcut problems get resolved. Also, I address the "entire menu removal on every reload" issue when I address the "extension update/remove reload" issue, instead of doing it now.

Let me know what you think.

[lkcampbell]

@redmunds, @RaymondLim, also, is there an issue filed for this menu shortcut problem or is someone currently investigating it? Because it seems like the entire menu removal process might be messed up by this problem, so I might be interested in investigating it if you guys have repro steps.

[peterflynn]

@redmunds @lkcampbell If shortcuts aren't getting properly cleaned up, does that mean the "Reload Without Extensions" isn't truly a clean reload with all traces of extensions erased? Extensions can add new shortcuts, or even remove/reassign existing core shortcuts...

[lkcampbell]

@peterflynn, I think most of the key binding info gets reset when Brackets is reloaded. I also assumed that when I removed a menuItem in the application shell, it also removed the shortcut key association with that menuItem.

Of course, a good portion of errors I have made have been related to assumptions on how the app shell menu code works. So, I could be wrong.

[redmunds]

@RaymondLim is looking at it. It should be relatively simple to add a call to remove the shortcut in RemoveMenuItem(), so let's wait to hear what he says.

@peterflynn There may be a memory/resource leak, but I just tried "Reload Without Extensions" and I'm not able to invoke an extension with the shortcut.

[peterflynn]

@redmunds It's hard to know just from that if everything's ok -- the extension's not loaded, so for all we know the native shell is still trying to invoke the command from the defunct, but CommandManager.execute() is silently no-opping since the extension never registered that command ID.

But the other case I worry about is where extensions have actually munged the default shortcuts, e.g. via KeyBindingManager.removeBinding(). Does "Reload Without Extensions" properly restore the shortcut to the default core command's menu item?

[RaymondLim]

Actually, I'm just guessing that we may not properly clean up shortcuts. I just tried to use the new shortcut with changes in this pull request and it highlights "Help" menu on first try, then highlights "Debug" menu on subsequent tries but no blink on "Debug" menu. Then I encounter this crash issue #6422.

[lkcampbell]

@peterflynn, "Reload Without Extensions" loads up the same way that "Reload With Extensions" loads up. Should be the same shortcut loading process. However, if the shortcuts are not being unloaded properly from the Brackets shell, which I am almost certain they are not, that make be affecting the subsequent reload.

@RaymondLim, I am going to jump over to issue #6422 from here and create a fix for you to try out.

[lkcampbell]

@redmunds, the only menu shortcut method I could find in the shell is SetMenuItemShortcut. I think its only purpose is to write the shortcut information next to the menu item in the menu. Could this be the problem? I am skeptical.

If it is the problem, then we have another problem, which is: we don't have a RemoveMenuItemShortcut method. Any other ideas or suggestions on things I might be missing?

[lkcampbell]

@redmunds, I'm not sure what my next steps are supposed to be on this. Can you give me some suggestions or guidance?

[redmunds]

@lkcampbell We're pretty sure the problem is in the Mac native code which Raymond is looking at that. You're welcome to look at that as well, but otherwise I don't think there's anything to do until we get that straightened out.

End of Sprint is Monday EOD. If we don't get it fixed by then, we need to decide to either:

1. If it's safe enough, then take string changes, but do not add new shortcut.
2. Otherwise, remove "Reload Without (User) Extensions" from Debug menu for now.

[RaymondLim]

@lkcampbell @redmunds Actually, the problem is not really in the native code. Mac OS always highlights the menu when one of its menu items is invoked via a keyboard shortcut, and then removes the highlight to show the blinking effect. It does the blinking of the menu based on the menu index when the shortcut is invoked. What happens with our new shortcut is that we immediately remove all the menus before the Mac OS has a chance to blink the invoked menu. So the resulting effect is that the OS may highlight the wrong item and subsequently cannot remove the highlight from the incorrect or newly created "Debug" menu (since it is not the same menu object). So we either have to live with this resulting effect if we really want to add a shortcut to this menu item. You can verify my discovery once #6435 is fixed; you just have to make a change to the current file and then keep pressing the shortcut. You will see Debug menu blinking while the save file dialog is blocking menus removal. Once you let go and menus are starting to be removed, then Mac OS won't be able to perform the blinking.

Update: One possible way to have the correct menu blinking is that we asynchronously trigger the "reload w/o extensions" and immediately return for the invoked command. This way, the menu should get blinked before menus removal starts and the OS won't be highlighting the wrong menu later on.

[redmunds]

@lkcampbell I verified that adding an intermediate function that invokes window.setTimeout(handleReloadWithoutUserExts, 100) seems to fix the problem.

@RaymondLim Thanks for researching this. Are you saying we don't need to cleanup the native shortcuts on Mac? Or just not to fix the blink?

[RaymondLim]

@redmunds Yes, we don't need to cleanup native shortcuts on Mac. Each shortcut is associated with a menu item and I believe menu item removal also takes care of it.

[RaymondLim]

@lkcampbell One more thing for you. When you implement with window.setTimeOut, make sure that you don't trigger the call multiple times as the user keeps pressing on the shortcut (just like I did in my experiment).

[redmunds]

Why shouldn't it get called multiple times? The user may want to Reload Brackets for some reason other than uninstalling or updating an extension, so we can't ignore it.

[RaymondLim]

I mean invoking the same command before the previously invoked one is still running as in #6452.

[lkcampbell]

@RaymondLim I will be glad to address #6452, but I want to do it separate from this pull request because it is an existing bug that has probably been around for a long time. Trying to keep this PR as clean and simple as possible so we can get it finished on time.

[RaymondLim]

@lkcampbell Thanks for grabbing all related issues to your plate. I agree with you that #6452 should not be part of this pull request.

[lkcampbell]

@redmunds, I wrapped all of the functionality in an anonymous function that fires 100 ms after the handler is called. Hopefully this fixes the highlighting problem but I am flying blind since I can't repro the problem. All I can tell you is it doesn't break on my end :).

[RaymondLim]

@lkcampbell Your change does fix the blinking or highlighting issue even though it may be hard to notice on the first try, but you should be able to see blinking on the second try.

[lkcampbell]

@RaymondLim I can't see the problem or the solution, it all looks the same to me. Maybe a difference in the OS, but as long as you and @redmunds can see it, that's good enough for me.

[redmunds]

Looks good. Merging.