Wayland, Windows, MacOS: Popup Implementation#4543
Conversation
facdfde to
87f6327
Compare
Reason: Because a normal window can have a parent window as well, like a Dialog
Reason: otherwise the child surface is anchored to the center
Reason: otherwise the height of the client side decoration is not considered and therefore the location is shifted
Reason: There are multiple pointers to the smithay popup. Once in state.windows and one time in the popup object it self. Just dropping the popup object releases only one pointer but we have to notify the state to release also the other
…he position Reason: Much easier in the resize handle
Reason: we have to call resize to initialize the viewport to map to correct window size
|
@kchibisov Just in case you have time. This is ready for review |
|
I'd like to review it too. I've been using an earlier version of this branch for my own project and I had some local bugfixes (various panics, protocol violations and commented out / unimplemented code). I'll try to rebase and see if your PR now contains all of them. |
Thanks for testing, just let me know if you have any problems |
| /// | ||
| /// See [`WindowType::Popup`] for the per-platform behavior, including how to obtain a | ||
| /// rounded, native-looking popup on macOS. | ||
| pub fn with_type(mut self, window_type: WindowType) -> Self { |
There was a problem hiding this comment.
I'm inclined to suggest to call this with_window_type. That would also make it symmetric to window_type().
There was a problem hiding this comment.
thanks I changed it
| /// tooltip. Requires a parent set via [`WindowAttributes::with_parent_window`], and its | ||
| /// position is interpreted relative to that parent. | ||
| /// | ||
| /// ## Platform-specific |
There was a problem hiding this comment.
I think this would be the place to mention that this is for example not supported on X11 (as opposed to just with with_type())
| let popup = crate::Popup::new(self, window_attributes)?; | ||
| Ok(Box::new(popup)) | ||
| }, | ||
| _ => panic!("Not implemented"), |
There was a problem hiding this comment.
Since the function returns a Result, wouldn't it perhaps make sense to return Err instead of a panic?
Alternatively, unimplemented!() perhaps?
(sucks that non_exhaustive doesn't kick in here - I wonder if there's a way?)
| Window((Window, Option<WindowConfigure>)), | ||
| Popup((Popup, XdgPositioner, Option<PopupConfigure>)), |
There was a problem hiding this comment.
Tuples are tempting, but .1 and .2 aren't super readable. I'd lean towards anonymous structs inside the enum.
There was a problem hiding this comment.
true, makes it much more readable :)
| // If we have a popup the position is relative to the parent window and not | ||
| // relative to the screen. Therefore we have to translate it from the parent | ||
| // coordinate system to the display coordinate system | ||
| fn translate_outer_position(&self, position: Position) -> (i32, i32) { |
There was a problem hiding this comment.
Perhaps this should return a PhysicalPosition instead of a tuple, to reduce changes of mixing this up?
Reason: So it is clear which kind of position it is
|
Alright, so far I've confirmed 2 out of 3 bugs:
Fix here: CryZe@a157156
Fix here: CryZe@3d03cab Both of these fixes are quickly put together, there's possibly a better way to do it. I'm still looking into the third bugfix, at least so far it doesn't seem to do anything anymore on your branch (as in, you probably already fixed it). |
|
I think the idea with those pop-ups were to have a separate type other than glue to window, because as you said, they are very restricted on some platforms and on some platform it's indeed just a so in top-level API I would have them, as a separate type with its own restrictions, and what not, I'm also not sure if those things can be owned by the user, really, I think popups should be more like something you create from e.g.
Exactly why I want to have them owned by Generally, I think the initial plan was to have some kind of @tronical would like to hear your opinion here since you're representing ui toolkit side 😅 I just want to account for other special surfaces Wayland people can create which has their own popup type that is certainly not different than standard popup. |
That's indeed an interesting constraint (reverse order required).
I see the discrepancy, but I also see that - wayland aside - popups have more in common with (disclaimer: I'm biased though as I'm Martin's colleague :) |
|
The problem with Wayland, is that if you randomly use it wrongly, it really likes killing applications with protocol errors, so my suggestion is mostly to try solve it on type system level, or at least make it less likely, so users won't have to debug Wayland when it breaks in their cross platform app. I also remember that popups can be discarded/removed on user behalf and winit window is kind of persistent now. |
|
For Wayland: shouldnt we also use the |
|
I think dialog protocol serves a different purpose, and also not widely available? |
Ah right, I didnt read the PR description carefully, my bad. Originally thought it was popup dialog.
All 3 most popular compositors right now, KWin, Mutter and Hyprland, implement it so I dont think it's a problem. |
Implement proper decorationless popups by specifying the type of the child window with
with_type()With this PR different kind of child windows can be created
The type can be specified during creation of the Window using the window_attributes and the
with_type()function. As default a normal Window is used. IfPopupis choosen a parent must be specified, otherwise the Popup creation fails with an Error returned by thenew()function.Related issues: #403 and #4256
changelogmodule if knowledge of this change could be valuable to usersPlatforms
Wayland:
Bildschirmaufzeichnung.vom.2026-06-04.13-18-16.mp4
This work is sponsored by the NLnet foundation