-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add get_locales() method for preference-order list of locales #22
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this PR! For the most part what's implemented so far looks good. My biggest suggestion would be that I think it may be better to have get_locales()
return impl Iterator<Item = String>
instead.
This would allow callers to save work depending on how many locales they actually needed in the Apple, (hypothetically) Android, (Windows as well, although to less of an extent), and WASM backends since all of those need to call a bunch of glue methods to access/convert strings from the associated runtime. Additionally it makes for nice code if you're not keeping the language list around for long:
let newer_two = sys_locale::get_locales().take(2);
let older_two = sys_locale::get_locales().into_iter().take(2);
This Implements the list behaviour on windows and wasm, but I'm not able to test the other providers atm to be able to convert those over - they'll just return either an empty vec or a vec with a single element ...
No problem, thank you for starting on this.
Apple should be easy to change for someone who can test it...
I should be able to take care of that. I could push a new branch from this one up and you should be able to cherry-pick it in here to avoid a half-working main
.
but I've no idea about linux...
For Linux, I don't think the standard C APIs support the concept of fallback locales. We would need to rely on the desktop environment (if any) to obtain those. It doesn't look like GNOME supports this concept but KDE does. It stores the preferences in ~/.config/plasma-localerc
:
I'm not 100% certain about that path though.
or android.
The short answer is that I don't think its going to be feasible to do in this PR. I spent quite a while looking around and so far the only options I've found for reading tiered language preferences require Java APIs, which means we'd need to use the JNI. To provide a little bit of extra context as well, Android gained support for having multiple active system languages in Android 7 but only gained the same tiered support other platforms have in Android 13 (the current latest). I still want to look around more for possible options that work in native code, so I think its fine to leave as-is for the moment.
a092308
to
f244947
Compare
Good idea, I've made that change now. In doing so, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating the API. There's a few more small items (I apologize for not noticing some of them sooner) but otherwise this looks good to merge.
let locales = get_locales(); | ||
for (i, locale) in locales.enumerate() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for just noticing this, but can we call get_locales()
twice and use the first result to make sure we're still always detecting and returning at least one locale? This will keep it equivalent in functionality to the old version of the test.
let locales = get_locales(); | |
for (i, locale) in locales.enumerate() { | |
// We must always have at least one active locale. | |
assert!(get_locales().count() != 0); | |
for (i, locale) in get_locales().enumerate() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strictly speaking it's not an error that the machine has no locales, as we provide for that case in the API - that's why I didn't fix this up earlier. I can add it back though as it's at least very likely a developer would have a locale, I guess!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I agree that it is not an error for a machine to have no locales set. This test is just meant to catch implementation errors that might otherwise silently break because all the inner methods don't distinguish between "encountered a fatal error" and "nothing present".
If the inner functions we're changed to return types that let you observe failure conditions directly, this could be removed but IMO thats far more complicated given what you said about most developers having some kind of locale configured (and CI).
Fixed comments, thanks for the review. I've pushed fixes as their own commits for ease of reviewing but I can squash it down (or you can during merge) if you'd prefer, it's a bit messy history now :D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again! I'll open followup issue(s) to expand this to the other platforms, primarily Apple.
It's much better to get and respect a list of preferred locales, rather than a single locale or nothing. For example, someone may prefer Dutch and then French, but if your app only supports English and French, you're likely going to give them a not-preferred English despite supporting something the user actually wants.
This Implements the list behaviour on windows and wasm, but I'm not able to test the other providers atm to be able to convert those over - they'll just return either an empty vec or a vec with a single element. Apple should be easy to change for someone who can test it, but I've no idea about linux or android.
This implements #14.