[PATCH weston v4] xwm: Add icon support to the frame
Derek Foreman
derekf at osg.samsung.com
Wed Jan 24 21:04:27 UTC 2018
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 <linkmauve at linkmauve.fr>
>> Reviewed-by: Quentin Glidic <sardemff7+git at 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.c
>> index 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;
This line makes a copy of the icon cairo surface that isn't updated when
(see below)
>> 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;
>> +
>> + 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);
This is where the cairo surface is destroyed and the copy in
button->icon from above lives on...
To result in a use after free crash when running terminology (or I
suppose anything else that updates its icon?) under weston.
Thanks,
Derek
>> +
>> + window->icon_surface = new_surface;
>> +}
>> +
>> static void
>> weston_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 *even
>> read_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);
>>
>
>
More information about the wayland-devel
mailing list