Idle inhibit cleanup #1092
Idle inhibit cleanup #1092
Conversation
martinetd
commented
Jun 28, 2018
martinetd
commented
- Added *data pointers to the wlr types
- Cleanup destroy handlers like I think I understood what we talked about, please correct me if I got it wrong
types/wlr_idle_inhibit_v1.c
Outdated
|
|
||
| wl_resource_destroy(inhibitor->resource); | ||
| /* make resource inert */ | ||
| idle_inhibitor_v1_handle_resource_destroy(resource); |
There was a problem hiding this comment.
Hmmm. Please use the template from CONTRIBUTING.md, I don't like how generic wl_resources are passed around.
There was a problem hiding this comment.
Also I don't like the fact that you need to call two separate functions to make the resource inert.
There was a problem hiding this comment.
Can do for this one, but the idle_inhibit_manager_v1_handle_resource_destroy does not allocate its own type (only set its link + user data) so I don't really have a choice there.
types/wlr_idle_inhibit_v1.c
Outdated
| @@ -142,13 +156,16 @@ void wlr_idle_inhibit_v1_destroy(struct wlr_idle_inhibit_manager_v1 *idle_inhibi | |||
| struct wlr_idle_inhibitor_v1 *inhibitor; | |||
| struct wlr_idle_inhibitor_v1 *tmp; | |||
| wl_list_for_each_safe(inhibitor, tmp, &idle_inhibit->inhibitors, link) { | |||
| wl_resource_destroy(inhibitor->resource); | |||
| struct wl_resource *resource = inhibitor->resource; | |||
There was a problem hiding this comment.
I think it's fine to destroy resources when tearing down the whole manager. Gobals like this aren't supposed to be plug-and-play anyway, and making resources inert introduces complexity for all interfaces of the protocol and is error-prone.
| .destroy = manager_destroy, | ||
| .create_inhibitor = manager_create_inhibitor, | ||
| .destroy = manager_handle_destroy, | ||
| .create_inhibitor = manager_handle_create_inhibitor, | ||
| }; | ||
|
|
||
| static void handle_display_destroy(struct wl_listener *listener, void *data) { |
There was a problem hiding this comment.
So I was reverting the idle inhibit manager to just wl_destroying on external destroy until I stumbled on this function... Many other globals types are the same so I'll let it be for now but I thought I'd bring this up: the compositors are just all supposed to know that if the display gets destroyed they can't access e.g. server->idle_inhibitor anymore?!
It's very perverse: even if the compositor would want to listen to display destroy to clean up its side of the manager properly, it cannot touch the wlr side of things at all because there is no ordering guarantee.
I think any type that can self-destruct like this needs to have a destroy event of its own so the compositor can have a chance to cleanup properly.
| } | ||
|
|
||
| struct wl_resource *resource; | ||
| struct wl_resource *tmp_resource; | ||
| wl_resource_for_each_safe(resource, tmp_resource, &idle_inhibit->wl_resources) { | ||
| wl_resource_destroy(inhibitor->resource); | ||
| wl_resource_destroy(resource); |
There was a problem hiding this comment.
fun little bug we had before here...
types/wlr_idle_inhibit_v1.c
Outdated
| if (!inhibitor) { | ||
| return; | ||
| } | ||
| wl_resource_set_user_data(inhibitor->resource, NULL); |
There was a problem hiding this comment.
We should probably do this after emitting the event. Some listener may be calling wlr_idle_inhibitor_v1_from_resource.
There was a problem hiding this comment.
Right, force-pushed. Head is turned off at this point so this comes straight from CONTRIBUTING.md, if/when you update it since the original subsurface_destroy has an emit event as well I'd suggest putting it in the "template" as well
- Rename handlers to <type>_handle_resource_destroy and <type>_handle_destroy to be coherent - Make sure we never destroy wl_resources when we shouldn't Updates #999
|
Thanks! |
