[PATCH weston v3] xwm: Add icon support to the frame

Quentin Glidic sardemff7+wayland at sardemff7.net
Tue May 23 09:57:32 UTC 2017


On 4/4/17 4:34 PM, Emmanuel Gil Peyrot wrote:
> From: Emmanuel Gil Peyrot <linkmauve at linkmauve.fr>
> 
> 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.
> 
> Signed-off-by: Emmanuel Gil Peyrot <linkmauve at linkmauve.fr>
> Signed-off-by: Emmanuel Gil Peyrot <emmanuel.peyrot at collabora.com>

We’re mostly good, still a few comments in the property handling.


> ---
>   clients/window.c               |  4 +--
>   libweston/compositor-wayland.c |  2 +-
>   shared/cairo-util.h            |  2 +-
>   shared/frame.c                 | 54 +++++++++++++++++++++++++++----------
>   xwayland/window-manager.c      | 61 ++++++++++++++++++++++++++++++++++++++++--
>   5 files changed, 103 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 14f2c8db..aa6e5043 100644
> --- a/libweston/compositor-wayland.c
> +++ b/libweston/compositor-wayland.c
> @@ -862,7 +862,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 d40d56ea..e10db47c 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) },
> @@ -931,8 +934,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);
> @@ -1271,6 +1275,49 @@ weston_wm_window_schedule_repaint(struct weston_wm_window *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;
> +	int length;
> +	unsigned *data, width, height;
> 

Istn’t that uint32_t ? unsigned works for 64bit systems only, doesn’t 
it? (For data specifically, but then maybe should we sanity-check width 
and height.)



> +	/* 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 || (unsigned)length < 2 + width * height)
> +		return;
> +
> +	if (window->icon_surface)
> +		cairo_surface_destroy(window->icon_surface);

Do we want to destroy here?


> +	window->icon_surface =
> +		cairo_image_surface_create_for_data((unsigned char *)data,
> +		                                    CAIRO_FORMAT_ARGB32,
> +		                                    width, height, width * 4);
> +
> +	/* Bail out in case anything wrong happened during surface creation. */
> +	if (cairo_surface_status(window->icon_surface) != CAIRO_STATUS_SUCCESS) {
> +		cairo_surface_destroy(window->icon_surface);
> +		window->icon_surface = NULL;
> +	}

If we wait until success to replace the icon, we could keep a good 
(though outdated) icon if we failed to import the new one.

With the uint32_t fix and that shifted around:
Reviewed-by: Quentin Glidic <sardemff7+git at sardemff7.net>


As a side thought, we probably want to handle net_wm_icon_name at some 
point, but it requires XDG icon theme code (I think?) and that would 
rather be in the compositor code (so it can share caching and stuff).


Thanks,

> +}
> +
> +static void
>   weston_wm_handle_property_notify(struct weston_wm *wm, xcb_generic_event_t *event)
>   {
>   	xcb_property_notify_event_t *property_notify =
> @@ -1290,6 +1337,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


More information about the wayland-devel mailing list