Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

This fixes 2 issues when changing the Close Other Preferences #7088

Merged
merged 3 commits into from
Mar 18, 2014

Conversation

TomMalbran
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor

njx commented Mar 10, 2014

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

@JeffryBooher
Copy link
Contributor

@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
Copy link
Contributor Author

@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
Copy link
Contributor

njx commented Mar 12, 2014

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

@njx njx added this to the Sprint 37 milestone Mar 12, 2014
@marcelgerber
Copy link
Contributor

Isn't menuItemsAdded the same as menuEntriesShown?

@redmunds redmunds removed this from the Sprint 37 milestone Mar 12, 2014
@JeffryBooher
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Contributor Author

@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
Copy link
Contributor

@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
Copy link
Contributor Author

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

@TomMalbran
Copy link
Contributor Author

@SAplayer Which kind of tests are you thinking about?

@marcelgerber
Copy link
Contributor

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
Copy link
Contributor

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.

JeffryBooher added a commit that referenced this pull request Mar 18, 2014
This fixes 2 issues when changing the Close Other Preferences
@JeffryBooher JeffryBooher merged commit 3b8c310 into master Mar 18, 2014
@JeffryBooher JeffryBooher deleted the tom/close-others-fixes branch March 18, 2014 02:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants