davidhealey / waistline

Libre calorie counter app for Android. Built with Cordova.
568 stars 67 forks source link

Translation issue #276

Closed faan11 closed 3 years ago

faan11 commented 3 years ago

I tried the last version (v3.0) and i found that only the menu is translated. The rest is written in English language. I'm using Android 10. image image

davidhealey commented 3 years ago

Translations are added by the community, if you notice specific strings are missing from the source code let me know, but I don't handle individual languages.

jfaltis commented 3 years ago

@davidhealey

Did you merge the Crowdin translation branch? I added German translations via Crowdin but only the menu is translated as @faan11 described, despite that I translated all other strings too.

davidhealey commented 3 years ago

@davidhealey

Did you merge the Crowdin translation branch? I added German translations via Crowdin but only the menu is translated as @faan11 described, despite that I translated all other strings too.

V3.0.0 has a new schema for the language files, so locales for the old version need to be updated to be compatible. If your German translation for V3.0.0 or the previous release?

faan11 commented 3 years ago

I translated it 3 days ago, https://github.com/davidhealey/waistline/pull/272 and i used the english translation file (en-locale.json) as a reference. Which schema I should follow to translate the application?

davidhealey commented 3 years ago

@faan11 That's the correct one.

As you can see, some strings are missing from the Settings section. I'll need to add these to the template file (locale-en.json).

"settings": { "title": "Impostazioni", "needs-restart": "Riavvia l' applicazione per applicare le modifiche.", "theme": { "title": "Tema", "dark": "Modalità dark", "animations": "Disabilita le animazioni dell' interfaccia", "start-page": "Inizio pagina" },

faan11 commented 3 years ago

Ok let us now when the file locale-en.json is ready

jfaltis commented 3 years ago

I translated it like 3 months ago, so I think it should also be version 3.0.0. I don't know if I am overlooking something but I think the https://github.com/davidhealey/waistline/tree/l10n_master2 branch needs to merged into master so the new translations are in the release.

davidhealey commented 3 years ago

@jfaltis I think you're right. I'll merge it into the master before the next build, thanks!

davidhealey commented 3 years ago

I've just looked into this a bit further and it seems all the strings are actually there - so something else is the problem. I'll find out what it is and fix it.

davidhealey commented 3 years ago

Ok I found the cause of the issue. I'll fix it soon, probably tomorrow.

EmilJunker commented 3 years ago

I noticed a few things regarding the localization and since this issue is still open, I will just add a comment here. I hope that's ok.

Issue 1: The branch https://github.com/davidhealey/waistline/tree/l10n_master2 still includes the old locale files located at /www/locales and /www/locaes so every time you merge that branch into master, these files will be recreated. It's probably best to remove them from there.

Issue 2: I believe you should rethink the way you localized the title in the food editor:

https://github.com/davidhealey/waistline/blob/f785c5225c2aa39da901eb65ec0a4118eae50ee4/www/activities/foods-meals-recipes/js/food-editor.js#L224

I'm not sure if this works (grammatically) in other languages. At least I'm having some trouble translating it into German. And you definitely can't just remove the letter 's' from the string to make it singular because this will interfere with other languages. For example, German "Lebensmittel" becomes "Lebenmittel" – that's not a word 😅

davidhealey commented 3 years ago

@EmilJunker Duly noted. I'll adjust that and update the old locale branch, I think it makes sense to remove everything from there except the locale files.

EmilJunker commented 3 years ago

One more thing: The link to request a USDA API key is also localized, but there are two problems with this.

https://github.com/davidhealey/waistline/blob/f785c5225c2aa39da901eb65ec0a4118eae50ee4/www/activities/settings/views/integration.html#L545

Problem 1: The source code references the string "settings.integration.usda-note-1" but in the locale files it's called "settings.integration.usera-note-1".

Problem 2: The fix you implemented (for issue #269) isn't yet reflected in the locale files.

davidhealey commented 3 years ago

@EmilJunker

This should fix the localization issues.

app-release.zip

I replaced the l10n_master2 branch with l10n_master3 branch which is taken from the current master, so it should be upto date. I don't understand crowdin enough to refine it more.

EmilJunker commented 3 years ago

Yes, the localization is working now. But I noticed a few strings that aren't localized. Here's a (hopefully complete) list:

Also, the USDA API link no longer works because it is now localized and the fix you implemented (for issue #269) isn't yet reflected in the locale files. You should either uncouple the link from the locale files or update it in the locale-en.json file.

EmilJunker commented 3 years ago

Also, importing and exporting no longer works in the version you uploaded. I guess there is an issue with the localized error/success messages, but I don't know for sure.

davidhealey commented 3 years ago

@EmilJunker

USDA link is fixed now, I did as you suggested and removed it from the localization since the URL is the same in all languages.

All of those missing localized strings plus a couple of others have been added.

I've improved the efficiency of the localization system. Previously it was re-reading the localization file on every reload, now it just does it once when the app is first opened. The automatic localization (handled by jquery localize) is now bypassed completely for English since this is the default and there is no need to relocalize these strings. This might give a slight performance boost on some devices when using English.

Importing/Exporting is fixed too. I'd switched my local branch to merge the locales branch, when I switched back to the master I had some issues and had to reinstall some Cordova plugins, I missed the Device plugin and this was causing the import/export to not work.

Assuming no major bugs in this version I'll push a new release soon.

app-release.zip

EmilJunker commented 3 years ago

@davidhealey

Looks good. Importing/Exporting is working again and I didn't notice any bugs.

For some reason, the locale strings on Crowdin are now split among two files. I don't know why this happened and I don't think it's a big problem, but it is a bit confusing. Maybe you can look into it...

In any event, I think you should wait a bit (at least a week) before pushing the next release so contributors have time to translate the new strings. Also, when you merge the l10n_master3 into master, you should check which languages aren't translated by then (currently Bulgarian, French and Swedish are all 0% translated) and remove them from the preferred language dropdown for the time being.

davidhealey commented 3 years ago

For some reason, the locale strings on Crowdin are now split among two files. I don't know why this happened and I don't think it's a big problem, but it is a bit confusing. Maybe you can look into it...

@yarons Any ideas?

yarons commented 3 years ago

The translations were pushed to the master branch while our working localization branch is actually l10n_master3.

We're also syncing the same file twice in the Crowdin integration, I can try and see if we can change it and if it's relevant to this case specifically.

davidhealey commented 3 years ago

@yarons Thanks, that would be helpful, let me know if I need to do anything.

yarons commented 3 years ago

Yup, we also need to remove l10n_master2, I removed l10n_master because it had no pending PRs but I can't remove the other one without closing the relevant PRs, should I just remove these?

davidhealey commented 3 years ago

@yarons Yes it should be fine as I merged those changes locally already

yarons commented 3 years ago

@davidhealey Done, what about all the other stale branches?

davidhealey commented 3 years ago

@yarons If they're not causing any issues we can leave them

yarons commented 3 years ago

Sure, I can't find the reason for the duplication in Crowdin but as long as it's hidden I don't really care.

EmilJunker commented 3 years ago

The l10n_master3 branch is 4 commits behind master, maybe that's why?

davidhealey commented 3 years ago

That branch will always be behind the master except for the locales which will always be ahead.

EmilJunker commented 3 years ago

Yeah that makes sense. Then I'm out of ideas too.

yarons commented 3 years ago

Yes, the localization is working now. But I noticed a few strings that aren't localized. Here's a (hopefully complete) list: @EmilJunker

Can you try and switch to another language to see if it happens with other languages as well?

EmilJunker commented 3 years ago

@yarons It affected all languages. But it should be fixed in the latest version from https://github.com/davidhealey/waistline/issues/276#issuecomment-890548095 that added a bunch of new locale strings.

To check if all the strings are now localized, we would need to add the translations first. I just completed the German translation on Crowdin, so if @davidhealey would merge it into master and publish a new APK, we could see if it really works.

Of course, the other languages need to be completed as well before the next release @huuhaa @faan11 @asdomingues

faan11 commented 3 years ago

ok i'll change and propose a pull request about the file it-locale.json in the master branch.

EmilJunker commented 3 years ago

@faan11 It's better and easier to add the translations via Crowdin: https://crowdin.com/project/waistline (you can sign in with your github account)

faan11 commented 3 years ago

ok I used Crowdin to add the new translations ( Android.app and locale json )

davidhealey commented 3 years ago

This has been resolved now so I'll close this issue