[Murmele]

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

• Tested on all platforms changed
• Added an entry to the changelog module if knowledge of this change could be valuable to users
• Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
• Created or updated an example program if it would help users understand this functionality

What is missing to be ready

• Popup PR merged Wayland, Windows, MacOS: Popup Implementation #4543
• PR at Smithay Client Toolkit merged
• Smithay Client Toolkit new release
• Update Smithay version in winit
• Window refactor merged: wayland: refactor window/popup state to be reusable across window types #4695

Os

• Wayland (Xdg Dialog v1)
• X11
• Windows
• MacOs

[redstrate]

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

Was that #4543 ?

[Murmele]

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

Was that #4543 ?

Yes it was

[ogoffart]

@dhardy , since you had opinions about the Popup change, maybe you want to have a look at this?

[dhardy]

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:

1. Moving attributes applicable to only one window type into WindowType variant fields (e.g. title and icon should only be applicable to full windows).
2. Having separate create_window, create_popup and create_dialog methods, each with their own _Attributes struct.
3. (Variant of above): create_window + create_subwindow with WindowType only used in the latter case; the rationale being that all subwindows need a parent.

[ogoffart]

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.

[Murmele]

Thanks. Fixed in 394dc1c

[ogoffart]

A few notes from my agent:

---

Wayland

• create_dialog fails unless the compositor exposes xdg_wm_dialog_v1 (sctk does bound_global()? on it), so dialogs don't work at all on those compositors. Could we fall
  back to a regular toplevel with set_parent when the global is missing? The wl_surface created before the call is also leaked on that error path.
• A dialog without surface_size fails with "Missing surface size for Dialog window". Other window types and the other backends default to 800x600; dialogs should probably do
  the same.
• Dialog::new ignores several attributes that Window::new applies: maximized/fullscreen at startup, the initial cursor, surface_resize_increments, and the app_id.
• request_decoration_mode is commented out for dialogs, so set_decorations() at runtime never reaches the compositor.
• finish_window_setup adds the window to state.windows / window_requests before waiting for the first configure. If the compositor dismisses the dialog before then, those
  entries are never removed.

Windows

• move_modal_owner checks IsZoomed(owner) but not whether the dialog itself is maximized, so maximizing a modal dialog moves the parent by the same offset (partly
  off-screen). It also runs while the dialog is hidden, so set_outer_position on a hidden dialog moves the parent. Something like:

  if !state.modal_owner_disabled || IsIconic(window) != 0 || IsZoomed(window) != 0 {
      return;
  }

• Dialogs keep their minimize button, unlike on macOS. A minimized owned dialog has no taskbar button while the parent stays disabled, so the app looks frozen. I'd remove
  WindowButtons::MINIMIZE for dialogs here too, at least modal ones.
• modal_owner stores the HWND exactly as passed. If the parent is a child window, Win32 makes its top-level ancestor the owner, so EnableWindow disables only the child, and the
  initial position and outer_position() are computed relative to different windows. Using GetAncestor(parent, GA_ROOT) would fix both.
• Minor: the initial centering ignores min/max size and the DPI-aware frame (AdjustWindowRectEx). It also centers over the parent's outer rect, while macOS centers over the
  content area.

Example

• examples/dialog.rs is enabled for x11_platform, but X11 returns DialogNotSupported, so pressing N or M panics on unwrap(). Either drop X11 from the cfg or handle the error.

[Murmele]

Wayland

* `create_dialog` fails unless the compositor exposes `xdg_wm_dialog_v1` (sctk does `bound_global()?` on it), so dialogs don't work at all on those compositors. Could we fall
  back to a regular toplevel with `set_parent` when the global is missing? The `wl_surface` created before the call is also leaked on that error path.

Thanks I am handling now by using a normal window instead in that case. wl_surface is not leaked, because in sctk it is owned and dropped if the creation of the dialog fails.

* A dialog without `surface_size` fails with "Missing surface size for Dialog window". Other window types and the other backends default to 800x600; dialogs should probably do
  the same.

Thanks done

* `Dialog::new` ignores several attributes that `Window::new` applies: maximized/fullscreen at startup, the initial cursor, `surface_resize_increments`, and the `app_id`.

I refactored to make as much as possible common between normal windows and dialogs. So now all attributes are handled

* `request_decoration_mode` is commented out for dialogs, so `set_decorations()` at runtime never reaches the compositor.

No sctk release was created since my PR there

* `finish_window_setup` adds the window to `state.windows` / `window_requests` before waiting for the first configure. If the compositor dismisses the dialog before then, those
  entries are never removed.

Fixed in dad3941

[Murmele]

Example

* examples/dialog.rs is enabled for x11_platform, but X11 returns DialogNotSupported, so pressing N or M panics on unwrap(). Either drop X11 from the cfg or handle the error.

I disabled for X11

[Murmele]

Windows

* `move_modal_owner` checks `IsZoomed(owner)` but not whether the dialog itself is maximized, so maximizing a modal dialog moves the parent by the same offset (partly
  off-screen). It also runs while the dialog is hidden, so `set_outer_position` on a hidden dialog moves the parent. Something like:
  ```
  if !state.modal_owner_disabled || IsIconic(window) != 0 || IsZoomed(window) != 0 {
      return;
  }
  ```

* Dialogs keep their minimize button, unlike on macOS. A minimized owned dialog has no taskbar button while the parent stays disabled, so the app looks frozen. I'd remove
  WindowButtons::MINIMIZE for dialogs here too, at least modal ones.

* modal_owner stores the HWND exactly as passed. If the parent is a child window, Win32 makes its top-level ancestor the owner, so EnableWindow disables only the child, and the
  initial position and outer_position() are computed relative to different windows. Using GetAncestor(parent, GA_ROOT) would fix both.

* Minor: the initial centering ignores min/max size and the DPI-aware frame (AdjustWindowRectEx). It also centers over the parent's outer rect, while macOS centers over the
  content area.

Thanks for the considerations. Implemented in the last commits