Fix localisation stipping in GTK, X11 and macos #1870
Conversation
JAicewizard
force-pushed
the
fix_localisation
branch
from
54d3603 to
1e5efc0
Compare
JAicewizard
changed the title
| @@ -778,14 +778,18 @@ impl Application { | |||
|
|
|||
| // from gettext manual | |||
| // https://www.gnu.org/software/gettext/manual/html_node/Locale-Environment-Variables.html#Locale-Environment-Variables | |||
| var_non_empty("LANGUAGE") | |||
| let mut locale = var_non_empty("LANGUAGE") | |||
There was a problem hiding this comment.
Would it be possible to make var_non_empty log some info (ideally at some verbose level if such a thing is available). It can be useful to the user to know which var is used and which value it has. Currently if the variable cannot be parse you just see "en_US". This could be in a different PR of course.
There was a problem hiding this comment.
I will leave this open for whoever is doing the final review. It does hinder readability but I can see its usefulness.
There was a problem hiding this comment.
I think this would be welcome, but I'd make it a separate PR.
There was a problem hiding this comment.
@ColinPitrat can you confirm that this fixes the original issue? It looks like it should, but would like to be sure.
| @@ -84,7 +84,12 @@ impl Application { | |||
| } | |||
|
|
|||
| pub fn get_locale() -> String { | |||
| glib::get_language_names()[0].as_str().into() | |||
| let mut locale: String = glib::get_language_names()[0].as_str().into(); | |||
There was a problem hiding this comment.
this index would panic if there get_language_names() is empty. Is that possible?
There was a problem hiding this comment.
Computes a list of applicable locale names, which can be used to e.g. construct locale-dependent filenames or search paths. The returned list is sorted from most desirable to least desirable and always contains the default locale "C".
If druid fails to parse the locale it will be "en_US" so this should work in all cases.
| @@ -111,7 +111,8 @@ impl Application { | |||
| let locale: id = msg_send![nslocale_class, currentLocale]; | |||
| let ident: id = msg_send![locale, localeIdentifier]; | |||
| let mut locale = util::from_nsstring(ident); | |||
| if let Some(idx) = locale.chars().position(|c| c == '@') { | |||
| // This is done because the locale parsing library we use expects an unicode locale, but these vars have an ISO locale | |||
| if let Some(idx) = locale.chars().position(|c| c == '.' || c == '@') { | |||
There was a problem hiding this comment.
I believe this is okay as-is, because macOS doesn't include encoding in the locale?
There was a problem hiding this comment.
I couldnt find any documentation on this. Having the code be the same across the backends seems nice to me. It hasnt caused any problems so far so it should indeed be fine without.
There was a problem hiding this comment.
I left the comment in since I think it still applies and is helpful
| @@ -778,14 +778,18 @@ impl Application { | |||
|
|
|||
| // from gettext manual | |||
| // https://www.gnu.org/software/gettext/manual/html_node/Locale-Environment-Variables.html#Locale-Environment-Variables | |||
| var_non_empty("LANGUAGE") | |||
| let mut locale = var_non_empty("LANGUAGE") | |||
There was a problem hiding this comment.
I think this would be welcome, but I'd make it a separate PR.
thanks @ColinPitrat #1870 (comment) Also changed the name and made it a function
on macos we already strip the modifiers, this extends this to all backends and also stripping the charset.
fixes #1864