[PATCH weston v2 2/2] compositor: allow to control the vt switching
Giulio Camuffo
giuliocamuffo at gmail.com
Mon Dec 5 13:45:49 UTC 2016
2016-11-25 15:10 GMT+01:00 Pekka Paalanen <ppaalanen at gmail.com>:
> On Wed, 29 Jun 2016 11:57:12 +0200
> Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
>
>> A compositor may want to control the vt switching, for example to
>> ensure to have a lock screen before it. To enable that add a vfunc
>> that will be called when CTRL+ALT+FN is pressed. The default behavior
>> is to do the switching, but the user can change it by using the new
>> weston_compositor_set_vt_switcher() function, so that it can delay the
>> switching to later, by calling weston_compositor_activate_vt().
>>
>> Signed-off-by: Giulio Camuffo <giuliocamuffo at gmail.com>
>> ---
>>
>> v2: split the get_vt addition to its own commit
>>
>> Makefile.am | 2 +-
>> compositor/main.c | 1 +
>> libweston/compositor.c | 18 ++++++++++++++++++
>> libweston/compositor.h | 39 +++++++++++++++++++++++++++++++++++++++
>> libweston/launcher-impl.h | 1 +
>> libweston/launcher-util.c | 6 +++++-
>> 6 files changed, 65 insertions(+), 2 deletions(-)
>>
>> diff --git a/Makefile.am b/Makefile.am
>> index 16433b8..a906dd3 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -66,7 +66,7 @@ libweston_la_CPPFLAGS = $(AM_CPPFLAGS) -DIN_WESTON
>> libweston_la_CFLAGS = $(AM_CFLAGS) $(COMPOSITOR_CFLAGS) $(LIBUNWIND_CFLAGS)
>> libweston_la_LIBADD = $(COMPOSITOR_LIBS) $(LIBUNWIND_LIBS) \
>> $(DLOPEN_LIBS) -lm $(CLOCK_GETTIME_LIBS) \
>> - $(LIBINPUT_BACKEND_LIBS) libshared.la
>> + $(LIBINPUT_BACKEND_LIBS) libshared.la libsession-helper.la
>
> Hmm... why is this change here?
Because compositor.c calls in weston_launcher* functions, which are
part of libsession-helper.so.
>
>> libweston_la_LDFLAGS = -release ${LIBWESTON_ABI_VERSION}
>>
>> libweston_la_SOURCES = \
>> diff --git a/compositor/main.c b/compositor/main.c
>> index 6cf9194..1285f44 100644
>> --- a/compositor/main.c
>> +++ b/compositor/main.c
>> @@ -54,6 +54,7 @@
>> #include "git-version.h"
>> #include "version.h"
>> #include "weston.h"
>> +#include "launcher-util.h"
>
> I don't see launcher-util.h being installed, so compositor should not
> be accessing it either. And it doesn't - a left-over?
Indeed.
>
>>
>> #include "compositor-drm.h"
>> #include "compositor-headless.h"
>> diff --git a/libweston/compositor.c b/libweston/compositor.c
>> index 0c405ac..7bf9c69 100644
>> --- a/libweston/compositor.c
>> +++ b/libweston/compositor.c
>> @@ -55,6 +55,8 @@
>>
>> #include "compositor.h"
>> #include "viewporter-server-protocol.h"
>> +#include "launcher-impl.h"
>> +#include "launcher-util.h"
>> #include "presentation-time-server-protocol.h"
>> #include "shared/helpers.h"
>> #include "shared/os-compatibility.h"
>> @@ -4599,6 +4601,22 @@ compositor_bind(struct wl_client *client,
>> }
>>
>> WL_EXPORT int
>> +weston_compositor_activate_vt(struct weston_compositor *compositor, int vt)
>> +{
>> + if (compositor->launcher)
>> + return weston_launcher_activate_vt(compositor->launcher, vt);
>> + return -1;
>> +}
>> +
>> +WL_EXPORT void
>> +weston_compositor_set_vt_switcher(struct weston_compositor *compositor,
>> + weston_compositor_vt_switcher_func_t switcher)
>> +{
>> + if (compositor->launcher)
>> + compositor->launcher->vt_switcher = switcher;
>> +}
>> +
>> +WL_EXPORT int
>> weston_environment_get_fd(const char *env)
>> {
>> char *e, *end;
>> diff --git a/libweston/compositor.h b/libweston/compositor.h
>> index 099d4bf..cda6c97 100644
>> --- a/libweston/compositor.h
>> +++ b/libweston/compositor.h
>> @@ -1371,6 +1371,45 @@ void
>> weston_compositor_set_default_pointer_grab(struct weston_compositor *compositor,
>> const struct weston_pointer_grab_interface *interface);
>>
>> +/**
>> + * Request a vt switch for the compositor.
>> + *
>> + * This will only work if the compositor is running as the owner of
>> + * the session.
>> + *
>> + * \param launcher The compositor instance.
>
> launcher?
Oops.
>
>> + * \param vt The vt to switch to.
>> + *
>> + * Returns 0 on success, -1 otherwise.
>> + *
>> + * \sa weston_compositor_set_vt_switcher
>> + */
>> +int
>> +weston_compositor_activate_vt(struct weston_compositor *compositor, int vt);
>> +
>> +typedef void (*weston_compositor_vt_switcher_func_t)(
>> + struct weston_compositor *compositor, int vt);
>> +/**
>> + * Set the vt switcher for the compositor.
>> + *
>> + * If the compositor is the owner of the session, the CTRL+ALT+FN key
>> + * combinations will trigger a vt switch. The default behavior is to do
>> + * the switching immediately, but some compositors may want to make sure to
>> + * e.g. draw a lock screen before doing the switch.
>> + * This function allows to register a custom vt switcher so that the actual
>> + * vt switch can be controlled by calling \a weston_compositor_activate_vt.
>
> The rationale is good.
>
>> + *
>> + * \param compositor The compositor instance.
>> + * \param switcher The vt switcher function, which will be called when a
>> + * CTRL+ALT+FN key combination is pressed, carrying the
>> + * requested vt.
>> + *
>> + * \sa weston_compositor_activate_vt
>> + */
>> +void
>> +weston_compositor_set_vt_switcher(struct weston_compositor *compositor,
>> + weston_compositor_vt_switcher_func_t switcher);
>> +
>> int
>> weston_environment_get_fd(const char *env);
>>
>> diff --git a/libweston/launcher-impl.h b/libweston/launcher-impl.h
>> index 0bc3245..8b0531d 100644
>> --- a/libweston/launcher-impl.h
>> +++ b/libweston/launcher-impl.h
>> @@ -39,6 +39,7 @@ struct launcher_interface {
>>
>> struct weston_launcher {
>> struct launcher_interface *iface;
>> + weston_compositor_vt_switcher_func_t vt_switcher;
>> };
>>
>> extern struct launcher_interface launcher_logind_iface;
>> diff --git a/libweston/launcher-util.c b/libweston/launcher-util.c
>> index 5fbfdcf..7bd91ae 100644
>> --- a/libweston/launcher-util.c
>> +++ b/libweston/launcher-util.c
>> @@ -99,11 +99,15 @@ switch_vt_binding(struct weston_keyboard *keyboard,
>> struct weston_compositor *compositor = data;
>> struct weston_launcher *launcher = compositor->launcher;
>> int vt = key - KEY_F1 + 1;
>> + weston_compositor_vt_switcher_func_t switcher = launcher->vt_switcher;
>>
>> if (vt == launcher->iface->get_vt(launcher))
>> return;
>>
>> - weston_launcher_activate_vt(launcher, vt);
>> + if (switcher)
>> + launcher->vt_switcher(compositor, vt);
>> + else
>> + weston_launcher_activate_vt(launcher, vt);
>> }
>>
>> WL_EXPORT void
>
> However, I think the architecture here is unnecessarily complicated.
> Now the call chain looks like:
>
> launcher-util sets key bindings
> user smashes keys -> launcher-util -> switcher callback ->
> weston_compositor_activate_vt -> launcher-util ->
> launcher-backend
>
> If the compositor (or shell) was resposible of setting the key bindings
> itself, it would look like this:
>
> shell sets key bindings
> user smashes keys -> shell -> weston_compositor_activate_vt -> ....
>
> Here in the shell step the compositor could retrieve Weston's VT, check
> if it's a no-op, do all the prep e.g. locking and call the VT switching
> function. This would be much easier to understand from API point of view.
>
> What do you think?
That's the way i went with originally. However that would mean
duplicating the code setting up the bindings and checking up if the vt
number is ok in every compositor, just to remove one step in the call
chain you outlined above. One step in a call chain that is hit rarely,
so there would be no performance concerns. Also i don't think this way
the API is any difficult to understand, it's just a callback.
Thanks,
Giulio
>
>
> Thanks,
> pq
More information about the wayland-devel
mailing list