layer-shell: refactor configure/state flow#3201
Conversation
ifreund
left a comment
ifreund
left a comment
There was a problem hiding this comment.
This is a nice simplification, I think it can be made even simpler though
vyivel
force-pushed
the
layer-surface-state-refactor
branch
from
69f68fd to
6efc586
Compare
vyivel
force-pushed
the
layer-surface-state-refactor
branch
from
6efc586 to
077ded3
Compare
vyivel
force-pushed
the
layer-surface-state-refactor
branch
from
077ded3 to
136e388
Compare
ifreund
left a comment
ifreund
left a comment
There was a problem hiding this comment.
I'm not entirely convinced that using the idle event source/configure scheduling strategy here makes sense. It feels like we're doing it just because the code is a bit closer to the xdg shell implementation, not because it actually provides a meaningful benefit for layer shell.
The refactor of the states you have done looks unambiguously good to me, and I'd be happy to merge that part. Maybe it would make sense to move the commit adding scheduling/batching to a separate PR and keep this one focused on the refactor without adding features?
|
I guess the benefits are 1) not sending two configure events one after other (although that's unlikely to happen) and 2) having more predictable logic and API, which I consider to be more important than the simplicity of implementation (partly the reason I made these PRs), especially considering that scheduling isn't that complex anyway. |
|
|
vyivel
force-pushed
the
layer-surface-state-refactor
branch
3 times, most recently
from
4b659af to
df2adff
Compare
|
Moved scheduling logic commit to #3212. |
vyivel
force-pushed
the
layer-surface-state-refactor
branch
from
df2adff to
8906365
Compare
ifreund
left a comment
ifreund
left a comment
There was a problem hiding this comment.
Thanks for splitting this up, this patch looks nearly ready to merge.
types/wlr_layer_shell_v1.c
Outdated
|
|
||
| static bool layer_surface_state_changed(struct wlr_layer_surface_v1 *surface) { | ||
| struct wlr_layer_surface_v1_state *state; | ||
| static bool layer_surface_state_changed(struct wlr_layer_surface_v1 *surface, |
There was a problem hiding this comment.
I wonder if we shouldn't just get rid of this function and the configure_next_serial field in this patch already.
There was a problem hiding this comment.
Though actually, I don't think the layer shell protocol ever requires the compositor to send a noop configure, unlike the xdg-shell protocol. This is fine as-is I believe.
There was a problem hiding this comment.
On the other hand, if the compositor saves the configure serial returned and waits for the client to ack that serial, this function can cause problems. If this function returns false no new configure will be sent or new serial generated, which means that the compositor may hang forever. I'm back to thinking we should just get rid of it and behave in the most predictable way possible for compositors.
There was a problem hiding this comment.
Removed the function and the associated check, if the caller wants to send a configure then why not. wlr_layer_surface_v1_configure() now returns surface->pending.configure_serial on memory allocation failure, not like it matters much.
Same logic as xdg-toplevel.
vyivel
force-pushed
the
layer-surface-state-refactor
branch
from
8906365 to
8f7a4f5
Compare
|
Thanks a lot for reviewing! |
2 participants
Same logic as xdg-toplevel.
Preliminary work for #3151.
This is a breaking change: compositors that used to check
wlr_layer_surface_v1.client_pendingwill now have to checkwlr_layer_surface_v1.pending