Fix #9779: add support for window.menuBarVisibility

[ChayaLau]

Signed-off-by: Esther Perelman [email protected], Chaya Lau [email protected]

What it does

Fix #9779 add support for window.menuBarVisibility
The pull-request adds the menu Bar Visibility setting to window setting, this allows compact and hidden layout for menu bar.

How to test

click on settings -> preferences ->
tap menu bar visibility in the search
menu bar visibility section should be displayed and drop down should appear with the options :
classic - menu bar should be displayed on the top - as original.
visible - behaves like classic.
hidden - menu bar does not appear anywhere.
compact - Menu is displayed as a compact button in the sidebar.

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[vince-fugnitto]

@ChayaLau thank you for the contribution 👍

Please ensure that:

• the breaking changes introduced in this pull-request are properly documented in the changelog - if we can avoid breakage please go that route.
• the default layout is broken when starting the application in the browser.
• please confirm the behavior when testing the changes in electron for the different settings.

[ChayaLau]

@ChayaLau thank you for the contribution 👍

Please ensure that:

• the breaking changes introduced in this pull-request are properly documented in the changelog - if we can avoid breakage please go that route.
• the default layout is broken when starting the application in the browser.
• please confirm the behavior when testing the changes in electron for the different settings.

hi,
Thanks for the reply
I fixed the PR according to your comments - point 2 and 3 - thanks!
about point 1 - I am new in this repo, and I did not fully understand what you meant about the breakage changes.
should i add a line in the changelog describing this PR? or did you mean something more than that?

in addition i wanted to mention this setting in VS CODE - has one additional option to this - toggle which i still did not develop, and planning on adding it soon. will this be OK adding it in like this and adding the option in the future, or will it be merged only if we add the toggle now?

Thanks,
Chaya

[vince-fugnitto]

should i add a line in the changelog describing this PR? or did you mean something more than that?

I'll take a closer look and identify any breaking changes. Ideally, if something is breaking for downstream applications and extensions we should aim to not break, and if its not possible we should add a note in the changelog about possible breakage like so:

• https://github.com/eclipse-theia/theia/blob/master/CHANGELOG.md#breaking_changes_1.16.0

...will this be OK adding it in like this and adding the option in the future, or will it be merged only if we add the toggle now?

We can always add the toggle ability in the future 👍 No need to have everything at once initially.

[vince-fugnitto]

@ChayaLau the main-menu is broken in the electron target with these changes, please address the issue before we proceed with an in-depth review.

The following is with classic (default), no main-menu exists:

[ChayaLau]

Thanks @vince-fugnitto for this important point, i fixed the broken electron menu

[colin-grant-work]

What is the intended behavior of the hidden setting in the browser? When I set to hidden, I get this:

The menu has been removed from the top section, but the top section remains, so the user doesn't get back any vertical space by hiding the menu. In Electron, the bar disappears completely, but it also doesn't contain the branding icon that the browser does.

[vince-fugnitto]

The menu has been removed from the top section, but the top section remains, so the user doesn't get back any vertical space by hiding the menu. In Electron, the bar disappears completely, but it also doesn't contain the branding icon that the browser does.

I agree, the whole top-menu should be hidden else there is no value for end-users.

[colin-grant-work]

Minor, and related only to old code: this method uses the click as the anchor for the menu. In VSCode, the menu is always anchored to the right of the element, so it likely uses the element's coordinates instead.

[EstherPerelman]

Reference for VSCode open menu code: https://github.com/microsoft/vscode/blob/99f71a032ea932670744d14d6364903a00b99ae3/src/vs/base/browser/ui/menu/menubar.ts#L957

[colin-grant-work]

The description for visible indicates that the menu should always be visible, but setting to visible and then double-clicking a tab hides the menu.

[colin-grant-work]

Because of the way MacOS handles menu bars, this preference doesn't have any effect in Theia - the menu bar can be made compact, but it always appears in the OS menu bar anyway. The preference doesn't appear to be present in the MacOS version of VSCode, so perhaps we should also conditionally bind it so that it doesn't appear as a preference in Electron if we detect that we're running on MacOS.

[ChayaLau]

The menu has been removed from the top section, but the top section remains, so the user doesn't get back any vertical space by hiding the menu. In Electron, the bar disappears completely, but it also doesn't contain the branding icon that the browser does.

i saw this is the behavior in VS code, are we sure we want the whole line to disappear not like VS Code?

[colin-grant-work]

I saw this is the behavior in VS code, are we sure we want the whole line to disappear not like VS Code?

If it's the intended behavior, then it is working as intended 👍. I would agree with @vince-fugnitto that that's somewhat odd behavior, since there doesn't then seem much point in hiding the menu, but I guess it reduces visual noise a bit.

[ChayaLau]

@colin-grant-work - I looked at this behavior, i think visible in VS Code refers only to the top menu, and not the sidebar. In order to refer to menuBarVisibility preference i tried to inject corePreferences to TheiaDockPanel class, but corePreferences is undefined, since I am new, i am not sure if i need to inject it in a different way or how to avoid the corePreference to be undefined?

[ChayaLau]

Because of the way MacOS handles menu bars, this preference doesn't have any effect in Theia - the menu bar can be made compact, but it always appears in the OS menu bar anyway. The preference doesn't appear to be present in the MacOS version of VSCode, so perhaps we should also conditionally bind it so that it doesn't appear as a preference in Electron if we detect that we're running on MacOS.

@colin-grant-work - i will add the included property to PreferenceDataProperty, i saw this property is still not supported in theia (although it is used in several places).

[colin-grant-work]

@colin-grant-work - I looked at this behavior, i think visible in VS Code refers only to the top menu, and not the sidebar.

Definitely - the sidebar is not visible here because I hid it, and that's fine. But the top bar should be visible.

In order to refer to menuBarVisibility preference i tried to inject corePreferences to TheiaDockPanel class, but corePreferences is undefined, since I am new, i am not sure if i need to inject it in a different way or how to avoid the corePreference to be undefined?

That's a little surprising - I'm happy to take a look at this to see what's going on.

PS: I originally clicked 'Edit' on your message rather than 'Quote Reply' by accident. Sorry if I restored the content wrong!

[ChayaLau]

@colin-grant-work - i added the injection and handled the maximized mode.
i added the included prop for the preference (in order to not include this on mac OS)
i added the class name to the changelog - but will be happy if you have a different advice how to prevent the breaking changes and handle this in a prettier way.
thanks for all the great advices and help!

[colin-grant-work]

I'll take a more careful look tomorrow, but I'm still seeing some oddities with the visible option:

The menu is still visible, but (in Electron) the maximized tab isn't truly maximized on the vertical axis, and the shell is still visible behind it. It appears to be working correctly in the browser, so I would guess that it has something to do with reserving space for the top panel, which doesn't exist in the Electron target.

[vince-fugnitto]

I noticed an odd bug, but it might indicate issues with other menu items as well:

• set menuBarVisibility to compact
• attempt to open the preferences-view through the menu

preferences-menu.mp4

I was only able to reproduce through compact, the default menu worked fine for the same use-case.

Also, when no workspace is present and menuBarVisibility is set to something other than classic, the menu appears as default then is updated to the correct value. It might be something to look into but is not super critical.

menu-flicker.mp4

[vince-fugnitto]

@msujew I noticed the bug #9931 as part of this pull-request, I assume you've noticed it on master?

[vince-fugnitto]

@ChayaLau disregard the comment about the preferences, I was able to confirm it is a bug on master by using the bottom sidebar context-menu 👍 Mark was kind enough to already provide a fix #9932.

[ChayaLau]

@vince-fugnitto - thanks for the comments, i fixed accordingly.
i saw the issue you were talking about, the top panel is hidden when the preference is set either to hidden or compact.
for some reason, which i still don't understand when the workspace is not set, the preferences are set only after the top menu is build.
i can - as a default, open the window without the top panel and show it when preference is auto populated, but maybe that will be worse, because most users will probably still use the classic menu, and they will experienced their top menu delay

[colin-grant-work]

for some reason, which i still don't understand when the workspace is not set, the preferences are set only after the top menu is build.

One way to handle this might be to delay the initial population of the top panel until after PreferenceService.ready has resolved, and then check the value before proceeding. At the moment, the default top-bar creation routine runs, and then the actual preference setting takes effect through PreferenceService.onPreferenceChanged listeners, but always a moment after the bar is initially created.

[colin-grant-work]

@ChayaLau, you seem to have made a change to handle the flashing menu, but your change has had the consequence that now the menu is never shown if the user has not set the preference:

It is not safe to rely on PreferenceChangeEvents alone, because the defaults are set during the construction phase of the PreferenceContribution, and if no other setting is detected, no event will ever fire. The correct way to wait for preferences to be read and then act on the settings is this:

await this.preferenceService.ready
const myValue = this.preferenceService.get('my-preference');

And that should be done wherever the menu is constructed so that it can be created according to whatever values is found when the PreferenceService is initialized, even if no event fires. Then, any subsequent changes can be handled by the event handler.

[ChayaLau]

@colin-grant-work - i tried to test before, and because i saw the preference change was firing - i thought that is enough.
according to your comment - i did change the code and registered the preference.ready.then() to set the top panel visibility

[colin-grant-work]

@ChayaLau, I also notice that you're doing checks for whether the application is running in Electron, but in fact a parallel problem occors in Electron.

Here I've set the menu bar visibility to 'compact,' but the menu appears for a moment before disappearing when the preference change event fires.

[ChayaLau]

@colin-grant-work - thanks for this point, i fixed this by - first i hide the menu, then i run the setMenu function which applies the preference value after the preference is ready

[ChayaLau]

@colin-grant-work , @vince-fugnitto - is there any chance merging this into the next branch?
i know this is more than a small issue to review and test, but we will be grateful if you can try pushing it in.
thanks

[colin-grant-work]

Though testing this in Electron is a little tricky at the moment, I retested this and found that

• Changing from all any preference selection to another works in both Electron and browser
• Maximizing a pane works with all preference settings in both Electron and browser
• Reloading the application doesn't produce any odd transitory behaviors in either Electron or browser

[vince-fugnitto]

The changes work well for me 👍

• changing the preference value works well for both the browser and electron targets
• changing the visibility is quick and does not produce flickering
• reloading the application properly restores the menu state without flickering

[msujew]

I can confirm the observations mentioned by Vince and Colin:

• Changing between different menu bar states works flawlessly
• Reloading the application produces no odd behavior
• Works both on electron and browser targets