[PATCH weston v5 3/7] compositor: Add public interface support for client-requested idle inhibition

[Quentin Glidic]

On 30/08/2016 22:39, Bryce Harrington wrote:
> On Tue, Aug 30, 2016 at 12:53:46PM +0200, Quentin Glidic wrote:
>> On 30/08/2016 03:34, Bryce Harrington wrote:
 >>> [snip]
>>> +}
>>> +
>>> +static void
>>> +idle_inhibit_manager_create_inhibitor(struct wl_client *client, struct wl_resource *resource,
>>> +				      uint32_t id, struct wl_resource *surface_resource)
>>> +{
>>> +	struct weston_surface *surface = wl_resource_get_user_data(surface_resource);
>>> +	struct weston_idle_inhibitor *inhibitor;
>>> +	struct wl_resource *cr;
>>> +
>>> +	cr = wl_resource_create(client, &zwp_idle_inhibitor_v1_interface,
>>> +				wl_resource_get_version(resource), id);
>>> +	if (cr == NULL) {
>>> +		wl_client_post_no_memory(client);
>>> +		return;
>>> +	}
>>> +
>>> +	inhibitor = zalloc(sizeof *inhibitor);
>>> +	if (inhibitor == NULL) {
>>> +		wl_client_post_no_memory(client);
>>> +		return;
>>> +	}
>>> +
>>> +	inhibitor->surface = surface;
>>> +	inhibitor->surface->inhibit_idling = true;
>>> +
>>> +	wl_resource_set_implementation(cr, &idle_inhibitor_interface,
>>> +				       inhibitor, destroy_idle_inhibitor);
>>
>> I think you can use the surface as user_data directly here?
>
> Do you mean instead of setting it as inhibitor->surface, pass it as the
> user_data parameter for wl_resource_set_implementation()?

Yes. All the inhibitor stuff (even the signal) is in the surface struct.
But that depends on the actual behaviour you want for the manager 
destructor. See below.

>>> +}
>>> +
>>> +static const struct zwp_idle_inhibit_manager_v1_interface idle_inhibit_manager_interface = {
>>> +	idle_inhibit_manager_destroy,
>>> +	idle_inhibit_manager_create_inhibitor
>>> +};
>>> +
>>> +static void
>>> +bind_idle_inhibit_manager(struct wl_client *client,
>>> +			  void *data, uint32_t version, uint32_t id)
>>> +{
>>> +	struct wl_resource *resource;
>>> +
>>> +	resource = wl_resource_create(client, &zwp_idle_inhibit_manager_v1_interface,
>>> +				      version, id);
>>> +	if (resource == NULL) {
>>> +		wl_client_post_no_memory(client);
>>> +		return;
>>> +	}
>>> +
>>> +	wl_resource_set_implementation(resource, &idle_inhibit_manager_interface,
>>> +				       NULL, NULL);
>>
>> Maybe add a comment that a NULL destructor is fine because you don’t
>> have any memory to free here?
>> Or maybe you need some memory, to keep track of inhibitors? Is the
>> behaviour defined if you destroy the manager while having
>> inhibitors?
>
> That behavior has not been defined, to be honest I didn't give it any
> consideration.  Is there a use case where one might want to do it?
 >
> When the client exits normally (or abnormally), the inhibitors are
> getting the destroy events appropriately, so I'm not certain we would
> need to deliberately track and destroy the inhibitors individually.

A use case? Not really.

1. No need for inhibitors, you don’t bind the manager
2. The need is created (e.g. user configured it)
3. Bind the manager
4. Create inhibitors
5. The need is destroyed (e.g. user unchecked the box)
6. You want everything cleaned up

If you destroy the manager (which is not weird, as you won’t need 
inhibition), what happens?

A) All inhibitors are still active until destroyed
This is what your code does, but it should be in the spec then.

B) Inhibitors (from this specific manager bind) are “destroyed”
This will need a way to list all inhibitors associated with this 
manager. The rest should work with this code: with the inhibitor struct, 
setting the surface to NULL will mean “dangling inhibitor”.

C) Make it an error, so inhibitors must be destroyed first
You need at least a “ref count” on the manager. Usually we use a wl_list 
and check for wl_list_empty().

Maybe there are other options, but these are the obvious ones.

Cheers,


-- 

Quentin “Sardem FF7” Glidic