<div dir="ltr">Hi Derek,<br><div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Mar 23, 2018 at 11:34 AM, Derek Foreman <span dir="ltr"><<a href="mailto:derekf@osg.samsung.com" target="_blank">derekf@osg.samsung.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks for this. This particular feature is getting on my nerves<br>
because it seemed like a simple thing and has result in a lot of breakage.<br>
<br>
Would like to see it repaired or removed by late next week.<br></blockquote><div><br></div><div>Thanks for reviewing this.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
This seems to be a good step towards repair, comments inline below.<br>
<div><div class="m_-1464151011597123774h5"><br>
On 2018-03-22 12:47 AM, Scott Moreau wrote:<br>
> Xwayland clients can offer multiple icon sizes in no particular order.<br>
> Previously xwm was selecting the first one unconditionally. This patch<br>
> selects the icon that matches the size closest to the target size. The<br>
> target size is hard coded to 16 since there is only one theme and the<br>
> data used to create the theme is hard coded.<br>
> ---<br>
><br>
> Changed in v2:<br>
><br>
> - Fix typo setting width to height<br>
><br>
> Changed in v3:<br>
><br>
> - Move checks for malformed input into data handling function<br>
><br>
> xwayland/window-manager.c | 74 ++++++++++++++++++++++++++++++<wbr>++++++++++++-----<br>
> 1 file changed, 67 insertions(+), 7 deletions(-)<br>
><br>
> diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c<br>
> index c307e19..30c4b1c 100644<br>
> --- a/xwayland/window-manager.c<br>
> +++ b/xwayland/window-manager.c<br>
> @@ -1352,6 +1352,70 @@ weston_wm_window_schedule_repa<wbr>int(struct weston_wm_window *window)<br>
> weston_wm_window_do_repaint, window);<br>
> }<br>
><br>
> +static uint32_t *<br>
> +get_icon_size_from_data(int target_width, int target_height,<br>
> + uint32_t *width, uint32_t *height,<br>
> + uint32_t length, uint32_t *data)<br>
> +{<br>
> + uint32_t *d = data, **img = NULL, *ret = NULL, w, h;<br>
> + int num_sizes = 0, *sizes = NULL, a1, a2, a3;<br>
> +<br>
> + *width = *height = 0;<br>
> +<br>
> + while (d - data < length / 4) {<br>
> + w = *d++;<br>
> + h = *d++;<br>
> +<br>
> + /* Some checks against malformed input. */<br>
> + if (w == 0 || h == 0 || length < 2 + w * h) {<br>
> + free(sizes);<br>
> + free(img);<br>
> + return NULL;<br>
> + }<br>
<br>
</div></div>I think since ret is init to NULL, and free(NULL) is a nop, all these<br>
checks could just goto out; and an out label could be added immediately<br>
before free(sizes) at the end of function.<br>
<br>
Should shrink it a bit...<br></blockquote><div><br></div><div>I had the same thought but shied away from using goto. I'll do it in the next patch.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="m_-1464151011597123774h5"><br>
> +<br>
> + sizes = realloc(sizes, 2 * (num_sizes + 1) * sizeof(uint32_t));<br>
> + if (!sizes) {<br>
> + free(img);<br>
> + return NULL;<br>
> + }<br>
> +<br>
> + img = realloc(img, (num_sizes + 1) * sizeof(uint32_t *));<br>
> + if (!img) {<br>
> + free(sizes);<br>
> + return NULL;<br>
> + }<br>
> +<br>
> + sizes[(num_sizes * 2) + 0] = w;<br>
> + sizes[(num_sizes * 2) + 1] = h;<br>
> + img[num_sizes] = d;<br>
> +<br>
> + num_sizes++;<br>
> + d += w * h;<br>
> + }<br>
> +<br>
> + /* Choose closest match by comparing icon dimension areas */<br>
> + a1 = target_width * target_height;<br>
> +<br>
> + while (num_sizes--) {<br>
> + w = sizes[(num_sizes * 2) + 0];<br>
> + h = sizes[(num_sizes * 2) + 1];<br>
> +<br>
> + a2 = w * h;<br>
> + a3 = *width * *height;<br>
> +<br>
> + if (a2 != a3 && abs(a2 - a1) < abs(a3 - a1)) {<br>
<br>
</div></div>When running terminology, which only provides a single 128x128 icon, I<br>
run through this and exit with ret == NULL, and no icon is displayed.<br></blockquote><div><br></div><div>Ah yes, I can reproduce this.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Without thinking too hard, I guess the initial 0x0 is closer to 16x16<br>
than 128x128 is. :)</blockquote><div><br></div><div>Yes, this is the problem.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> I guess maybe this should accept any icon if no<br>
icon has been picked yet (or just before this loop we could init to the<br>
first icon...)<br></blockquote><div><br></div><div>That would probably be a good bet.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="m_-1464151011597123774h5"><br>
> + *width = w;<br>
> + *height = h;<br>
> + ret = img[num_sizes];<br>
> + }<br>
> + }<br>
> +<br>
> + free(sizes);<br>
> + free(img);<br>
> +<br>
> + return ret;<br>
> +}<br>
> +<br>
> static void<br>
> weston_wm_handle_icon(struct weston_wm *wm, struct weston_wm_window *window)<br>
> {<br>
> @@ -1361,9 +1425,6 @@ weston_wm_handle_icon(struct weston_wm *wm, struct weston_wm_window *window)<br>
> uint32_t *data, width, height;<br>
> cairo_surface_t *new_surface;<br>
><br>
> - /* TODO: icons don’t have any specified order, we should pick the<br>
> - * closest one to the target dimension instead of the first one. */<br>
> -<br>
> cookie = xcb_get_property(wm->conn, 0, window->id,<br>
> wm->atom.net_wm_icon, XCB_ATOM_ANY, 0,<br>
> UINT32_MAX);<br>
> @@ -1375,11 +1436,10 @@ weston_wm_handle_icon(struct weston_wm *wm, struct weston_wm_window *window)<br>
> return;<br>
><br>
> data = xcb_get_property_value(reply);<br>
> - width = *data++;<br>
> - height = *data++;<br>
><br>
> - /* Some checks against malformed input. */<br>
> - if (width == 0 || height == 0 || length < 2 + width * height)<br>
> + data = get_icon_size_from_data(16, 16, &width, &height, length, data);<br>
<br>
</div></div>In your next patch you replace the 16s with ICON_SIZE, a temporarily<br>
#defined constant...<br>
<br>
I wouldn't mind seeing #define XWM_ICON_SIZE or similar defined as part<br>
of this patch, at the top of the file, and not undefined later...<br></blockquote><div><br></div><div>Noted.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Thanks,<br>
Derek<br>
<div class="m_-1464151011597123774HOEnZb"><div class="m_-1464151011597123774h5"><br>
> +<br>
> + if (!data)<br>
> return;<br>
><br>
> new_surface =<br>
><br>
<br>
</div></div></blockquote></div><br>Thanks,<br>Scott<br></div></div></div>