Add config setting to control whether alt should show the menu bar #17517

Merged
merged 15 commits into from Jan 9, 2017

Projects

None yet

4 participants

@xwvvvvwx
Contributor

Hey, first of all thanks for the great work on vscode, I love it.

Anyway, I use alt key shortcuts for a lot of window management tasks, so if the menu bar is hidden, it is always popping in and out when I switch windows. (which is super annoying).

I added a new settings entry (window.autoHideMenuBar) that allows this behavior to be configured. When autoHideMenuBar is set to false then the alt key no longer shows / hides the menu bar.

I had a couple of things that I couldn't figure out though, so I was hoping someone on the team could give me some pointers:

  1. How do I make it so that the default is respected?
  2. How do I make it so that the change in settings is immediately applied (right now it does not take effect until the menu bar visibility is toggled).
@msftclas

Hi @xwvvvvwx, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@xwvvvvwx xwvvvvwx add config setting to control whether alt should show the menu bar wh…
…en hidden
c388fd9
@bpasero bpasero self-assigned this Dec 19, 2016
@bpasero
Member
bpasero commented Dec 19, 2016

You can listen on configuration changes on the main side to update this dynamically, e.g. see https://github.com/Microsoft/vscode/blob/master/src/vs/code/electron-main/menus.ts#L88

@xwvvvvwx xwvvvvwx autoHideMenubar: respect default and dynamically update on config change
a4ed430
@xwvvvvwx
Contributor

@bpasero Thanks!

Menu bar now dynamically updates, and takes default value if setting is not specified by the user.

Not sure if I'm setting the default correctly though? Doesn't feel really nice that I specify the default in so many places...

Is there a way I can write tests for this? I didn't see any tests for the existing menu bar behavior.

src/vs/code/electron-main/menus.ts
@@ -242,6 +255,10 @@ export class VSCodeMenu {
private install(): void {
+ // Auto hide menu bar
+ const windows = this.windowsService.getWindows();
+ windows.forEach(w => w.win.setAutoHideMenuBar(this.currentAutoHideMenuBar));
@xwvvvvwx
xwvvvvwx Dec 19, 2016 Contributor

Is this the right place to put this?

Feels a bit wasteful to create the whole menu from scratch when I just want to update a property of the window?

@xwvvvvwx xwvvvvwx formatting
ae0a494
@msftgits

Hi, I am closing and re-opening this PR to bump the CLA bot. Sorry for the inconvenience!

@msftgits msftgits closed this Dec 19, 2016
@msftgits msftgits reopened this Dec 19, 2016
@msftclas

Hi @xwvvvvwx, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@msftgits msftgits removed the cla-required label Dec 19, 2016
@bpasero

Added a first round of review feedback 👍

@@ -200,6 +200,11 @@ let properties: { [path: string]: IJSONSchema; } = {
'type': 'boolean',
'default': false,
'description': nls.localize('showFullPath', "If enabled, will show the full path of opened files in the window title.")
+ },
@bpasero
bpasero Dec 20, 2016 Member

Two things:

  • this setting should be wrapped around a isLinux/isWindows check so that it does not show up on macOS where it is not supported
  • I find the setting name a bit confusing because autoHideMenuBar is not really clear other than that it matches the Electron method we are using it for. I would try to change it to the intent it actually does (if this option is set to true, you can press Alt key to show the menu even if the menu is hidden)
src/vs/code/electron-main/windows.ts
@@ -1144,13 +1144,20 @@ export class WindowsManager implements IWindowsMainService {
// Update in settings
const menuBarHidden = this.storageService.getItem(VSCodeWindow.menuBarHiddenKey, false);
const newMenuBarHidden = !menuBarHidden;
+
+ const windowConfig = this.configurationService.getConfiguration<IWindowSettings>('window');
@bpasero
bpasero Dec 20, 2016 Member

It is true that on the main side we do not have access to the default values because they get defined on the browser side. Maybe we should turn this setting around and make it so that the default of a false value is OK for us. That avoids having to fill in a default on our end (instead of having true as default, change the setting so that false as default is good).

@xwvvvvwx
xwvvvvwx Dec 20, 2016 Contributor

Yes this makes sense, and I also had the same thought, but all the names I tried ended up with double negatives (e.g. "disableAutoHideMenubar": false) which always feels clumsy to me.

With that said which name works best for you (or another if you have suggestions):

  • neverShowHiddenMenuBar
  • altShowsHiddenMenuBar
src/vs/code/electron-main/menus.ts
@@ -196,6 +200,15 @@ export class VSCodeMenu {
updateMenu = true;
}
+ let newAutoHideMenuBar = config && config.window && config.window.autoHideMenuBar;
@bpasero
bpasero Dec 20, 2016 Member

I think this should move out of menu.ts into window.ts because it rather belongs there. You can have the same listener installed in window.ts and do the updating. Also note that I think it should only update if the menu is actually hidden.

@xwvvvvwx
xwvvvvwx Dec 20, 2016 Contributor

Thanks for the review!

Everything makes sense, except I'm not quite sure why it should only update when the menu is hidden? For efficiency?

@bpasero
bpasero Dec 20, 2016 Member

Does setAutoHideMenuBar make any sense when the menu is visible?

xwvvvvwx added some commits Dec 20, 2016
@xwvvvvwx xwvvvvwx MenuBar: move update of autohide setting from menus.ts to windows.ts
9f0cb4a
@xwvvvvwx xwvvvvwx menuBar: autohide setting is only used on linux/windows
4ef9909
@bpasero
Member
bpasero commented Dec 21, 2016 edited

The more I think about it, wouldn't it make more sense to introduce some kind of enumeration on a new setting window.menu that has these values:

  • menu visible (default)
  • menu hidden but Alt click brings it back
  • menu hidden without Alt click support

Overall it seems weird to have a setting for the Alt-behaviour but not a setting for actually hiding the menu.

@xwvvvvwx
Contributor

Nice idea :) yes, that sounds best to me also.

I am not 100% how to integrate this with the existing behavior though:

  • What should I do with the existing menu entry, add the options in a submenu? (this feels a bit cluttered).
  • How should the existing toggleMenuBar action interact with this setting?
@bpasero
Member
bpasero commented Dec 21, 2016

I would think UI wise everything stays the same and toggling just sets the new setting in a way as it works today but users can go in there to alter the behaviour as needed.

xwvvvvwx added some commits Dec 23, 2016
@xwvvvvwx xwvvvvwx menu bar visibility is now stored in settings 3013c92
@xwvvvvwx xwvvvvwx show notification that menu bar can be shown with alt 6b2fcaa
@xwvvvvwx xwvvvvwx remove unused code related to old menu bar toggling mechanism 727f4bb
@xwvvvvwx xwvvvvwx menu bar: only notify about hide / show when changing settings (not w…
…hen toggling fullscreen)
b5302ed
src/vs/code/electron-main/window.ts
- this.setMenuBarVisibility(!this.storageService.getItem<boolean>(VSCodeWindow.menuBarHiddenKey, false)); // restore as configured
- }
- }
+ // respect configured menu bar visibility
@xwvvvvwx
xwvvvvwx Dec 23, 2016 Contributor

@bpasero I changed the existing behavior here. Does this make sense for you?

xwvvvvwx and others added some commits Dec 23, 2016
@xwvvvvwx xwvvvvwx formatting
4f58636
@bpasero bpasero Merge remote-tracking branch 'upstream/master' into feature/allow_aut…
…o_hide_menu_bar_configuration
a71353b
@bpasero

@xwvvvvwx I like where this is going 👍

Gave some feedback. Also I am seeing a bug: When I enter fullscreen, I see the menu does not hide. This is a breaking change from current behaviour where when you enter fullscreen the menu hides and alt brings it back. I think we should preserve that behaviour independent from the setting. Alternatively maybe we should introduce a setting value of "default" that handles these cases (show the menu when not in fullscreen, hide the menu when in fullscreen but allow alt to bring it back). Then, if someone modifies the menu behaviour to something else it would also impact the menu when going to fullscreen (if menu is set to visible it would stay even in fullscreen and when set to hidden it would hide in fullscreen and not show on alt click).

src/vs/code/electron-main/windows.ts
@@ -90,6 +90,12 @@ interface INativeOpenDialogOptions {
window?: VSCodeWindow;
}
+interface IConfiguration {
@bpasero
bpasero Jan 4, 2017 Member

@xwvvvvwx I suggest to move this into window.ts including the configuration change listener. I think it is OK to let each window handle this change from within instead of from outside.

@xwvvvvwx
xwvvvvwx Jan 6, 2017 Contributor

totally happy to do this, just curious what the reason would be?

@bpasero
bpasero Jan 7, 2017 Member

Just having things closer to the location where it is needed.

src/vs/code/electron-main/window.ts
+ this.win.setAutoHideMenuBar(false);
+ break;
+ }
+ default: {
@bpasero
bpasero Jan 4, 2017 Member

@xwvvvvwx can we merge this with the 'visible' case? that avoids code duplication here.

xwvvvvwx added some commits Jan 6, 2017
@xwvvvvwx xwvvvvwx menubar defaults to toggle when switching to fullscreen
506c737
@xwvvvvwx xwvvvvwx tidy switch statement
90857a5
@xwvvvvwx xwvvvvwx menubar visibility is updated from within each window, rather than fr…
…om outside
3e37331
@xwvvvvwx xwvvvvwx Merge remote-tracking branch 'upstream/master' into feature/allow_aut…
…o_hide_menu_bar_configuration
63aefa5
@xwvvvvwx
Contributor
xwvvvvwx commented Jan 6, 2017

@bpasero I think that all the changes (including the bug) are addressed now.

Thanks for the feedback, let me know if there are more changes needed 😊

@bpasero bpasero added this to the January 2017 milestone Jan 9, 2017
@bpasero bpasero changed the title from [WIP] add config setting to control whether alt should show the menu bar to Add config setting to control whether alt should show the menu bar Jan 9, 2017
@bpasero
Member
bpasero commented Jan 9, 2017

Thanks, LGTM. Also cool side effect is that you can now chose to always show the top level menu when in fullscreen mode if wanted.

@bpasero bpasero merged commit a4873c2 into Microsoft:master Jan 9, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment