Skip to content
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

Translate monaco using default keys #10946

Merged
merged 5 commits into from
Mar 30, 2022
Merged

Translate monaco using default keys #10946

merged 5 commits into from
Mar 30, 2022

Conversation

msujew
Copy link
Member

@msujew msujew commented Mar 29, 2022

What it does

Now that #10736 has been merged, our previous method of translating monaco doesn't work anymore. Instead it can be accomplished way easier :)

Before loading any monaco related code, we simply override the localize function to delegate to our nls.localize function. This doesn't work in all cases (210 of 4200 strings remain untranslated this way), but most of these aren't visible to users anyway. I've made the getDefaultKey function a bit more lenient to support labels which have only changed in their capitalization. For example VSCode changed from Previous match to Previous Match, which should still be translated correctly.

Also updates some generated nls.localizeByDefault calls which weren't working correctly. I updated the preference extractor for that. It seems like the eslint rule doesn't pick these up for some reason. Really weird :/

I will probably come back to this next month and translate the missing pieces as well.

How to test

  1. Running Theia in English shouldn't affect any strings displayed by Monaco
  2. Download and install a language pack of your choice
  3. Open the editor and associated menus/widgets
  4. Confirm that most of the user visible labels are translated.

Review checklist

Reminder for reviewers

@msujew msujew added monaco issues related to monaco localization issues related to localization/internalization/nls labels Mar 29, 2022
@colin-grant-work
Copy link
Contributor

Also updates some generated nls.localizeByDefault calls which weren't working correctly. I updated the preference extractor for that. It seems like the eslint rule doesn't pick these up for some reason. Really weird :/

I think it's not picking them up because I disabled the rule for the file:

/* eslint-disable @typescript-eslint/quotes,max-len,@theia/localization-check,no-null/no-null */

/* eslint-disable @typescript-eslint/quotes,max-len,@theia/localization-check,no-null/no-null */

Following our discussion on the Monaco PR.

If you've now fixed the cases that need fixing, you can enable the rule for the file.

@msujew
Copy link
Member Author

msujew commented Mar 29, 2022

@colin-grant-work Ah, thanks, I didn't notice that. I updated the generator/generated file accordingly.

Copy link
Contributor

@colin-grant-work colin-grant-work left a 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.

Before:

image

After:

image

Mostly unrelated to this issue, @msujew, what's the status of translations of plugins? Looking at TS error messages, I get this in VSCode:

image

and this in Theia:

image

The TS versions are different, which accounts for some of the difference, but in Theia, it looks like the TS message isn't translated, but in VSCode, it is?

@msujew
Copy link
Member Author

msujew commented Mar 29, 2022

@colin-grant-work Good catch, I actually didn't bother to test with the TypeScript extension. Our vscode.env.language value that we pass to the extension host is incorrect. It should be the currently selected locale nls.locale, but it's navigator.language instead.

I sneaked another change into this PR, if you care you can check this feature again :)

Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the new changes, plugin text is also correctly localized:

image

@paul-marechal
Copy link
Member

paul-marechal commented Mar 30, 2022

@msujew I just pushed a commit to fix some of the issues I was having with understanding the code...

Reading monaco-editor-preference-extractor.ts still gives me the heebie jeebies, but it's less worse for me now.

edit: I also renamed deQuote... to dequote... as we usually do this with words like unregister or deregister.

@colin-grant-work
Copy link
Contributor

colin-grant-work commented Mar 30, 2022

@paul-marechal, I somewhat disprefer the change. The new class is setting state based on things that are happening outside of it. Previously, the main class was tracking state based on its progress through the stringification of the list of preferences. @msujew, one option would be to do the stringification of the properties inside the for loop that builds the interfaceEntries array. Inside that loop, we know the name of the preference, so we could pass that into the replace function without tracking it as state on the outer object, and we could build the text as we do for the interfaceEntries and then insert that text into the template (the formatting would be tricky, though, probably). That would obviate the state tracking that's bothering @paul-marechal. What do you think of that approach. I'm also fine with either the new or the old code, with a slight preference for the old code.

@paul-marechal
Copy link
Member

paul-marechal commented Mar 30, 2022

Previously, the main class was tracking state based on its progress through the stringification of the list of preferences.

@colin-grant-work This should still be the current logic. I'm assuming that the command extract-editor-preference-schema is only executed once and state is local to that call. The stateful instance is discarded once the function has ended.

@msujew
Copy link
Member Author

msujew commented Mar 30, 2022

I'm fine with either implementation. @colin-grant-work @paul-marechal If you don't see any critical issues in this PR, I would merge it tomorrow morning, so we definitely get it into the release.

Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, the generated file is the same after the new changes.

@paul-marechal
Copy link
Member

@msujew you can merge anytime, you don't have to wait tomorrow morning :)

@msujew msujew merged commit da55b6a into master Mar 30, 2022
@msujew msujew deleted the msujew/monaco-nls branch March 30, 2022 18:47
@github-actions github-actions bot added this to the 1.24.0 milestone Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
localization issues related to localization/internalization/nls monaco issues related to monaco
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants