Winit: `get_current_monitor()` panics on Wayland.

[icefoxen]

See: https://github.com/ggez/ggez/issues/579

Basically, this function does a some_vec.last().unwrap() of a Vec that can be null. The vec appears to be acquired from sctk::surface::get_outputs() here: https://github.com/Smithay/client-toolkit/blob/2c28ada5c7dd067ac1245382bc371cc933d4c416/src/surface.rs#L109 . As far as I can tell, the outputs vec is created empty and only has things added to it on SurfaceUserData::enter(), but I don't know enough about the Wayland protocol to understand what's actually going on there.

[vberger]

Welp.

If this Vec is empty, it means that the surface is currently not being displayed on any monitor, likely because the method was called early in the init process and the window is not currently displayed yet.

I'm not sure what would be the best thing to do here, given the API winit enforces. Would it make sens to take advantage of the pending breaking changes to change the prototype of get_current_motinor() to return an Option<MonitorId> ? (or even to return a Vec<MonitorId> tbh)

[Osspial]

I'd be fine with either returning Option<MonitorId> or Vec<MonitorId>.

If we go with Vec<MonitorId> we should consider if we define an order for the returned monitors (say, order it by the area of intersection of the window of each monitor). That should be possible on win32, but I don't know what the story on that looks like.

[vberger]

order it by the area of intersection of the window of each monitor

That would not be possible for wayland. The only order that can be implemented there would be « the order in which the window entered the monitors », which is hardly useful.

[Osspial]

Thanks, good to know. Is there any way to find out a window's primary monitor on Wayland? I ask because on systems where different monitors have different DPIs, it can be useful to know which monitor the system considers to be the main monitor for DPI scaling, which I'd like to support if possible (on Windows that's done through the MonitorFromWindow function). If we can expose that alongside a list of all monitors the window overlaps with that would be awesome.

[vberger]

There is no concept of "primary monitor" in wayland. Regarding DPI, what happens is that the client app actually tells the server that it is drawing its contents using a given dpi factor, and the server takes care of upscaling or downscaling if that does not match the dpi factor of the monitor.

So what winit does currently is it takes the max() of the dpi factors of the monitors the window is currently displayed on, and lets the server down scale if necessary.

Le 13 février 2019 23:04:37 GMT+01:00, Osspial [e-mail] a écrit :

Thanks, good to know. Is there any way to find out a window's primary
monitor on Wayland? I ask because on systems where different monitors
have different DPIs, it can be useful to know which monitor the system
considers to be the main monitor for DPI scaling, which I'd like to
support if possible (on Windows that's done through the
MonitorFromWindow
function). If we can expose that alongside a list of all monitors the
window overlaps with that would be awesome.

--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
https://github.com/tomaka/winit/issues/793#issuecomment-463394121

[icefoxen]

I need to solve this for ggez. Is there a default I can replace the unwrap() with? Or if it returns Option<MonitorId>, any ideas when this breaking change will result in a release?

[Osspial]

My guess is that we'd wrap that breaking change into EL2.0, or a release shortly afterwords.

[aleksander]

Does anyone work on this bug? I want to try to fix it.

[vberger]

I don't think anyone is carrently working on it, so please do!

[Luraktinus]

any news?

[betseg]

nothing?

[icefoxen]

Well, still fails on my system using winit 0.19.5

[kchibisov]

I'm working on that issue right now during some other rework, just could take a bit of time for me to finish.

[kchibisov]

The main issue here is that returning Vec<MonitorHandle> won't help to start window in a fullscreen, since this is why this function(get_current_monitor) is being used. We should start return Vec<MonitorHandle>, since it makes a bit more sense, but it's other story.

winit uses Option<Fullscreen> to detect on which monitor to set fullscreen, and on None it unsets it, however since there's no current monitor when users create a window, they can't make a fullscreen. On Wayland, there's an option to let compositor decide on which monitor to enter fullscreen. So, if winit's API will expose that in set_fullscreen and with_fullscreen calls it could help a lot for use cases where get_current_monitor will return empty Vec.

I'm not sure how parameters to for such calls could be changed. I'm a bit afraid of adding things like unset_fullscreen and just reworking the current API internally, since it doesn't solve a problem with with_fullscreen. We can pass something like Option<Option<Fullscreen>> for both calls, but I'm not sure that it's something nice(but it'll work).

If anyone has a suggestion on how API to enter fullscreen could look like wrt indication of system picking a monitor for you let me know. I kind of think that Option<Option<Fullscreen>> is the simplest solution here, but it'll look a bit weird.

[kchibisov]

Just thinking more about that fullscreen startup problem on Wayland. I feel like we can change the Fullscreen enum from

#[derive(Clone, Debug, PartialEq)]
pub enum Fullscreen {
    Exclusive(VideoMode),
    Borderless(MonitorHandle),
}

To

#[derive(Clone, Debug, PartialEq)]
pub enum Fullscreen {
    Exclusive(VideoMode),
    Borderless(Option<MonitorHandle>),
}

The None will be treated like 'let the system decide', which will most of the time result in a current monitor being picked on at least Wayland. If the platform doesn't have a concept of 'let the system decide' like on Wayland, it may fallback to picking current monitor by itself.

I'm not sure what to do about Exclusive fullscreen, since Wayland doesn't have such things at all, so having it under Option isn't something I can speak for.

What to do you think @chrisduerr , @murarth , @Osspial, @ryanisaacg , @francesca64 ?

[bestouff]

I'd say this calls for a third enum variant.

[kchibisov]

I'm not sure about the third one, since it's actually a Borderless, it's not like give me random fullscreen with a random video mode, it's exactly give me a fullscreen on a random screen with Borderless. So that's why it's an option, since the intent is to get Borderless fullscreen.

[chrisduerr]

Having a None parameter to refer to just a random default value is relatively consistent with the rest of the API of both winit and glutin I'd say. I would prefer Borderless(None) over some new enum variant.

[francesca64]

I'd also prefer Borderless(None) over having some third variant. Users already have plenty of tools for working with Option, and it expresses the meaning correctly.

[bestouff]

Ok ok, I get it :)

[kchibisov]

I've opened #1670 and #1671 to address raised issues.