Skip to content
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

New attached windows need to be placed #3676

Merged
merged 4 commits into from
Nov 26, 2024
Merged

Conversation

AlanGriffiths
Copy link
Contributor

Alternative to #3675

Copy link
Contributor

@mattkae mattkae left a comment

Choose a reason for hiding this comment

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

This does not fix the problem that I was having with cosmic-bg because LayerSurfaceV1::configure() happens before BasicWindowManager::place_new_surface(). The surface has not yet been placed when we send the configure event

@AlanGriffiths
Copy link
Contributor Author

That sequence seems ... wrong. But I see it too:

[ 635173.015]  -> wl_compositor#6.create_surface(new id wl_surface#11)
[ 635173.030]  -> zwlr_layer_shell_v1#8.get_layer_surface(new id zwlr_layer_surface_v1#12, wl_surface#11, wl_output#3, 0, "wallpaper")
[ 635173.045]  -> zwlr_layer_surface_v1#12.set_anchor(15)
[ 635173.052]  -> zwlr_layer_surface_v1#12.set_exclusive_zone(-1)
[ 635173.058]  -> zwlr_layer_surface_v1#12.set_keyboard_interactivity(0)
[ 635173.066]  -> wl_surface#11.commit()
[ 635173.073]  -> wp_viewporter#9.get_viewport(new id wp_viewport#13, wl_surface#11)
[ 635173.102]  -> wp_fractional_scale_manager_v1#10.get_fractional_scale(new id wp_fractional_scale_v1#14, wl_surface#11)
[ 635173.788] zwlr_layer_surface_v1#12.configure(9, 0, 0)
[ 635173.807] zwlr_layer_surface_v1#12.configure(10, 1600, 900)

@AlanGriffiths
Copy link
Contributor Author

The surface has not yet been placed when we send the configure event

Fixed now

@mattkae
Copy link
Contributor

mattkae commented Nov 21, 2024

This makes it not crash 👍 But it doesn't make it work for cosmic-bg unlike my other PR. I'll investigate and let you know why. They appear the same on the surface

@mattkae
Copy link
Contributor

mattkae commented Nov 22, 2024

I have fixed it with the latest commit @AlanGriffiths:

image

@Saviq
Copy link
Collaborator

Saviq commented Nov 22, 2024

32 - wlcs.Anchor/LayerSurfaceLayoutTest.* (Failed)

:)

@AlanGriffiths
Copy link
Contributor Author

32 - wlcs.Anchor/LayerSurfaceLayoutTest.* (Failed)

:)

I understand why that broke, working on a fix

@AlanGriffiths
Copy link
Contributor Author

Oops! Pulled the rug out of another corner

@AlanGriffiths
Copy link
Contributor Author

@mattkae I'm not sure what the essential behaviour you want from 9247cc9 is. I've fixed the stuff it broke, but now not sure if the last two commits are even changing anything. (A shame you couldn't add a test)

@AlanGriffiths AlanGriffiths dismissed mattkae’s stale review November 26, 2024 09:02

Mattermost comment that "That [latest commit] did not break me"

@AlanGriffiths
Copy link
Contributor Author

Not sure why (but I never tracked down the cause) this also seems to fix some intermittent yambar crashing

Copy link
Contributor

@mattkae mattkae left a comment

Choose a reason for hiding this comment

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

Working well for me now 👍

@mattkae mattkae added this pull request to the merge queue Nov 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 26, 2024
@mattkae mattkae added this pull request to the merge queue Nov 26, 2024
Merged via the queue into main with commit 9e1ccc8 Nov 26, 2024
36 of 39 checks passed
@mattkae mattkae deleted the place-new-attached-windows branch November 26, 2024 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants