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

Submitted by Derek Foreman on March 27, 2018, 5:43 p.m.

Details

Message ID 20180327174336.811-1-derekf@osg.samsung.com
State New
Headers show
Series "xwm: Choose icon closest to target size" ( rev: 3 ) in Wayland

Not browsing as part of any series.

Commit Message

Derek Foreman March 27, 2018, 5:43 p.m.
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@gmail.com>

---

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(-)

Patch hide | download patch | download mbox

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)
+{
+	uint32_t *d = data, *ret = NULL, w, h;
+	int a1, a2, a3;
+
+	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) {
+		w = *d++;
+		h = *d++;
+
+		/* Some checks against malformed input. */
+		if (w == 0 || h == 0 || length < 2 + w * h)
+			return ret;
+
+		a2 = w * h;
+		a3 = *width * *height;
+		if (!a3 || abs(a2 - a1) < abs(a3 - a1)) {
+			*width = w;
+			*height = h;
+			ret = d;
+		}
+
+		d += a2;
+	}
+
+	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);
@@ -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;
 	}

Comments

On Tue, 27 Mar 2018 12:43:36 -0500
Derek Foreman <derekf@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@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
On Thu, 29 Mar 2018 12:17:46 +0300
Pekka Paalanen <ppaalanen@gmail.com> wrote:

> On Tue, 27 Mar 2018 12:43:36 -0500
> Derek Foreman <derekf@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@gmail.com>

> > +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)  

...

> > +			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.

Oh btw, wasn't the intention to pick the closest *larger* sized icon
from the list and then downscale? Only pick smaller if there is no
larger provided.

This seems to pick the closest larger or smaller in terms of area.


Thanks,
pq
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@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@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
>