Handle error cases in MonitorHandle

[madsmtm]

Part of #971.

Makes all MonitorHandle methods fallible. The same is not done for VideoMode, since they are just static blobs of data (rather than something queried dynamically) (but that might change in the future?).

• Tested on all platforms changed
  • Android
  • iOS
  • macOS
  • Wayland
  • Web
  • Windows
  • X11
  • Orbital
• 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

Unresolved

I'm unsure what we should do in the case where some functionality isn't implemented yet? Is the correct thing to return a dummy value? Perhaps it would actually be better if we just panic with an unimplemented!()?

Though in any case I think this PR is an improvement over the status quo.

[kchibisov]

I'm unsure what we should do in the case where some functionality isn't implemented yet? Is the correct thing to return a dummy value? Perhaps it would actually be better if we just panic with an unimplemented!()?

That's not an option. Would suggest to return a Result instead.

[kchibisov]

I think we could add a MonitorDescription or something like that and access it within the callback?

Then on the monitor handle itself we could have 2 methods.

fn is_alive(&self) -> bool;
fn with_description<T, F: Fn(MonitorDescription) -> T>(&self) -> Result<T>;

Then on that data you can access the things you need, that way it'll be less result unwraps and such.

[madsmtm]

fn is_alive(&self) -> bool;
fn with_description<T, F: Fn(MonitorDescription) -> T>(&self) -> Result<T>;

There is no callback you could write in which MonitorDescription will always be available, since the user could remove their monitor at any time - you must do a copy of the underlying data, in which case -> Result<MonitorDescription> would work just fine.

If we do that though, then I think I'd rather just rename MonitorHandle to MonitorDescription altogether.

I'm unsure what we should do in the case where some functionality isn't implemented yet? Is the correct thing to return a dummy value? Perhaps it would actually be better if we just panic with an unimplemented!()?

That's not an option. Would suggest to return a Result instead.

I actually think the unimplemented!() strategy would be a fairly good option:

• If someone doesn't use the functionality, or don't support the platform it's being used on, there's no problem
• If someone use it on a platform where it's not supported, they must manually cfg-gate the functionality
• Which in turn would drive people to actually implement the functionality!

Vs. returning a Result<T, (Unimplemented | MonitorGone)>:

• People will likely just .unwrap_or(...), which will coalesce the two very different errors together
• Once the feature has been implemented, we now have an unused error case (and an implementation that wasn't there before could even somewhat be a breaking change).

Of course, for the things that are fundamentally not possible to support, then we should do change the API.

---

In any case, we should consider making video modes match monitors: So either we have MonitorHandle and VideoModeHandle, or MonitorDescription and VideoMode.

[dhardy]

MonitorGone where? MonitorMissing better. (Excuse the missing prepositions.)

Unfortunately this clashes with #2658.

So either we have MonitorHandle and VideoModeHandle, or MonitorDescription and VideoMode.

This sounds like unnecessary complexity for the sake of consistency. That said, returning a MonitorDescription would simplify the current API. I guess the cost is unimportant (extra platform requests for details the app may not want anyway)?

The one part of MonitorHandle that confuses me: it appears to be both an identifier and a descriptor? In any case, the only real purpose of Winit returning monitor details, as far as I can tell, is to support window.set_fullscreen(..).

[kchibisov]

I did a small error in the API. We need a ref to MonitorDescription and ability to clone it, that way you access the monitor data with-in the callback, and if it doesn't exist anymore, you'll get a Result when trying to get the MonitorDescription

fn is_alive(&self) -> bool;
fn with_description<T, F: Fn(&MonitorDescription) -> T>(&self) -> Result<T>;

If we do that though, then I think I'd rather just rename MonitorHandle to MonitorDescription altogether.

No, the point is that you have a handle which might be long dead (is_alive check), no need to merge them, that will complicate the API.

I actually think the unimplemented!() strategy would be a fairly good option:
If someone doesn't use the functionality, or don't support the platform it's being used on, there's no problem
If someone use it on a platform where it's not supported, they must manually cfg-gate the functionality
Which in turn would drive people to actually implement the functionality!

No, that will drive people to whine and complain, not to implement. Maybe that's my experience, but for example, over the years in alacritty no-one bothers to fix bugs on macOS even though they are relatively simple, the same will be about other bugs. Having potential crashes like that isn't a good idea for a library that is around for a long time.

[xiaopengli89]

How to deal with the situation that the handle value is reused, which will cause consistency problems, how about using std::os::windows::io::BorrowedHandle::try_clone_to_owned?

[daxpedda]

Small nits, otherwise LGTM!

[daxpedda]

Great change!
The documentation on EventLoopWindowTarget::primary_monitor() has to be updated though.

[daxpedda]

I believe in this case it should actually be unreachable!(), though unimplemented!() is nice, in this case it should be literally unreachable.

[daxpedda]

Suggested change

	pub struct MonitorGone {
	_inner: (),
	}
	pub struct MonitorGone(());

I don't have a preference here, but I think we should stick with something, I've seen this in tuple-form more often in our code base then in struct-form.

Alternatively (not advocating for it):

Suggested change

	pub struct MonitorGone {
	_inner: (),
	}
	#[non_exhaustive]
	pub struct MonitorGone;

[kchibisov]

I don't like the approach because the problem is that lifetime should be attached to the event loop, where we don't do that. This problem is solved on winit-next where monitor handle has its own ID and you retrieve it from the event loop, thus none of that can happen on all the platforms.

Adding results to every method doesn't sound appealing to me.

[madsmtm]

We talked a bit about this in Friday's meeting, the API should stay mostly the same, but instead each backend should cache the required data about the monitor, such that the data can always be retrieved.

I don't think I communicated this properly then, but I think it'd be fine for the backend to keep a reference to the monitor data alive, if that's enough that it can be fetched.

Internally, it would be something like:

// Backend 1
struct MonitorHandle {
    // Not retrievable from the handle if the monitor is disconnected, so cached here
    cached_data: {
        name: String,
        refresh_rate: u32,
        ...
    },
    // Used when entering fullscreen on the monitor
    handle: *mut MyHandle,
}

// Backend 2
struct MonitorHandle {
    // Used when entering fullscreen on the monitor, and to retrieve data about the monitor
    handle: Arc<MyHandle>,
}

[daxpedda]

I don't think I communicated this properly then, but I think it'd be fine for the backend to keep a reference to the monitor data alive, if that's enough that it can be fetched.

I think we should keep it consistent and not do fetching on some backends and on others we cache. Because we will have to add a big note saying "this data is cached and reflects the state when it was retrieved". Adding backend specific exceptions makes this even more unintuitive.

I would still be fine to leaving things as proposed here, with returning a Result, as I don't really see a problem with that, but just caching the data seems to be the only compromise we managed to reach.

[kchibisov]

In the current state of thins it shouldn't cache, since there's no way to propagate that the cache got invalidated or the data got updated.

[daxpedda]

In the current state of thins it shouldn't cache, since there's no way to propagate that the cache got invalidated or the data got updated.

What changed your mind? Because that is what we agreed on the last time.

[kchibisov]

All backends right now don't cache and kind of live update or query the data. I guess I misunderstood it, since I thought that we shouldn't cache in the future.

Right now to remove reload/cache of data in handles it'll require to likely rewrite all of them all together, so this PR will be way bigger than report errors.

[kchibisov]

Or what do you mean by caching in particular, like remember last asked value or use only POD and update when the user queries the monitor?

[daxpedda]

Or what do you mean by caching in particular, like remember last asked value or use only POD and update when the user queries the monitor?

The idea was to use POD only, no updating. If you want updated values you have to query the monitor from the event loop again. It might be sensible to add an refresh() method though.

[kchibisov]

The idea was to use POD only, no updating. If you want updated values you have to query the monitor from the event loop again. It might be sensible to add an refresh() method though.

Yeah, it's just will have to rewrite all the monitor handles code, since everything is doing the opposite. I'm not against that though, just way more work for this patch and it does completely different compared to this patch.