[PATCH weston v3 2/2] Implement data_device interface destructor

[Pekka Paalanen]

On Wed, 27 Aug 2014 16:34:25 +0530
kabeer.khan at samsung.com wrote:

> From: kabeer <kabeer.khan at samsung.com>
> 
> window: use data_device interface destructor
> data-device: implement data_device_release destructor
> 

Should mention the bug link here:
https://bugs.freedesktop.org/show_bug.cgi?id=81745

> Signed-off-by: kabeer <kabeer.khan at samsung.com>
> ---
>  clients/window.c  |   14 ++++++++++----
>  src/data-device.c |   14 ++++++++++----
>  2 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/clients/window.c b/clients/window.c
> index 5d64022..e32c2b3 100644
> --- a/clients/window.c
> +++ b/clients/window.c
> @@ -133,6 +133,7 @@ struct display {
>  
>  	int has_rgb565;
>  	int seat_version;
> +    int data_device_manager_version;

Looks like all indentations in this patch have been converted from tabs
to spaces. That should be fixed.

>  };
>  
>  struct window_output {
> @@ -5036,6 +5037,7 @@ display_add_input(struct display *d, uint32_t id)
>  			 EPOLLIN, &input->repeat_task);
>  }
>  
> +#define DATA_DEVICE_MANAGER_VERSION_CURRENT 2 /* Current version of data_device_manager */

So far only xdg-shell has been doing this, because xdg-shell is special
while it is under development: every bump breaks backward compatibility.

>  static void
>  input_destroy(struct input *input)
>  {
> @@ -5048,9 +5050,12 @@ input_destroy(struct input *input)
>  	if (input->selection_offer)
>  		data_offer_destroy(input->selection_offer);
>  
> -	if (input->data_device)
> -		wl_data_device_destroy(input->data_device);
> -
> +    if (input->data_device) {
> +        if(input->display->data_device_manager_version>=DATA_DEVICE_MANAGER_VERSION_CURRENT)

Need space after reserved word. Need spaces around binary operator, and
breaking long lines.

The check needs to be against the version that added the release
request, not the latest supported version (i.e. using
DATA_DEVICE_MANAGER_VERSION_CURRENT is not correct, if someone later
adds another feature and bumps VERSION_CURRENT).

> +            wl_data_device_release(input->data_device);
> +        else
> +            wl_data_device_destroy(input->data_device);
> +    }

Yes, this is exactly how destroying should be done. I think this might
even be the first time we actually do it right for the interfaces that
got destructor protocol added later.

>  	if (input->display->seat_version >= 3) {
>  		if (input->pointer)
>  			wl_pointer_release(input->pointer);
> @@ -5134,9 +5139,10 @@ registry_handle_global(void *data, struct wl_registry *registry, uint32_t id,
>  		d->shm = wl_registry_bind(registry, id, &wl_shm_interface, 1);
>  		wl_shm_add_listener(d->shm, &shm_listener, d);
>  	} else if (strcmp(interface, "wl_data_device_manager") == 0) {
> +        d->data_device_manager_version=MIN(version,DATA_DEVICE_MANAGER_VERSION_CURRENT);

This is the only place where would need the VERSION_CURRENT, so having
the #define is not that useful, even for grep-ability.

>  		d->data_device_manager =
>  			wl_registry_bind(registry, id,
> -					 &wl_data_device_manager_interface, 1);
> +                     &wl_data_device_manager_interface, d->data_device_manager_version);

Good.

>  	} else if (strcmp(interface, "xdg_shell") == 0) {
>  		d->xdg_shell = wl_registry_bind(registry, id,
>  						&xdg_shell_interface, 1);
> diff --git a/src/data-device.c b/src/data-device.c
> index 75fc60c..697ed55 100644
> --- a/src/data-device.c
> +++ b/src/data-device.c
> @@ -761,10 +761,16 @@ data_device_set_selection(struct wl_client *client,
>  				  wl_resource_get_user_data(source_resource),
>  				  serial);
>  }
> +static void
> +data_device_release(struct wl_client *client, struct wl_resource *resource)
> +{
> +       wl_resource_destroy(resource);
> +}
>  
>  static const struct wl_data_device_interface data_device_interface = {
>  	data_device_start_drag,
>  	data_device_set_selection,
> +	data_device_release
>  };
>  
>  static void
> @@ -844,8 +850,8 @@ get_data_device(struct wl_client *client,
>  	struct wl_resource *resource;
>  
>  	resource = wl_resource_create(client,
> -				      &wl_data_device_interface, 1, id);
> -	if (resource == NULL) {
> +                      &wl_data_device_interface, wl_resource_get_version(manager_resource), id);
> +    if (resource == NULL) {
>  		wl_resource_post_no_memory(manager_resource);
>  		return;
>  	}
> @@ -869,7 +875,7 @@ bind_manager(struct wl_client *client,
>  
>  	resource =
>  		wl_resource_create(client,
> -				   &wl_data_device_manager_interface, 1, id);
> +				   &wl_data_device_manager_interface, MIN(version, 2), id);

I think we should fail here if version > 2, because it means a client
tries to bind with a version greater than what the server advertized,
which I believe is a protocol error.

Except, we do not have to check it here at all, because registry_bind()
in libwayland-server already does the check.

So simply use 'version' here, no MIN.

>  	if (resource == NULL) {
>  		wl_client_post_no_memory(client);
>  		return;
> @@ -909,7 +915,7 @@ WL_EXPORT int
>  wl_data_device_manager_init(struct wl_display *display)
>  {
>  	if (wl_global_create(display,
> -			     &wl_data_device_manager_interface, 1,
> +                 &wl_data_device_manager_interface, 2,
>  			     NULL, bind_manager) == NULL)
>  		return -1;
>  

Otherwise looks ok.


Thanks,
pq