api: make VideoModeHandle into VideoMode #4060
Conversation
kchibisov
requested review from
jackpot51,
notgull,
daxpedda,
madsmtm and
MarijnS95
as code owners
The video mode is generally a static data and not a reference to some video mode. This changes the exclusive fullscreen API to match that an accept a monitor now.
kchibisov
force-pushed
the
kchibisov/video-mode
branch
from
b2c9c0b to
6c0c47d
Compare
| pub fn video_modes(&self) -> std::iter::Empty<VideoMode> { | ||
| unreachable!() |
There was a problem hiding this comment.
On orbital you implemented this to return std::iter::empty(), I'm not sure what to do here. There, the type is impl Iterator and not std::iter::Empty too.
There was a problem hiding this comment.
You can not have unreachable!() with impl Iterator.
There was a problem hiding this comment.
Hence the question is: should Android return std::iter::empty() with std::iter::Empty<> type?
There was a problem hiding this comment.
it doesn't really matter, the function can not be reached anyway. If you feel like something else should be returned send a separate patch, since I just did it the same as other functions were.
| let video_mode = | ||
| match monitor.video_mode_handles().find(|mode| &mode.mode == video_mode) { | ||
| Some(monitor) => monitor, | ||
| None => return, |
There was a problem hiding this comment.
Perhaps this is planned for a future change, but are we missing any feedback to the user here that the passed VideoMode isn't valid and wasn't applied?
I expect that video_mode_handles() might change during app runtime.
There was a problem hiding this comment.
Video modes usually don't change, unless monitor is reconfigured. In general though, there's no guarantee that the API will work, though, on windows it likely should generally work.
There was a problem hiding this comment.
I'd add an error for that, but once we have monitor handle changed.
There was a problem hiding this comment.
Thanks, this is much easier to review!
Just for completeness, I'll note that the approach here forces us to do the fallible operation monitor.video_modes_handles().find(|mode| &mode.mode == video_mode) when requesting exclusive fullscreen, but I think that's fine.
We might want to consider making the size, bit_depth and refresh_rate_millihertz fields public in the future, but such a decision should consider the increased probability that users will encounter an error here, so let's defer that.
|
You can still encounter error if you pass a video mode from a different monitor. |
Co-authored-by: Mads Marquart <[email protected]>
The video mode is generally a static data and not a reference to some video mode. This changes the exclusive fullscreen API to match that and accept a monitor now.
Extracted from #3927