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

Implement version 0.4 of the HasRawWindowHandle trait #2418

Merged
merged 1 commit into from Aug 11, 2022

Conversation

rib
Copy link
Contributor

@rib rib commented Aug 10, 2022

This makes Winit 0.27 compatible with crates like Wgpu 0.13 that are
using the raw_window_handle v0.4 crate and aren't able to upgrade to 0.5
until they do a new release (since it requires a semver change).

The change is intended to be self-contained (instead of pushing
the details into all the platform_impl backends) since this is only
intended to be a temporary trait implementation for backwards
compatibility that will likely be removed before the next Winit release.

Addresses: #2415

  • Tested on all platforms changed
  • 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
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

Update: I've marked it a tested since Emil left this comment about testing this branch with Egui: emilk/egui#1877 (comment)

Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

I'm fine with this, unless we find a way to do the semver trick again (I'll have a go at it as well as @maroider).

Cargo.toml Outdated Show resolved Hide resolved
@rib
Copy link
Contributor Author

rib commented Aug 10, 2022

I'm fine with this, unless we find a way to do the semver trick again (I'll have a go at it as well as @maroider).

We could use use this same code to do something similar to the semver trick (the terminology is maybe a bit muddled here though) where we could update the raw_window_handle 0.5 crate to depend on v0.4 so that it can provide a blanket implementation of the 0.4 trait in terms of the new one.

One trade-off there might be that it would be harder to back that out as a temporary change for backwards compatibility, which I think might be more reasonable to do here in Winit.

@emilk
Copy link
Contributor

emilk commented Aug 10, 2022

This is a really good idea from my point of view, and would unblock egui/eframe updating to winit 0.27 (provided it is followed up by a 0.27.2 patch release)

@rib
Copy link
Contributor Author

rib commented Aug 10, 2022

For reference, an alternative solution I'm experimenting with is the "semver trick" approach, which updates raw_window_handle 0.4 to implement its trait automatically for anything that implements the the new traits. This would effectively make wgpu compatible with anything built against raw_window_handle 0.5.

This is the blanket implementation I have for raw_window_handle 0.4 currently:

/// For forwards compatibility this provides a blanket implementation of the `RawWindowHandle`
/// for anything that is based on the newer 0.5 version of the `raw_window_handle` crate and
/// implements both `HasRawWindowHandle` and `HasRawDisplayHandle`
unsafe impl<T: raw_window_handle_05::HasRawWindowHandle + raw_window_handle_05::HasRawDisplayHandle> HasRawWindowHandle for T
{
    fn raw_window_handle(&self) -> RawWindowHandle {
        match raw_window_handle_05::HasRawWindowHandle::raw_window_handle(self) {
            raw_window_handle_05::RawWindowHandle::UiKit(window_handle) => {
                let mut handle = UiKitHandle::empty();
                handle.ui_view = window_handle.ui_view;
                handle.ui_window = window_handle.ui_window;
                handle.ui_view_controller = window_handle.ui_view_controller;
                RawWindowHandle::UiKit(handle)
            },
            raw_window_handle_05::RawWindowHandle::AppKit(window_handle) => {
                let mut handle = AppKitHandle::empty();
                handle.ns_window = window_handle.ns_window;
                handle.ns_view = window_handle.ns_view;
                RawWindowHandle::AppKit(handle)
            },
            raw_window_handle_05::RawWindowHandle::Orbital(window_handle) => {
                let mut handle = OrbitalHandle::empty();
                handle.window = window_handle.window;
                RawWindowHandle::Orbital(handle)
            },
            raw_window_handle_05::RawWindowHandle::Xlib(window_handle) => {
                if let raw_window_handle_05::RawDisplayHandle::Xlib(display_handle) = raw_window_handle_05::HasRawDisplayHandle::raw_display_handle(self) {
                    let mut handle = XlibHandle::empty();
                    handle.display = display_handle.display;
                    handle.window = window_handle.window;
                    handle.visual_id = window_handle.visual_id;
                    RawWindowHandle::Xlib(handle)
                } else {
                    panic!("Unsupported display handle associated with Xlib window");
                }
            },
            raw_window_handle_05::RawWindowHandle::Xcb(window_handle) => {
                if let raw_window_handle_05::RawDisplayHandle::Xcb(display_handle) = raw_window_handle_05::HasRawDisplayHandle::raw_display_handle(self) {
                    let mut handle = XcbHandle::empty();
                    handle.connection = display_handle.connection;
                    handle.window = window_handle.window;
                    handle.visual_id = window_handle.visual_id;
                    RawWindowHandle::Xcb(handle)
                } else {
                    panic!("Unsupported display handle associated with Xcb window");
                }
            },
            raw_window_handle_05::RawWindowHandle::Wayland(window_handle) => {
                if let raw_window_handle_05::RawDisplayHandle::Wayland(display_handle) = raw_window_handle_05::HasRawDisplayHandle::raw_display_handle(self) {
                    let mut handle = WaylandHandle::empty();
                    handle.display = display_handle.display;
                    handle.surface = window_handle.surface;
                    RawWindowHandle::Wayland(handle)
                } else {
                    panic!("Unsupported display handle associated with Xcb window");
                }
            },
            raw_window_handle_05::RawWindowHandle::Win32(window_handle) => {
                let mut handle = Win32Handle::empty();
                handle.hwnd = window_handle.hwnd;
                handle.hinstance = window_handle.hinstance;
                RawWindowHandle::Win32(handle)
            },
            raw_window_handle_05::RawWindowHandle::WinRt(window_handle) => {
                let mut handle = WinRtHandle::empty();
                handle.core_window = window_handle.core_window;
                RawWindowHandle::WinRt(handle)
            },
            raw_window_handle_05::RawWindowHandle::Web(window_handle) => {
                let mut handle = WebHandle::empty();
                handle.id = window_handle.id;
                RawWindowHandle::Web(handle)
            },
            raw_window_handle_05::RawWindowHandle::AndroidNdk(window_handle) => {
                let mut handle = AndroidNdkHandle::empty();
                handle.a_native_window = window_handle.a_native_window;
                RawWindowHandle::AndroidNdk(handle)
            },
            raw_window_handle_05::RawWindowHandle::Haiku(window_handle) => {
                let mut handle = HaikuHandle::empty();
                handle.b_window = window_handle.b_window;
                handle.b_direct_window = window_handle.b_direct_window;
                RawWindowHandle::Haiku(handle)
            },
            _ => panic!("No HasRawWindowHandle version 0.4 backwards compatibility for new Winit window type"),
        }
    }
}

The problem with that is that I'm not sure atm how to avoid conflicting with with the other blanket implementation that exists in the raw_window_handle crate:

unsafe impl<'a, T: HasRawWindowHandle + ?Sized> HasRawWindowHandle for &'a T {
    fn raw_window_handle(&self) -> RawWindowHandle {
        (*self).raw_window_handle()
    }
}

I tried adding a + Sized requirement for the new impl to try and avoid conflicting with this other implementation but that didn't work. maybe someone more familiar with how rust trait bounds work has a clue for how to let these both exist together?

@madsmtm
Copy link
Member

madsmtm commented Aug 10, 2022

Heh, I tried this in rust-windowing/raw-window-handle#97 as well, I don't think it's possible to do (without specialization or other unstable features)

@madsmtm
Copy link
Member

madsmtm commented Aug 10, 2022

Yet another alternative: Make version 0.5 not a breaking change (add type-aliases and deprecated fields), though that only delays the issue until when we want to actually remove the deprecated functionality...

Edit: See rust-windowing/raw-window-handle#99

@rib
Copy link
Contributor Author

rib commented Aug 10, 2022

Yet another alternative: Make version 0.5 not a breaking change (add type-aliases and deprecated fields), though that only delays the issue until when we want to actually remove the deprecated functionality...

My initial approach had been to handle this in 0.5 and depend on the 0.4 crate but the orphan rules (or something like that) mean you can't provide a blanket implementation for the 0.4 trait that's not implemented in the current crate - so then it wouldn't provide compatibility with wgpu that depends on the 0.4 crate.

@madsmtm
Copy link
Member

madsmtm commented Aug 10, 2022

My initial approach had been to handle this in 0.5 and depend on the 0.4 crate but the orphan rules (or something like that) mean you can't provide a blanket implementation for the 0.4 trait that's not implemented in the current crate - so then it wouldn't provide compatibility with wgpu that depends on the 0.4 crate.

Yeah, those blanket traits are really interfering with the semver trick, whichever way you do it

@rib
Copy link
Contributor Author

rib commented Aug 10, 2022

atm I guess I would er towards handling the backwards compatibility in Winit - I think mainly because it seems cleaner to just remove it before releasing 0.28 and it just provides a short-term stop-gap until wgpu updates to 0.5

@madsmtm
Copy link
Member

madsmtm commented Aug 10, 2022

I'm fine with either

@kchibisov
Copy link
Member

Winit should work around that issue, since it'll be solved by either a wgpu or winit release. Given that wgpu is harder to bump we can solve it in winit, since there's a clear demand from the consumers.

raw-window-handle shouldn't be touched at all here, since its api is stable to some point and keeping shim for old api will result in a more breaking change in the future.

@rib
Copy link
Contributor Author

rib commented Aug 11, 2022

Just for reference Emil mentioned here that they were able to test this branch with Egui and it seemed to work as intended: emilk/egui#1877 (comment)

src/window.rs Outdated Show resolved Hide resolved
This makes Winit 0.27 compatible with crates like Wgpu 0.13 that are
using the raw_window_handle v0.4 crate and aren't able to upgrade to 0.5
until they do a new release (since it requires a semver change).

The change is intended to be self-contained (instead of pushing
the details into all the platform_impl backends) since this is only
intended to be a temporary trait implementation for backwards
compatibility that will likely be removed before the next Winit release.

Addresses: rust-windowing#2415
@rib
Copy link
Contributor Author

rib commented Aug 11, 2022

I just noticed that I needed to rebase my patch on top of a "breaking" change conflict in the changelog which I guess will need to be branched around now to be able to spin a 0.27.2 release with this patch.

That's kinda awkward timing

@kchibisov
Copy link
Member

I don't think that Windows change is actually a breaking change. Given that missing support for the Api was added. I'll send a patch to correct that.

@rib
Copy link
Contributor Author

rib commented Aug 11, 2022

I don't think that Windows change is actually a breaking change. Given that missing support for the Api was added. I'll send a patch to correct that.

ah, would make things easier if that's the case 🤞

@maroider
Copy link
Member

I don't think that Windows change is actually a breaking change.

AIUI, it changes the default behavior for device events from "always delivered" to "delivered when application is focused", which I would argue is techinally breaking, although I suspect very few people have actually updated to 0.28.0 (since we introduced the API in said version), so the breakage will likely be minor. The "best practice" is to ignore device events when you don't have the focus anyway, so that is likely also a mitigating factor.

@kchibisov kchibisov merged commit 11d4a30 into rust-windowing:master Aug 11, 2022
@kchibisov
Copy link
Member

AIUI, it changes the default behavior for device events from "always delivered" to "delivered when application is focused", which I would argue is techinally breaking, although I suspect very few people have actually updated to 0.28.0 (since we introduced the API in said version), so the breakage will likely be minor. The "best practice" is to ignore device events when you don't have the focus anyway, so that is likely also a mitigating factor.

The documented default behavior is Auto, so if someone relies on Never, they should explicitly state that by using a function. So it's not a breaking change, since relying on that means that they rely on missing impl in winit.

@rib rib deleted the raw_window_handle_04 branch November 29, 2022 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants