Added ability to change windowOptions & it's defaults. Moved disallowReloadKeybinding inside electron subsection

[owlJaeger]

What it does

Fixes #7811

My preferred alternative to #7787 with overall better handling of windowOptions.
Clients can override windowOptions by using the IPC-Event (frontend):

import { ipcRenderer } from 'electron';

ipcRenderer.send('set-window-options', {
  frame: false
});

This will add frame: false to the windowOptions. If a property is already specified by the defaults, setting it will override the default:

import { ipcRenderer } from 'electron';

ipcRenderer.send('set-window-options', {
  minWidth: 300
});

// instead of 200 (default)

This PR has the advantage, that the windowState problem described here is not existent because only changes to the default windowOptions will be stored. This PR also enables clients to change these default options through the package.json like this:

"theia": {
  "target": "electron",
  "frontend": {
    "config": {
      "applicationName": "Theia Electron Example",
      "electron": {
        "windowOptions": {
          "frame": false
        }
      }
    }
  }
},

Preferred (top to bottom):

1. Options set through IPC-Event
2. Options set inside package.json
3. Built-in options.

It also moves disallowReloadKeybinding into the new electron subsection:

"theia": {
  "target": "electron",
  "frontend": {
    "config": {
      "applicationName": "Theia Electron Example",
      "electron": {
        "disallowReloadKeybinding": true
      }
    }
  }
},

How to test

Modify the electron example:

"theia": {
  "target": "electron",
  "frontend": {
    "config": {
      "applicationName": "Theia Electron Example",
      "electron": {
        "disallowReloadKeybinding": true,
        "windowOptions": {
          "frame": false
        }
      }
    }
  }
},

If the window starts without a frame and can't be reloaded by using the keybinding (Ctrl+r on windows), the changes work.

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

[owlJaeger]

@thegecko @spoenemann @kittaakos @westbury Any thoughts about this? I prefer this implementation due to the described problems of the old PR.

[vince-fugnitto]

I would prefer a more generic, and extensible solution than always having to modify the generator or including new props when attempting to update electron to suit an application's needs.

@marechal-p has previously worked on #4869 which updates electron-main to make use of inversify so that extenders can potentially use dependency injection to extend the default behavior to suit their needs. He is currently planning on reworking the changes again (as discussed in our dev-meeting)

• (Paul) Rework the Electron main process architecture to allow clients to properly customize the behavior of their desktop application. I want to discuss if anyone has any concerns about such a rework, but I would work on this right away to make it easier for extenders to customize the Electron process. e.g. to permit nicely handle CLI arguments.

I don't think adding new props and customizing the generator each time we need new functionality is the way to go or scales well long-term.

[owlJaeger]

I would prefer a more generic, and extensible solution than always having to modify the generator or including new props when attempting to update electron to suit an application's needs.

@marechal-p has previously worked on #4869 which updates electron-main to make use of inversify so that extenders can potentially use dependency injection to extend the default behavior to suit their needs. He is currently planning on reworking the changes again (as discussed in our dev-meeting)

• (Paul) Rework the Electron main process architecture to allow clients to properly customize the behavior of their desktop application. I want to discuss if anyone has any concerns about such a rework, but I would work on this right away to make it easier for extenders to customize the Electron process. e.g. to permit nicely handle CLI arguments.

I don't think adding new props and customizing the generator each time we need new functionality is the way to go or scales well long-term.

I am looking forward to see these changes land. @marechal-p Do you have some estimate of how long it might take to see these changes? Are you planning on doing these changes on your own?

[paul-marechal]

Until I have something nice I'm fine with the changes you have here.

But I cannot predict exactly if I'll have to undo what you're doing now once I'm done, what I mean is that the API to change those options might change with the refactoring. Is that ok with you?

I'm ok with either PR being a temporary solution.

@akosyakov @thegecko @spoenemann do you have any preferences?

[owlJaeger]

Until I have something nice I'm fine with the changes you have here.

But I cannot predict exactly if I'll have to undo what you're doing now once I'm done, what I mean is that the API to change those options might change with the refactoring. Is that ok with you?

I'm ok with either PR being a temporary solution.

@akosyakov @thegecko @spoenemann do you have any preferences?

I do not have any problems with you discarding these changes as soon as the new API is done. I am looking forward to using it to set the defaults etc. in the client I am working on. So yes, it's ok with me 👍

I'd prefer this PR as this offers more features, flexibility and at the same time less problems with the windowState.

[akosyakov]

Generally customisations should happen via DI, package.json should be used in addition only. I understand though that right now the main electron process is not really well structured and does not have clear lifecycle, so as a temporary solution it should be fine.

[vince-fugnitto]

Is it still necessary now that #7787 is merged?

[owlJaeger]

Is it still necessary now that #7787 is merged?

Yes, I'll create an issue which explains why this should be merged. I had a problem with github, that's the reason I think the other one got merged in the first place. I reviewed the code myself, noting an error. Github showed me that it was visible but it simply wasn't. I noticed it after looking at the page in incognito mode after it was merged... I am sorry for that. This PR reverts the old and implements the right way to handle the windowOptions and the windowState.

[vince-fugnitto]

Is it still necessary now that #7787 is merged?

Yes, I'll create an issue which explains why this should be merged. I had a problem with github, that's the reason I think the other one got merged in the first place. I reviewed the code myself, noting an error. Github showed me that it was visible but it simply wasn't. I noticed it after looking at the page in incognito mode after it was merged... I am sorry for that. This PR reverts the old and implements the right way to handle the windowOptions and the windowState.

Since it is a workaround until the real solution is created and merged (dependency injection), I believe the original approach is sufficient? It is not a common use-case and will probably not require additional workaround improvements.

[owlJaeger]

#7811 explains that downstream projects using the next version of Theia will receive #7787 and thus won't restore the old windowState.

[vince-fugnitto]

I've tested the pull-request as is (without performing additional modifications) and my menu (traffic lights) is completely gone. The same is true after #7787.

Can someone else on macOS confirm this is the case for them?
@akosyakov @kittaakos

Update: I've cleared the cache and it seems to work again properly.

[owlJaeger]

I've tested the pull-request as is (without performing additional modifications) and my menu (traffic lights) is completely gone. The same is true after #7787.

Can someone else on macOS confirm this is the case for them?
@akosyakov @kittaakos

Have you tested the frameless option before? If yes, you may have to clear the cache where electron-store saves its stuff.

[vince-fugnitto]

Have you tested the frameless option before? If yes, you may have to clear the cache where electron-store saves its stuff.

@owlJaeger I just cleared it and saw you posted this :)

[vince-fugnitto]

@owlJaeger do you mind cleaning up the commits (I don't believe such commit granularity is required for such a small change). The revert commit should be separated however (like it is already done).

Nitpick: if possible can you improve the commit descriptions, they don't accurately describe the features in enough detail nor actually mention electron.

[owlJaeger]

@owlJaeger do you mind cleaning up the commits (I don't believe such commit granularity is required for such a small change). The revert commit should be separated however (like it is already done).

Nitpick: if possible can you improve the commit descriptions, they don't accurately describe the features in enough detail nor actually mention electron.

Will do👍

[akosyakov]

@vince-fugnitto Could you finish the review and merge then you think it is good?

[owlJaeger]

@owlJaeger do you mind cleaning up the commits (I don't believe such commit granularity is required for such a small change). The revert commit should be separated however (like it is already done).

Nitpick: if possible can you improve the commit descriptions, they don't accurately describe the features in enough detail nor actually mention electron.

@vince-fugnitto What do you think about the new description?

[vince-fugnitto]

@owlJaeger the changes look good to me, thank you for the contribution! 👍

I confirmed that the pull-request fixes #7811 (the maximized electron state is properly stored upon reloads), and that it is possible for application-developers to configure their respective behavior for windowOptions.

[vince-fugnitto]

@owlJaeger do you mind rebasing the pull-request (conflict with the changelog)?
I'll merge tomorrow if there are no objections :)

[owlJaeger]

Will do 👍

[owlJaeger]

@owlJaeger do you mind rebasing the pull-request (conflict with the changelog)?
I'll merge tomorrow if there are no objections :)

The conflict is resolved and I will check every day so there won't be any conflicts

[vince-fugnitto]

@owlJaeger do you mind rebasing the pull-request (conflict with the changelog)?
I'll merge tomorrow if there are no objections :)

The conflict is resolved and I will check every day so there won't be any conflicts

Sorry, I wasn't notified of the rebased, I'll merge now :) thank you for your contribution.