B3nedikt / restring

Restring is a android library to replace string resources dynamically
Apache License 2.0
314 stars 30 forks source link

Locale selection fallback not working #147

Closed SoftXperience closed 1 month ago

SoftXperience commented 2 months ago

Unfortunately I found another problem:

I have strings for locale "de" and update them. So they will be stored as locale "de" in the stringRepository.

But the device is running with the locale "de_DE", so the library tries to get strings for "de_DE" and won't find anything because it is stored for just "de".

I'm not sure if there is a simple fix for that. Maybe try to find translations with the given system locale (for example de_DE) and if that won't return a list, also try the base locale (in that case just "de") and if that also fails, fall back to system resources?

SoftXperience commented 2 months ago

I further investigated the problem. The fallback is working properly, I'm sorry.

Obviously the problem is the ObservableMap.kt:46. When the lib tries to resolve a locale, it creates the default value. So when the app runs with "de_DE" and there's no translation for that, it creates a default value. After that the put is called and this way this language goes to the supportedLocales list. So the lookup in ResourcesDelegate.kt:152 assumes the language "de_DE" is supported and doesn't do the fallback to "de" anymore.

SoftXperience commented 2 months ago

Ok, root cause seems to be getting an resource identifier for an unknown key at ResourcesDelegate:28.

Steps to reproduce:

  1. set device to an unsupported locale (e.g. "de_DE", where only "de" is supported)
  2. try to get an identifier for an unknown key (I use resources.getIdentifier directly for some dynamic string resolution)
  3. I guess the above mentioned line should check if the stringRepo has that key, otherwise return 0 as that key is unknown
  4. but this line uses the get in ObservableMap.kt:32 which can't find that unsupported Locale-Key ("de_DE"), so it calls createDefaultValue (ObservableMap.kt:43), which returns an empty list and puts that to the strings repo map.
  5. CachedStringRepository.kt:34 gets called and adds the unsupportedLocale to the supportedLocales, which also gets persisted
  6. all further calls to getLocale (ResourcesDelegate.kt:148) then find the locale "de_DE" in supportedLocales (:152) and thus the fallback to "de" isn't used anymore. And because the stringsRepo for "de_DE" is empty, all calls fallback to bundled strings instead of the updated strings.

I'm not really sure how to fix that properly. Hopefully you have a better understanding of your code and can come up with a quick fix again.

Maybe do a contains check on the strings repo before the get in ResourcesDelegate:28 to make sure to not create an unwanted Locale entry in the stringsRepo. But I don't know if there are more locations where this can happen (at least in ResourceDelegate.kt:130). Maybe remove the implicit creation of entries on a failed get at all? But don't know about the side effects...

Would be great if you can give me some feedback about it because at the moment it doesn't work in my app and I can't deploy it.

B3nedikt commented 2 months ago

Are you using my library App Locale ? I think it's DefaultLocaleMatchingStrategy is doing what you want out of the box.

SoftXperience commented 2 months ago

No, I'm not using it, because I don't need dynamic switching of the locale. Please see my last comment. The problem is, that the stringRepository cache does not work correctly. The user sets his device to "de_DE" which should be the usual case in Germany, but the app only provides the strings for locale "de". As soon as you try to get the identifier for an unknown key, the stringRepository creates that language with empty values.

Should also happen with other cases for example you provide only english strings with locale "en" but the user sets his device for example to "en_US".

But maybe a custom LocaleProvider that checks the available supported locales may help. So if it returns "de" instead of the system locale "de_DE" because the latter is not supported, that may workaround the issue. I'll give it a try.

B3nedikt commented 2 months ago

You can also use AppLocale without dynamically switching the Locale.

But I agree, that the behaviour you describe is somewhat of a bug. One way to solve it would be a DefaultLocaleProvider that implements the fallback behaviour you describe. Another option would be a new API like Restring.localeMatchingStrategy. This could then be set to e.g. AppLocales LocaleMatchingStrategy with an adapter, if the user wants to use AppLocale, and defaults to a strategy which implements the behavior you describe.

I don't have time for this right now, but maybe might look into it next week if I find the time, should not take too long to implement. If this issue is important to you, you can of course try implementing it yourself and create a PR. Or just use any workaround, like the one you describe with the custom LocaleProvider.

SoftXperience commented 2 months ago

Ok, I'm going with a custom LocaleProvider now. It checks if the system locale is supported directly, otherwise tries to fallback to the language and if that is also not supported, fallback to the default locale "en". So there will never be a get call to the stringsRepo with an unsupported locale and thus it won't be created.

Seems to work for now.