[CLOSED] This fixes 2 issues when changing the Close Other Preferences

[core-ai-bot]

Issue by TomMalbran
Wednesday Mar 05, 2014 at 07:37 GMT
Originally opened as adobe/brackets#7088

---

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 included the following code: https://github.com/adobe/brackets/pull/7088/commits

[core-ai-bot]

Comment by TomMalbran
Monday Mar 10, 2014 at 19:31 GMT

---

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

[core-ai-bot]

Comment by njx
Monday Mar 10, 2014 at 19:38 GMT

---

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

[core-ai-bot]

Comment by JeffryBooher
Tuesday Mar 11, 2014 at 18:15 GMT

---

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

[core-ai-bot]

Comment by TomMalbran
Tuesday Mar 11, 2014 at 20:08 GMT

---

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

[core-ai-bot]

Comment by njx
Wednesday Mar 12, 2014 at 01:39 GMT

---

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

[core-ai-bot]

Comment by MarcelGerber
Wednesday Mar 12, 2014 at 15:56 GMT

---

Isn't menuItemsAdded the same as menuEntriesShown?

[core-ai-bot]

Comment by JeffryBooher
Wednesday Mar 12, 2014 at 18:07 GMT

---

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.

[core-ai-bot]

Comment by TomMalbran
Wednesday Mar 12, 2014 at 19:22 GMT

---

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

[core-ai-bot]

Comment by MarcelGerber
Wednesday Mar 12, 2014 at 20:18 GMT

---

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

[core-ai-bot]

Comment by JeffryBooher
Wednesday Mar 12, 2014 at 22:54 GMT

---

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

[core-ai-bot]

Comment by TomMalbran
Thursday Mar 13, 2014 at 00:32 GMT

---

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

[core-ai-bot]

Comment by MarcelGerber
Thursday Mar 13, 2014 at 18:43 GMT

---

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

[core-ai-bot]

Comment by TomMalbran
Thursday Mar 13, 2014 at 20:20 GMT

---

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

[core-ai-bot]

Comment by TomMalbran
Saturday Mar 15, 2014 at 22:32 GMT

---

@SAPlayer Which kind of tests are you thinking about?

[core-ai-bot]

Comment by MarcelGerber
Saturday Mar 15, 2014 at 23:46 GMT

---

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

[core-ai-bot]

Comment by JeffryBooher
Tuesday Mar 18, 2014 at 02:34 GMT

---

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.