Skip to content
This repository has been archived by the owner on Nov 22, 2022. It is now read-only.

Fix missing language translations bug #79

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fluks
Copy link
Contributor

@fluks fluks commented Aug 29, 2016

The bug was that if Firefox's locale (browser.useragent.locale in
about:config) was such that there weren't translations for that locale
in data/languages.json file, gtranslate's menucontext didn't show up at
all.

To fix this, a support for a locale is tested and in case a locale isn't
supported, a fallback locale is used.

See PR #77 for additional conversation.

The bug was that if Firefox's locale (browser.useragent.locale in
about:config) was such that there weren't translations for that locale
in data/languages.json file, gtranslate's menucontext didn't show up at
all.

To fix this, a support for a locale is tested and in case a locale isn't
supported, a fallback locale is used.

See PR bpierre#77 for additional conversation.
@fluks
Copy link
Contributor Author

fluks commented Aug 29, 2016

There are two things up for a discussion.

  1. If a locale isn't supported, the same locale without a region code is tried. Is this ok?

  2. A support for a locale is tested by having a translation for the 'auto' string. All the language name strings must be translated for a locale. I think the code could be modified to test if there is a translation for any string. In that case, you could translate only some of the language names.

I want to emphasis; when a locale or translations are mentioned, they are about data/languages.json file only.

if (languages.auto[locale]) {
return locale
}
const i = locale.indexOf('-')
Copy link
Collaborator

Choose a reason for hiding this comment

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

As some improvement, you could try to remove the dialect first. Then remove the region code.
Here is the format of the locale codes.

Try the locale support with the whole locale string, (possibly) without
a dialect and without a region.
@fluks
Copy link
Contributor Author

fluks commented Aug 30, 2016

I added trying the locale without a dialect also.

There was a bug in the previous commit, I didn't try whether the locale without a region was actually supported, I just returned it.

@PerfectSlayer
Copy link
Collaborator

About the data/languages.json, why not split it to some properties file in locale folder as every other translations?
It should fix the issue and there won't be no more two translations mechanisms inside the addon.

What do you think about it?

@fluks
Copy link
Contributor Author

fluks commented Aug 30, 2016

Yeah, that's probably a better way than the current one. I'll look into it. Or, I don't know if it can be done with properties files, but maybe the file could be split into a different locales. Then we would just need to load the locale's languages file and remove the rest of the localization related code.

@PerfectSlayer
Copy link
Collaborator

It should work with properties file format.
But we will have to reorder keys of the translations. As the first key is the word to translate and the second the language of translation, we should only take the right second key translation value for each primary key to fill a locale file. And we have to create one locale file for each translation language available… 😴

Notice: onlyTo, onlyFrom or rtl should remain in the json file. It's more configuration related than translation related.

@bpierre
Copy link
Owner

bpierre commented Sep 5, 2016

It looks good to me, but yes I agree that we should probably move these translations in the locale folder. It would also prevent mixing “available languages to translate” with “available translations of the addon itself”.

@fluks @PerfectSlayer I plan to publish a new version once this and the french translation are merged, if that’s ok for you?

@PerfectSlayer
Copy link
Collaborator

I'm starting to convert languages.json file to several locale properties.
I already did it but I still have to test a bit more the addon port.
I just pushed the PR #80 about it. Fell free to contribute and finish the task.

After the #80 is merged, you could merge #77 and then publish a new version.
Note the #79 may not be useful after merging #80 .

@bpierre
Copy link
Owner

bpierre commented Nov 28, 2016

Do we still need getSupportedLocale, now that we moved to .properties for everything?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants