Skip to content

Wayland: Deduplicate cleanup logic#116513

Merged
Repiteo merged 1 commit into
godotengine:masterfrom
deralmas:wl/dedup-global-remove
Mar 19, 2026
Merged

Wayland: Deduplicate cleanup logic#116513
Repiteo merged 1 commit into
godotengine:masterfrom
deralmas:wl/dedup-global-remove

Conversation

@deralmas
Copy link
Copy Markdown
Member

We now reuse the global_remove event handler. This removes a considerable amount of duplication, minimizes human error (such as cleaning up a global in one place but not the other), and helps test the dynamic global removal logic.


NOTE: I think that the window cleanup logic in WaylandThread::destroy is broken. That said, the DS is supposed to clean all windows in the first place (and does so quite well), so I preserved it for now and added a warning. That'll do for now :P

@deralmas
Copy link
Copy Markdown
Member Author

deralmas commented Mar 16, 2026

Oops I just noticed that we're iterating while removing elements from the HashSet. For some reason it works but I don't think that it's a good thing to do?

Edit: The HashSet header says:

/**
 * Implementation of Set using a bidi indexed hash map.
 * Use RBSet instead of this only if the following conditions are met:
 *
 * - You need to keep an iterator or const pointer to Key and you intend to add/remove elements in the meantime.

I'm afraid I might have ran into undefined behavior indeed, or at least something unpredictable. I'll fix this ASAP.

@deralmas deralmas force-pushed the wl/dedup-global-remove branch from a9c39ab to f8f12d1 Compare March 16, 2026 10:42
@deralmas
Copy link
Copy Markdown
Member Author

Should be fixed, but I'm double-checking on my VM with some debug logging to make sure.

While RBSet should support this well with the common "Element loop" idiom, I chose to be double-safe and iterate on a copy of the keys, so that we're completely isolated from whatever happens to the names set. It's a bunch of extra allocations, but the average client receives less than a hundred globals and it's cleanup code.

@deralmas
Copy link
Copy Markdown
Member Author

All right, no regressions on the usual compositors I test with. Everything should be good now, sorry for the mess.

@Repiteo
Copy link
Copy Markdown
Contributor

Repiteo commented Mar 16, 2026

Needs a rebase after #102987

We now reuse the `global_remove` event handler. This removes a
considerable amount of duplication, minimizes human error (such as
cleaning up a global in one place but not the other), and helps test the
dynamic global removal logic.
@deralmas deralmas force-pushed the wl/dedup-global-remove branch from f8f12d1 to 29e1bc5 Compare March 19, 2026 12:11
@deralmas
Copy link
Copy Markdown
Member Author

Done!

@Repiteo Repiteo merged commit fa4d24c into godotengine:master Mar 19, 2026
20 checks passed
@Repiteo
Copy link
Copy Markdown
Contributor

Repiteo commented Mar 19, 2026

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants