[PATCH v3] xwm: Choose icon closest to target size

[Scott Moreau]

Hi Derek,

On Fri, Mar 23, 2018 at 11:34 AM, Derek Foreman <derekf at osg.samsung.com>
wrote:

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

Thanks for reviewing this.


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

I had the same thought but shied away from using goto. I'll do it in the
next patch.


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

Ah yes, I can reproduce this.


>
> Without thinking too hard, I guess the initial 0x0 is closer to 16x16
> than 128x128 is. :)


Yes, this is the problem.


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

That would probably be a good bet.


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

Noted.


>
> Thanks,
> Derek
>
> > +
> > +     if (!data)
> >               return;
> >
> >       new_surface =
> >
>
>
Thanks,
Scott
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180323/1ddee9a6/attachment-0001.html>