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 =
>
_______________________________________________
wayland-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
