[PATCH weston v4] xwm: Add icon support to the frame
Derek Foreman
derekf at osg.samsung.com
Mon Feb 5 20:50:07 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;
>> 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 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