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

Fix Could not fetch translation json for locale: 'de' #1493

Closed
wants to merge 6 commits into from

Conversation

Nils1729
Copy link

@Nils1729 Nils1729 commented Feb 7, 2024

Fixes #1419.

Checklist

  • Ensure your code works with manual testing
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

In implementation it turned out that it was the opposite problem than in the comment: There is no generic de, only the specific de_DE. I tried to cover this case as generically as possible, although it only occurs with en and de at the time of writing.

Type: defect

@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Feb 7, 2024
@Nils1729
Copy link
Author

Nils1729 commented Feb 7, 2024

I am not really happy with the amount of logging, as every locale that cannot be found produces a stack trace like this:

Changing application language to de
Fetching translation json for locale: de
Could not fetch translation json for locale: 'de' Error: Cannot find module './i18n/strings/de.json'
Require stack:
- /opt/Element/resources/app.asar/lib/language-helper.js
- /opt/Element/resources/app.asar/lib/tray.js
- /opt/Element/resources/app.asar/lib/settings.js
- /opt/Element/resources/app.asar/lib/ipc.js
- /opt/Element/resources/app.asar/lib/electron-main.js
- 
    at node:internal/modules/cjs/loader:1084:15
    at Function._resolveFilename (node:electron/js2c/browser_init:2:116646)
    at node:internal/modules/cjs/loader:929:27
    at Function._load (node:electron/js2c/asar_bundle:2:13327)
    at Module.require (node:internal/modules/cjs/loader:1150:19)
    at require (node:internal/modules/cjs/helpers:121:18)
    at AppLocalization.fetchTranslationJson (/opt/Element/resources/app.asar/lib/language-helper.js:96:20)
    at /opt/Element/resources/app.asar/lib/language-helper.js:109:39
    at Array.filter (<anonymous>)
    at AppLocalization.setAppLocale (/opt/Element/resources/app.asar/lib/language-helper.js:108:86) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/opt/Element/resources/app.asar/lib/language-helper.js',
    '/opt/Element/resources/app.asar/lib/tray.js',
    '/opt/Element/resources/app.asar/lib/settings.js',
    '/opt/Element/resources/app.asar/lib/ipc.js',
    '/opt/Element/resources/app.asar/lib/electron-main.js',
    undefined
  ]
}
Fetching translation json for locale: de_DE
Resetting the UI components after locale change

This only affects en and de. But a stack every time Element launches with one of these locales clutters the journal with non-errors. On the other hand, these missing locales might be errors sometimes. To the reviewer: Is just logging everything acceptable? What are the alternatives?

@Nils1729 Nils1729 marked this pull request as ready for review February 7, 2024 16:37
@Nils1729 Nils1729 requested a review from a team as a code owner February 7, 2024 16:37
@Nils1729 Nils1729 force-pushed the language-variations branch 2 times, most recently from e969eb7 to 0d510d0 Compare February 17, 2024 18:30
Nils Hanff added 3 commits February 21, 2024 10:40
AppLocalization.setAppLocale uses `!!translations` to check if a
locale is avaliable, but `!!{} === true`.
Thus, any localization was considered valid, even `{}`, which was
returned from `AppLocalization.fetchTranslationJson` when no such
locale was found.

This does not happen when we return `null` since it is falsy.

Signed-off-by: Nils Hanff <[email protected]>
The method returns suitable fallbacks in case a locale is not found.

Signed-off-by: Nils Hanff <[email protected]>
const parts = locale.split("-");
if (parts.length > 1) {
parts[1] = parts[1].toUpperCase();
}
return parts.join("_");
}

public fetchTranslationJson(locale: string): Record<string, string> {
public fetchTranslationJson(locale: string): object | null {
Copy link
Member

Choose a reason for hiding this comment

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

object is too loose here, please use a better type

@@ -91,25 +91,35 @@ export class AppLocalization {
this.resetLocalizedUI();
}

// Fallbacks to a locale (e.g. en-gb to [en-gb, en, en-en] or just en to [en, en-en])
private variations(locale: string): string[] {
Copy link
Member

Choose a reason for hiding this comment

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

Can we use getNormalizedLanguageKeys from matrix-web-i18n to remain consistent across the layers?

@Nils1729 Nils1729 marked this pull request as draft March 28, 2024 09:09
@Nils1729
Copy link
Author

Drafting this because it is not critical and seems rather complex. I'll come back once I have some more time at hand.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ Nils Hanff
❌ Nils1729


Nils Hanff seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Nils1729 Nils1729 closed this Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Defect Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Could not fetch translation json for locale: 'de'
3 participants