Skip to content

Implement Application::get_locale() for Windows backend #1874

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

Merged
cmyr merged 3 commits into from
Jul 16, 2021
Merged

Implement Application::get_locale() for Windows backend #1874

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
adc03c9
Implement Application::get_locale() for Windows backend
dfrg Jul 15, 2021
1e3fc7d
Cleanup Application::get_locale() in Windows backend
dfrg Jul 15, 2021
1b5ed64
Formatting in Application::get_locale() for Windows backend
dfrg Jul 16, 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
2 changes: 1 addition & 1 deletion druid-shell/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ version = "0.3.9"
features = ["d2d1_1", "dwrite", "winbase", "libloaderapi", "errhandlingapi", "winuser",
"shellscalingapi", "shobjidl", "combaseapi", "synchapi", "dxgi1_3", "dcomp",
"d3d11", "dwmapi", "wincon", "fileapi", "processenv", "winbase", "handleapi",
"shellapi"]
"shellapi", "winnls"]

[target.'cfg(target_os="macos")'.dependencies]
block = "0.1.6"
Expand Down
16 changes: 13 additions & 3 deletions druid-shell/src/backend/windows/application.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ use winapi::shared::windef::{DPI_AWARENESS_CONTEXT_PER_MONITOR_AWARE_V2, HCURSOR
use winapi::shared::winerror::HRESULT_FROM_WIN32;
use winapi::um::errhandlingapi::GetLastError;
use winapi::um::shellscalingapi::PROCESS_PER_MONITOR_DPI_AWARE;
use winapi::um::winnls::GetUserDefaultLocaleName;
use winapi::um::winnt::LOCALE_NAME_MAX_LENGTH;
use winapi::um::winuser::{
DispatchMessageW, GetAncestor, GetMessageW, LoadIconW, PeekMessageW, PostMessageW,
PostQuitMessage, RegisterClassW, TranslateAcceleratorW, TranslateMessage, GA_ROOT,
Expand All @@ -40,7 +42,7 @@ use crate::application::AppHandler;
use super::accels;
use super::clipboard::Clipboard;
use super::error::Error;
use super::util::{self, ToWide, CLASS_NAME, OPTIONAL_FUNCTIONS};
use super::util::{self, FromWide, ToWide, CLASS_NAME, OPTIONAL_FUNCTIONS};
use super::window::{self, DS_REQUEST_DESTROY};

#[derive(Clone)]
Expand Down Expand Up @@ -194,7 +196,15 @@ impl Application {
}

pub fn get_locale() -> String {
//TODO ahem
"en-US".into()
unsafe {
let mut buf: [u16; LOCALE_NAME_MAX_LENGTH] = std::mem::zeroed();
let len_with_null = GetUserDefaultLocaleName(buf.as_mut_ptr(), buf.len() as _) as usize;
let locale = if len_with_null > 1 {
buf.get(..len_with_null - 1).and_then(FromWide::from_wide)
} else {
None
};
locale.unwrap_or_else(|| "en-US".into())
}
Copy link
Member

@cmyr cmyr Jul 15, 2021

Choose a reason for hiding this comment

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

Thanks, definitely good to fill in these missing pieces.

I would encourage you to try and minimize the size of the unsafe block; when reading code, unsafe introduces additional cognitive burden, and requires much more careful review. By only unsafe around the specific unsafe portions of the code, it better expresses where the danger is.

I would probably also skip the intermediate Option, although that's a minor stylistic thing that I wouldn't fight about.

In any case, I think I'd write this as,

Suggested change
unsafe {
let mut buf: [u16; LOCALE_NAME_MAX_LENGTH] = std::mem::zeroed();
let len_with_null = GetUserDefaultLocaleName(buf.as_mut_ptr(), buf.len() as _) as usize;
let locale = if len_with_null > 1 {
buf.get(..len_with_null - 1).and_then(FromWide::from_wide)
} else {
None
};
locale.unwrap_or_else(|| "en-US".into())
}
let mut buf: [u16; LOCALE_NAME_MAX_LENGTH] = std::mem::zeroed();
let len_with_null = unsafe {
GetUserDefaultLocaleName(buf.as_mut_ptr(), buf.len() as _) as usize
};
if len_with_nul > 0 {
buf.get(..len_with_null - 1).and_then(FromWide::from_wide)
} else {
tracing::warn!("Failed to get user locale");
"en-US".into()
}

Here I'm also using > 0 to check the error; I'm not sure what it means if we get a string that only contains a NUL but I'm not sure it's worth being defensive against this? I'm genuinely unsure, here, but my inclination is to not second-guess the underlying API, and just transparently pass through the result. 🤷

Copy link
Contributor Author

@dfrg dfrg Jul 15, 2021

Choose a reason for hiding this comment

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

Thanks for the review, Colin. I was just making sure druid-shell had a function to retrieve locales and was surprised to see this one missing.

I was also unsure about the > 1. My general thinking was that some default locale was better than an empty one but upon further consideration, I imagine the probability of Windows returning an empty string here is likely zero.

mem::zeroed() is also unsafe so would either need its own block or to be merged into the following one.

The arms of the branch in your suggestion don't match as the first yields an Option. I took the temporary rather than repeating "en-US".into() which seemed like more of a smell. We could just unwrap there and it's probably fine.

How about this?

Suggested change
unsafe {
let mut buf: [u16; LOCALE_NAME_MAX_LENGTH] = std::mem::zeroed();
let len_with_null = GetUserDefaultLocaleName(buf.as_mut_ptr(), buf.len() as _) as usize;
let locale = if len_with_null > 1 {
buf.get(..len_with_null - 1).and_then(FromWide::from_wide)
} else {
None
};
locale.unwrap_or_else(|| "en-US".into())
}
let mut buf: [u16; LOCALE_NAME_MAX_LENGTH];
let len_with_null = unsafe {
buf = std::mem::zeroed();
GetUserDefaultLocaleName(buf.as_mut_ptr(), buf.len() as _) as usize
};
if len_with_null > 0 {
buf.get(..len_with_null - 1).and_then(FromWide::from_wide).unwrap()
} else {
tracing::warn!("Failed to get user locale");
"en-US".into()
}

Copy link
Member

@cmyr cmyr Jul 15, 2021

Choose a reason for hiding this comment

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

Instead of mem::zeroed() we can just do,

let mut buf = [0u16; LOCALE_NAME_MAX_LENGTH];

I hadn't noticed that from_wide returns an Option. I do agree that it should never fail, but... idk. I could go either way, but your previous implementation definitely makes more sense to me, now. I do like avoiding unwraps in druid-shell, but it's hard to imagine a failure case here. (famous last words 😬)

Copy link
Contributor Author

@dfrg dfrg Jul 15, 2021

Choose a reason for hiding this comment

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

Instead of mem::zeroed() we can just do,

let mut buf = [0u16; LOCALE_NAME_MAX_LENGTH];

🤦

I hadn't noticed that from_wide returns an Option. I do agree that it should never fail, but... idk. I could go either way, but your previous implementation definitely makes more sense to me, now. I do like avoiding unwraps in druid-shell, but it's hard to imagine a failure case here. (famous last words 😬)

Completely agree on avoiding unwrap. I was unhappy about it as soon as I submitted the comment. Pushed a new commit that should address all these issues.

}
}