Modal Dialog - #4627
Modal Dialog#4627Murmele wants to merge 5 commits into
Conversation
Was that #4543 ? |
Yes it was |
3a46b55 to
8ac3fb9
Compare
1a70787 to
e33c0e8
Compare
#Conflicts: # winit-appkit/src/window_delegate.rs # winit-wayland/src/event_loop/mod.rs # winit-wayland/src/popup.rs # winit-wayland/src/window/common.rs # winit-win32/src/window.rs # winit-x11/src/window.rs
|
@dhardy , since you had opinions about the Popup change, maybe you want to have a look at this? |
dhardy
left a comment
dhardy
left a comment
There was a problem hiding this comment.
Looking just at the API, this seems acceptable.
That said, I'm not wildly enthusiastic about pushing all of the window properties into WindowAttributes: it is left to documentation to describe which attributes are applicable to which window types. I can see two possible alternatives here:
- Moving attributes applicable to only one window type into
WindowTypevariant fields (e.g.titleandiconshould only be applicable to full windows). - Having separate
create_window,create_popupandcreate_dialogmethods, each with their own_Attributesstruct. - (Variant of above):
create_window+create_subwindowwithWindowTypeonly used in the latter case; the rationale being that all subwindows need a parent.
| pub window_type: WindowType, | ||
| /// See [`WindowAttributes::with_positioner`]. | ||
| pub positioner: Option<WindowPositioner>, | ||
| pub modal: Option<bool>, |
There was a problem hiding this comment.
Why Option<bool>? What happens if this is None with WindowType::Dialog?
There was a problem hiding this comment.
For wayland it unwraped or default to false
There was a problem hiding this comment.
So why not use a simple bool ?
There was a problem hiding this comment.
good point, I changed it so every platform uses the same default
| /// - **X11, Web, Android, iOS, Orbital:** An error is returned because it is not implemented. | ||
| /// | ||
| /// [owned windows]: https://learn.microsoft.com/en-us/windows/win32/winmsg/window-features#owned-windows | ||
| Dialog, |
There was a problem hiding this comment.
Possibly it would make sense to put modal: bool under WindowType::Dialog since it is not applicable to any other window type.
(The same may apply to popup positioning information, but if so best leave that to another PR.)
There was a problem hiding this comment.
This was a design decision to keep the WindowType a simple type instead of a complex one with properties
| // Dialogs are placed at creation, relative to their parent (an explicit position is relative to | ||
| // the parent's client area, otherwise the dialog is centered over the parent). This can't be | ||
| // done afterwards: the parent isn't yet queryable via `GetParent` during `WM_CREATE`, and a | ||
| // position set before the first show is overridden by the `CW_USEDEFAULT` cascade. |
There was a problem hiding this comment.
I don't think that's a good behavior.
IMHO, the position from the attribue and the outer_position later should be in the same coordinate system (either global or parent). But mixing both is confusing.
Imagine you have a parent windoc located at, say (500, 300)
Then you create a modal window with
WindowAttributes::default()
.with_window_type(WindowType::Dialog)
.with_parent_window(Some(parent_handle))
.with_position(LogicalPosition::new(10, 10)),Here you pass (10,10), then it will show on the screen at 510, 310 (relative to the parent)
dialog.outer_position();
// Returns 510,310 not 10,10 which was the input.// Will set the position to absolute coordinate 10,10 instead of relative to the parent
dialog.set_outer_position(LogicalPosition::new(10, 10));So this makes an asymmetry between the position in WindowAttribues and the position afterwards, and i'm not sure it is a good.
| if let Some(owner) = self.owner { | ||
| unsafe { | ||
| EnableWindow(owner.hwnd(), 1); | ||
| SetForegroundWindow(owner.hwnd()); |
There was a problem hiding this comment.
What if there is another modal dialog open at the same time? Will that cause the parent to be set to the foreground, or the other modal dialog?
| _ => None, | ||
| }) | ||
| .map(|owner_hwnd| { | ||
| unsafe { EnableWindow(owner_hwnd, 0) }; |
There was a problem hiding this comment.
The owner is disabled as soon as the modal dialog is created, whether or not it is visible.
So the parent can be locked even if one didn't call set_visible.
(Or if the dialog is hidden rather than Drop'ed)
Possible fix would be to disable the parent from set_visible instead
This branch has not been deployed
4 participants
Implement modal dialogs. This branch is based on the work of the Popup branch so this must be merged first. Only the last commit here is relevant for dialog
changelogmodule if knowledge of this change could be valuable to usersWhat is missing to be ready
Os