custom_shell_surface_needs_commit is unreliable

[alebastr]

I've been debugging an issue where Waybar fails to switch from the GTK_LAYER_SHELL_LAYER_BOTTOM to OVERLAY or TOP with Sway 1.10.
The following fragment of WAYLAND_DEBUG log has sent me to the scary depths of GTK

[2653097.622] {Default Queue}  -> zwlr_layer_surface_v1#46.set_layer(3)
[2653321.642] {Default Queue}  -> zwlr_layer_surface_v1#46.set_layer(1)
[2653673.080] {Default Queue}  -> zwlr_layer_surface_v1#46.set_layer(3)
[2653824.898] {Default Queue}  -> zwlr_layer_surface_v1#46.set_layer(1)
[2653961.042] {Default Queue}  -> zwlr_layer_surface_v1#46.set_layer(3)
[2654040.982] {Default Queue}  -> zwlr_layer_surface_v1#46.set_layer(1)
[2654153.370] {Default Queue}  -> zwlr_layer_surface_v1#46.set_layer(3)
[2654249.116] {Default Queue}  -> zwlr_layer_surface_v1#46.set_layer(1)

and helped me realize two facts

• gdk_window_invalidate_rect schedules a redraw and commit on the next frame callback
• wlr_scene may stop delivering frame callbacks to a fully occluded surface

So once the layer surface is fully hidden behind an opaque surface, it won't be able to get out without external help.

It appears that we have to use a direct wl_surface_commit, but without reintroducing #47 or #51. Looking at these issues, it occurs to me that it would be safe to commit if GdkWindowImplWayland.pending_commit and GdkWindowImplWayland.pending_buffer_attached are unset, which we can easily check via private struct accessors.

In the meantime, I'm going to work around that in waybar by adding a conditional wl_surface_commit that is only allowed between GdkFrameClock::after-paint and GdkFrameClock::before-paint. That's the closest approximation of the check above possible with public GTK APIs.

[wmww]

Hmm, I believe you that there are problems, but I'm scared to change anything because

• as you mentioned, there have been multiple subtle, hard to debug bugs and crashes in this area
• what we have now doesn't crash, and at least works in the standard case where surface properties don't change
• gtk-priv is bad and every additional usage increases maintenance burden and chance of GTK breaking us by changing it's internal logic
• this whole project is somewhat legacy (replaced by the gtk4 version), so I'm biased against risking instability for the sake of new things working (even if it is technically a bug fix)

my preference would be either for app devs to work around the issue in their code if possible, or failing that to somehow make the fixed but potentially less stable code path opt-in (so we don't break existing apps that work fine as-is)

[alebastr]

my preference would be either for app devs to work around the issue in their code if possible,

I really don't like the workaround code: alebastr/Waybar@6416f7d. And it isn't something I want to see being copied from app to app.

or failing that to somehow make the fixed but potentially less stable code path opt-in (so we don't break existing apps that work fine as-is)

How about exposing this code as gtk_layer_force_commit(GtkWindow *window)?

[wmww]

How about exposing this code as gtk_layer_force_commit(GtkWindow *window)?

That would be acceptable, as long as the library continues to work the same when apps don't call it. I realize this isn't a great solution, but I think it's the best compromise if more private struct usage is indeed needed.

Taking a closer look at the issue, it looks like the call to custom_shell_surface_needs_commit() is already on an optional code path:

gtk-layer-shell/src/layer-surface.c

Line 370 in 07653ce

if (version >= ZWLR_LAYER_SURFACE_V1_SET_LAYER_SINCE_VERSION) {

. The alternative is to remap the window. Might a solution to your problem be to remap the window whenever the layer (or other property?) changes? This could be done either in your app or the library. Might cause a flicker, but that shouldn't be that big a deal, right?

[alebastr]

Updated the PR with explicit commit method.

Remapping seems a bit too heavy when you only need to send two Wayland protocol messages. And yes, there is an obnoxious flickering that would accompany any layer surface state updates, as you can't determine when you really require a remap. Layer change is not the only reason the surface can become occluded or visible.

[wmww]

How often are you updating the layer surface state in Waybar? Does this happen in cases other than when user configuration changes?

[alebastr]

Waybar partially implements sway-bar(5) IPC protocol, and I've been planning to add support for more properties.

At the moment, user can change layer/exclusivity/visibility via IPC and bind bar visibility to a press/release of the modifier (Super) key. Which is really often when you work with a tiling WM.

[wmww]

I don't understand why flickering on property change would be an issue for those usecases?

[alebastr]

This is how it works with custom_shell_surface_remap (web player seems to skip a few frames and make it look better than it actually is). Not sure of your definition of issue, but for me it looks very annoying.

The bar is configured to toggle between the bottom and the overlay layers depending on the modifier key state.

bar.webm

[wmww]

All right fine, I'm convinced