-
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
Implement internationalization support #9538
Conversation
// Prefix the command label with the category if it exists, else return the simple label. | ||
return command.category ? `${command.category}: ${command.label}` : command.label; | ||
return command.category ? `${command.category}: ${label}` : label; |
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 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.
Totally forgot about those. Done. You can test this with the commands in the File
category.
Also, you reminded me that vscode shows the original text under the localized text. I implemented this as well.
73a225f
to
a2e31ae
Compare
A general observation: as this is currently implemented, a lot of services need to know about it or opt into it. For example: export function compareCommands(a: Command, b: Command): number {
if (a.label && b.label) {
const aLabel = LocalizationInfo.localize(a.label);
const bLabel = LocalizationInfo.localize(b.label);
const aCommand = (a.category ? `${LocalizationInfo.localize(a.category)}: ${aLabel}` : aLabel).toLowerCase();
const bCommand = (b.category ? `${LocalizationInfo.localize(b.category)}: ${bLabel}` : bLabel).toLowerCase();
return (aCommand).localeCompare(bCommand);
} else {
return 0;
}
} The commands module, in order to implement a simple utility, needs to know that it may have to translate the input, and the same will need to be done everywhere we touch a string that might ever want to be localized - and at the limit, that's basically anywhere a string is handled. By contrast, VSCode's solution looks like it happens at load time, and thereafter the application can behave as though it's dealing with fixed strings, but you have to reload the application to apply a new localization. In general, we can't make very many assumptions about what we know at load time, so we may not be able to achieve the same level of parsimony that VSCode has, but I wonder if there's an architectural alternative that reduces the impact relative to the current approach. |
Hi @colin-grant-work, thanks for the input. I wasn't too happy about the |
@msujew Serving things over regular HTTP is not completely crazy if we are talking about fetching data (think stateless things like REST API). Remote logic with side effects and state are usually better encapsulated with the RPC system. So it sounds fine here. On the other hand, the way you defined the express route is pretty weird:
|
@paul-marechal True, if we look at it from a pure fetching perspective, that makes more sense. I always had the additional overhead of storing the locale in the backend (for the purpose of localizing vscode extensions) in the back of my mind. However, just fetching Theias own localization is completely independent of that, so doing this through an http call would be appropriate. For everything with else we could still use the RPC based API. And yes, I just wanted to have a quick and dirty solution for presenting the express based approach. Implementing all of that as a proper I'll refactor my original code and come back to you. |
8acb1dc
to
3543dc6
Compare
@paul-marechal I implemented everything with a |
6a72b3c
to
8db5529
Compare
I just implemented a vscode style However, it increases maintenance efforts and is a breaking change. WDYT, is that a feature we want to support in Theia? Or do we rather use something like |
loadTranslations
is awaited before importing dependent source files.
I actually didn't like my first approach because of the breaking changes it introduces. Therefore I already implemented the |
6c80000
to
6913978
Compare
@iamtangram I'm currently waiting for further reviews. If you're so eager for this feature to release, you can join the discussion as well, discussing open questions that I posted in the initial PR comment or doing a review of my code ;) Any input - even from non-commiters of Theia - is greatly appreciated. |
Sure, I just created #9708 where we can discuss anything related to that. |
@msujew I see there are currently merge conflicts - it could be a good idea to rebase the PR on latest master? |
We need be careful about integrating an other person's changes within a pull-request - any contribution should be done "above board", without obfuscating the provenance/authorship, and are subject to the usual Eclipse Foundation rules. For example: When the contributor has signed the Eclipse Contributor Agreement (ECA) and the following conditions are met, a CQ is not required:
See here for more details. |
packages/core/src/node/i18n/localization-backend-contribution.ts
Outdated
Show resolved
Hide resolved
@tsmaeder I wrote a small proposal for the coding guidelines: I18n
1.1. Parameters for messages should be passed as the
// bad
command: Command = { label: nls.localize(key, defaultValue), originalLabel: defaultValue };
// good
command = Command.toLocalizedCommand({ id: key, label: defaultValue }); |
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.
LGTM :-)
Since the last requests have been resolved by @msujew and no more have been received for about two weeks, I think it's okay to approve it. If there are no further objections, we will merge it next week.
FYI, I will drop the german custom translation before merging this PR and follow up with another PR that includes all translations contributed by the vscode language packs. Or would you rather see this as a part of this PR? We would like to include everything in the August release if possible. |
@msujew I agree on having two separate PRs, one for the actual code implementation and another for the translations. This will reduce the noise in this one. |
Can you point to the code that is the cause of loading everything in memory?
Sounds like state that should live in the frontend only?
I was about to comment on this. If the plan is to not support this yet then OK to merge. |
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 followed the "How to test" steps and everything worked.
Code LGTM, although regarding the various pending issues I am fine with merging this PR before the upcoming release if we are ready to potentially make breaking changes, as required to add/fix the rest later.
We load everthing here: theia/packages/plugin-ext/src/hosted/node/hosted-plugin-deployer-handler.ts Lines 137 to 139 in 44fec6f
I agree. There's two issues with that though:
We can work around both points I guess. For 1. we only need to know the selected language on startup, and forget about it afterwards. This is basically how it works already, so we don't have to change anything. For 2. we would need to only send the translation keys to the frontend, which will then translate the message on its own. This is quite unintuitive though.
This was fixed by #9928. I'll rebase everything, and then that'll work out of the box. |
@paul-marechal After rebasing filtering by the original command name is now possible. The only command still getting translated is the |
What it does
Closes #1327
Adds internationalization support for Theia. The following features are supported:
package.json
and runtime translations vianls.localize()
)LocalizationContribution
interface.Things that are out of scope for this PR (and will probably be addressed in another PR):
How it works
core/browser
package contains anls
namespace with alocalize
function similiar to vscode. It accepts a translation key, a default value (usually the original english value) and additional args. The translations are loaded before any other imports so that they are available during load-time of any other code.LocalizationContribution
s and places them in aLocalizationProvider
. Additionally, the backend exposes a/i18n/<locale>
HTTP endpoint to retrieve localizations for any locale.monaco-loader.ts
uses the frontend locale as well to configure monaco to use the currently active language.How to test
Configure display language
command can be invoked but only displaysen
(even though german translations exist in the backend, but no language pack is installed)german
in the Theia extension menu)Hello
andBye
command contributed by the i18n sample extension. Those will simply showHello
andBye
respectively in a notification.Configure Display Language
command and choose the german localede
.Hallo
(Hello) andAuf Wiedersehen
(Bye) command, which will now display german text as a notification. (Test for vscode extensions to correctly localize themselves and for Theia to read the translatedpackage.json
correctly)Download
menu item has been replaced withHerunterladen
. This was done with a germanLocalizationContribution
.editor.main.nls.de.js
in theexamples/browser/lib/vs/editor
source directory with this file. Rungzip -k editor.main.nls.de.js
in that directory. Reloading the Theia application and usingde
locale will now show german translation in the monaco search widget (Ctrl+F
) as well as in the context menu. This is a workaround, see below for more information.Open Issues
@theia/monaco-editor-core
package contains only english translations. Is this done on purpose? Replacing thenls.*.json
files with the correct translations will translate monaco correctly.de
,it
,fr
...), but makes it virtually impossible for vscode language packs to actually contribute to monaco's translation process.LocalizationProvider
holds a single active language for the whole application. Is there a way to hold a value for every user?Review checklist
Reminder for reviewers