Skip to content

layer_shell: destroy layer_surface on assigned output destruction #8668

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 17, 2025

Conversation

llyyr
Copy link
Contributor

@llyyr llyyr commented Apr 16, 2025

According to the spec, the closed event should be sent when the surface is no longer shown, because the output may have been destroyed or the user may have asked for it to be removed. In such cases, the clients should destroy the resource.

This fixes mako not being able to show notifications if the assigned output was destroyed while a notificataion was still visible

@llyyr
Copy link
Contributor Author

llyyr commented Apr 16, 2025

I can't really understand what the previous code was trying to do here, so I don't know if this is completely incorrect but this behavior matches what every other compositor does with on the output disable event. Even sway before the scene port, so probably just a tab complete fail that went unnoticed?

@llyyr llyyr force-pushed the layer-shell-destroy branch from 3cbea64 to 52f1286 Compare April 16, 2025 04:17
@emersion
Copy link
Member

It seems like wlr_layer_surface_v1_destroy() was called on output destroy before 188811f.

I think we're missing a layer shell surface destroy handler? Here the struct sway_layer_surface seems to be leaked when wlr_layer_surface_v1_destroy() is called? I'm actually surprised we don't hit asserts due to dangling listeners.

(There are other cases where a layer surface might get destroyed, e.g. when a client disconnects.)

@llyyr llyyr force-pushed the layer-shell-destroy branch from 52f1286 to 75e0f0a Compare April 16, 2025 12:04
@llyyr
Copy link
Contributor Author

llyyr commented Apr 16, 2025

I think we're missing a layer shell surface destroy handler?

Seems like this is handled by handle_node_destroy, including clearing out dangling listeners. I modified my patch destroy the layer surface in handle_node_destroy instead if we don't have any outputs

@emersion
Copy link
Member

I think this still leaves the struct sway_layer_surface behind when a client destroys the layer shell surface.

@llyyr
Copy link
Contributor Author

llyyr commented Apr 17, 2025

I think this still leaves the struct sway_layer_surface behind when a client destroys the layer shell surface.

handle_node_destroy is notified when a client destroys the layer shell surface, and we free struct sway_layer_surface there. It's a bit confusing that we're using wlr_scene_node's destroy event like this, but I think it makes sense

@emersion
Copy link
Member

handle_node_destroy is notified when a client destroys the layer shell surface

What is the mechanism destroying the node when the layer shell surface is destroyed?

@llyyr
Copy link
Contributor Author

llyyr commented Apr 17, 2025

@emersion
Copy link
Member

Oh right, my bad, I forgot that we were using the scene-graph layer shell helper now…

I think I preferred the previous version of this patch.

According to the spec, the closed event should be sent when the surface
is no longer shown, because the output may have been destroyed or the
user may have asked for it to be removed. In such cases, the clients
should destroy the resource.

This fixes mako not being able to show notifications if the assigned
output was destroyed while a notificataion was still visible

Fixes: 188811f ("scene_graph: Port layer_shell")
@llyyr llyyr force-pushed the layer-shell-destroy branch from 75e0f0a to f79d688 Compare April 17, 2025 18:44
@llyyr
Copy link
Contributor Author

llyyr commented Apr 17, 2025

Changed it back

Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

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

Thank you, and sorry for the noise!

@emersion emersion merged commit 8a8c78d into swaywm:master Apr 17, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants