Update raw-window-handle to v0.6

[declantsien]

• Tested on all platforms changed
  Only tested on devices using Linux
• Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
• Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  None
• Created or updated an example program if it would help users understand this functionality

[kchibisov]

We already have #1582 for that, though we have an option for 0.6 just for the time being unless I have time to look into the safe handle stuff and how it could be approached in glutin.

[kchibisov]

I'd just post a review, since there's nothing wrong with just using raw version of 0.6, since it's just small update.

[MarijnS95]

A lot of changes here may snoop the implementation from ash-rs/ash#799 which - to my knowledge - treats (non)-nullable parameters accordingly.

[sarthakvk]

Is there a plan to merge this PR? @declantsien

[kchibisov]

In general, re-request the review with github UI when it's ready or explicitly indicate the state if you're not interested anymore.

[MarijnS95]

Overall seeing a bit too many semantical changes that I'm not immediately content with. raw-window-handle 0.6 gives us the ability to know that values are intentionally 0/null() by setting an Option to None, but we're not taking this opportunity to validate whether the underlying API can and should handle that value, or that we should bail. In some cases there are unwraps, or None propagation, or ? propagation making things harder to follow and validate.

[MarijnS95]

unwrap_or_else?

I'm unfamiliar with this code to say if it's okay to pass 0 for HINSTANCE. There are multiple getters but don't know if any should be used.

[kchibisov]

it's the same as before, because it could be 0, which is default for rwh 0.5 and lower.

[MarijnS95]

@kchibisov yes that is the point: it could have been passed before, but now that we know the raw-window-handle API allows zero, we should take a second to realize whether that is actually valid inside super::load_extra_functions().

[MarijnS95]

Our own safety docs already say that this must be "valid":

glutin/glutin/src/api/wgl/display.rs

Lines 40 to 44 in 052399d

	/// # Safety
	///
	/// The `native_window` must point to the valid platform window and have
	/// valid `hinstance`.
	pub unsafe fn new(

So I think this should be a panic!()/.unwrap()/ok_or...()?.

[MarijnS95]

Looking further than that, load_extra_functions() calls GetClassInfoExW() and CreateWindowExW(), both of which allow NULL, so the safety docs are likely too strict and should be removed if we keep this unwrap_or(0).

[declantsien]

So I think this should be a panic!()/.unwrap()/ok_or...()?.

I go with .unwrap().

[MarijnS95]

Yes that is a good idea. If people were previously implicitly passing 0 (mismatching the # Safety docs), we'll catch that and can always amend the code with a new patch release.

[declantsien]

@kchibisov Can you help with the CI error. Wired cfg errors.

[declantsien]

@kchibisov Can you help with the CI error. Wired cfg errors.

The error is from the rust nightly. Tried with an old build nightly-2024-04-05 the errors are gone. Since we don't have a rust-toolchain.toml file, no idea how to deal with it.
Another question, why don't we use Cargo.lock? Cargo.lock is useful for reproducible build.

[kchibisov]

Another question, why don't we use Cargo.lock? Cargo.lock is useful for reproducible build.

The old default behavior for rust was to not include Cargo.lock for libraries. They changed that to include last year iirc.

[kchibisov]

Should be good after rebase.

[declantsien]

Should be good after rebase.

Nice

[declantsien]

Another question, why don't we use Cargo.lock? Cargo.lock is useful for reproducible build.

The old default behavior for rust was to not include Cargo.lock for libraries. They changed that to include last year iirc.

Right. Good to know. Thanks.

[MarijnS95]

Another question, why don't we use Cargo.lock? Cargo.lock is useful for reproducible build.

A reproducible build (for libraries) is "useless" when a published crate is on crates.io. Only semver version (range) dependencies from Cargo.toml are taken into account when your crate is used in another project. cargo doesn't magically "merge" its Cargo.lock with the lockfile of the current project:

https://doc.rust-lang.org/cargo/faq.html#:~:text=However%2C%20this%20determinism%20can%20give%20a%20false%20sense%20of%20security%20because%20Cargo.lock%20does%20not%20affect%20the%20consumers%20of%20your%20package%2C%20only%20Cargo.toml%20does%20that.

[MarijnS95]

Question for @kchibisov: the changelog seems to have a mix of past- and present-tense. What should we pick?

[MarijnS95]

One more cleanup request for an unreachable if display.is_null(), but seems like we're getting somewhere otherwise.

Not happy with how valid open comments get marked as resolved.

[MarijnS95]

One item remaining, the rest looks good:

• Update raw-window-handle to v0.6 #1670 (comment)

[declantsien]

One item remaining, the rest looks good:

• Update raw-window-handle to v0.6 #1670 (comment)

@MarijnS95 I think I am done with your requests.

[marc2332]

Any reason this winit was left unchanged with rwh_05 instead of rwh_06 ?

I just noticed it when resolving conflicts in #1675

[kchibisov]

it's example, doesn't really matter. Could use 06.

[marc2332]

Yeah, seems to work just fine, I simply found it odd 😄