-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Wait for plugins to deploy before restoring the current language #11472
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution 👍
In order to accept your changes please be sure to sure to sign the eclipse contributor agreement (eca) with the same email as your authorship.
Please also be sure to properly fill out the original pull-request template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that this is the right way to go about this, since it doesn't actually resolve the issue. When getting all available languages, including those which don't have language packs loaded, we will definitely have to deal with the problem of 90% of Theia not being translated.
As far as I can tell this is a race condition, right? So instead, the express endpoint should await the plugin loading process, where all language packs are loaded into memory. This should be done fairly quickly without impacting the load time on Electron.
I can probably contribute a fix for this within the next few days.
This would be true only during the loading time, right? But yes, I would go for the other solution too, I just couldn't find a way to await for the languages loading process. Probably I've just didn't dig deep enough (and also didn't know how waiting for it would affect the Theia loading time) |
Yes, but that's the issue. We already load translations during loading time. The |
@AlbyIanna Sure, something similar should work for the Theia codebase as well 👍 Don't forget to sign the ECA by the way. |
1c33a9e
to
af1e63f
Compare
@msujew done 👌 |
1c2a946
to
dd1c8d8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlbyIanna With "something similar" I didn't mean the exact same approach ;) Given our architecture, you can't simply import code from the plugin-ext
package into the core
package. You'll have to create a dedicated override in the plugin-ext
package for the LocalizationBackendContribution
and bind it there.
Also we can't accept your changes when you haven't signed the ECA. Please do so.
@msujew I'm not sure how to proceed here. Could you please provide me with an example of how to do that?
I should have done it now. Can you please confirm I did it right? |
dd1c8d8
to
af13193
Compare
Sure, I recently did something similar: There is a
Are you sure you did it with the mail that's in the commit? It doesn't seem to validate. |
Signed-off-by: Alberto Iannaccone <[email protected]>
af13193
to
a664a95
Compare
Alright, cool. It seems to work now. No idea why it didn't work previously. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few linting errors across the changes. You should probably enable the ESLint
vscode extension so you can catch those during development.
packages/plugin-ext/src/main/node/plugin-localization-backend-contribution.ts
Outdated
Show resolved
Hide resolved
packages/plugin-ext/src/main/node/plugin-localization-backend-contribution.ts
Outdated
Show resolved
Hide resolved
@msujew thanks for the feedback. In the last commit I've made some changes according to your requests 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, just one change and this should be good to go 👍
Also, just for completeness' sake, do you mind filling out the PR template?
packages/core/src/node/i18n/localization-backend-contribution.ts
Outdated
Show resolved
Hide resolved
packages/plugin-ext/src/main/node/plugin-localization-backend-contribution.ts
Outdated
Show resolved
Hide resolved
packages/plugin-ext/src/main/node/plugin-localization-backend-contribution.ts
Outdated
Show resolved
Hide resolved
@AlbyIanna Are you still interested in contributing the changes? I still have a few outstanding comments before approving this. |
Hi @msujew, thanks for the review. Sorry for the delay in answering, I must have missed the notification 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me 👍
Please address the linting issues (see below) and then I'll merge this.
packages/plugin-ext/src/main/node/plugin-localization-backend-contribution.ts
Outdated
Show resolved
Hide resolved
6044ff3
to
2b64b48
Compare
2b64b48
to
452eca6
Compare
What it does
Wait for the plugins to be deployed before restoring the current language.
Fixes #11471
How to test
de
as the defaultde
de
✅zh-cn
as the default languagezh-cn
✅zh-cn
✅Review checklist
Reminder for reviewers