Skip to content
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.

Sign up for GitHub

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

Fix localisation stipping in GTK, X11 and macos #1870

Merged
merged 5 commits into from
Jul 16, 2021
Merged

Fix localisation stipping in GTK, X11 and macos #1870

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e801938
Fix localisation stipping in GTK and X11
JAicewizard Jul 11, 2021
1e5efc0
Normalise splitting across backends
JAicewizard Jul 11, 2021
bd57341
Add comment explaning the locale stipping
JAicewizard Jul 11, 2021
f9da6f9
Remove import
JAicewizard Jul 11, 2021
ca4b99d
Remove added check in mac backend
JAicewizard Jul 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion druid-shell/src/backend/gtk/application.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
JAicewizard marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this index would panic if there get_language_names() is empty. Is that possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// 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 == '@') {
locale.truncate(idx);
}
locale
}
}

Expand Down
3 changes: 2 additions & 1 deletion druid-shell/src/backend/mac/application.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 == '@') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is okay as-is, because macOS doesn't include encoding in the locale?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left the comment in since I think it still applies and is helpful

locale.truncate(idx);
}
locale
Expand Down
10 changes: 8 additions & 2 deletions druid-shell/src/backend/x11/application.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -778,14 +778,20 @@ 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")
Copy link

@ColinPitrat ColinPitrat Jul 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will leave this open for whoever is doing the final review. It does hinder readability but I can see its usefulness.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be welcome, but I'd make it a separate PR.

// the LANGUAGE value is priority list seperated by :
// See: https://www.gnu.org/software/gettext/manual/html_node/The-LANGUAGE-variable.html#The-LANGUAGE-variable
.and_then(|locale| locale.split(':').next().map(String::from))
.or_else(|| var_non_empty("LC_ALL"))
.or_else(|| var_non_empty("LC_MESSAGES"))
.or_else(|| var_non_empty("LANG"))
.unwrap_or_else(|| "en-US".to_string())
.unwrap_or_else(|| "en-US".to_string());

// 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 == '@') {
locale.truncate(idx);
}
locale
}

pub(crate) fn idle_pipe(&self) -> RawFd {
Expand Down