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

Support for vscode l10n API #12192

Merged
merged 3 commits into from
Apr 20, 2023
Merged

Support for vscode l10n API #12192

merged 3 commits into from
Apr 20, 2023

Conversation

msujew
Copy link
Member

@msujew msujew commented Feb 17, 2023

What it does

Closes #11893

VSCode 1.73 not only brought the new l10n namespace with it, but also changed the way language packs for vscode built-ins work. Theia now supports both old and new language packs and extension localization APIs (l10n and vscode-nls).

How to test

You will need 4 extensions to test this feature:

  1. Install l10n-test and execute the l10n:* commands.
    1. The Hello World command will create a notification with "Hello World!"
    2. The Show URI command will show UNDEFINED
    3. The Show Bundle command will also show undefined.
  2. Install the German language pack and change the locale to German/de
  3. Repeat step 1 (search for l10n, the command names are localized)
    1. The Hello World command will create a notification with "Hallo Welt!"
    2. The Show URI command will show the file uri to the German language bundle of the l10n-test extension
    3. The Show Bundle command will display a JSON containing the language bundle content
  4. Delete the builtin css-language-features extension
  5. Install the provided css and css-language-features extensions.
  6. Open a CSS file, and hover over a selector. It should show Selektorspezifität (German) instead of Selector Specificity (English) in the hover. This step asserts that extensions can still be localized using language packs.

Also make sure to test for regressions with existing language pack functionality:

  1. Confirm that old (< 1.73 API version) language packs still work as expected.
  2. Confirm that this also works in conjunction with localized built-in extensions (such as the typescript-language-features extension)
  3. Deinstalling language packs and consequently reloading the application should actually delete all contributed localizations/language pack bundles.

Review checklist

Reminder for reviewers

@msujew msujew added vscode issues related to VSCode compatibility localization issues related to localization/internalization/nls labels Feb 17, 2023
@tsmaeder
Copy link
Contributor

@msujew I was finding the documentation on the new way of localizing things a bit sparse in https://code.visualstudio.com/api/references/vscode-api#l10n. Did you have any other sources when implementing this?

@msujew
Copy link
Member Author

msujew commented Feb 23, 2023

I was finding the documentation on the new way of localizing things a bit sparse

Just imagine the fun I had when I had to reverse-engineer the vscode-nls package for #10087 🙃. Compared to that, the new l10n API is fairly well thought out.

But to answer the question: No, it's mostly based on reverse engineering sources of vscode and the JSON files that are the deliverables of the language packs.

@tsmaeder
Copy link
Contributor

@msujew since the test suite has been "deflaked" recently, could you rebase to see if the suite passes?

Copy link
Contributor

@tsmaeder tsmaeder left a comment

Choose a reason for hiding this comment

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

First reading of the code.

@tsmaeder
Copy link
Contributor

When I execute step 1.iii in your testing instructions, I get a stack trace in the back end console:

2023-02-24T13:28:25.372Z root ERROR [hosted-plugin: 31348] Promise rejection not handled in one second: TypeError: Cannot read properties of undefined (reading 'replace') , reason: TypeError: Cannot read properties of undefined (reading 'replace')
With stack trace: TypeError: Cannot read properties of undefined (reading 'replace')
    at NotificationContentRenderer.renderMessage (file:///C:/Users/thomas/code/EclipseSource/theia/examples/electron/lib/packages_messages_lib_browser_messages-frontend-module_js.js:651:48)
    at NotificationManager.showMessage (file:///C:/Users/thomas/code/EclipseSource/theia/examples/electron/lib/packages_messages_lib_browser_messages-frontend-module_js.js:1223:50)
    at MessageService.processMessage (file:///C:/Users/thomas/code/EclipseSource/theia/examples/electron/lib/bundle.js:140158:28)
    at MessageService.info (file:///C:/Users/thomas/code/EclipseSource/theia/examples/electron/lib/bundle.js:140138:21)
    at MessageRegistryMainImpl.doShowMessage (file:///C:/Users/thomas/code/EclipseSource/theia/examples/electron/lib/packages_plugin-ext_lib_hosted_browser_hosted-plugin_js.js:7362:44)
    at MessageRegistryMainImpl.$showMessage (file:///C:/Users/thomas/code/EclipseSource/theia/examples/electron/lib/packages_plugin-ext_lib_hosted_browser_hosted-plugin_js.js:7346:35)
    at file:///C:/Users/thomas/code/EclipseSource/theia/examples/electron/lib/packages_plugin-ext_lib_common_disposable-util_js-packages_plugin-ext_lib_common_index_js-pac-d19cb9.js:1301:71
    at async RpcProtocol.handleRequest (file:///C:/Users/thomas/code/EclipseSource/theia/examples/electron/lib/bundle.js:139769:28)

@tsmaeder
Copy link
Contributor

Hovering over a selector never shows a hover for me: instead I see "Activating CSS Sprachfeatures" in the status bar and the "busy"-indicator keeps rotating.

@msujew
Copy link
Member Author

msujew commented Feb 24, 2023

@tsmaeder This extension code snippet probably isn't doing what it's supposed to do. It seems like JSON.stringify(undefined) yields undefined even though its signature says it always returns a string...

Anyway, the test step is ultimately just to prove that the API yields undefined, which it seems to do.

@msujew
Copy link
Member Author

msujew commented Feb 24, 2023

Hovering over a selector never shows a hover for me: instead I see "Activating CSS Sprachfeatures" in the status bar and the "busy"-indicator keeps rotating.

Oh, maybe I missed a testing step. Did you delete the built-in extension in the plugins folder of your Theia repo first? It works for me:

image

@tsmaeder
Copy link
Contributor

tsmaeder commented Feb 24, 2023

Did you delete the built-in extension in the plugins folder of your Theia repo first?

Yes I did. I'm willing to believe that there is a caching problem with exploded extensions, though. Lemme check.
Also, I'm on Windows.

@tsmaeder
Copy link
Contributor

Nope, this is not working for me: I do get the hover with "Selector Specificity" when the language is English, but I don't get a hover at all when the language is German.

@msujew
Copy link
Member Author

msujew commented Feb 27, 2023

@tsmaeder The CSS extension exhibited some weird race condition when using a non-english locale. I've fixed the race condition, please download the new version to test it :)

@msujew
Copy link
Member Author

msujew commented Feb 27, 2023

I can reproduce the issue experienced by @tsmaeder both on Electron and browser: The source of the issue is the old node version used in Electron. We currently use 16.5.0, which exhibits some unexpected behavior in regards to event emitters on processes. Using node > 16.14.0 (or upgrading Electron) fixes the issue.

@tsmaeder
Copy link
Contributor

@msujew to make this clear: with the current set of built-ins, we should be fine, right? Can you tell which version of the built-ins would start misbehaving (giving us a deadline for updating electron)?

@msujew
Copy link
Member Author

msujew commented Feb 28, 2023

@tsmaeder The same version that this API has been released with, 1.73.0.

@tsmaeder
Copy link
Contributor

@msujew if I understand correctly, there is nothing holding us back from merging this PR (except an approval), right? The trick is not to update the VS Code API level, right?

@msujew
Copy link
Member Author

msujew commented Apr 19, 2023

@tsmaeder Yep, that's right.

Copy link
Contributor

@tsmaeder tsmaeder left a comment

Choose a reason for hiding this comment

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

I rebased on master and tested with electron and now the tests related to css files work for me.

@msujew msujew merged commit a9471e7 into master Apr 20, 2023
@msujew msujew deleted the msujew/l10n-vscode-api branch April 20, 2023 09:49
This was referenced Nov 27, 2023
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 vscode issues related to VSCode compatibility
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

vscode: add support for the l10n namespace
3 participants