[PATCH v3] xwm: Choose icon closest to target size

[Derek Foreman]

Thanks for this.  This particular feature is getting on my nerves
because it seemed like a simple thing and has result in a lot of breakage.

Would like to see it repaired or removed by late next week.

This seems to be a good step towards repair, comments inline below.

On 2018-03-22 12:47 AM, Scott Moreau wrote:
> Xwayland clients can offer multiple icon sizes in no particular order.
> Previously xwm was selecting the first one unconditionally. This patch
> selects the icon that matches the size closest to the target size. The
> target size is hard coded to 16 since there is only one theme and the
> data used to create the theme is hard coded.
> ---
> 
> Changed in v2:
> 
> - Fix typo setting width to height
> 
> Changed in v3:
> 
> - Move checks for malformed input into data handling function
> 
>  xwayland/window-manager.c | 74 ++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 67 insertions(+), 7 deletions(-)
> 
> diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c
> index c307e19..30c4b1c 100644
> --- a/xwayland/window-manager.c
> +++ b/xwayland/window-manager.c
> @@ -1352,6 +1352,70 @@ weston_wm_window_schedule_repaint(struct weston_wm_window *window)
>  				       weston_wm_window_do_repaint, window);
>  }
>  
> +static uint32_t *
> +get_icon_size_from_data(int target_width, int target_height,
> +						uint32_t *width, uint32_t *height,
> +						uint32_t length, uint32_t *data)
> +{
> +	uint32_t *d = data, **img = NULL, *ret = NULL, w, h;
> +	int num_sizes = 0, *sizes = NULL, a1, a2, a3;
> +
> +	*width = *height = 0;
> +
> +	while (d - data < length / 4) {
> +		w = *d++;
> +		h = *d++;
> +
> +		/* Some checks against malformed input. */
> +		if (w == 0 || h == 0 || length < 2 + w * h) {
> +			free(sizes);
> +			free(img);
> +			return NULL;
> +		}

I think since ret is init to NULL, and free(NULL) is a nop, all these
checks could just goto out;  and an out label could be added immediately
before free(sizes) at the end of function.

Should shrink it a bit...

> +
> +		sizes = realloc(sizes, 2 * (num_sizes + 1) * sizeof(uint32_t));
> +		if (!sizes) {
> +			free(img);
> +			return NULL;
> +		}
> +
> +		img = realloc(img, (num_sizes + 1) * sizeof(uint32_t *));
> +		if (!img) {
> +			free(sizes);
> +			return NULL;
> +		}
> +
> +		sizes[(num_sizes * 2) + 0] = w;
> +		sizes[(num_sizes * 2) + 1] = h;
> +		img[num_sizes] = d;
> +
> +		num_sizes++;
> +		d += w * h;
> +	}
> +
> +	/* Choose closest match by comparing icon dimension areas */
> +	a1 = target_width * target_height;
> +
> +	while (num_sizes--) {
> +		w = sizes[(num_sizes * 2) + 0];
> +		h = sizes[(num_sizes * 2) + 1];
> +
> +		a2 = w * h;
> +		a3 = *width * *height;
> +
> +		if (a2 != a3 && abs(a2 - a1) < abs(a3 - a1)) {

When running terminology, which only provides a single 128x128 icon, I
run through this and exit with ret == NULL, and no icon is displayed.

Without thinking too hard, I guess the initial 0x0 is closer to 16x16
than 128x128 is. :)  I guess maybe this should accept any icon if no
icon has been picked yet (or just before this loop we could init to the
first icon...)

> +			*width = w;
> +			*height = h;
> +			ret = img[num_sizes];
> +		}
> +	}
> +
> +	free(sizes);
> +	free(img);
> +
> +	return ret;
> +}
> +
>  static void
>  weston_wm_handle_icon(struct weston_wm *wm, struct weston_wm_window *window)
>  {
> @@ -1361,9 +1425,6 @@ weston_wm_handle_icon(struct weston_wm *wm, struct weston_wm_window *window)
>  	uint32_t *data, width, height;
>  	cairo_surface_t *new_surface;
>  
> -	/* TODO: icons don’t have any specified order, we should pick the
> -	 * closest one to the target dimension instead of the first one. */
> -
>  	cookie = xcb_get_property(wm->conn, 0, window->id,
>  	                          wm->atom.net_wm_icon, XCB_ATOM_ANY, 0,
>  	                          UINT32_MAX);
> @@ -1375,11 +1436,10 @@ weston_wm_handle_icon(struct weston_wm *wm, struct weston_wm_window *window)
>  		return;
>  
>  	data = xcb_get_property_value(reply);
> -	width = *data++;
> -	height = *data++;
>  
> -	/* Some checks against malformed input. */
> -	if (width == 0 || height == 0 || length < 2 + width * height)
> +	data = get_icon_size_from_data(16, 16, &width, &height, length, data);

In your next patch you replace the 16s with ICON_SIZE, a temporarily
#defined constant...

I wouldn't mind seeing #define XWM_ICON_SIZE or similar defined as part
of this patch, at the top of the file, and not undefined later...

Thanks,
Derek

> +
> +	if (!data)
>  		return;
>  
>  	new_surface =
>