Enable clients to persistently change the electron windowOptions

[owlJaeger]

Signed-off-by: Luca Jaeger [email protected]

What it does

I think clients should be able to make the electron window borderless. This was implemented by enabling clients to persistently change the electron windowOptions. This also enables clients to change it through the Theia preferences ( they still have to restart the window ). Example for making the window borderless:

import { ipcRenderer } from 'electron';

const currentWindowOptions = ipcRenderer.sendSync('get-window-options');
ipcRenderer.send('set-window-options', {
  ...currentWindowOptions,
  frame: false
});

How to test

1. Modify some frontend-contribution or add a new one. Execute these commands:

const currentWindowOptions = ipcRenderer.sendSync('get-window-options');
ipcRenderer.send('set-window-options', {
  ...currentWindowOptions,
  frame: false
});

(I've modified packages/core/src/electron-browser/menu/electron-menu-contribution.ts, onStart, for testing)
2. Restart the example.

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]

Thank you for the contribution @owlJaeger.

I don't see the value of having such a configuration personally, frameless windows cannot be resized or closed properly and thus would not be suitable options for an IDE-like application. There are also limitations on macOS as described in the documentation which require additional support which is not handled properly in this pull-request.

[owlJaeger]

I am working on a client which want's this because it will handle window dragging and controls itself. At the moment I cannot check the MacOS problem. Resizing works for me on Windows though. I will test it on Linux.

[owlJaeger]

@vince-fugnitto I hope the addition of the titleBarStyle-property will address your concerns on the MacOS side. This will leave it up to the clients to freely choose how they wan't their borderless mode to work.

[owlJaeger]

Maybe it should be configurable depending on the OS? Or should we just give a frameless option that will automatically check whether or not it is on macOS and if it is, use titleBarStyle: hidden instead of frame: false? It would have the benefit that the client wouldn't have to bother with the settings for a specific OS. Although it wouldn't make a true frameless easier, but a true frameless is, like you've already said, not quite usable for an IDE. But I think it would be very nice for clients with own titlebar-implementations on windows and Linux. It would also work very well if the client only draws the custom titlebar when not on macOS, which is easy to implement. This should give clients the opportunity to have a custom titlebar on windows and Linux but still use the Traffic-Lights on macOS and the menu implementation of macOS while not drawing the custom titlebar.

I think the single frameless option may be a bit less flexible, but still is way better than my current approach. What do you think about the single frameless option @vince-fugnitto

[owlJaeger]

@akosyakov @vince-fugnitto What do you think about these changes? These enable clients to take full control over the windowOptions. Clients can implement a preference that will for example give users the option to disable a custom titlebar.

[vince-fugnitto]

I verified the changes and they work well, I believe it is a better implementation than the first draft 👍

[vince-fugnitto]

The changes look good to me!
I'll give others a chance to review as well.

[paul-marechal]

Code is LGTM.

[owlJaeger]

As soon as this gets merged, I will create a follow-up PR enabling clients to set the default windowOptions through a tag inside package.json. In that PR I will implement the electron-subsection, just like you've suggested @akosyakov. This should then for example enable clients to implement a custom titlebar in electron which can be turned on or off (on by default). In my opinion this follow-up PR is necessary because changing the windowOptions will only change the options after a restart and thus would require a workaround to change the windowOptions-defaults.

[akosyakov]

@owlJaeger I am not involved much in electron products like @spoenemann @kittaakos @thegecko @westbury for instance. It would be good to hear from them whether we need such flexibility.

[akosyakov]

@marechal-p @thegecko please merge then you think it is good

[owlJaeger]

I am sorry... GitHub made me think that my review worked, but it didn't. So I'll copy what I've written in my review.

Review 1:

I am concerned however, when saving

width: windowState.width

through electronStore.set. If windowState changed, the persisted options will ignore that change and use the very first windowState it has seen?

Review 1 Comment 2:

I've developed a new approach. The new approach doesn't have that problem because it just stores what gets overwritten. So if the user doesn't change anything on purpose, everything works just as usual. For this new approach I will open a new Pull Request though because it not only introduces that, but also two other things. I'd prefer the new approach to be merged as it doesn't have the aforementioned flaws. After I've created it, the new PR should be reviewed and changed if needed. I'd like to keep this PR open until the other one is merged. If that happens, this can be closed. The other PR may need to be split into multiple PR's. At the moment it has multiple commits which I think, are related. But that's then up to you and the other reviewers.

Edit:

The PR is created now and doesn't look like it has to be split.

[thegecko]

@akosyakov @marechal-p
Regarding #7811, would your preference be to revert this PR and then consider merging #7803 ?

[akosyakov]

@thegecko If it takes a while to review #7803 then it is better to revert #7811 first