[core] Add and use application name

[paul-marechal]

Add customizable application name.
Display application name when no workspace is opened, instead of showing
the URL. It was particulary bad on Electron.

Remove native menus when creating a new Electron BrowserWindow, which will be
re-added by the application when ready.

Signed-off-by: marechal-p paul.marechal@ericsson.com

[elaihau]

looks good to me, except for that email :)

[vince-fugnitto]

it lworks and looks great @marechal-p, perhaps we can simply display Theia when no workspace is present instead of <No Workspace> - Theia? I know vscode does something similar, namely:

• Visual Studio Code for no workspaces.
• ${Workspace Name} - Visual Studio Code when a workspace is present.

[vince-fugnitto]

thanks @marechal-p works and looks great!

[akosyakov]

@kittaakos can you have a look

I feel a bit unsure since it does additional remote calls at runtime to fetch information which actually is available at the build time. Maybe it should be mixed into frontend config to avoid fetching what is already known.

[paul-marechal]

@akosyakov it was my concern, part of why I waited to merge.

Is there a cache of the props already, or should I make one?

[kittaakos]

I feel a bit unsure since it does additional remote calls at runtime to fetch information which actually is available at the build time. Maybe it should be mixed into frontend config to avoid fetching what is already known.

I am OK with both approaches. The build-time solution is faster, no questions at all. But fetching the application name once when setting the workspace also seems OK.

[kittaakos]

It works nicely. @akosyakov, if you prefer having the application name at build time instead of fetching it, please request the change.

[paul-marechal]

TBH I would prefer having the applicationName set at build time too, but while I know how to write something inside the generated sources (modifying the generators), I fail to see a clean way of fetching the value afterward, from any Theia component...

How would you go about it? @akosyakov @kittaakos

[kittaakos]

How would you go about it? @akosyakov @kittaakos

Let's spare @akosyakov for the review ;)

I can take a look but then, let's fuse this PR with #3459. What do you think, @marechal-p?

[paul-marechal]

I'll fuze them now

[kittaakos]

I will create my patch on the generate-app-name-patch branch which is a branch from your mp/startup-window-title.

[kittaakos]

@marechal-p, I could not create a PR against your branch (due to the force push) but here is the commit: 9aa985d.

It worked without a problem locally. Let me know if you have questions.

[paul-marechal]

I didn't know about that FrontendApplicationConfigProvider, thank you very much :)

Do you allow me to simply fuze your changes inside this PR?

[kittaakos]

Sure. Use as much as you need.

[paul-marechal]

@kittaakos updated everything accordingly.

• I used the build time string like advised.
• I changed the initial setting of the menus to an empty template instead of null because it wasn't working on OSX, it now does.
• Also fixed menu instantiation under OSX to handle the case where we can only set the whole application menu globally, so I hooked the menu creation to each Electron BrowserWindow on the focus event. Tested on our OSX machine and it seemed to work, if anyone wants to also give it a try.

[kittaakos]

• I used the build time string like advised.

👍

• instead of null because it wasn't working on OSX, it now does.

I am a bit puzzled; your change was already approved twice: #3445 (review) and #3445 (review) and you are saying; it works now. I still do not understand what does this fix.

• handle the case where we can only set the whole application menu globally

I do not understand this.

• if anyone wants to also give it a try.

I would like to. What was the original issue?

[paul-marechal]

• I changed the initial setting of the menus to an empty template instead of null because it wasn't working on OSX, it now does.

I am a bit puzzled; your change was already approved twice: #3445 (review) and #3445 (review) and you are saying; it works now. I still do not understand what does this fix.

Exactly what I said. Despite being approved already, it wasn't working on OSX when I tested after (I remembered we had a OSX machine to test on). So I covered that case. It now works on Ubuntu/Windows/OSX. Now correctly hides the default electron menus until the application sets it up.

• Also fixed menu instantiation under OSX to handle the case where we can only set the whole application menu globally, so I hooked the menu creation to each Electron BrowserWindow on the focus event. Tested on our OSX machine and it seemed to work, if anyone wants to also give it a try.

Well, again, on Ubuntu/Windows, we can set the menus on a per-window basis. But not on OSX, where the menu bar is unique and global to all windows, and lives at the top of the screen. Problem is, when you have two electron windows opened, and only one menu bar (OSX), in what window should New Terminal open a new terminal in? So my fix sets the "global" OSX menus each time you change windows so that it will target the one you will use, because it is focused.

But I will move that menu change to an other PR, no issues :)

[kittaakos]

Problem is, when you have two electron windows opened, and only one menu bar (OSX), in what window should New Terminal open a new terminal in? So my fix sets the "global" OSX menus each time you change windows so that it will target the one you will use, because it is focused.

Thank you for taking the time and explaining it to me again. It makes sense.

[paul-marechal]

@kittaakos it should be better now :)

[paul-marechal]

Ok, I didn't see the failing tests, still addressing that :(

[paul-marechal]

@kittaakos I know this PR is taking longer than it should to get ready, but there is a last point where I want your opinion:

Currently, the application configuration requires us to define the package.json in the following way:

{
  "theia": {
    "frontend": {
      "config": {
        "applicationName": "Theia Browser Example"
      }
    }
  }
}

Wouldn't it be better to have the application name as a "global", along with the target?

{
  "theia": {
    "applicationName": "Theia Browser Example"
  }
}

It sounds rather specific to have it in the frontend config member. On the other hand, only the frontend could care about that, because it is only in the frontend logic that we would try to display this name. The backend would hardly need such a string, or I'm just failing to find a use case for it.

Other than that I'm happy with how the changes are right now, feel free check if everything is ready to merge :)

[kittaakos]

Wouldn't it be better to have the application name as a "global", along with the target?

I would leave as is.

Other than that I'm happy with how the changes are right now, feel free check if everything is ready to merge :)

It looks great! I'll try it out once more on Windows and merge the changes.