Errata on `WindowType::Popup`

[dhardy]

Nice to see that popup/overlay windows are finally supported, but I do have a few questions.

How do I tell whether WindowType::Popup is available? I guess we just assume that if create_window fails with Error::NotSupported(_) it is because of the window_type (and not because of something else, like transparency).

Is there any guarantee that the scale factor matches that of the parent window?

Does sizing necessarily work the same way as with full windows (usage of logical/physical pixels)? From the code it looks Wayland at least uses logical pixels.

[Murmele]

Hi,
thanks for looking into. Currently you can just request create_window and if available you get a Popup back, otherwise an error. What do you mean by guarantee? For the Wayland popup I set explicitly the scalefactor from the parent, so if this is not correct the scale factor of the Popup is also not correct.

The interface is the same for Popup and normal Windows. It is defined in winit-core (surface_size, request_surface_size)

[dhardy]

if available you get a Popup back, otherwise an error.

The error doesn't specifically tell me that popup windows are unavailable; it could be because something else failed. Possibly winit's NotSupported error should be more specific, e.g. Error::NotSupported(Unsupported::PopupWindows).

What do you mean by guarantee?

Documentation stating that the scale factor always matches the parent window's scale factor on all supported platforms.

The interface is the same for Popup and normal Windows.

It's not the interface; it's the way sizing happens. If you write an app using winit and specify a window size in physical pixels (applying the scaling yourself), then on X11 you get exactly the size requested because the backend request uses physical pixels; if however you try the same on Wayland (with a scale factor != 1) you get a different size because first winit converts from physical pixels to logical (with the wrong scale factor because Wayland does not tell you the correct factor in advance) then makes the request in logical pixels. If you do the same thing but specifying the size in logical pixels then you get a more consistent result (if I remember correctly) because the scale factor is known in advance. (As a consequence I don't think it's ever a good idea to specify the window size in physical pixels unless your app only targets platforms using physical pixels internally.)

I think with popups it's the same, that it is best to specify sizes in logical pixels. The difference is that the actual scale factor should be known in advance.

Edit: it looks like the Wayland backend has code specifically to handle this situation (initial window size specified in physical pixels with unknown scale factor), so what I wrote above is likely outdated.

[Murmele]

Hi @dhardy ,
I understand your issue. For popup everywhere Error::NotSupported is used (https://github.com/rust-windowing/winit/blob/master/winit-wayland/src/popup.rs#L38-L40) or on X11 https://github.com/rust-windowing/winit/blob/master/winit-x11/src/window.rs#L76-L78
regardless if it is really not supported or during initialization something went wrong.
Maybe we should add to winit-core RequestError something like a normal Error entry to distinguish between not supported and an other error

Edit: it looks like the Wayland backend has code specifically to handle this situation (initial window size specified in physical pixels with unknown scale factor), so what I wrote above is likely outdated.

You can specify the size in logical pixels or physical pixel. At the end we calculate the physical pixels with the scale factor from the parent

[Murmele]

@dhardy regarding error message I created a PR: #4706

[dhardy]

From having a bit of a look at the code (especially possible errors) in #4707 it is clear that using a separate fn create_popup method would simplify error handling. Using a separate Popup type too could be considered since not everything a Window can do makes sense for a popup (e.g. drag_window).

I notice that fn outer_position does something a little different for popups than windows. Is there any rationale for implementing it at all for popups?

[Murmele]

@dhardy Thanks for looking into the code and reviewing it :) Are you talking about a specifc platform? On wayland there exist no outer_position for toplevel windows. Only for popups relative to the parent

[dhardy]

@Murmele we already have positioner and set_positioner for adjusting the position of popup windows, right? So is there any point supporting outer_position and set_outer_position too? (This applies to all platforms.)

Eh, the code already exists; might as well keep it.

[dhardy]

I see that on Wayland the positioner, set_positioner and set_outer_position methods can fail silently in case self.common.state.upgrade() fails (i.e. the parent window is destroyed). I'm not quite sure if the best option here is to panic or to adjust the API to allow reporting errors in those methods. I suspect the latter, so I won't try to change this in #4707 (which is more about revising existing error reporting).

@Murmele is this upgrade failure something likely to be seen in practice (e.g. if users destroy windows in the wrong order) or only possible given an internal winit code bug?

[Murmele]

@Murmele we already have positioner and set_positioner for adjusting the position of popup windows, right? So is there any point supporting outer_position and set_outer_position too? (This applies to all platforms.)

Eh, the code already exists; might as well keep it.

Right now the positioner is not implemented for all. It should be a problem to implement for all other platforms as well like it was done for Windows and Apple (simulating the anchor feature from wayland)

[Murmele]

I see that on Wayland the positioner, set_positioner and set_outer_position methods can fail silently in case self.common.state.upgrade() fails (i.e. the parent window is destroyed). I'm not quite sure if the best option here is to panic or to adjust the API to allow reporting errors in those methods. I suspect the latter, so I won't try to change this in #4707 (which is more about revising existing error reporting).

@Murmele is this upgrade failure something likely to be seen in practice (e.g. if users destroy windows in the wrong order) or only possible given an internal winit code bug?

If you close a popup which still has a popup open first the child popup will be closed. In this case the event loop removes the Popup RC and the Popup item has only a weak pointer on it and silently fails.
Currently we don't have a mechanism to indicate if the popup is still present.

For compatibility the Popup item it self was not made a weak pointer because in this case you would see when the upgrade fails.
So in this area some architecture investigations are required