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

[Derek Foreman]

Since this is a little further from landing than I thought, as is the
scaling patch, I think the best move for the short term is to revert the
icon stuff entirely and move forward after the release.

Thanks,
Derek

On 2018-03-29 04:17 AM, Pekka Paalanen wrote:
> 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
>