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

feat(web): translations #9854

Merged
merged 35 commits into from
Jun 4, 2024
Merged

feat(web): translations #9854

merged 35 commits into from
Jun 4, 2024

Conversation

Manic-87
Copy link
Contributor

See Discussion #1688

Ignore the hooks.server.ts and the changes to vite.config.js.
The en-EN.json I created manually.
The de-DE.json I created initially manually but it was filled by weblate.
fr.json was created by weblate.
What you see in those files is everything I entered into weblate just to test if it works.

Copy of my comment from the discussion:

So I did invest some time on this and I currently could display quite a few texts in different languages and also make use of weblate.
Disregarding the weblate portion, I want to make sure I'm on the right track about translatability of the web.

  1. I added svelte-i18n as a dependency as it looked good to me (mind you, I don't have any experience with svelte, so I can't properly asses if this is actually good). This would be the first question: Is the usage of svelte-i18n ok or are there any concerns?
  2. I started with the "page.svelte" files, took the hardcoded texts, moved them into a "en-EN.json" file and used the svelte-i18n functions to get the texts back. Regarding the structure of the file would be the next important question. For Android it's a flat list. For the web I thought about using a hierarchical structure as it, in my opinion at least, improves later readability/extendability.

This means my file currently would look like this for example:

page: {
  admin: {
    library_management: {
      error_create: "Unable to create library"
      ...
      ...
    },
    system_settings: {
      auth_settings_title: "..."
      auth_settings_subtitle: "..."
      ...
      ...
    }
  }
}

For components the top level would be "component". For stuff which is used more often like "Video" or "User" there would be a top level "common" and so on. In the svelte files you'd then use
$t(page.admin.user_management.error_create)
for example. For stuff with placeholders you'd use
$t(page.admin.user_management.error_create, {values: {placeholdername: placeholdervalue}}).
With the placeholders you can also use plurals, so instead of having two rules strings for ""Matched 1 item" and "Matched 10 items" there would only be one "ICU plural messages" Matched {count, plural, one {# item} other {# items}}

What do you think? Is this the correct way for you or should I go at it some other way?

web/src/lib/i18n/locales/de-DE.json Outdated Show resolved Hide resolved
web/src/lib/i18n/locales/de-DE.json Outdated Show resolved Hide resolved
web/src/lib/i18n/locales/de-DE.json Outdated Show resolved Hide resolved
web/src/lib/i18n/locales/de-DE.json Outdated Show resolved Hide resolved
web/src/lib/i18n/locales/de-DE.json Outdated Show resolved Hide resolved
@jrasm91
Copy link
Contributor

jrasm91 commented May 29, 2024

This PR is about the technical aspects of translating the web, so please refrain from commenting about specific translations. Those can be dealt with independently of these changes. (The translation values will be managed through an external platform).

With that said, I think this looks really good so far. I wonder what would be the best way to move forward. Presumably we want to share translation strings between mobile and web. Personally, I prefer the flat structure as the hierarchy might become coupled with the implementation (code), which might easily change in the future. Also, I think translating the admin settings are a much lower priority than the sidebar, asset viewer, albums page, and the asset grid component, so maybe we start with those pages. I think it would be good to get a solid foundation and then merge it in, we can incrementally externalize translation strings in follow-up PRs, thus avoiding merge conflicts that inevitably come from long standing PRs.

import type { Handle } from '@sveltejs/kit'
import { locale } from 'svelte-i18n'

export const handle: Handle = async ({ event, resolve }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good default. Using browser locale is a generally close to accurate. However, I do think we should only treat this as a default/fallback. I think there should be a account-setting that allows you to change the language for your user, overriding the browser locale.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I absolutely agree. This is in a early stage and I was just testing around until now so I didn't bother to also look into how I could achieve something like that.

Copy link
Member

Choose a reason for hiding this comment

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

there should be a account-setting that allows you to change the language

Strong agree, and ideally this setting works for the mobile app as well. @jrasm91 with the new user preferences this should be relatively straightforward right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup.

@Manic-87
Copy link
Contributor Author

Manic-87 commented May 30, 2024

Presumably we want to share translation strings between mobile and web.

I did look into that a bit with weblate. AFAIK you can have different file structures for different components. If you want to make it really easy you'd need to define a glossary (weblate can use existing translations as a glossary) and need to use the same keys for the same translations.
Meaning:
My en-EN.json file has the String share under common. Weblate interprets it as common.share. If the mobile translation file also would have a string with this key the translation of the glossary would be applied to both the web and mobile translation.
The translations for web and mobile could still be translated to something different, but apparently would be overwritten by the glossary translation if the glossary changes.
This would either mean the current translationkeys of the mobile would need to be changed to use . instead of _, or the web translations would need to use the same keys as the mobile version.
Both options aren't optimal in my opinion. Changing all translation keys on the mobile seems like an unnecessary effort. Using the same keys on the web as the mobile will sometimes be strange, e.g. control_bottom_app_bar_add_to_album. On the web the Add to album button is on the top and it's obviously not the app.
Also I think it is preferable to make use of these ICU plural messages and the mobile don't (or perhaps even can't?) use them.

Unfortunately I haven't found a way to tell weblate to connect a translation to a specific glossary item even if the keys differ or at least mark the translation as "To check" at least. But perhaps I just didn't find it.

Most important question how to proceed would be: How should the keys be defined as changing them later on will be a hassle, but I'm afraid this will lead to neverending discussions.
What could be done would be to just do the translations for the web in the manner I already started and when this is done little by little the strings and translations of the web and mobile could be consolidated. For example: Look at all occurences of "Cancel" and decide on a common key for both web and mobile (mobile currently has 8 occurences in the translation file by the way). This could already be taken into account for strings that exist in both.

@jrasm91
Copy link
Contributor

jrasm91 commented May 30, 2024

Yeah I think this is a good point. We should decide on our optimal format and start using that. The mobile app should migrate to it in the future as well.

I would prefer having the translations be somewhat independent of the code/UI though. We don't need "app bar" or "bottom sheet" in the key. I think a "cancel" key would probably be fine.

@Manic-87
Copy link
Contributor Author

Manic-87 commented Jun 2, 2024

I changed my approach a bit. Wrote a simple script to extract the strings from the files, automatically create keys and replace the occurences. The automatic creation of the keys I only did for strings of at most 4 words and just replaced whitespaces with underscores and made the string lowercased. So "Add to" has the key "add_to".
This worked reasonably well and the result you can see in my latest commit. There are a few hickups, which need to be fixed again, and obviously a lot of strings which weren't touched but in my opinion this should be a good starting point.

I added an automatically translated german file for testing purposes.

grafik

@MickLesk
Copy link
Contributor

MickLesk commented Jun 2, 2024

Looks good! Massive Update on source Code, Ive only briefly skimmed through it (the individual commits) and everything looks clean so far.

PS: the German translation is crappy :D

@waclaw66
Copy link
Contributor

waclaw66 commented Jun 3, 2024

A small correction of constant_rate_factor_(-crf) to constant_rate_factor_crf would be fine.
However many thanks for you huge effort, this will help to further widespread of Immich. Many users don't want/can use Immich in English.

@Manic-87
Copy link
Contributor Author

Manic-87 commented Jun 3, 2024

I extracted and replaced some more strings. As these are mostly longer sentences I just used "placeholder_X" for the key for now. I suppose these keys should be decided by you.
I also changed a few of the keys. There were multiple ones with parantheses or a minus. I removed those.

Big Question: How to proceed.

Do you want to merge the currently available stuff? If so, with or without the autotranslated german file? Personally, I wouldn't include it, because.... it's really really crappy and I only changed those which annoyed me the most (translation for cancel for example).
Do you want to wait until at least all occurences of "placeholder_X" are changed?
Do you want to wait until even more/all strings are translatable?

root and others added 10 commits June 3, 2024 14:10
@jrasm91
Copy link
Contributor

jrasm91 commented Jun 3, 2024

I think it looks good so far. We probably could just commit the English values for now and then finish setting up weblate for other languages

@bo0tzz
Copy link
Member

bo0tzz commented Jun 3, 2024

cc @alextran1502, is weblate just ready to go or do we need to prep anything more?

@Manic-87
Copy link
Contributor Author

Manic-87 commented Jun 3, 2024

FYI: I just rebased the branch to the current main branch of immich and also removed the german translation file.

Comment on lines 2 to 9
"thirty_minutes": "30 minutes",
"one_hour": "1 hour",
"six_hours": "6 hours",
"one_day": "1 day",
"seven_days": "7 days",
"thirty_days": "30 days",
"three_months": "3 months",
"one_year": "1 year",
Copy link
Member

@bo0tzz bo0tzz Jun 3, 2024

Choose a reason for hiding this comment

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

For fields like this, should we use plurals? Per https://formatjs.io/docs/core-concepts/icu-syntax/#plural-format (though I may have made a mistake here):

{"hour": "{count, plural, one {count day} other {{count, number} days}"}

And then using that like

{$_('hour', { values: { count: 3 }})}

Copy link
Contributor

@waclaw66 waclaw66 Jun 3, 2024

Choose a reason for hiding this comment

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

Plurals are very welcomed, especially for non-germanic languages (e.g. slavic).

Comment on lines 318 to 322
"thumbnail_resolution": "Thumbnail resolution",
"tone-mapping_npl": "TONE-MAPPING NPL",
"tone-mapping": "TONE-MAPPING",
"transcode_policy": "Transcode policy",
"two-pass_encoding": "TWO-PASS ENCODING",
Copy link
Member

Choose a reason for hiding this comment

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

I think there's some inconsistency in the use of casing here (and of toUpperCase where these values are used)

@waclaw66
Copy link
Contributor

waclaw66 commented Jun 3, 2024

Those placeholders can be named according previous label... welcome_message, placeholder_0 => welcome_message_desc

@jrasm91
Copy link
Contributor

jrasm91 commented Jun 3, 2024

I'll go ahead and finish cleaning up the translations and then merge this. Once merged, I'll configure weblate with the git integration and we can go from there.

@jrasm91 jrasm91 changed the title Web translatability feat(web): translations Jun 3, 2024
Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

@jrasm91 jrasm91 marked this pull request as ready for review June 3, 2024 21:44
Copy link
Member

@danieldietzler danieldietzler left a comment

Choose a reason for hiding this comment

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

Thanks a lot for pushing this forward! :)

Comment on lines +45 to +51
export const lang = persisted<string | undefined>('lang', undefined, {
serializer: {
parse: (text) => text,
stringify: (object) => object ?? '',
},
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Dedicated preference

@@ -155,7 +157,7 @@ export const locales = [
{ code: 'en-TT', name: 'English (Trinidad and Tobago)' },
{ code: 'en-VI', name: 'English (U.S. Virgin Islands)' },
{ code: 'en-GB', name: 'English (United Kingdom)' },
{ code: 'en-US', name: 'English (United States)' },
{ code: 'en-US', name: 'English (United States)', loader: () => import('$lib/i18n/en-US.json') },
Copy link
Contributor

Choose a reason for hiding this comment

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

Languages are a subset of locales.

Comment on lines +252 to +255
export const langs = [
...locales.filter((item): item is LanguageLoader => !!item.loader),
{ name: 'Development', code: 'dev', loader: () => Promise.resolve({}) },
];
Copy link
Contributor

Choose a reason for hiding this comment

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

Locales with loaders + development language (shows raw translation keys)

Comment on lines +17 to +24
for (const { code, loader } of langs) {
register(code, loader);
}

const preferenceLang = get(lang);

await init({ fallbackLocale: fallbackLang, initialLocale: preferenceLang || fallbackLang });

Copy link
Contributor

Choose a reason for hiding this comment

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

Load languages and default to preference or english if unset.

Comment on lines +68 to +81
const handleLanguageChange = async (newLang: string | undefined) => {
newLang = newLang || fallbackLang;
$lang = newLang;

const previousLang = get(i18nLocale);

if (newLang === 'dev') {
await init({ fallbackLocale: 'dev', initialLocale: 'dev' });
} else if (previousLang == 'dev' && newLang !== 'dev') {
await init({ fallbackLocale: 'en-US', initialLocale: newLang });
}
$i18nLocale = newLang;
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Handle language change (need to remove the english fallback to see the development keys)

@jrasm91 jrasm91 merged commit f446bc8 into immich-app:main Jun 4, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants