[PATCH weston v5 5/7] libweston-desktop: Add listener and API to drop the idle inhibitor
Derek Foreman
derekf at osg.samsung.com
Tue Aug 30 14:46:37 UTC 2016
On 30/08/16 05:46 AM, Quentin Glidic wrote:
> 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.
Actually untrue - assert() may be a NOP on NDEBUG builds.
I think using assert() to catch a buggy value that a non-debug build
doesn't even have to test for is common practice?
If NULL is a bug, only assert is required. if NULL is valid the assert
would be wrong and a conditional should be added.
Thanks,
Derek
>> +}
>> +
>> +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);
>>
>
>
More information about the wayland-devel
mailing list