Add an invalid raw handle

[ogoffart]

When we want to return an empty or invalid handle, it is possible to create an empty handle for an arbitrary platform, but if there is a dedicated enum value for it, then we don't have to pick an arbitrary platform.

The usecase here is to implement HasRawWindowHandle for a Window struct that can be backed by any backed decided at runtime (example winit, or no backend at all). And the question is what handle to return when we don't have an handle. An Invalid one seems a better choice than Haiku(HaikuDisplayHandle::empty()) (just an arbitrary random pick)

What do you think?

[kchibisov]

In case of display handles empty is a correct thing actually, since if platform doesn't have any display any is valid. Like on macOS.

Only a few has real display handle like Wayland. What you're trying to solve is already done via Result. I don't think that adding Invalid provides any real value. It's also a breaking change(logically), since you want to abort on Invalid or throw error back, which just sounds flowed.

[ogoffart]

Thanks for your answer. The thing is that HasRawWindowHandle doesn't return a Result, so that can't be done with a result.

Here is my situation:

unsafe impl HasRawWindowHandle for MyWindow {
    fn raw_window_handle(&self) -> RawWindowHandle {
        match &self.backend {
            // forward to the backend's raw_window_handle implementation
            Some(b) => b.raw_window_handle(),
            None => {
               // What should i answer here?  Can't be a Result.
               //RawWindowHandle::Invalid
               // This is a bit awkward.    An explicit Invalid would be nicer.
               RawWindowHandle::Haiku(HaikuDisplayHandle::empty())
            }
        } 
    }
}

Do you suggest returning an invalid handle from an arbitrary enum? Or should MyWindow not implement HasRawWindowHandle?, instead, there could be a fn MyWindow::as_has_window_handle(&self) -> Option<&dyn HasRawWindowHandle> but i'm not sure that's a nicer API.

It's also a breaking change(logically), since you want to abort on Invalid or throw error back, which just sounds flowed.

I don't think so, because RawWindowHandle is #[non_exhaustive], and you would do the same for Invalid as what _ => does

[kchibisov]

Could you not implement it for Haiku target?

Also, you just build and return you don't have to implement for haiku? You clearly know what type of RawWindowHandle your backend returns, so you can clearly use unreachable!(). Your window should return a fixed set of entries it shouldn't return all of them.

[ogoffart]

Well, Haiku was just an example, I could have returned anything in there. The point is that the platform is not Haiku (well, or maybe it is), we don't know what the platform is so i just return "something" at random.

You clearly know what type of RawWindowHandle your backend returns,

No i don't know what my backend return. the backend is anything that implements RawWindowHandle (Imagine backend is a Option<Box<dyn RawWindowHandle>>)

so you can clearly use unreachable!().

I don't want to panic in that case. I want to return something invalid that could be passed along to any other crate, and since they wouldn't know what to do with that handle, they would probably return an error.

Your window should return a fixed set of entries it shouldn't return all of them.

I'm not sure what you mean by that.

[madsmtm]

so you can clearly use unreachable!().

I don't want to panic in that case. I want to return something invalid that could be passed along to any other crate, and since they wouldn't know what to do with that handle, they would probably return an error.

I agree with @kchibisov here, it seems like you should just panic in this case, since there really is nothing to do for the HasRawWindowHandle consumer if you don't have any handle.

Alternatively, you could always do something like:

None => {
    #[cfg(target_os = "ios")]
    RawWindowHandle::UIKit(UIKitDisplayHandle::empty())
    #[cfg(target_os = "macos")]
    RawWindowHandle::AppKit(AppKitDisplayHandle::empty())
    #[cfg(target_os = "linux", ...)]
    RawWindowHandle::X11(X11DisplayHandle::empty())
    // ...
}

(which is clearly still flawed, but may be better than only using one)

[ogoffart]

it seems like you should just panic in this case, since there really is nothing to do for the HasRawWindowHandle consumer if you don't have any handle.

The HasRawWindowHandle consuler can report the same error as when we give empty handle of any platform.

[Lokathor]

The lib providing a handle should not panic, and the lib consuming the handle shouldn't panic either.

The bin controlling both libs should probably not even panic, there's much more graceful ways to report an error to the user and close up shop than giving them a panic message.

[madsmtm]

Hmm, let me reframe.

I think I understand your use-case of wanting to swap-out the backend @ogoffart - but I believe that the desire to have a window that's "backed by no backend at all" is wrong, in the sense that such a situation should either be impossible at compile-time, or be considered a programmer error (hence why I think a panic here is acceptable).

If not, could you elaborate on why it would be a user-error that no windowing backend exist at runtime? Or why you couldn't just do something like:

struct MyWindow(Box<dyn RawWindowHandle>);

impl HasRawWindowHandle for MyWindow { ... }

struct MaybeMyWindow(Option<MyWindow>);

impl MaybeMyWindow {
    fn get(&self) -> Option<&MyWindow>;
}

[ogoffart]

Thank you all for taking the time to review this PR.

To put a bit of context, the issue i'm trying to solve is slint-ui/slint#877: Exposing the raw window handle from Slint.

Based on your feedback, it seems that you are recommending the use of a getter that returns an Option:

impl Window {
     pub fn raw_handles<'a>(&'a self) -> Option<impl HasRawWindowHandle + HasRawDisplayHandle + 'a> {
         // return a wrapper around the backend if there is one.
     }
}

Note that I still believe it would be more practical to implement the traits directly on the Window and allow it to be passed to consumer libraries. This way, the consumer libraries can simply report an error if the handle is unknown or invalid. In my opinion, this approach would be less work for the user crate (the one that passes the HRWH around) as it would only need to handle the error from the consuming crate, rather than both the error, and a None value from the window provider.

[MarijnS95]

Note that I still believe it would be more practical to implement the traits directly on the Window and allow it to be passed to consumer libraries. This way, the consumer libraries can simply report an error if the handle is unknown or invalid. In my opinion, this approach would be less work for the user crate (the one that passes the HRWH around) as it would only need to handle the error from the consuming crate, rather than both the error, and a None value from the window provider.

Fwiw in my app I am also always fighting whether to pass a "window" around (opaque thing that implements HRWH) or the RWH directly, which is cheaply copyable and doesn't need to have a lifetime/borrow. That is also an advantage for the former, where liveliness of the window (for a RWH to stay valid / be retrievable) becomes an explicit part of your Rust API design.

IIRC @kchibisov tried to remove the traits a while ago, but ended up not doing so because it'd break ABI. Furthermore, the trait isn't implemented for RWH either making it impossible to use any API that accepts the trait if you already have a RWH/RDH. Having that would make your proposed fn raw_handles() more convenient though.

For that reason Ash for example takes the value type directly: ash-rs/ash#645, to not force/restrict the caller to rely on a trait-implementer. Perhaps we should ask ourselves again what value the trait is adding?

[ogoffart]

@MarijnS95

Any API that accept a RWH directly would have to be unsafe, because the raw pointer inside the RWH could be dangling.
Implementing HRWH for the RWH would be unsound, because the trait contract states that it can only return valid handle.
Passing a "window" around allow for safe APIs

[kchibisov]

IIRC @kchibisov tried to remove the traits a while ago, but ended up not doing so because it'd break ABI.

You can't break something that doesn't exist. But yes, a lot of crates rely on taking &impl Trait, and I think that it's fine.

For that reason Ash for example takes the value type directly: ash-rs/ash#645, to not force/restrict the caller to rely on a trait-implementer. Perhaps we should ask ourselves again what value the trait is adding?

Any API that accept a RWH directly would have to be unsafe, because the raw pointer inside the RWH could be dangling.
Implementing HRWH for the RWH would be unsound, because the trait contract states that it can only return valid handle.
Passing a "window" around allow for safe APIs

That's not true unless you model the lifetimes as well. Since you must ensure it yourself, so it's unsafe in the end. If you look at the wgpu it takes impl &Trait but the api is still unsafe, since you must ensure the lifetime yourself. So your argument is mostly irrelevant, since you can't really design the API in a way that it'll take the window... At least I don't know such a crate doing so with rwhnd.

[ogoffart]

If you look at the wgpu it takes impl &Trait but the api is still unsafe, since you must ensure the lifetime yourself. So your argument is mostly irrelevant, since you can't really design the API in a way that it'll take the window...

For example rfd::FileDialog::set_parent is safe:
https://docs.rs/rfd/latest/rfd/struct.FileDialog.html#method.set_parent

I thought this was the whole point of this trait, allowing a safe way to pass the handle from a provider crate (eg, winit) to a consumer crate (eg, rfd)

[Lokathor]

yes, that's the point of the trait.

[MarijnS95]

@ogoffart That's what I said. However, Ash being a low-level wrapper with no concept of lifetimes on raw Vulkan handles like vk::SurfaceKHR means we cannot propagate this, hence the aforementioned API is marked unsafe.

@kchibisov removing public API is considered a breaking change.

[kchibisov]

Continuing on this thread, maybe we should make HasRaw{Window,Display}Handle return a Result instead for v1.0.0? I just don't like hiding the error inside the normal enum.

The issue I could see is that we bleed the error into the Api's accepting something implementing HasRawWindowHandle, but such code should already be in place.

@Lokathor how do you feel about such change to the trait? The Error could be simply a NotSupported.

[Lokathor]

Actually yeah, I would definitely prefer the error outside the enum, so people use Result<Handle, Error> or similar.

[kchibisov]

@ogoffart, if you still want to go forward on this PR, I'd suggest to refactor the traits to return the Result instead.

[ogoffart]

I think it is important for such crate to not break semver compatibility otherwise it break the ecosystem.
Changing the signature of the trait would be a breaking change, and i don't think it is justified.

I think a Invalid handle is harmless. And while i'm still wondering how to have a safe interface that proxy the HasRaw*Handle, i think i'll just go with a null value for some random backend.

[kchibisov]

I think it is important for such crate to not break semver compatibility otherwise it break the ecosystem.

We can break semver however we feel like until 1.0.0. Having poor code in the first place is the least thing I want to over see.

The Result or simply Option makes the most sense.

[Lokathor]

As long as we don't do it too often it's reasonable to have breaking changes. If people need to have fallibility much more than was initially expected, a change in the API to use the proper error handling types is preferred, particularly in the long term. One week or even one month of small trouble isn't a big deal compared to hopefully using this library for the next many years.

[rib]

I just raised a related issue for Winit (rust-windowing/winit#2769) while I don't really like how the Android backend for Winit will currently simply panic if you try and query a raw-window-handle while the app is suspended, and I'd much rather return an Option to indicate that no handle is currently available.

I also think that Winit's Wayland backend could potentially benefit from being able to return None for the raw-window-handle until it gets a configuration ack from the compositor after creating a new Window.

In terms of invalid enum, vs Result vs Option, I think my instinct would be to go for an Option.

A special enum could potentially make sense if it was useful for the consumer to be able to differentiate the "no backend" situation from the "currently unavailable" situation that could apply to existing backends (such as for Android, and potentially Wayland).

[kchibisov]

But isn't Result more robust? You can simply encode NotReady and NotSupport into it? rust already has similar flow with io::Result with error values meaning "retry later", like WouldBlock.

[rib]

But isn't Result more robust? You can simply encode NotReady and NotSupport into it? rust already has similar flow with io::Result with error values meaning "retry later", like WouldBlock.

Yeah, I suppose the "currently unavailable" case is basically an error condition that would make sense as a more descriptive NotReady error of some kind.

[ids1024]

Any API that accept a RWH directly would have to be unsafe, because the raw pointer inside the RWH could be dangling. Implementing HRWH for the RWH would be unsound, because the trait contract states that it can only return valid handle. Passing a "window" around allow for safe APIs

This particular issue should be addressed by the the new WindowHandle<'a> type. You can safely accept that, unlike RawWindowHandle. And it implements HasRawWindowHandle too, if you need to use it with an API accepting that.

So if you have something that may or may not be backend in a window handle, you can safely provide a method returning Option<WindowHandle<'a>>, which should work for the use case this issue is about? Which is possibly better than making HasRawWindowHandle/HasWindowHandle return an Option/Result if most implementations would always have a valid handle.

That said...

I just raised a related issue for Winit (rust-windowing/winit#2769) while I don't really like how the Android backend for Winit will currently simply panic if you try and query a raw-window-handle while the app is suspended, and I'd much rather return an Option to indicate that no handle is currently available.

...this seems like a more compelling case. Panicking here doesn't sound great. Would this case on Android be the only instance in which Winit would return anything other than a valid window handle?

[rib]

Would this case on Android be the only instance in which Winit would return anything other than a valid window handle?

A case could be made for returning an invalid handle on wayland before getting a configuration acknowledgement from the compositor, and allowing Window::new() to return without forcing a synchronous round-trip to the server.

[ids1024]

Would that require returning an invalid handle, though? WaylandWindowHandle just contains a wl_surface pointer. A roundtrip isn't required for that to be valid. It's not necessarily guaranteed that the surface even is an xdg_toplevel.

[kchibisov]

Yeah, with Wayland only some protocols require a configure dance on the startup, and it doesn't make such surface invalid right away, you can use it to initialize your EGL platform, you just can't draw with it in that case, unless you got a configure.

[kchibisov]

Closing in favor of #122.