Skip to content
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

Merged
merged 15 commits into from
Jan 9, 2017
Merged

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

merged 15 commits into from
Jan 9, 2017

Conversation

d-xo
Copy link

@d-xo d-xo commented Dec 19, 2016

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

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;

@bpasero bpasero self-assigned this Dec 19, 2016
@bpasero
Copy link
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

@d-xo
Copy link
Author

d-xo commented Dec 19, 2016

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

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?

@msftgits
Copy link

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

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;

Copy link
Member

@bpasero bpasero left a 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.")
},
Copy link
Member

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');
Copy link
Member

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

Copy link
Author

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;
Copy link
Member

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.

Copy link
Author

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?

Copy link
Member

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?

@bpasero
Copy link
Member

bpasero commented Dec 21, 2016

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

d-xo commented Dec 21, 2016

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

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

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?

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

@bpasero bpasero left a 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 {
Copy link
Member

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.

Copy link
Author

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?

Copy link
Member

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: {
Copy link
Member

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.

@d-xo
Copy link
Author

d-xo 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 [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
Copy link
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
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
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.

4 participants