[PATCH] Bug fix client apps because of output change

[Pekka Paalanen]

Hi,

overall this looks fine now, but I still haven't tested it. I'm moving
on to nitpick on the minor details. ;-)

The patch title (topic line) is usually prefixed with the component the
patch is affecting. Looking at the clients/desktop-shell.c history, the
appropriate prefix would be "desktop-shell:". Also, this patch does not
affect any other clients.

On Thu,  6 Mar 2014 18:31:14 +0800
Quanxian Wang <quanxian.wang at intel.com> wrote:

> 1)
> Width and height of Panel and Background depend
> on output's, therefore they should be bound
> with output changes including mode, transform and scale.
> 
> 2)
> Update the min_allocation before resize the panel and
> background window. Add window_set_min_allocation function
> because it is invisible outside window.c.

An alternative to adding the set_min_allocation would be to cause the
min_allocation to be something very small to begin with. The
min_allocation is not important for the panel and background surfaces,
because they follow special rules, and cannot be resized freely anyway.

I'm fine with either way.

> 
> Signed-off-by: Quanxian Wang <quanxian.wang at intel.com>
> ---
>  clients/desktop-shell.c | 80
> +++++++++++++++++++++++++++++++++++++++++++++++--
> clients/window.c        |  7 +++++ clients/window.h        |  2 ++
>  3 files changed, 87 insertions(+), 2 deletions(-)
> 
> diff --git a/clients/desktop-shell.c b/clients/desktop-shell.c
> index a0c6b6d..4373448 100644
> --- a/clients/desktop-shell.c
> +++ b/clients/desktop-shell.c
> @@ -103,6 +103,15 @@ struct output {
>  
>  	struct panel *panel;
>  	struct background *background;
> +	struct {
> +		int height;
> +		int width;
> +		uint32_t refresh;
> +	} mode;
> +
> +	uint32_t interface_version;
> +	uint32_t transform;
> +	uint32_t scale;
>  };
>  
>  struct panel_launcher {
> @@ -1145,6 +1154,45 @@ desktop_destroy_outputs(struct desktop
> *desktop) }
>  
>  static void
> +update_output(struct output *output)
> +{
> +	struct panel *panel = output->panel;
> +	struct background *background = output->background;
> +	int width, height;
> +
> +	if (!output)

You already dereferenced 'output' above, checking for NULL here is
useless. 'output' can never be NULL anyway, right?

> +		return;
> +
> +	width = output->mode.width;
> +	height = output->mode.height;
> +
> +	switch (output->transform) {
> +	case WL_OUTPUT_TRANSFORM_90:
> +	case WL_OUTPUT_TRANSFORM_270:
> +	case WL_OUTPUT_TRANSFORM_FLIPPED_90:
> +	case WL_OUTPUT_TRANSFORM_FLIPPED_270:
> +		/* Swap width and height */
> +		width = output->mode.height;
> +		height = output->mode.width;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (output->scale != 0) {

If scale was initialized to 1, there would be no need for 'if'. Right?

> +		width /= output->scale;
> +		height /= output->scale;
> +	}
> +
> +	/* Set min_allocation of panel */

Needless comment.

> +	window_set_min_allocation(panel->window, width, 32);
> +	window_set_min_allocation(background->window, width, height);
> +
> +	window_schedule_resize(background->window, width, height);
> +	window_schedule_resize(panel->window, width, 32);
> +}
> +
> +static void
>  output_handle_geometry(void *data,
>                         struct wl_output *wl_output,
>                         int x, int y,
> @@ -1157,6 +1205,11 @@ output_handle_geometry(void *data,
>  {
>  	struct output *output = data;
>  
> +	output->transform = transform;
> +
> +	if (output->interface_version < 2)
> +		update_output(output);
> +
>  	window_set_buffer_transform(output->panel->window,
> transform); window_set_buffer_transform(output->background->window,
> transform); }
> @@ -1169,12 +1222,29 @@ output_handle_mode(void *data,
>  		   int height,
>  		   int refresh)
>  {
> +	struct output *output = data;
> +
> +	if (flags & WL_OUTPUT_MODE_CURRENT) {
> +		if (!output)
> +			return;
> +
> +		output->mode.width = width;
> +		output->mode.height = height;
> +		output->mode.refresh = refresh;
> +
> +		if (output->interface_version < 2)
> +			update_output(output);
> +	}
>  }
>  
>  static void
>  output_handle_done(void *data,
>                     struct wl_output *wl_output)
>  {
> +	struct output *output = data;
> +
> +	if (output->interface_version >= 2)
> +		update_output(output);

The only case when this can be called is when the interface version is
at least 2, so no need to check. The protocol specification guarantees
that.

>  }
>  
>  static void
> @@ -1184,6 +1254,11 @@ output_handle_scale(void *data,
>  {
>  	struct output *output = data;
>  
> +	output->scale  = scale;
> +
> +	if (output->interface_version < 2)
> +		update_output(output);

The only case when this can be called is when the interface version is
at least 2, so no need to call update_output() here.

> +
>  	window_set_buffer_scale(output->panel->window, scale);
>  	window_set_buffer_scale(output->background->window, scale);
>  }
> @@ -1212,7 +1287,7 @@ output_init(struct output *output, struct
> desktop *desktop) }
>  
>  static void
> -create_output(struct desktop *desktop, uint32_t id)
> +create_output(struct desktop *desktop, uint32_t id, uint32_t version)
>  {
>  	struct output *output;
>  
> @@ -1223,6 +1298,7 @@ create_output(struct desktop *desktop, uint32_t
> id) output->output =
>  		display_bind(desktop->display, id,
> &wl_output_interface, 2); output->server_output_id = id;
> +	output->interface_version = version;

This is not completely correct. In the future, a compositor may
advertise wl_output interface version 3 or greater, if the protocol has
been augmented. However, the valid version is the minimum of what the
compositor advertises and what the client is written to support.
Therefore the interface version should be min(version, 2).

Note that create_output() already has a small bug in this: it always
binds with version 2 regardless of what the compositor advertised.

You can compare to the "desktop_shell" global handling which seems
correct.

>  
>  	wl_output_add_listener(output->output, &output_listener,
> output); 
> @@ -1247,7 +1323,7 @@ global_handler(struct display *display,
> uint32_t id, desktop->interface_version);
>  		desktop_shell_add_listener(desktop->shell,
> &listener, desktop); } else if (!strcmp(interface, "wl_output")) {
> -		create_output(desktop, id);
> +		create_output(desktop, id, version);
>  	}
>  }
>  
> diff --git a/clients/window.c b/clients/window.c
> index c8287e2..6c01222 100644
> --- a/clients/window.c
> +++ b/clients/window.c
> @@ -3839,6 +3839,13 @@ undo_resize(struct window *window)
>  }
>  
>  void
> +window_set_min_allocation(struct window *window, int width, int
> height) +{
> +	window->min_allocation.width = width;
> +	window->min_allocation.height = height;
> +}
> +
> +void
>  window_schedule_resize(struct window *window, int width, int height)
>  {
>  	/* We should probably get these numbers from the theme. */
> diff --git a/clients/window.h b/clients/window.h
> index 54b848b..b221b76 100644
> --- a/clients/window.h
> +++ b/clients/window.h
> @@ -336,6 +336,8 @@ void
>  window_schedule_redraw(struct window *window);
>  void
>  window_schedule_resize(struct window *window, int width, int height);
> +void
> +window_set_min_allocation(struct window *window, int width, int
> height); 
>  void
>  window_damage(struct window *window, int32_t x, int32_t y,

Thanks,
pq