Skip to content

client: Support destroying GlobalList through wl_fixes#886

Merged
ids1024 merged 1 commit intomasterfrom
feat/client_wl_fixes
Mar 27, 2026
Merged

client: Support destroying GlobalList through wl_fixes#886
ids1024 merged 1 commit intomasterfrom
feat/client_wl_fixes

Conversation

@Drakulix
Copy link
Copy Markdown
Member

Mostly a perquisite for supporting wl_fixes v2, when that lands, but it doesn't hurt to expose the ability to destroy the registry. Luckily wl_fixes has no events yet, so this makes it really easy to bypass the clients State and handle it fully internally.

@ids1024
Copy link
Copy Markdown
Member

ids1024 commented Mar 24, 2026

Luckily wl_fixes has no events yet

And I guess if a future version of wl_fixes adds events (not sure if that's likely) this still won't require any immediate breaking change. (At worst we'd ignore the event, and clients needing to handle the fixes event would need to bind a separate instance of the global.)

@ids1024
Copy link
Copy Markdown
Member

ids1024 commented Mar 24, 2026

The other question here would be if we need to bind a wl_fixes instance here all the time, instead of lazily creating one when .destroy() is called. But we ultimately will want that for #887.

It could also make sense for GlobalList to have a Drop impl that destroys the registry, but that would be a breaking API change. So at least for now we should leave that out.

Comment on lines +214 to +217
pub fn destroy(self) {
if let Some(fixes) = self.fixes.get() {
fixes.destroy_registry(&self.registry);
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

After calling fixes.destroy_registry(), we also need to call Backend::destroy_object(). Compare https://github.com/rust-windowing/softbuffer/pull/218/changes which is probably still the only thing that's using wl_fixes in a wayland-rs client so far.

This PR also seems to be missing a call to fixes.destroy() to destroy the instance of wl_fixes itself. Probably in Drop.

Copy link
Copy Markdown
Member Author

@Drakulix Drakulix Mar 25, 2026

Choose a reason for hiding this comment

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

I have added the backend.destroy_object() call as well as fixes.destroy(), but to the destroy-method.

The reason being, that for #887 we technically could still be using the wl_fixes-object for acknowledging global removals, even after the GlobalList has been dropped.

This can be changed as well, if we decide to free the registry on Drop.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The downside of this is that if GlobalList::destroy isn't called, that wl_fixes instance is leaked. But it isn't really a big deal to leak an object without any events, compared to leaking the registry. Since we'll need wl_fixes for #887, that seems acceptable.

@ids1024
Copy link
Copy Markdown
Member

ids1024 commented Mar 27, 2026

Testing in softbuffer, this seems to behave as expected.

rust-windowing/softbuffer#350

@ids1024 ids1024 merged commit 8b9772b into master Mar 27, 2026
16 of 17 checks passed
ids1024 added a commit to rust-windowing/softbuffer that referenced this pull request Mar 27, 2026
@Drakulix Drakulix deleted the feat/client_wl_fixes branch March 27, 2026 08:17
ids1024 added a commit to rust-windowing/softbuffer that referenced this pull request Mar 31, 2026
ids1024 added a commit to rust-windowing/softbuffer that referenced this pull request Mar 31, 2026
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.

2 participants