-
Notifications
You must be signed in to change notification settings - Fork 30.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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
Hi @xwvvvvwx, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! TTYL, MSBOT; |
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 |
@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. |
@@ -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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Hi, I am closing and re-opening this PR to bump the CLA bot. Sorry for the inconvenience! |
Hi @xwvvvvwx, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! The agreement was validated by Microsoft and real humans are currently evaluating your PR. TTYL, MSBOT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.") | |||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
@@ -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'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -196,6 +200,15 @@ export class VSCodeMenu { | |||
updateMenu = true; | |||
} | |||
|
|||
let newAutoHideMenuBar = config && config.window && config.window.autoHideMenuBar; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does setAutoHideMenuBar
make any sense when the menu is visible?
The more I think about it, wouldn't it make more sense to introduce some kind of enumeration on a new setting
Overall it seems weird to have a setting for the Alt-behaviour but not a setting for actually hiding the menu. |
Nice idea :) yes, that sounds best to me also. I am not 100% how to integrate this with the existing behavior though:
|
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. |
this.setMenuBarVisibility(!this.storageService.getItem<boolean>(VSCodeWindow.menuBarHiddenKey, false)); // restore as configured | ||
} | ||
} | ||
// respect configured menu bar visibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bpasero I changed the existing behavior here. Does this make sense for you?
…o_hide_menu_bar_configuration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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).
@@ -90,6 +90,12 @@ interface INativeOpenDialogOptions { | |||
window?: VSCodeWindow; | |||
} | |||
|
|||
interface IConfiguration { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totally happy to do this, just curious what the reason would be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just having things closer to the location where it is needed.
this.win.setAutoHideMenuBar(false); | ||
break; | ||
} | ||
default: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xwvvvvwx can we merge this with the 'visible' case? that avoids code duplication here.
@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 😊 |
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. |
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. WhenautoHideMenuBar
is set tofalse
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: