Fix current IDE language on start-up #1261

AlbyIanna · 2022-07-27T17:54:09Z

Motivation

I noticed the root of the problem is here:

https://github.com/eclipse-theia/theia/blob/f4327c2f0c0f47f960a302590a51698879357841/packages/core/src/node/i18n/localization-backend-contribution.ts#L38-L39

req.params.locale is always restored correctly, but this.localizationProvider.getAvailableLanguages() doesn't return all the available languages. This happens because getAvailableLanguages() returns all the registered localizations with languagePack set to true (unless we call getAvailableLanguages(true), in that case, it would always return all the languages):

https://github.com/eclipse-theia/theia/blob/f4327c2f0c0f47f960a302590a51698879357841/packages/core/src/node/i18n/localization-provider.ts#L50

The weird thing is that this happens also with some languages registered with a language pack (so they'll eventually have languagePack set to true). I say some languages because some others work as expected. I strongly believe this is happening because of a race condition happening because getAvailableLanguages is called before the language packs are finished loading.

EDIT:
The root cause of this appears to be that PluginDeployer completes the execution of deployPlugins after the express backend tries to restore the current language

https://github.com/eclipse-theia/theia/blob/26649452aeac342f7551b8cae502740799085772/packages/plugin-ext/src/main/node/plugin-deployer-impl.ts#L269

The solution I've found is to wait for this event to emit

Change description

Re-implemented the restoration of the language in localization-backend-contribution.ts

Wait for the plugins to complete the deploy before restoring the current language:
https://github.com/arduino/arduino-ide/pull/1261/files#diff-19d891f1ffb8ce2979b6bee0d166700e20a6b965d1be5f72b3e331d0442eba35R16-R18

Other information

This PR is a workaround for a Theia issue.
I opened an issue in the Theia repo here: eclipse-theia/theia#11471
As soon as Theia fixes it, we can remove this code and the rebinding.

I've also opened a PR on Theia here: eclipse-theia/theia#11472

Fixes #1219

Reviewer checklist

PR addresses a single concern.
The PR has no duplicates (please search among the Pull Requests before creating one)
PR title and description are properly filled.
Docs have been added / updated (for bug fixes / features)

kittaakos

It does not work for me. I tried the steps from #1219.

language_fix_verification.mp4

AlbyIanna · 2022-07-28T14:25:53Z

Thanks for testing it @kittaakos. I've just updated a PR with a new fix, now it should work.

kittaakos

Besides a few remarks, it's looking great. Please react to them, and I will do the review afterward with the final changeset. Thank you!

kittaakos · 2022-07-28T15:45:23Z

arduino-ide-extension/src/node/i18n/localization-backend-contribution.ts

+@injectable()
+export class LocalizationBackendContribution extends TheiaLocalizationBackendContribution {
+  @inject(PluginDeployer)
+  protected readonly pluginDeployer: PluginDeployerImpl;


can be private.

kittaakos · 2022-07-28T15:45:33Z

arduino-ide-extension/src/node/i18n/localization-backend-contribution.ts

+  @inject(PluginDeployer)
+  protected readonly pluginDeployer: PluginDeployerImpl;
+
+  private initialized = new Deferred<void>();


should be readonly

kittaakos · 2022-07-28T15:46:26Z

arduino-ide-extension/src/node/i18n/localization-backend-contribution.ts

+    this.pluginDeployer.onDidDeploy(() => {
+      this.initialized.resolve();
+    });
+    await this.localizationRegistry.initialize();


Why not calling super.initialize(). The default Theia behavior is what IDE2 wants to use besides the listener.

kittaakos · 2022-07-28T15:48:45Z

arduino-ide-extension/src/node/i18n/localization-backend-contribution.ts

+  private initialized = new Deferred<void>();
+
+  override async initialize(): Promise<void> {
+    this.pluginDeployer.onDidDeploy(() => {


It's an OK solution for now, but the deferred promise will never resolve if the plugins are deployed by the time this initialize function is called.

@kittaakos is that a likely scenario? (getting to this initialize post deploy)

No, I do not think so.

Based on the method names (initialize and start), I do not think it will cause an issue at runtime, but I did not thoroughly check the Theia code.

@AlbyIanna lets keep note of this, maybe we'll get a hint here anyway.

kittaakos · 2022-07-28T15:49:36Z

arduino-ide-extension/src/node/i18n/localization-backend-contribution.ts

+        Waiting for the deploy of the language plugins is neecessary to avoid checking the available
+        languages before they're finished to be loaded: https://github.com/eclipse-theia/theia/issues/11471 
+      */
+      await this.initialized.promise;


Just wondering, did you check if it slows down the IDE2 startup? The frontend must wait for this, right?

I'm not sure how to calculate this.

With the current build, I can see these two log lines:

root INFO [2c0ce3a6-ca5a-4f40-b831-533e06823a9f] Load contributions of 16 plugins: 86.5 ms [Finished 4.873 s after frontend start] root INFO [2c0ce3a6-ca5a-4f40-b831-533e06823a9f] Start of 16 plugins: 575.4 ms [Finished 5.460 s after frontend start]

Then, in this case, I think the frontend is slowed down by 86.5 ms + 574.4 ms = 660.9 ms

Am I correct?

const start = performance.now(); await this.initialized.promise; console.log('took: ' + (performance.now() - start) + ' ms')

root INFO took: 405.16497199982405 ms

Quite significant. I suppose this delay will increase as we keep adding new language packs.

This should be done fairly quickly without impacting the load time on Electron

from: here

I've pushed the code to log the performance so that we see how the time grows as we keep adding new language packs.

kittaakos · 2022-07-29T09:30:00Z

arduino-ide-extension/src/node/i18n/localization-backend-contribution.ts

+    this.pluginDeployer.onDidDeploy(() => {
+      this.initialized.resolve();
+    });
+    super.initialize();


This is a bug. You either must return with super.initialize() or await for it.

kittaakos

This is working great ✅

Thank you so much for tracking down the bug upstream, opening an issue, and proposing fix for the Theia community. Excellent effort 💪

davegarthsimpson

Nice one @AlbyIanna

AlbyIanna requested review from kittaakos and davegarthsimpson July 27, 2022 17:54

kittaakos suggested changes Jul 28, 2022

View reviewed changes

AlbyIanna force-pushed the fix-current-language branch from 4ddf0a0 to 6a767ea Compare July 28, 2022 14:22

AlbyIanna changed the title ~~Fix IDE current language on restart~~ Fix current IDE language on start-up Jul 28, 2022

AlbyIanna requested a review from kittaakos July 28, 2022 15:33

kittaakos reviewed Jul 28, 2022

View reviewed changes

AlbyIanna force-pushed the fix-current-language branch from 6a767ea to c7ed2a2 Compare July 29, 2022 07:37

AlbyIanna requested a review from kittaakos July 29, 2022 08:29

AlbyIanna force-pushed the fix-current-language branch from c7ed2a2 to 7ef1f02 Compare July 29, 2022 08:30

kittaakos reviewed Jul 29, 2022

View reviewed changes

wait for language packs to be deployed

f613397

AlbyIanna requested a review from kittaakos July 29, 2022 09:48

AlbyIanna force-pushed the fix-current-language branch from 7ef1f02 to f613397 Compare July 29, 2022 10:13

kittaakos approved these changes Jul 29, 2022

View reviewed changes

davegarthsimpson approved these changes Jul 29, 2022

View reviewed changes

AlbyIanna merged commit 124738d into main Jul 29, 2022

AlbyIanna deleted the fix-current-language branch July 29, 2022 13:08

per1234 added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Jul 30, 2022

kittaakos mentioned this pull request Nov 11, 2022

chore: Updated to Theia 1.31.1 #1662

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix current IDE language on start-up #1261

Fix current IDE language on start-up #1261

AlbyIanna commented Jul 27, 2022 •

edited

Loading

kittaakos left a comment

AlbyIanna commented Jul 28, 2022

kittaakos left a comment

kittaakos Jul 28, 2022

kittaakos Jul 28, 2022

kittaakos Jul 28, 2022

kittaakos Jul 28, 2022

davegarthsimpson Jul 28, 2022

kittaakos Jul 28, 2022

davegarthsimpson Jul 29, 2022

kittaakos Jul 28, 2022

AlbyIanna Jul 29, 2022

kittaakos Jul 29, 2022

AlbyIanna Jul 29, 2022

davegarthsimpson Jul 29, 2022 •

edited

Loading

AlbyIanna Jul 29, 2022

kittaakos Jul 29, 2022 •

edited

Loading

kittaakos left a comment

davegarthsimpson left a comment

Fix current IDE language on start-up #1261

Fix current IDE language on start-up #1261

Conversation

AlbyIanna commented Jul 27, 2022 • edited Loading

Motivation

Change description

Other information

Reviewer checklist

kittaakos left a comment

Choose a reason for hiding this comment

AlbyIanna commented Jul 28, 2022

kittaakos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davegarthsimpson Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kittaakos Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

kittaakos left a comment

Choose a reason for hiding this comment

davegarthsimpson left a comment

Choose a reason for hiding this comment

AlbyIanna commented Jul 27, 2022 •

edited

Loading

davegarthsimpson Jul 29, 2022 •

edited

Loading

kittaakos Jul 29, 2022 •

edited

Loading