Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

scene: add wlr_scene_xdg_popup_create #3298

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

emersion
Copy link
Member

This allows compositors to easily add an xdg_popup to the
scene-graph while retaining the ability to unconstraint the popup
and decide its final position.

Compositors can handle new popups with the wlr_xdg_shell.new_surface
event, get the parent scene-graph node via wlr_xdg_popup.parent.data,
create a new scene-graph node via wlr_scene_xdg_popup_tree_create,
and unconstraint the popup if they want to.

@emersion emersion changed the title scene: add wlr_scene_xdg_popup_tree_create scene: add wlr_scene_xdg_popup_create Oct 27, 2021
emersion added a commit to emersion/cage that referenced this pull request Oct 27, 2021
@emersion
Copy link
Member Author

emersion commented Oct 27, 2021

Example usage in cage: emersion/cage@bf2a40d

@ifreund
Copy link
Member

ifreund commented Oct 27, 2021

Added a commit to the tinywl scene PR to use this as well: 346e648

Comment on lines 55 to 67
static void scene_popup_handle_popup_configure(struct wl_listener *listener,
void *data) {
struct wlr_scene_xdg_popup *scene_popup =
wl_container_of(listener, scene_popup, popup_configure);
scene_popup_update_position(scene_popup);
}
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this new state applied on the next commit after the client acks the configure? Is updating the position immediately ok?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this needs to be changed to use ack_configure. But this reveals an additional issue: wlr_xdg_popup.geometry might be overwritten by the compositor in a new configure event before the client acks. So we'd need to have more geometry fields: one for the last one configured by the compositor, one for the last one ack'ed by the client.

None of this matters in practice currently since we don't support repositioning xdg_popup yet (#2045).

Copy link
Member

Choose a reason for hiding this comment

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

Why don't we just remove this listener entirely then and leave a note saying that this feature needs to be implemented with #2045?

Copy link
Member Author

Choose a reason for hiding this comment

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

There would be a footgun with this approach: the compositor would need to be careful to unconstrain the popup before wlr_scene_xdg_popup_create is created, otherwise the node position in the scene-graph will be (and stay) invalid.

Copy link

@ghost ghost Oct 27, 2021

Choose a reason for hiding this comment

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

There are times (some moving and/or resizing of the view) when a popup needs to be re-unconstrained, fwiw, so not even fixing the position before adding the popup to the scene would suffice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Popups shouldn't be implicitly re-positionned by the compositor unless the client has enabled the set_reactive flag on the popup.

@ghost
Copy link

ghost commented Oct 28, 2021

Could this support popups on layer-shell as well? Currently an assert fails in wlr_xdg_popup_get_position.

This allows compositors to easily add an xdg_popup to the
scene-graph while retaining the ability to unconstraint the popup
and decide its final position.

Compositors can handle new popups with the wlr_xdg_shell.new_surface
event, get the parent scene-graph node via wlr_xdg_popup.parent.data,
create a new scene-graph node via wlr_scene_xdg_popup_tree_create,
and unconstraint the popup if they want to.
@emersion
Copy link
Member Author

Updated to listen to ack_confgure instead of configure, and stop using the parent geometry to compute the scene-node relative position.

@ghost ghost mentioned this pull request Oct 30, 2021
6 tasks
@emersion
Copy link
Member Author

emersion commented Nov 1, 2021

wlroots has migrated to gitlab.freedesktop.org. This pull request has been moved to:

https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/3298

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants