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