On 2017-12-01 02:47 PM, Quentin Glidic wrote:
On 12/1/17 7:20 PM, Emmanuel Gil Peyrot wrote:This fetches the _NET_WM_ICON property of the X11 window, and use the first image found as the frame icon.This has been tested with various X11 programs, and improves usability and user-friendliness a bit. Changes since v1: - Changed frame_button_create() to use frame_button_create_from_surface() internally. - Removed a check that should never have been commited. Changes since v2: - Request UINT32_MAX items instead of 2048, to avoid cutting valid icons. - Strengthen checks against malformed input. - Handle XCB_PROPERTY_DELETE to remove the icon. - Schedule a repaint if the icon changed. Changes since v3: - Keep the previous Cairo surface until the new one has been successfully loaded. - Use uint32_t for cardinals. Unsigned is the same type except on 16-bit machines, but uint32_t is clearer. - Declare length as uint32_t too, like in xcb_get_property_reply_t. Signed-off-by: Emmanuel Gil Peyrot <linkma...@linkmauve.fr> Reviewed-by: Quentin Glidic <sardemff7+...@sardemff7.net>Just re-checked to be sure, found a tiny thing I overlooked (see below), but the Rb still stands anyway.--- clients/window.c | 4 +-- libweston/compositor-wayland.c | 2 +- shared/cairo-util.h | 2 +- shared/frame.c | 54 ++++++++++++++++++++++++++---------xwayland/window-manager.c | 65 ++++++++++++++++++++++++++++++++++++++++--5 files changed, 107 insertions(+), 20 deletions(-) diff --git a/clients/window.c b/clients/window.c index 95796d46..15a86e15 100644 --- a/clients/window.c +++ b/clients/window.c@@ -2546,7 +2546,7 @@ window_frame_create(struct window *window, void *data)frame = xzalloc(sizeof *frame); frame->frame = frame_create(window->display->theme, 0, 0, - buttons, window->title); + buttons, window->title, NULL); frame->widget = window_add_widget(window, frame); frame->child = widget_add_widget(frame->widget, data); @@ -5449,7 +5449,7 @@ create_menu(struct display *display, menu->user_data = user_data; menu->widget = window_add_widget(menu->window, menu); menu->frame = frame_create(window->display->theme, 0, 0, - FRAME_BUTTON_NONE, NULL); + FRAME_BUTTON_NONE, NULL, NULL); fail_on_null(menu->frame, 0, __FILE__, __LINE__); menu->entries = entries; menu->count = count;diff --git a/libweston/compositor-wayland.c b/libweston/compositor-wayland.cindex 3bdfb03e..c5290d85 100644 --- a/libweston/compositor-wayland.c +++ b/libweston/compositor-wayland.c@@ -869,7 +869,7 @@ wayland_output_set_windowed(struct wayland_output *output)return -1; } output->frame = frame_create(b->theme, 100, 100, - FRAME_BUTTON_CLOSE, output->title);+ FRAME_BUTTON_CLOSE, output->title, NULL);if (!output->frame) return -1; diff --git a/shared/cairo-util.h b/shared/cairo-util.h index 84cf005e..7893ca24 100644 --- a/shared/cairo-util.h +++ b/shared/cairo-util.h @@ -126,7 +126,7 @@ enum { struct frame *frame_create(struct theme *t, int32_t width, int32_t height, uint32_t buttons,- const char *title); + const char *title, cairo_surface_t *icon); void frame_destroy(struct frame *frame); diff --git a/shared/frame.c b/shared/frame.c index eb0cd77a..32779856 100644 --- a/shared/frame.c +++ b/shared/frame.c @@ -106,9 +106,9 @@ struct frame { }; static struct frame_button * -frame_button_create(struct frame *frame, const char *icon, - enum frame_status status_effect, - enum frame_button_flags flags)+frame_button_create_from_surface(struct frame *frame, cairo_surface_t *icon,+ enum frame_status status_effect, + enum frame_button_flags flags) { struct frame_button *button;@@ -116,12 +116,7 @@ frame_button_create(struct frame *frame, const char *icon,if (!button) return NULL; - button->icon = cairo_image_surface_create_from_png(icon); - if (!button->icon) { - free(button); - return NULL; - } - + button->icon = icon; button->frame = frame; button->flags = flags; button->status_effect = status_effect;@@ -131,6 +126,30 @@ frame_button_create(struct frame *frame, const char *icon,return button; } +static struct frame_button * +frame_button_create(struct frame *frame, const char *icon_name, + enum frame_status status_effect, + enum frame_button_flags flags) +{ + struct frame_button *button; + cairo_surface_t *icon; + + icon = cairo_image_surface_create_from_png(icon_name); + if (cairo_surface_status(icon) != CAIRO_STATUS_SUCCESS) + goto error;
Hi, it's me again :)Soooo, this bit, checking surface status, is a change from previous behaviour.
Previously we always succeeded, even if the icon files were missing, and make unclickable junk decor. Arguably a dumb thing to do.
Now when icon files are missing we crash with a NULL pointer deref.Why this becomes a massive pain is... our make check infrastructure is apparently looking for these icon files in their installed location. So, make distcheck now always fails, and make check will fail if make install has never taken place.
So I guess we have to decide if crashing when the icons aren't found is reasonable behaviour, independently of fixing our bogus test infra...
Any opinions? Thanks, Derek
++ button = frame_button_create_from_surface(frame, icon, status_effect,+ flags); + if (!button) + goto error; + + return button; + +error: + cairo_surface_destroy(icon); + return NULL; +} + static void frame_button_destroy(struct frame_button *button) { @@ -303,7 +322,7 @@ frame_destroy(struct frame *frame) struct frame *frame_create(struct theme *t, int32_t width, int32_t height, uint32_t buttons,- const char *title) + const char *title, cairo_surface_t *icon) { struct frame *frame; struct frame_button *button;@@ -330,10 +349,17 @@ frame_create(struct theme *t, int32_t width, int32_t height, uint32_t buttons,} if (title) { - button = frame_button_create(frame, - DATADIR "/weston/icon_window.png", - FRAME_STATUS_MENU, - FRAME_BUTTON_CLICK_DOWN); + if (icon) { + button = frame_button_create_from_surface(frame, + icon, + FRAME_STATUS_MENU,+ FRAME_BUTTON_CLICK_DOWN);+ } else { + button = frame_button_create(frame,+ DATADIR "/weston/icon_window.png",+ FRAME_STATUS_MENU, + FRAME_BUTTON_CLICK_DOWN); + } if (!button) goto free_frame; } diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c index 3e8c4c7c..61e9d36a 100644 --- a/xwayland/window-manager.c +++ b/xwayland/window-manager.c @@ -138,6 +138,8 @@ struct weston_wm_window { xcb_window_t frame_id; struct frame *frame; cairo_surface_t *cairo_surface; + int icon; + cairo_surface_t *icon_surface; uint32_t surface_id; struct weston_surface *surface; struct weston_desktop_xwayland_surface *shsurf;@@ -471,6 +473,7 @@ weston_wm_window_read_properties(struct weston_wm_window *window) { wm->atom.net_wm_state, TYPE_NET_WM_STATE, NULL }, { wm->atom.net_wm_window_type, XCB_ATOM_ATOM, F(type) }, { wm->atom.net_wm_name, XCB_ATOM_STRING, F(name) }, + { wm->atom.net_wm_icon, XCB_ATOM_CARDINAL, F(icon) }, { wm->atom.net_wm_pid, XCB_ATOM_CARDINAL, F(pid) }, { wm->atom.motif_wm_hints, TYPE_MOTIF_WM_HINTS, NULL }, { wm->atom.wm_client_machine, XCB_ATOM_WM_CLIENT_MACHINE, F(machine) }, @@ -971,8 +974,9 @@ weston_wm_window_create_frame(struct weston_wm_window *window)buttons |= FRAME_BUTTON_MAXIMIZE; window->frame = frame_create(window->wm->theme, - window->width, window->height, - buttons, window->name); + window->width, window->height, + buttons, window->name, + window->icon_surface); frame_resize_inside(window->frame, window->width, window->height); weston_wm_window_get_frame_size(window, &width, &height);@@ -1311,6 +1315,53 @@ weston_wm_window_schedule_repaint(struct weston_wm_window *window)weston_wm_window_do_repaint, window); } +static void+weston_wm_handle_icon(struct weston_wm *wm, struct weston_wm_window *window)+{ + xcb_get_property_reply_t *reply; + xcb_get_property_cookie_t cookie; + uint32_t length; + 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); + 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) + 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) + return; + + new_surface = + cairo_image_surface_create_for_data((unsigned char *)data, + CAIRO_FORMAT_ARGB32, + width, height, width * 4);Sorry I missed this one in my previous review.Do we want to use the stride value from Cairo instead of this hardcoded "width * 4"? I can’t find "width * 4" in the EWMH spec[1], I guess it’s well-known enough in this case. Maybe we need to check the Cairo stride matches "width * 4", just in case?Cheers,+ /* Bail out in case anything wrong happened during surface creation. */+ if (cairo_surface_status(new_surface) != CAIRO_STATUS_SUCCESS) { + cairo_surface_destroy(new_surface); + return; + } + + if (window->icon_surface) + cairo_surface_destroy(window->icon_surface); + + window->icon_surface = new_surface; +} + static voidweston_wm_handle_property_notify(struct weston_wm *wm, xcb_generic_event_t *event){@@ -1331,6 +1382,16 @@ weston_wm_handle_property_notify(struct weston_wm *wm, xcb_generic_event_t *evenread_and_dump_property(wm, property_notify->window, property_notify->atom); + if (property_notify->atom == wm->atom.net_wm_icon) { + if (property_notify->state != XCB_PROPERTY_DELETE) { + weston_wm_handle_icon(wm, window); + } else { + cairo_surface_destroy(window->icon_surface); + window->icon_surface = NULL; + } + weston_wm_window_schedule_repaint(window); + } + if (property_notify->atom == wm->atom.net_wm_name || property_notify->atom == XCB_ATOM_WM_NAME) weston_wm_window_schedule_repaint(window);
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
