search

bpierre / gtranslate

Translate the selected text using Google Translate.
https://addons.mozilla.org/firefox/addon/gtranslate/
Mozilla Public License 2.0
95 stars 27 forks source link

Fix missing language translations bug #79

Open fluks opened 8 years ago

fluks commented 8 years ago

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.

fluks commented 8 years ago

There are two things up for a discussion.

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

1) 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.

fluks commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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… :sleeping:

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

bpierre commented 8 years ago

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 commented 8 years ago

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 commented 7 years ago

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