This fixes 2 issues when changing the Close Other Preferences

[TomMalbran]

When all 3 Close other preferences are false, the commands are never registered, but it was still trying to change them which makes the working set context menu not open.

It was also trying to remove the menu Items when those were never added in the first place.

[TomMalbran]

@JeffryBooher @njx Can we merge this for Release 37? This fixes a pretty nasty bug that won't let you open the working set context menu when disabling all close other preferences.

[njx]

Sure - leaving it up to @JeffryBooher to assess the risk.

[JeffryBooher]

@TomMalbran Please file a bug for the problem with context menus so we can track and regress the issue. I'm not clear on the steps to reproduce the symptoms you describe above.

[TomMalbran]

@JeffryBooher Is easy to reproduce. For the context menu issue open the preferences file and add the following preferences:

    "closeOthers.above": false,
    "closeOthers.below": false,
    "closeOthers.others": false,

For the second issue, after adding any of those preferences, restart Brackets and check the developers tools.

I can make an issue if you want.

[njx]

@JeffryBooher can you bring this up in the standup tomorrow so we can discuss merging it for release 37? I'll go ahead and tag it so it's on the radar.

[marcelgerber]

Isn't menuItemsAdded the same as menuEntriesShown?

[JeffryBooher]

Some observations here:

The original impl came long before preferences could be changed and reloaded so that means that some of the original impl should be updated to reflect that. Primarily with respect to registering commands. It seems better to just register the commands regardless of whether or not they are enabled in prefs and then enable / disable them as needed.

Additionally the code enables and disables commands when the the context menu is opened regardless of the fact that the commands may not have been registered. This should generate an error or warning in the console. If we just register all commands regardless of whether or not their prefs are turned on then we don't have this problem. So line 108 should be:

if (!commandsRegistered) {
      ...
}

This allows external methods (such as another commands) to invoke the command even if all of the context menu prefs for the commands are turned off.

The team discussed this today and, since these prefs are not documented, the risk of anyone hitting any of these scenarios is minimal. We need to get sprint 37 wrapped up today so we're going to move this to sprint 38 when the changes discussed above are done.

[TomMalbran]

@SAplayer Not really. The items in menuItemsAdded will be always true after the item is added to the menu. The point is that after setting any of the close others preference to false and restarting Brackets, the code tries to remove a menu item, since for example, menuEntriesShown.closeBelow = undefined and prefCloseBelow = false, so (prefCloseBelow !== menuEntriesShown.closeBelow) = true so it goes into the if, and then to the else branch where it tries to remove a menu item that was never added.

@JeffryBooher That would solve the issue too and it would work fine. Would be the same behavior as before the preference were added, but you were able to remove the items from the settings.

I am thinking that we should just separate the initial creation from the preferences update, since the code can be quite different now, and will be a better solution that using menuItemsAdded.

[marcelgerber]

@JeffryBooher No, in fact this code was updated by me a few days ago, introducing these prefs.
So it's actually my fault. I may should have changed more architectural things.

[JeffryBooher]

@SAplayer I think we need to separate the model from the view in this case so that the commands are always available and the menu is updated according to the prefs.

[TomMalbran]

@JeffryBooher @SAplayer I just split the initialization part from the preferences updates which makes the code look cleaner. The Commands now are always register.

Since this fixes an issue created in this Sprint it would be nice if we can fix it for Sprint 37, The fix is kind of simple too, but I guess is a lower priority bug.

[marcelgerber]

@TomMalbran Looks much better now, thanks. Haven't tested it out yet.

Another point, but not a showstopper (it would definitely be nice to get this PR landed in Sprint 37), is to add more tests.

[TomMalbran]

I'll try to add them later, if I have time.

[TomMalbran]

@SAplayer Which kind of tests are you thinking about?

[marcelgerber]

Maybe one to check that the prefs are minded at all and one to check the behaviour on pref change, if that's possible.

[JeffryBooher]

Yeah it is possible but a lot of work. I'm going to merge this and confer internally to see what we should do about it. We had a discussion today around having unit tests to ensure bug fixes don't regress but this is hardly the bug to start with.

We can file a separate issue for unit tests on this pull request since it really involves testing for menu changes.