[PATCH v5 weston] xwm: Choose icon closest to target size

[Pekka Paalanen]

On Tue, 27 Mar 2018 12:43:36 -0500
Derek Foreman <derekf at osg.samsung.com> 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.
> 
> Co-authored-by: Scott Moreau <oreaus at gmail.com>
> 

Missing signed-off-by.

Scott, are you happy with your attribution here?


> ---
> 
> Changed in v2:
> 
> - Fix typo setting width to height
> 
> Changed in v3:
> 
> - Move checks for malformed input into data handling function
> 
> Changed in v4:
> 
> - #define XWM_ICON_SIZE in this patch and do not #undef it
> - Start with first icon found before choosing icon\
> - Check for NULL data in get_icon_size_from_data()
> 
> Changed in v5:
> - remove allocations, process in a single pass
> - rebase on top of memory leak fix
> 
>  xwayland/window-manager.c | 51 ++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 44 insertions(+), 7 deletions(-)
> 
> diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c
> index dad117fa..5209ac66 100644
> --- a/xwayland/window-manager.c
> +++ b/xwayland/window-manager.c
> @@ -127,6 +127,8 @@ struct motif_wm_hints {
>  #define _NET_WM_MOVERESIZE_MOVE_KEYBOARD    10   /* move via keyboard */
>  #define _NET_WM_MOVERESIZE_CANCEL           11   /* cancel operation */
>  
> +#define XWM_ICON_SIZE 16 /* width and height of frame icon */
> +
>  struct weston_output_weak_ref {
>  	struct weston_output *output;
>  	struct wl_listener destroy_listener;
> @@ -1352,6 +1354,44 @@ 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)

This may be a static function, but it really needs doxygen doc to
explain all the arguments and return value, particularly for 'length',
see below.

> +{
> +	uint32_t *d = data, *ret = NULL, w, h;
> +	int a1, a2, a3;

I'd really like better names than a1, a2, a3. There are so many
variables in this function that they need descriptive names.

> +
> +	if (!data)
> +		return NULL;
> +
> +	/* Choose closest match by comparing icon dimension areas */
> +	a1 = target_width * target_height;
> +
> +	*width = *height = 0;
> +
> +	while (d - data < length / 4) {

Maybe using
	uint32_t *end = data + length;
would make this easier to read.

Isn't length/4 wrong, because length is already in uint32_t units by
the caller?

> +		w = *d++;
> +		h = *d++;
> +
> +		/* Some checks against malformed input. */
> +		if (w == 0 || h == 0 || length < 2 + w * h)

Checking against 'length' is incorrect, because we need to be checking
against the remaining length, not against the whole length, as we may
have skipped an icon alrady. Checking against 'end' would help here too.

> +			return ret;
> +
> +		a2 = w * h;
> +		a3 = *width * *height;
> +		if (!a3 || abs(a2 - a1) < abs(a3 - a1)) {
> +			*width = w;
> +			*height = h;
> +			ret = d;
> +		}
> +
> +		d += a2;

The computations here seem to be correct.

> +	}
> +
> +	return ret;
> +}
> +
>  static void
>  handle_icon_surface_destroy(void *data)
>  {
> @@ -1367,9 +1407,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);

Type is XCB_ATOM_ANY.

The context here is missing to show this:

	reply = xcb_get_property_reply(wm->conn, cookie, NULL);
	length = xcb_get_property_value_length(reply);

	/* This is in 32-bit words, not in bytes. */
	if (length < 2) {
		free(reply);
		return;
	}

	data = xcb_get_property_value(reply);

What are the units of 'length'?

The comment here says words, but if you look at the example in 'man
xcb_get_property', it is in bytes. If they are both right, then the
length unit must depend on something.

The type is not checked from the reply, and neither is 'format' checked
from the reply. My wild guess would be that 'format' determines the
units of 'length', and so it is controllable by the X11 client that set
the property.

I think weston_wm_window_read_properties() is equally very much broken
against badly formatted properties, because it assumes a certain length
from just the type or even atom name(!) without actually checking the
length. dump_property() looks suspicious as well. It's probably
everything that ever parses an xcb_get_property_reply_t in XWM.

The EWMH spec may say that a certain property needs to have certain
format and type, but I don't believe there is anything actually
guaranteeing to follow the spec, so it's up to us to verify the data is
proper.

> @@ -1383,11 +1420,11 @@ weston_wm_handle_icon(struct weston_wm *wm, struct weston_wm_window *window)
>  	}
>  
>  	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(XWM_ICON_SIZE, XWM_ICON_SIZE,
> +					&width, &height, length, data);
> +
> +	if (!data) {
>  		free(reply);
>  		return;
>  	}

Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180329/6cef516d/attachment.sig>