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.

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

ndk/native_window: Implement HasWindowHandle for 0.4 and 0.6 (and maintain 0.5) #434

Merged
merged 1 commit into from
Oct 8, 2023

Conversation

MarijnS95
Copy link
Member

@MarijnS95 MarijnS95 commented Oct 7, 2023

Following the same strategy in winit, move raw-window-handle 0.5 behind a rwh_05 feature and add raw-window-handle 0.4 and the newly released raw-window-handle 0.6 behind a corresponding rwh04/rwh_06 feature. The new non-raw trait and lifetimed type since 0.6 have been implemented instead, as raw-window-handle provides blanket implementations to coerce/convert back to Raw* types.

ndk/src/native_window.rs Outdated Show resolved Hide resolved
@MarijnS95
Copy link
Member Author

@kchibisov perhaps you also want to take a look at this PR in conjunction with rust-windowing/winit#3126?

Copy link
Contributor

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

In winit we kept the raw-window-handle v0.4, which is missing here. It would be nice to have it as well?

@MarijnS95
Copy link
Member Author

@kchibisov we never had it so I don't really feel like it adds any value unless there are compelling reasons to? Perhaps to keep the implementations in winit consistent?

@MarijnS95
Copy link
Member Author

In addition, what do folks think about having any of these features in the default set (e.g. the latest rwh_06)?

@kchibisov
Copy link
Contributor

@MarijnS95 yes, just for consistency reasons. But I don't care that much if it's fine with you.

In addition, what do folks think about having any of these features in the default set (e.g. the latest rwh_06)?

Whatever you prefer as a library writer. You could suggest updating to 0.6.0 if you do breaking changes anyway.

@MarijnS95 MarijnS95 changed the title ndk/native_window: Implement HasWindowHandle for 0.6 (and maintain 0.5) ndk/native_window: Implement HasWindowHandle for 0.4 and 0.6 (and maintain 0.5) Oct 8, 2023
@MarijnS95
Copy link
Member Author

@kchibisov added. I'll think about the default features for a bit... 0.5 used to be available OOTB so I feel like that "please upgrade" nudge might work if we provide 0.6 by default as well. default-features = false is pretty easy.

…aintain 0.5)

Following [the same strategy in winit], move `raw-window-handle 0.5`
behind a `rwh_05` feature and add `raw-window-handle 0.4` and the newly
released `raw-window-handle 0.6` behind a corresponding `rwh04`/`rwh_06`
feature.  The new **non-raw** trait and **lifetimed** type since `0.6`
have been implemented instead, as `raw-window-handle` provides blanket
implementations to coerce/convert back to `Raw*` types.

[the same strategy in winit]: rust-windowing/winit#3126
@MarijnS95 MarijnS95 merged commit 3b3aae9 into master Oct 8, 2023
4 of 6 checks passed
@MarijnS95 MarijnS95 deleted the rwh-0.6 branch October 8, 2023 19:24
@MarijnS95 MarijnS95 restored the rwh-0.6 branch October 8, 2023 19:39
@MarijnS95 MarijnS95 deleted the rwh-0.6 branch October 8, 2023 20:00
@MarijnS95 MarijnS95 restored the rwh-0.6 branch October 8, 2023 20:01
@MarijnS95 MarijnS95 deleted the rwh-0.6 branch October 8, 2023 20:06
Vrixyz added a commit to Vrixyz/android-activity that referenced this pull request Oct 25, 2023
The `ndk` crate enables `raw-window-handle 0.6` by default (because of rust-mobile/ndk#434 (comment)) which might not be used by consumers of the `android-activity` crate at all, or might (still) be a mismatching version. Even if the `rwh_0x` features are additive, figuring that out leads to cryptic errors and it is best to turn off these defaults completely and leave it to the user to turn it back on in their own `[dependencies]` section if desired.
MarijnS95 pushed a commit to rust-mobile/android-activity that referenced this pull request Oct 25, 2023
The `ndk` crate enables `raw-window-handle 0.6` by default (because of
rust-mobile/ndk#434 (comment))
which might not be used by consumers of the `android-activity` crate
at all, or might (still) be a mismatching version. Even if the `rwh_0x`
features are additive, figuring that out leads to cryptic errors and it
is best to turn off these defaults completely and leave it to the user
to turn it back on in their own `[dependencies]` section if desired.
MarijnS95 added a commit to rust-windowing/winit that referenced this pull request Nov 3, 2023
We decided to add `rwh_06` to the `default` list of features in the
`ndk` to [nudge users to upgrade], but this forces `winit` to always
(transitively) include `raw-window-handle 0.6` even if the user has
set a different `rwh_xx` feature on the `winit` crate.  `winit` already
forwards the respective `rwh_xx` feaure to the `ndk` crate anyway, so
this default should just be turned off.

At the time of writing this is the only `default` feature of the `ndk`.

[nudge users to upgrade]: rust-mobile/ndk#434 (comment)
kchibisov pushed a commit to rust-windowing/winit that referenced this pull request Nov 4, 2023
We decided to add `rwh_06` to the `default` list of features in the
`ndk` to [nudge users to upgrade], but this forces `winit` to always
(transitively) include `raw-window-handle 0.6` even if the user has
set a different `rwh_xx` feature on the `winit` crate.  `winit` already
forwards the respective `rwh_xx` feaure to the `ndk` crate anyway, so
this default should just be turned off.

At the time of writing this is the only `default` feature of the `ndk`.

Links: rust-mobile/ndk#434 (comment)
kchibisov pushed a commit to kchibisov/winit that referenced this pull request Nov 24, 2023
We decided to add `rwh_06` to the `default` list of features in the
`ndk` to [nudge users to upgrade], but this forces `winit` to always
(transitively) include `raw-window-handle 0.6` even if the user has
set a different `rwh_xx` feature on the `winit` crate.  `winit` already
forwards the respective `rwh_xx` feaure to the `ndk` crate anyway, so
this default should just be turned off.

At the time of writing this is the only `default` feature of the `ndk`.

Links: rust-mobile/ndk#434 (comment)
kchibisov pushed a commit to rust-windowing/winit that referenced this pull request Nov 24, 2023
We decided to add `rwh_06` to the `default` list of features in the
`ndk` to [nudge users to upgrade], but this forces `winit` to always
(transitively) include `raw-window-handle 0.6` even if the user has
set a different `rwh_xx` feature on the `winit` crate.  `winit` already
forwards the respective `rwh_xx` feaure to the `ndk` crate anyway, so
this default should just be turned off.

At the time of writing this is the only `default` feature of the `ndk`.

Links: rust-mobile/ndk#434 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants