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

Make raw_*_handle return a result #122

Merged
merged 5 commits into from
Jun 17, 2023
Merged

Make raw_*_handle return a result #122

merged 5 commits into from
Jun 17, 2023

Conversation

notgull
Copy link
Member

@notgull notgull commented Jun 9, 2023

This is a breaking change. Supersedes #104.

@Lokathor
Copy link
Contributor

Lokathor commented Jun 9, 2023

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

@notgull
Copy link
Member Author

notgull commented Jun 9, 2023

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

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.

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.

f,
"The underlying handle cannot be represented using the types in this crate"
),
Self::Unavailable => write!(f, "The underlying handle is not available"),
Copy link
Member

Choose a reason for hiding this comment

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

Error Display text should start with a lowercase letter

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Well the Error trait itself recommends it

src/borrowed.rs Outdated Show resolved Hide resolved
#[non_exhaustive]
pub enum RawHandleError {
/// The underlying handle cannot be represented using the types in this crate.
NotSupported,
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

@ogoffart
Copy link

ogoffart commented Jun 9, 2023

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
Copy link
Member

madsmtm commented Jun 9, 2023

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
Copy link
Member Author

notgull commented Jun 9, 2023

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
Copy link
Member

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

Ack from me.

@madsmtm
Copy link
Member

madsmtm commented Jun 9, 2023

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
Copy link
Contributor

Lokathor commented Jun 9, 2023

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

@notgull
Copy link
Member Author

notgull commented Jun 10, 2023

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
Copy link
Member

madsmtm commented Jun 10, 2023

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.

Copy link
Member

@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.

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
Copy link
Contributor

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
Copy link
Member Author

notgull commented Jun 10, 2023

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
Copy link
Member

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
Copy link
Member Author

notgull commented Jun 10, 2023

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!

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.

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
Copy link
Member Author

notgull commented Jun 11, 2023

@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
Copy link

rib commented Jun 11, 2023

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
Copy link
Member

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
Copy link

rib commented Jun 12, 2023

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
Copy link

rib commented Jun 12, 2023

@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
Copy link
Member

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
Copy link
Member Author

notgull commented Jun 15, 2023

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
Copy link
Contributor

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.

@notgull notgull merged commit 814a922 into master Jun 17, 2023
@notgull notgull deleted the notgull/err branch June 17, 2023 03:33
@ogoffart
Copy link

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

@madsmtm
Copy link
Member

madsmtm commented Jun 17, 2023

I'll try to if possible, see #125

This was referenced Jul 15, 2023
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.

6 participants