xdg-toplevel: refactor configure/state flow#3199
Conversation
ifreund
left a comment
ifreund
left a comment
There was a problem hiding this comment.
LGTM! Splitting off the things not tied to the surface commit into a separate struct is a great idea and much cleaner. I'm also quite happy with the new names you've chosen, I find "scheduled" and "requested" much more clear.
vyivel
force-pushed
the
xdg-toplevel-refactor
branch
from
a9b9e23 to
114950e
Compare
|
Nevermind that. :P |
vyivel
force-pushed
the
xdg-toplevel-refactor
branch
from
114950e to
309d6f8
Compare
vyivel
changed the title
|
Technically |
vyivel
force-pushed
the
xdg-toplevel-refactor
branch
from
8a83907 to
309d6f8
Compare
ifreund
left a comment
ifreund
left a comment
There was a problem hiding this comment.
Separating configure and state into different structs seems like a great idea to me!
vyivel
force-pushed
the
xdg-toplevel-refactor
branch
from
309d6f8 to
a1c5055
Compare
vyivel
force-pushed
the
xdg-toplevel-refactor
branch
from
a1c5055 to
c6efee1
Compare
Previously, `wlr_xdg_toplevel` didn't follow the usual "current state + pending state" pattern and instead had confusingly named `client_pending` and `server_pending`. This commit removes them, and instead introduces `wlr_xdg_toplevel.scheduled` to store the properties that are yet to be sent to a client, and `wlr_xdg_toplevel.requested` to store the properties that a client has requested. They have different types to emphasize that they aren't actual states.
vyivel
force-pushed
the
xdg-toplevel-refactor
branch
from
743ab8f to
2151257
Compare
This commit removes any checks whether a configure will change anything
and makes configures be sent unconditionally. Additionally, configures
are scheduled on xdg_toplevel.{un,}set_{maximized,fullscreen} events.
vyivel
force-pushed
the
xdg-toplevel-refactor
branch
from
2151257 to
e68d801
Compare
| wl_array_init(&states); | ||
| if (surface->toplevel->server_pending.maximized) { | ||
| if (surface->toplevel->scheduled.maximized) { | ||
| uint32_t *s = wl_array_add(&states, sizeof(uint32_t)); |
There was a problem hiding this comment.
Unrelated note: seems like this code could be cleaner by filling a uint32_t states[32], then creating a wl_array pointing to it. Something like:
uint32_t states[32];
size_t states_len = 0;
if (scheduled.maximized) {
states[states_len++] = XDG_TOPLEVEL_STATE_MAXIMIZED;
}
…
assert(states_len < sizeof(states) / sizeof(states[0]));
struct wl_array states_arr = {
.data = states,
.size = states_len * sizeof(states[0]),
};There was a problem hiding this comment.
Seems like a pretty unconventional way to create a wl_array, but sure, why not. Also avoids potential OOM errors.
There was a problem hiding this comment.
Yeah… Ideally wl_array could be marked const so that wayland-util functions like wl_array_add can't be called, but it's a bit too late. Here wl_array is just a read-only structure used by libwayland to send events.
3 participants
See individual commits.
Fixes #2330
Preliminary work for #3151.
If this is fine, I'll make a similar PR for
wlr_layer_surface.This is a breaking change: compositors that used to check
wlr_xdg_toplevel.client_pendingwill now have to checkwlr_xdg_toplevel.requested.