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

d-xo · 2016-12-19T07:43:43Z

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:

How do I make it so that the default is respected?
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 · 2016-12-19T07:43:46Z

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;

…en hidden

bpasero · 2016-12-19T08:46:04Z

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

d-xo · 2016-12-19T15:33:07Z

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

d-xo · 2016-12-19T15:35:28Z

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));


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?

msftgits · 2016-12-19T17:13:31Z

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

msftclas · 2016-12-19T17:13:37Z

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;

bpasero

Added a first round of review feedback 👍

bpasero · 2016-12-20T06:19:29Z

src/vs/workbench/electron-browser/main.contribution.ts

@@ -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.")
+	},


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)

bpasero · 2016-12-20T06:21:36Z

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');


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

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

bpasero · 2016-12-20T06:24:17Z

src/vs/code/electron-main/menus.ts

@@ -196,6 +200,15 @@ export class VSCodeMenu {
 			updateMenu = true;
 		}

+		let newAutoHideMenuBar = config && config.window && config.window.autoHideMenuBar;


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.

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?

Does setAutoHideMenuBar make any sense when the menu is visible?

bpasero · 2016-12-21T10:23:29Z

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.

d-xo · 2016-12-21T10:50:45Z

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 · 2016-12-21T13:01:04Z

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.

…hen toggling fullscreen)

d-xo · 2016-12-23T15:18:38Z

src/vs/code/electron-main/window.ts

-				this.setMenuBarVisibility(!this.storageService.getItem<boolean>(VSCodeWindow.menuBarHiddenKey, false)); // restore as configured
-			}
-		}
+		// respect configured menu bar visibility


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

…o_hide_menu_bar_configuration

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

bpasero · 2017-01-04T10:53:03Z

src/vs/code/electron-main/windows.ts

@@ -90,6 +90,12 @@ interface INativeOpenDialogOptions {
 	window?: VSCodeWindow;
 }

+interface IConfiguration {


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

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

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

bpasero · 2017-01-04T10:54:12Z

src/vs/code/electron-main/window.ts

+				this.win.setAutoHideMenuBar(false);
+				break;
+			}
+			default: {


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

…om outside

…o_hide_menu_bar_configuration

d-xo · 2017-01-06T22:56:31Z

@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 · 2017-01-09T08:27:46Z

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.

msftclas added the cla-required label Dec 19, 2016

add config setting to control whether alt should show the menu bar wh…

c388fd9

…en hidden

bpasero self-assigned this Dec 19, 2016

autoHideMenubar: respect default and dynamically update on config change

a4ed430

d-xo commented Dec 19, 2016

View reviewed changes

formatting

ae0a494

msftgits closed this Dec 19, 2016

msftgits reopened this Dec 19, 2016

msftclas added the cla-already-signed label Dec 19, 2016

msftgits removed the cla-required label Dec 19, 2016

d-xo mentioned this pull request Dec 19, 2016

Add Option to Disable "Alt" Showing Menu Bar When Toggled to be Hidden #14567

Closed

bpasero reviewed Dec 20, 2016

View reviewed changes

David Terry added 2 commits December 20, 2016 18:03

MenuBar: move update of autohide setting from menus.ts to windows.ts

9f0cb4a

menuBar: autohide setting is only used on linux/windows

4ef9909

David Terry added 4 commits December 23, 2016 15:47

menu bar visibility is now stored in settings

3013c92

show notification that menu bar can be shown with alt

6b2fcaa

remove unused code related to old menu bar toggling mechanism

727f4bb

menu bar: only notify about hide / show when changing settings (not w…

b5302ed

…hen toggling fullscreen)

d-xo commented Dec 23, 2016

View reviewed changes

David Terry and others added 2 commits December 23, 2016 16:23

formatting

4f58636

Merge remote-tracking branch 'upstream/master' into feature/allow_aut…

a71353b

…o_hide_menu_bar_configuration

bpasero requested changes Jan 4, 2017

View reviewed changes

David Terry added 4 commits January 6, 2017 22:05

menubar defaults to toggle when switching to fullscreen

506c737

tidy switch statement

90857a5

menubar visibility is updated from within each window, rather than fr…

3e37331

…om outside

Merge remote-tracking branch 'upstream/master' into feature/allow_aut…

63aefa5

…o_hide_menu_bar_configuration

bpasero added this to the January 2017 milestone Jan 9, 2017

bpasero changed the title ~~[WIP] add config setting to control whether alt should show the menu bar~~ Add config setting to control whether alt should show the menu bar Jan 9, 2017

bpasero merged commit a4873c2 into microsoft:master Jan 9, 2017

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

d-xo commented Dec 19, 2016

msftclas commented Dec 19, 2016

bpasero commented Dec 19, 2016

d-xo commented Dec 19, 2016

d-xo Dec 19, 2016

msftgits commented Dec 19, 2016

msftclas commented Dec 19, 2016

bpasero left a comment

bpasero Dec 20, 2016

bpasero Dec 20, 2016

d-xo Dec 20, 2016

bpasero Dec 20, 2016

d-xo Dec 20, 2016

bpasero Dec 20, 2016

bpasero commented Dec 21, 2016 •

edited

Loading

d-xo commented Dec 21, 2016

bpasero commented Dec 21, 2016

d-xo Dec 23, 2016

bpasero left a comment

bpasero Jan 4, 2017

d-xo Jan 6, 2017

bpasero Jan 7, 2017

bpasero Jan 4, 2017

d-xo commented Jan 6, 2017

bpasero commented Jan 9, 2017

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

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

Conversation

d-xo commented Dec 19, 2016

msftclas commented Dec 19, 2016

bpasero commented Dec 19, 2016

d-xo commented Dec 19, 2016

Choose a reason for hiding this comment

msftgits commented Dec 19, 2016

msftclas commented Dec 19, 2016

bpasero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpasero commented Dec 21, 2016 • edited Loading

d-xo commented Dec 21, 2016

bpasero commented Dec 21, 2016

Choose a reason for hiding this comment

bpasero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-xo commented Jan 6, 2017

bpasero commented Jan 9, 2017

bpasero commented Dec 21, 2016 •

edited

Loading