Implement Application::get_locale() for Windows backend

[dfrg]

No description provided.

[cmyr]

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. 🤷

[dfrg]

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()
	}

[cmyr]

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 😬)

[dfrg]

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.

[cmyr]

Great, I agree this looks like the right approach. Thanks!