Skip to content

Implement xdg_positioner.set_parent_size and set_parent_configure (xdg-shell stable v5)#4874

Open
Copilot wants to merge 5 commits intomainfrom
copilot/fix-xdg-shell-protocol-5-implementation
Open

Implement xdg_positioner.set_parent_size and set_parent_configure (xdg-shell stable v5)#4874
Copilot wants to merge 5 commits intomainfrom
copilot/fix-xdg-shell-protocol-5-implementation

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 21, 2026

xdg_positioner.set_parent_size and set_parent_configure (added in xdg-shell stable protocol v3, required for v5 compliance) were stubbed with TODO warnings. This implements set_parent_size and retains set_parent_configure as a TODO stub pending the necessary infrastructure.

What's new?

  • set_parent_size: Stores the client's expected parent geometry at popup placement time. Flows through SurfaceSpecificationWindowSpecificationBasicWindowManager, where it replaces parent_scene_surface->content_size() during constrained popup placement in both place_new_surface() and modify_window(). Uses value_or() for a cleaner fallback to the actual parent content size. This fixes popup positioning when a parent is mid-resize.

  • set_parent_size validation: Non-positive dimensions are now rejected with a ProtocolError(invalid_input), matching the existing set_size() validation pattern.

  • set_parent_configure: Remains a TODO stub (log_warning) — no unused state is stored. Infrastructure to act on the parent configure serial (e.g. ack_configure on XdgSurfaceStable) is not yet in place.

  • WindowSpecification::parent_size() accessor added as MirAL 5.8 public API, with corresponding symbols.map and Debian symbols entries.

  • Unit tests: modify_window_uses_provided_parent_size and place_new_surface_uses_provided_parent_size verify that when parent_size is set on the positioner, anchor clamping uses the provided size rather than the actual surface content_size().

How to test

Use a Wayland client that creates reactive popups tied to a resizing parent window (e.g. a menu anchored to a toolbar that shrinks). With the fix, the popup placement correctly uses the client-provided parent size rather than the stale actual surface size during the resize cycle.

Checklist

  • Tests added and pass
  • Adequate documentation added
  • (optional) Added Screenshots or videos

Copilot AI linked an issue Apr 21, 2026 that may be closed by this pull request
Copilot AI changed the title [WIP] Fix incomplete implementation of XDG shell stable protocol version 5 Implement xdg_positioner.set_parent_size and set_parent_configure (xdg-shell stable v5) Apr 21, 2026
Copilot AI requested a review from AlanGriffiths April 21, 2026 11:11
Comment thread src/miral/basic_window_manager.cpp Outdated
@AlanGriffiths AlanGriffiths force-pushed the copilot/fix-xdg-shell-protocol-5-implementation branch from 97fff7d to 9a6c9f1 Compare April 21, 2026 11:56
Copilot AI requested a review from AlanGriffiths April 21, 2026 11:57
Copilot AI and others added 3 commits April 21, 2026 14:18
…le positioner

Agent-Logs-Url: https://github.com/canonical/mir/sessions/4669666b-2d76-4fc5-a6c4-49ffa69b0c6d

Co-authored-by: AlanGriffiths <9048879+AlanGriffiths@users.noreply.github.com>
Agent-Logs-Url: https://github.com/canonical/mir/sessions/4669666b-2d76-4fc5-a6c4-49ffa69b0c6d

Co-authored-by: AlanGriffiths <9048879+AlanGriffiths@users.noreply.github.com>
Agent-Logs-Url: https://github.com/canonical/mir/sessions/26c86e6a-5a56-443f-ad63-fd9c40e17bf3

Co-authored-by: AlanGriffiths <9048879+AlanGriffiths@users.noreply.github.com>
@AlanGriffiths AlanGriffiths force-pushed the copilot/fix-xdg-shell-protocol-5-implementation branch from ad1207f to 20fe301 Compare April 21, 2026 13:18
@AlanGriffiths AlanGriffiths marked this pull request as ready for review April 21, 2026 13:19
@AlanGriffiths AlanGriffiths requested a review from a team as a code owner April 21, 2026 13:19
Copilot AI review requested due to automatic review settings April 21, 2026 13:19
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements missing xdg-shell stable v5 positioner requests by plumbing xdg_positioner.set_parent_size through Mir’s surface/window specifications into popup placement logic, and capturing set_parent_configure for future synchronization. Also exposes the new parent_size() accessor as a MirAL 5.8 public API (including ABI/symbol updates).

Changes:

  • Add parent_size to mir::shell::SurfaceSpecification, propagate it through miral::WindowSpecification, and use it in BasicWindowManager popup placement calculations.
  • Implement xdg_positioner.set_parent_size (store parent size) and set_parent_configure (store configure serial) in the stable xdg-shell frontend.
  • Extend MirAL ABI metadata (symbols.map, Debian symbols) for WindowSpecification::parent_size().

Reviewed changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/server/shell/surface_specification.cpp Includes parent_size in emptiness/equality/update propagation checks.
src/server/frontend_wayland/xdg_shell_stable.cpp Implements stable-v5 positioner setters for parent size/configure serial.
src/miral/window_specification_internal.h Adds parent_size storage to internal WindowSpecification::Self.
src/miral/window_specification_internal.cpp Copies parent_size into the produced SurfaceSpecification.
src/miral/window_specification.cpp Plumbs parent_size from SurfaceSpecification and adds accessors.
src/miral/basic_window_manager.cpp Uses parent_size (if set) instead of parent surface content_size() for constrained popup placement.
src/include/server/mir/shell/surface_specification.h Declares parent_size in the server-side surface specification.
include/miral/miral/window_specification.h Adds public MirAL 5.8 WindowSpecification::parent_size() API.
src/miral/symbols.map Adds new MIRAL_5.8 symbol version entry for parent_size().
debian/libmiral7.symbols Adds Debian symbols entries for the new MirAL 5.8 API.

Comment thread src/miral/basic_window_manager.cpp
Comment thread src/miral/basic_window_manager.cpp
Comment thread src/server/frontend_wayland/xdg_shell_stable.cpp
@AlanGriffiths
Copy link
Copy Markdown
Contributor

@copilot apply changes based on the comments in this thread

Agent-Logs-Url: https://github.com/canonical/mir/sessions/e051af04-0b25-4293-90f5-97955b7c2e11

Co-authored-by: AlanGriffiths <9048879+AlanGriffiths@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 21, 2026

@copilot apply changes based on the comments in this thread

Done in ca5abde. Applied all three changes from that review thread:

  • Added input validation for set_parent_size (rejects non-positive dimensions with ProtocolError(invalid_input))
  • Added modify_window_uses_provided_parent_size unit test
  • Added place_new_surface_uses_provided_parent_size unit test

Both tests set parent_size = {200, 200} while the actual parent content size is 600x400. They place an aux_rect with its northeast corner at x+260 — within the actual parent but outside the provided parent_size — and verify that constrain_to() clamps the anchor to x+200.

Comment on lines +668 to +678
auto parent_size() -> mir::optional_value<Size>&;

/// The expected size of the parent window for constrained popup placement.
///
/// Set when the compositor should use a specific parent geometry (rather than
/// the current parent geometry) to constrain the popup placement. This is used
/// when repositioning a popup in response to a parent resize.
///
/// \returns the expected parent size
/// \remark Since MirAL 5.8
auto parent_size() const -> mir::optional_value<Size> const&;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"deducing this" would've allowed us to have only one definition, but it requires the function to be defined in the header, which is a big no-no for miral 🫠

/// Serial of the parent's xdg_surface configure event this positioner is a response to.
/// Stored for popup repositioning synchronization (protocol: xdg_positioner.set_parent_configure).
std::optional<uint32_t> parent_configure_serial;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't used anywhere. Is this some set up for future work?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot this needs fixing

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 84da320 — removed the unused parent_configure_serial field and reverted set_parent_configure to its original TODO/log_warning stub, since there's no infrastructure yet to act on the serial.

…ure to TODO stub

Agent-Logs-Url: https://github.com/canonical/mir/sessions/74dbe151-a144-4306-9f7b-745b3208620e

Co-authored-by: AlanGriffiths <9048879+AlanGriffiths@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

XDG shell stable protocol version 5 implementation incomplete

4 participants