nail down language pref handling for localized index metadata
This supports a priority list of locales based on the user preference. In >= android-24
, there is a locale preference list already, so F-Droid doesn't have to work so hard. For older Android versions, we make it try to find a useful match if the default locale is not available.
Also, I removed the "About" category so that there isn't a redundant header in the Settings view.
Merge request reports
Activity
assigned to @pserwylo
- Resolved by username-removed-24982
- Resolved by username-removed-24982
- Resolved by username-removed-24982
Looks great, a great improvement on what we had before.
I know you prefer me not to add commits on top of yours, but I thought that this code, while good, is complex, but it is also eminently testable. As such, I have written a two sets of tests (49535de5d), one that runs on < 24 and one that runs on >= 24. In the process I picked up a couple of issues that I then addressed:
- cf5cd81f5a: Don't just pick the first English language which we find, include all of them. This prevents non-determinism in the case that both "en-GB" and "en-AU" are specified. The order of which one gets selected is still non-deterministic, but at least they both get selected this time. Hopefully the generation of the JSON on the server is deterministic so that this works well in practice.
-
1df483569: Fallback to any language which matches the language code. Previously,
fr-CH
would only fall back tofr
, but notfr-FR
. With this change, people whos language isfr-CH
will getfr-FR
, orindeed any other
fr-*` dialects instead of defaulting to English. This is more similar (indeed I believe it is the same as) how Android N behaves. -
68634dcfc7: Be more explicit about the fact language preferences are ordered. This wasn't actually broken, but it is more by coincidence than anything else. The fact that the
getLocalized*()
methods accept aSet
of acceptable locales means it is stating it doesn't care about the order. It happened to work, because theLinkedHashSet
we actually pass in is ordered. However the actual contract between the methods was incorrect, so I changed it to accept aList
instead. - 6c4c8af9c1: The behaviour for >= 24 vs < 24 is different. I didn't know which to use, so I dropped a comment in this commit and will leave it to you to decide.
If you could please cherry-pick my tests, and preferably the fixes (subject to discussion) then I'd be happy to merge this.
Edited by username-removed-25042Also, could you please clarify what the expected Android N behaviour is in the following case, because it seems not to behave as I expect (not related to your code, more the system language chooser stuff):
- I set my preferred languages to:
-
- Thai (which doesn't translate "Latest" or "Categories" but does translate the rest of the bottom bar).
-
- Spanish (which translates all of the bottom bar strings)
-
- I expected that for any given string, it would try to load the Thai version, then if it failed, load the Spanish version, then if it failed, load the English version.
- What actually happened is it tried to load the Thai version, failed, then defaulted to English for the "Latest" and "Categories".
Am I right in concluding that the Android N language resolution stuff seems to say "Well, there is a
values-th
folder, so don't bother going toes
ever"?Not that we can do anything about this, just curious.
- I set my preferred languages to:
mentioned in issue #987
My problem with when you start putting commits on top of other people's merge requests is that it usually ends up duplicating efforts, and spending too much time on one specific issue. In this case, yes, we can improve things a lot, but now is not the time to spend polishing every last detail of this process. We need to get 0.103 done, and the details of this stuff are not going to affect anyone for a while. There is not localized stuff in the main repo yet even, and hardly anyone has 7.x devices. For example, reviewing your comments here means I no longer have time to do the 0.103-alpha4 release today, so it'll be delayed a couple of days more. So I think you should set aside your commits, and we can come back to that later. Put them into a separate WIP merge request if you want. I opened #987 to track this.
I did include cf5cd81f5a.
For example, reviewing your comments here means I no longer have time to do the 0.103-alpha4 release today
If that is what it takes, so be it. It is either I request you to add tests to this feature (which is a perfectly reasonable request in this case, just like you have requested test coverage of certain features in the past), or I add tests and ask you to CR and include them. Given the timezone difference, it seems like the total amount of work required by you is less for you to CR my changes than add them yourself.
I should also add: I do appreciate your enthusiasm for diving in deep, I guess this just highlights the difference between working on free software for fun, versus getting paid to deliver free software. When getting paid, there comes an inevitable focus on the bottom line of what it takes to deliver something minimally functional. That's in contrast to working on something with the goal of writing beautiful software.
That also goes for tests here: what would be tested will affect very few people at this point, and not affect core functionality, just edge cases for a new feature. Tests take time to do, and we have a huge improvements waiting to be launched that will affect all users.