[PATCH weston v5 5/7] libweston-desktop: Add listener and API to drop the idle inhibitor

[Quentin Glidic]

On 30/08/2016 03:34, Bryce Harrington wrote:
> Listen for the drop_idle_inhibitor signal from libweston, and propagate
> the call to a corresponding libweston-desktop API.
>
> Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>

In the current form, this patch is useless. Details inside.


> ---
>  libweston-desktop/internal.h          |  3 +++
>  libweston-desktop/libweston-desktop.c |  8 ++++++++
>  libweston-desktop/libweston-desktop.h |  2 ++
>  libweston-desktop/surface.c           | 20 ++++++++++++++++++++
>  4 files changed, 33 insertions(+)
>
> diff --git a/libweston-desktop/internal.h b/libweston-desktop/internal.h
> index a9c974b..e41c1e5 100644
> --- a/libweston-desktop/internal.h
> +++ b/libweston-desktop/internal.h
> @@ -47,6 +47,9 @@ void
>  weston_desktop_api_surface_removed(struct weston_desktop *desktop,
>  				   struct weston_desktop_surface *surface);
>  void
> +weston_desktop_api_surface_drop_idle_inhibitor(struct weston_desktop *desktop,
> +					       struct weston_desktop_surface *surface);

"weston_desktop_api_*" is for libweston-desktop to poke into the 
compositor shell. I think you understood that, but there is something 
strange below.
Also, the "surface" prefix should be omitted, as the others. Or added 
for all callbacks.

> +void
>  weston_desktop_api_committed(struct weston_desktop *desktop,
>  			     struct weston_desktop_surface *surface,
>  			     int32_t sx, int32_t sy);
> diff --git a/libweston-desktop/libweston-desktop.c b/libweston-desktop/libweston-desktop.c
> index 0ee1139..43ac081 100644
> --- a/libweston-desktop/libweston-desktop.c
> +++ b/libweston-desktop/libweston-desktop.c
> @@ -61,6 +61,7 @@ weston_desktop_create(struct weston_compositor *compositor,
>
>  	assert(api->surface_added);
>  	assert(api->surface_removed);
> +	/* assert(api->surface_drop_idle_inhibitor); -- optional, for now */

If you do not assert…


>
>  	desktop = zalloc(sizeof(struct weston_desktop));
>  	desktop->compositor = compositor;
> @@ -166,6 +167,13 @@ weston_desktop_api_surface_removed(struct weston_desktop *desktop,
>  }
>
>  void
> +weston_desktop_api_surface_drop_idle_inhibitor(struct weston_desktop *desktop,
> +				   struct weston_desktop_surface *surface)
> +{
> +	desktop->api.surface_drop_idle_inhibitor(surface, desktop->user_data);

… you must check for NULL here.
Does libweston switch the monitor off if there is no fade? If so it must 
remain optional, otherwise it must be required.

> +}
> +
> +void
>  weston_desktop_api_committed(struct weston_desktop *desktop,
>  			     struct weston_desktop_surface *surface,
>  			     int32_t sx, int32_t sy)
> diff --git a/libweston-desktop/libweston-desktop.h b/libweston-desktop/libweston-desktop.h
> index 0f0e031..e3bc515 100644
> --- a/libweston-desktop/libweston-desktop.h
> +++ b/libweston-desktop/libweston-desktop.h
> @@ -59,6 +59,8 @@ struct weston_desktop_api {
>  			      void *user_data);
>  	void (*surface_removed)(struct weston_desktop_surface *surface,
>  				void *user_data);
> +	void (*surface_drop_idle_inhibitor)(struct weston_desktop_surface *surface,
> +					    void *user_data);
>  	void (*committed)(struct weston_desktop_surface *surface,
>  			  int32_t sx, int32_t sy, void *user_data);
>  	void (*show_window_menu)(struct weston_desktop_surface *surface,
> diff --git a/libweston-desktop/surface.c b/libweston-desktop/surface.c
> index 42258db..37be659 100644
> --- a/libweston-desktop/surface.c
> +++ b/libweston-desktop/surface.c
> @@ -54,6 +54,7 @@ struct weston_desktop_surface {
>  	struct weston_position buffer_move;
>  	struct wl_listener surface_commit_listener;
>  	struct wl_listener surface_destroy_listener;
> +	struct wl_listener surface_drop_idle_inhibitor_listener;
>  	struct wl_listener client_destroy_listener;
>  	struct wl_list children_list;
>
> @@ -130,6 +131,7 @@ weston_desktop_surface_destroy(struct weston_desktop_surface *surface)
>
>  	wl_list_remove(&surface->surface_commit_listener.link);
>  	wl_list_remove(&surface->surface_destroy_listener.link);
> +	wl_list_remove(&surface->surface_drop_idle_inhibitor_listener.link);
>  	wl_list_remove(&surface->client_destroy_listener.link);
>
>  	if (!wl_list_empty(&surface->resource_list)) {
> @@ -218,6 +220,20 @@ weston_desktop_surface_resource_destroy(struct wl_resource *resource)
>  }
>
>  static void
> +weston_desktop_surface_drop_idle_inhibitor(struct wl_listener *listener,
> +					   void *data)
> +{
> +	struct weston_desktop_surface *surface =
> +		wl_container_of(listener, surface, surface_drop_idle_inhibitor_listener);
> +	struct weston_desktop *desktop = surface->desktop;
> +
> +	printf("weston_desktop_surface_drop_idle_inhibitor\n");
> +	weston_desktop_api_surface_drop_idle_inhibitor(desktop, surface);

This …

> +	// TODO: Need to call shell.c's desktop_surface_drop_idle_inhibitor
> +	//shell_desktop_api.surface_drop_idle_inhibitor(surface, NULL /*data?*/);

… is exactly that, but safer and fitting the existing wrappers design.

This code does basically nothing that the compositor couldn’t do 
directly by listening to the signal. Or rather, it makes the compositor 
crash.

libweston-desktop uses "weston_desktop_surface" for two kind of 
surfaces: toplevel surfaces, and non-toplevel surfaces.
Here, you expose surfaces that the shell doesn’t know about, so with no 
associated "shell_surface".

Pekka, Jonas, how will we interface idle_inhibitor with wl_shell and 
xdg_shell? Do we allow inhibitors on popup/transient?
If so, libweston-desktop will have to somehow proxy these in a nice way, 
as well as inheritance and stuff.
If not, the shell will listen for the destroy signal, and 
libweston-desktop need a way to hook on inhibitor creation, so it can 
error the client.

Xwayland surfaces are safe, I guess, so override-redirect surfaces don’t 
need a special case here.

Cheers,


> +}
> +
> +static void
>  weston_desktop_surface_committed(struct weston_surface *wsurface,
>  				 int32_t sx, int32_t sy)
>  {
> @@ -277,6 +293,10 @@ weston_desktop_surface_create(struct weston_desktop *desktop,
>  		weston_desktop_surface_surface_destroyed;
>  	wl_signal_add(&surface->surface->destroy_signal,
>  		      &surface->surface_destroy_listener);
> +	surface->surface_drop_idle_inhibitor_listener.notify =
> +		weston_desktop_surface_drop_idle_inhibitor;
> +	wl_signal_add(&surface->surface->drop_idle_inhibitor_signal,
> +		      &surface->surface_drop_idle_inhibitor_listener);
>
>  	wl_list_init(&surface->client_link);
>  	wl_list_init(&surface->resource_list);
>


-- 

Quentin “Sardem FF7” Glidic