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

xdg-shell: introduce wlr_xdg_surface_state #3106

Merged
merged 4 commits into from
Sep 30, 2021

Conversation

emersion
Copy link
Member

@emersion emersion commented Aug 11, 2021

This is preliminary work for properly handling cached state in
xdg-shell.

cc @vyivel

@emersion emersion added the breaking Breaking change in public API label Aug 11, 2021
@vyivel
Copy link

vyivel commented Aug 11, 2021

Ah, I added it myself already :D
Also, the state should probably have configure serial as well.

@emersion
Copy link
Member Author

Ah, I added it myself already :D

Yeah :P this PR is a way to get this initial change merged sooner.

Also, the state should probably have configure serial as well.

Yeah, but not to be confused with the existing wlr_xdg_surface.configure_next_serial. configure_next_serial is the outgoing serial we send to clients. The pending state should be what the client has set, not what the compositor has sent. So pending.configure_serial should be set in the ack_configure request handler.

We actually have a buggy ack_configure implementation which immediately makes the ACK'ed serial part of the current state. Instead we need to populate the pending state in the request handler, and move out existing ack_configure logic into the role commit handler.

@emersion
Copy link
Member Author

I've updated this PR to just cherry-pick @vyivel's commit from #3107. Also added a small description of the change in the commit message.

@vyivel
Copy link

vyivel commented Sep 30, 2021

Could you please update this to have wlr_xdg_surface.current as well? surface_commit_state() can be inlined, as #3107 has been obsoleted.

struct wlr_xdg_surface_state is introduced to hold the geometry
and configure serial to be applied on next wl_surface.commit.

This commit fixes our handling for ack_configure: instead of making
the request mutate our current state, it mutates the pending state
only.

Co-authored-by: Simon Ser <contact@emersion.fr>
@emersion
Copy link
Member Author

Done!

emersion added a commit to emersion/sway that referenced this pull request Sep 30, 2021
@@ -81,14 +81,13 @@ void unmap_xdg_surface(struct wlr_xdg_surface *surface) {
}

surface->configured = surface->mapped = false;
surface->configure_serial = 0;
if (surface->configure_idle) {
wl_event_source_remove(surface->configure_idle);
surface->configure_idle = NULL;
}
surface->configure_next_serial = 0;
Copy link

Choose a reason for hiding this comment

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

With surface->configure_serial = 0 removed, this should be removed as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I'm not sure. surface->configure_serial = 0 is removed because the memset below takes care of resetting it in current.

include/wlr/types/wlr_xdg_shell.h Outdated Show resolved Hide resolved
if (surface->configure_idle) {
wl_event_source_remove(surface->configure_idle);
surface->configure_idle = NULL;
}
surface->configure_next_serial = 0;

memset(&surface->geometry, 0, sizeof(struct wlr_box));
surface->current = surface->pending;
memset(&surface->pending, 0, sizeof(struct wlr_xdg_surface_state));
Copy link

Choose a reason for hiding this comment

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

Once the window geometry of the surface is set, it is not possible to unset it, and it will remain the same until set_window_geometry is called again, even if a new subsurface or buffer is attached.

I don't think the state should be cleared here. Not sure why was this done in the first place though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, good catch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, wait, I think I mixed things up. This is the unmap handler, it needs to reset all of the state.

Fixed.

types/xdg_shell/wlr_xdg_surface.c Show resolved Hide resolved
This holds the current state, and avoids having ad-hoc fields in
wlr_xdg_surface.
Rename it to scheduled_serial for consistency with the rest of
wlroots.
The protocol doesn't say we should, so let's not.

Also it's pointless to reset scheduled_serial, since 0 is a valid
serial.
Copy link

@vyivel vyivel left a comment

Choose a reason for hiding this comment

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

This iteration LGTM.
Thanks!

@vyivel vyivel merged commit 744a5c2 into swaywm:master Sep 30, 2021
emersion added a commit to emersion/sway that referenced this pull request Sep 30, 2021
vyivel pushed a commit to swaywm/sway that referenced this pull request Sep 30, 2021
@vyivel vyivel mentioned this pull request Oct 1, 2021
@emersion emersion deleted the xdg-surface-state branch October 1, 2021 14:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking Breaking change in public API
Development

Successfully merging this pull request may close these issues.

None yet

2 participants