[PATCH weston v4] xwm: Add icon support to the frame
Emmanuel Gil Peyrot
linkmauve at linkmauve.fr
Sun Dec 3 13:32:23 UTC 2017
On Fri, Dec 01, 2017 at 09:47:57PM +0100, 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;
> > +
> > + 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 made the conscious decision of not using the stride recommended by
cairo, exactly because of the EWMH specification.
> 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?
To quote the specification:
“This is an array of 32bit packed CARDINAL ARGB with high byte being A,
low byte being B. The first two cardinals are width, height. Data is in
rows, left to right and top to bottom.”
There is no mention of stride or alignment, so the assumption I and
everyone are doing is that it’s strictly packed. There also can’t be
anything else than *4 due to the format requirement.
Maybe I should add a comment instead.
>
> 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);
> >
>
>
> --
>
> Quentin “Sardem FF7” Glidic
--
Emmanuel Gil Peyrot
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20171203/d83490f2/attachment.sig>
More information about the wayland-devel
mailing list