Make raw_*_handle return a result

[notgull]

This is a breaking change. Supersedes #104.

[Lokathor]

This looks great, but I want others to have a chance to raise any concerns before we merge it.

[notgull]

Sounds good! I'll loop in @ogoffart since this was originally their idea.

[madsmtm]

My primary objection to this is still: How will this look for the user? Like, how are they expected to actually handle the error?

As I see it, most people will just end up doing an unwrap, since:

• They are correctly managing event lifecycles on Android, meaning that the handle will always be available.
• Or for Slint, the window they want to render into will be backed by a backend.

If any of those are not true, then that's a programmer error, and i.e. a good use of panics, not something that needs to be recoverable.

[madsmtm]

Error Display text should start with a lowercase letter

[notgull]

Is there a precedent for this? I've always started my error messages with a capital letter.

[madsmtm]

Well the Error trait itself recommends it

[madsmtm]

What is the use-case for this?

The use-case discussed in #104 would use Unavailable, since that's what most accurately describes their situation?

[kchibisov]

When the library can't provide a any handle for the current platform, but still somehow built for it. Or when multiple could be provided at the same time, like XLIB and XCB, but it was compiled without xlib, so it'll be not-available in generic cfg less code.

[notgull]

The idea is that:

• NotSupported is when the underlying windowing system does not support returning any of the C window handles. For instance, if you're using a pure Rust implementation of X11 or Wayland, or if you're using something that raw-window-handle doesn't support (like a game console).
• Unavailable is when the window is temporarily not available. The use case here is Android after a suspend event. There are probably others.

Handling these two error conditions may require separate code for certain cases.

[madsmtm]

Would be cool to have some of those example uses in the docs

[ogoffart]

Thanks for keeping me in the loop.

I think that now that there are the safe handle, i'm not going to implement the HasRaw*Handle for my window type. I'm just going to use the new non-raw handle. The Has*Handle (without Raw) already return an error. So that's good enough (although NotSupported would be a nice extension to the error)

But What is concerns me is the fact that it is a breaking change that this will require a semver version increase which will fragment the ecosystem.
The whole point of this crate is that it can be used to glue other crates together, and it is exposed in the public api of other crates. So this crate should be trying to limit its major version increase.
So I'd not do that now and wait with this change until there are other good reason to require a major version upgrade.

[madsmtm]

The Has*Handle (without Raw) already return an error.

I think HasRawWindowHandle and HasWindowHandle should return the same thing, so just as I think HasRawWindowHandle shouldn't return an error, so do I think HasWindowHandle shouldn't.

So this crate should be trying to limit its major version increase.

Agreed. Or alternatively use the semver trick to avoid the issues.

[notgull]

If the choice is between "return an error" or "not returning an error", I would say we should return an error. On the Android winit backend, there is a case where the function panics if the window is currently suspended. I would rather return an error than panic in this case.

[kchibisov]

We could have similar issue as Android with Wayland layer shell windows, so it's a good idea in general.

Ack from me.

[madsmtm]

n the Android winit backend, there is a case where the function panics if the window is currently suspended. I would rather return an error than panic in this case.

I understand; I'm trying to figure out how the user is expected to handle that error?

[Lokathor]

Any handling more graceful than a panic is a good idea, honestly.

[notgull]

But What is concerns me is the fact that it is a breaking change that this will require a semver version increase which will fragment the ecosystem.

The whole point of this crate is that it can be used to glue other crates together, and it is exposed in the public api of other crates. So this crate should be trying to limit its major version increase.

The tree of robust architecture must be refreshed from time to time with the blood of breaking changes. Making the HasRawWindowHandle trait have an infallible function was reasonable at the time that the trait was created but has become unreasonable in the modern day. The current model already results in panics that can be avoided through the use of Rust's error handling system. Keeping the model as it is right now will result in code that assumes that raw_window_handle is infallible; code that is incorrect.

To address the comments above, except for obvious cases that are blatantly programmer bugs (like using the wrong ordering in compare_exchange or using zero-sized chunks in array_chunks), it is never correct to design an API where panicking is an expected operation. By definition, panics are not expected to be recovered from. If an API user wants to panic if the window handle is inaccessible, they can do that with this API. The added complexity is minimal; only an unwrap() or an if let Ok(..) in the vast majority of cases will be necessary. At worst, the program will never become less robust that it currently is.

We already have a breaking change that needs to be made (the removal of Active). There are other breaking changes that may be on the table as well. For instance, with the introduction of borrowed window handles, does HasRawWindowHandle still need to be unsafe?

[madsmtm]

obvious cases that are blatantly programmer bugs

What I'm not convinced of is that this isn't such a case? If the application has been suspended, you shouldn't even try to do any rendering, i.e. trying to access the window handle in that case is a "blatant programmer bug". It is not something the application can recover from, it is something that must change in the code.

Am I missing something here? Is there a feasible way to recover? If someone can give me a wgpu, glutin or softbuffer + winit example where the error was properly handled (and not just unwrapped), my concerns would be sated.

[kchibisov]

It's not clear to me whether this is a breaking change or not.

I think the CHANGELOG is sort of desynced, given that we have 0.5.2 on crates.io and the file is at 0.5.1.

[Lokathor]

That's probably my fault on the changelog. I'm not sure I'll have time to look into it myself today, but I'll definitely have to fix it before the next release, yeah.

[notgull]

It's not clear to me whether this is a breaking change or not.

This is a breaking change since it modifies the return value of a method of a trait in a backwards-incompatible way.

I think the CHANGELOG is sort of desynced, given that we have 0.5.2 on crates.io and the file is at 0.5.1.

Fixed in #124

[kchibisov]

This is a breaking change since it modifies the return value of a method of a trait in a backwards-incompatible way.

Then it should be stated in the changelog.

[notgull]

What I'm not convinced of is that this isn't such a case? If the application has been suspended, you shouldn't even try to do any rendering, i.e. trying to access the window handle in that case is a "blatant programmer bug". It is not something the application can recover from, it is something that must change in the code.

Am I missing something here? Is there a feasible way to recover? If someone can give me a wgpu, glutin or softbuffer + winit example where the error was properly handled (and not just unwrapped), my concerns would be sated.

Let's say I create a windowing framework that may use either a pure Rust implementation of X11 or the libxcb implementation. Let's also say that I have a rendering library built on top of this windowing framework. I want this renderer to use a hardware implementation like wgpu if it can, but fall back to software rendering if it can't. In that case I would have code like this:

if let Ok(handle) = my_window.window_handle() {
    use_wgpu_to_render_to_this_handle(handle);
} else {
    use_software_rasterization();
}

Granted, a similar affect could be achieved with feature flags, but this is more idiomatic because it doesn't rely on out-of-band compiler mechanisms.

For the Unavailable use case, it's good to be fault-tolerant of mistakes, especially ones that can come up during lifecycle handling. I would argue that even experienced programmers can and will make mistakes over what is valid during what parts of the GUI lifecycle. At the very least, something like this should be possible:

if let Ok(handle) = my_window.window_handle() {
    // Do rendering here.
}

// If we can't access the window handle, just do nothing. It's better than panicking
// here, especially in production.

Then it should be stated in the changelog.

Fixed!

[madsmtm]

Thanks for the examples @notgull!

I still feel like the argument is kinda weak, if the issue is that people make mistakes regarding lifetime management, then we should work on making that easier to avoid, not adding a hack to this library.

But I realize that I'm in the minority, and probably arguing about something fundamentally inconsequential, so I'll withdraw my objections.

[notgull]

@rib or @MarijnS95, if you have a moment could you provide your thoughts regarding Android. The Unavailable error case exists mostly to make Android support a little easier.

[rib]

NB there is also a potential wayland use case here too. The wayland winit backed currently jumps through significant hoops to make it look like there is no async configure where it can't return a valid handle beforehand. It leads to the whole backend buffering wayland events which shouldn't be necessary.

[kchibisov]

The wayland winit backed currently jumps through significant hoops to make it look like there is no async configure where it can't return a valid handle beforehand. It leads to the whole backend buffering wayland events which shouldn't be necessary.

That's not correct, the buffering is due to winit not using &mut and me refusing do the unsafe transmute, because it's not maintainable for no reason, you get events in the batches anyway.

The Wayland use case I've mentioned above, and layer shell behaves exactly like Android, where surface destroys when you power off or dpms the output. So it's required for it as well.

[rib]

The wayland winit backed currently jumps through significant hoops to make it look like there is no async configure where it can't return a valid handle beforehand. It leads to the whole backend buffering wayland events which shouldn't be necessary.

That's not correct, the buffering is due to winit not using &mut and me refusing do the unsafe transmute, because it's not maintainable for no reason, you get events in the batches anyway.

I might be misremembering some details but, from what I recall they are related. The backend wouldn't be needing to use &mut if it wasn't trying to insert a synchronous roundtrip to the compositor within Window::new(), which it's doing to ensure the surface is configured.

If the backend could expose the fact that you can't query a raw handle until the configure comes back (asynchronously) from the compositor then it seems like it would be possible to remove any need for a round trip outside of the main event loop and so there would be no need to buffer events in the wayland backend.

[rib]

@rib or @MarijnS95, if you have a moment could you provide your thoughts regarding Android. The Unavailable error case exists mostly to make Android support a little easier.

This change looks good to me, thanks!

[kchibisov]

If the backend could expose the fact that you can't query a raw handle until the configure comes back (asynchronously) from the compositor then it seems like it would be possible to remove any need for a round trip outside of the main event loop and so there would be no need to buffer events in the wayland backend.

I think what we could do is to make a window return just WindowId(we know it ahead of time) and the real window will be send to you later on or immediately if it's possible.

[notgull]

Alright, it's been a week. Does anyone else have any comments before this is merged?

I've already reached out to the wgpu people and they seem fine with it.

cc @rust-windowing/winit, @rust-windowing/glutin, since you're the primary consumers of the HasRawWindowHandle trait

[Lokathor]

Using Error with no prefix (eg: HandleError) is definitely not great. The official naming guidelines are just wrong, and that's not how the standard library works. io::Error and fmt::Error are the outliers, everything else is a ParseIntError or a Utf8Error or some other fully named error type. I think we should use a fully named enum here too.

Other than that, which you could consider non-blocking I guess, then I approve the PR.

[ogoffart]

Is there going to be a "server trick" to avoid breaking the ecosystem?

[madsmtm]

I'll try to if possible, see #125