Support vscode's titleBarStyle

[msujew]

What it does

Adds support for vscode's window.titleBarStyle setting that allows to use a browser-like menu in the electron environment:

custom-title-bar-style.mp4

Note that this is only supported in Windows and Linux distributions. I actually have no idea how it works on MacOS.

How to test

1. Start Electron Theia on either Windows or Linux.
2. On Windows, Theia will use by default the custom (browser-like) title bar. On Linux the native one.
3. Assert that switching between the preference values is possible. The application has to be restarted afterwards.
4. Assert that the window functions are working correctly. This includes the newly added buttons, the draggable menu bar, and the resizable window.
5. Assert that both custom and native state work correctly with the different configurations of the window.menuBarVisibility setting.
6. Confirm that the application still renders correctly on MacOS.

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]

Starting the application on Ubuntu I am getting some odd behavior with my app (no electron top-menu):

[vince-fugnitto]

In vscode, the preference also controls the display of context-menus, meaning we can have browser-like menus in electron instead of native. That's why when I first thought of the issue I was thinking we can control the binding we do to not bind native electron menus and instead use that of the browser:

theia/packages/core/package.json

Lines 123 to 124 in 068e78d

	"frontend": "lib/browser/menu/browser-menu-module",
	"frontendElectron": "lib/electron-browser/menu/electron-menu-module"

With a bit more extra work, we can simply use the browser menus in electron (main menu, and context menus) by modifying the way we bind.

[msujew]

Do we always want to have browser-like menus in Electron? If not, I'm not quite sure how we would be able to accomplish that during the binding phase, as we need the preference service to identify which title bar style is selected.

[vince-fugnitto]

@msujew I believe dynamic binding might be possible, and is something we likely did in #9186.

As for the behavior, I would expect that:

• native displays the native menus (main-menu and context-menus) similarly to vscode
• custom displays the browser-like menus (main-menu and context-menus) similarly to vscode

You're right that there might be issues especially on startup, but perhaps it's something to try.
cc @paul-marechal any thoughts?

[msujew]

You reminded me that the context menus where not displayed in the browser styling if custom is used. I addressed that.

I believe dynamic binding might be possible, and is something we likely did in #9186.

I only saw conditional binding based on os based constants. That works fairly well. However, during binding we have no access to preferences as they are 1. a service in themselves 2. we would have to wait for the async ready state.

[msujew]

Starting the application on Ubuntu I am getting some odd behavior with my app (no electron top-menu)

Should be fixed now 👍

[vince-fugnitto]

@msujew when updating the preference, we probably want to ask users to confirm a restart of the application similarly to vscode (confirmation dialog)?

I was able to get the following state:

• update preference from native to custom
• execute the command reload application

---

You reminded me that the context menus where not displayed in the browser styling if custom is used. I addressed that.

The custom context-menus (with custom enabled) do not appear for me on Ubuntu.

[msujew]

when updating the preference, we probably want to ask users to confirm a restart of the application similarly to vscode (confirmation dialog)?

Yes, I also thought about that. I tried implementing it, but got discouraged by the electron app restarting without starting the backend server. I'll take another look at it 👍

The custom context-menus (with custom enabled) do not appear for me on Ubuntu.

Weird, it works fine for me on Windows:

Time to bring out my old Ubuntu vm...

[vince-fugnitto]

Weird, it works fine for me on Windows:

Hopefully the video helps :)

reload-bug.mp4

[msujew]

Hopefully the video helps :)

I got that part, but do have a video of the native context menus (on custom) as well? ;)

[vince-fugnitto]

I got that part, but do have a video of the native context menus (on custom) as well? ;)

Sure!

context-menu-bug.mp4

[msujew]

when updating the preference, we probably want to ask users to confirm a restart of the application similarly to vscode (confirmation dialog)?

@vince-fugnitto I managed to implement that without the whole application crashing. However, the onPreferenceChanged event handler fires instantly on startup on all preferences (and therefore always shows a restart dialog). Is this a feature or a bug?

[colin-grant-work]

Is this a feature or a bug?

Both :-/. Ideally, the initial setting of preferences wouldn't fire an event, and they would just take on those values and then the preference service would resolve its .ready promise and everyone would see the initial values and work from there. In practice, because there's a (reasonably) predictable flood of events (under most conditions), there are a number of places where code has been written in such a way to depend on those events, in the framework and almost certainly downstream. The bug earlier this week re: the browser menu bar was an example of the consequences of such code, but it would almost certainly be a pain in adopters' butts to remove that flood of events, however unsafe it is to rely on it in general.

Re: this particular case, you should be able to do something like setting a flag when you first create the menu bar and not showing the dialog before that. That way, you don't show it for the first flood of events (before you've even decided what to do), but any subsequent changes do trigger the dialog.

[msujew]

this particular case, you should be able to do something like setting a flag when you first create the menu bar and not showing the dialog before that

Thank you for the explanation, I implemented something similiar that ignores the first wave of preference changes 👍

I was also able to fix the issue Vince experienced (changing preference -> ignoring the dialog -> resetting the view).

[msujew]

@vince-fugnitto I can't reproduce your context-menu issue using Ubuntu 20.04 (running emulated on Windows):

Do you mind retesting it? I think I addressed every other issue.

[vince-fugnitto]

I think the behavior I was noticing was something in cache, since if I switch between two different electron instances (ex: master and this pr) I end up with odd behaviors.

One thing I did notice is the following:

1. start the application, update the preference to custom
2. accept the restart prompt (the app restarts with the new menu)
3. close the application
4. modify the preference in the user's home (remove the custom titleBarStyle preference)
5. when restarting the application I get back the custom menu

It's only when I remove the electron cache do I see the proper menu again:

$ rm -rf ~/.config/Theia\ Electron\ Example/

I'm not sure if there is anything we can improve in this aspect, but I don't think it should block the pull-request, just a bug end-users might experience.

[msujew]

I'm not sure if there is anything we can improve in this aspect

I don't think so either. We don't have access to the users preferences during startup, which would be needed in order to display the correct window title. Instead, everything is saved in the windowStore.

I also just noticed that the context menu uses the default electron menu if you set the preference from custom to native (without restarting) and reset the view. I fixed that as well 👍

[vince-fugnitto]

I don't think so either. We don't have access to the users preferences during startup, which would be needed in order to display the correct window title. Instead, everything is saved in the windowStore.

@msujew unfortunately, the window.titleBarStyle preference does not ultimately behave like other preferences in the framework for that reason. It means that downstream applications cannot set the default value for the preference (in the package.json) as it will not apply like others.

The following for instance does not work for me:

"theia": {
    "target": "electron",
    "frontend": {
      "config": {
        "applicationName": "Theia Electron Example",
        "preferences": {
          "window.titleBarStyle": "custom"
        }
      }
    }
  },

The app starts with custom but it is never applied (even on subsequent starts).

[msujew]

@vince-fugnitto Is this an approval? :)
@colin-grant-work Do you still plan on reviewing it?

It would be great, if we were able to include it in this month's release. Although, I don't have an issue with delaying it.

[colin-grant-work]

@msujew, Sorry, got distracted. I'll take a look later today.

[colin-grant-work]

This seems to be working well for me. I was able to switch between modes without issue, and the behavior was correct.

Re: styling, the Theia icon certainly, and I think the other menu bar icons less obviously, are hovering a little higher than they should. It looks like about 10px below and only 6px above.

[msujew]

Re: styling, the Theia icon certainly, and I think the other menu bar icons less obviously, are hovering a little higher than they should. It looks like about 10px below and only 6px above.

@colin-grant-work I agree (in regards to the Theia icon), but this also appears in the browser version (master):

I think this is unrelated to my changes and should be addressed in a separate PR.

The other title bar icons are actually one pixel lower compared to vscode (vscode in the foreground, Theia in the background):

But that is kind of related to Theia's top panel being 32px high, while vscode's is only 30px high. Should I change this to 30px in Theia as well?

[colin-grant-work]

I think this is unrelated to my changes and should be addressed in a separate PR.

Sounds fair.

But that is kind of related to Theia's top panel being 32px high, while vscode's is only 30px high. Should I change this to 30px in Theia as well?

I don't have strong feelings here. I certainly have no objection in principle to 30px, but I don't think the 1px shift is objectionable either, and there is always the chance that downstream (or other upstream) code / CSS expects 32px. For now, I'd probably opt not to worry about changing it.

[colin-grant-work]

This looks good to me. Good work finding a way to get electron to play nicely.

[colin-grant-work]

@msujew, @vince-fugnitto, this is working well for me, and the code looks good. Depending on your feelings about its complexity / likelihood of bugs, it might be worth putting off merging until after tomorrow's release?

[msujew]

It might be worth putting off merging until after tomorrow's release?

Sure, fine with me 👍

[orange-pig]

Starting the application on Ubuntu I am getting some odd behavior with my app (no electron top-menu):

I get the same problem now, when I try upgrade theia to 1.23.0. from 1.18.0.

After build the electron and start, I got it. No titlebar & top menu, only the content be display.

And I try to set window: Title bar style custom -> native, it will be have a windows tile bar. (other: The restart window look like do not show always.)

So, the window title bar style be default custom in new version, but it can't work in my PC, and then build electron installer running in other PC still so.

And, I try to delete .theia/ in user folder, still so.
And, I try to delete electron/ in AppData folder, still so.

I am using:
node.js: 14.18.1
typescript: 4.5.5

[kittaakos]

@msujew, was there a technical obstacle to allowing the custom title bar on macOS?

theia/packages/core/src/electron-browser/window/electron-window-preferences.ts

Line 50 in 52d6e87

included: !isOSX

VS Code supports it. Thank you!

[msujew]

@kittaakos No real technical reason. I just don't have a macOS machine to test it. So we should still support it sometime on macOS.

[kittaakos]

Excellent, thank you! We must implement it downstream, and if it works, we will contribute it back.