[PATCH weston v2 2/2] compositor: allow to control the vt switching

Mon Dec 5 14:21:12 UTC 2016

On Mon, 5 Dec 2016 14:45:49 +0100
Giulio Camuffo <giuliocamuffo at gmail.com> wrote:

> 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.

Oh right, previously these were called from the few backends only.

Maybe we should stop linking libsession-helpers into the backends then
to avoid duplicating the code?

However, the problem with libsession-helpers is that it will pull in
libdrm and optionally logind and dbus. That's why it has been on the
backends that really need it and not in libweston. Otherwise there
would be no reason to have it as a helper lib at all.

Maybe the functions should be plumbed through to the backends instead?

> >> 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.

Hah! I had a feeling something like that might have been. :-)
I was even scared I might have suggested the more complex approach
myself. Talking about being on the fence.

> 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.

I have no performance concerns, my concerns are totally about
readability here. I'm already dizzy about all the vfuncs and wrappers
in libweston-desktop. :-)

If you think the architecture as layered components, the extra step in
the call chain is going in the opposite direction than everything else.
That's the surprise. Originally it is the shell that decides if the
VT-switching keys will be hooked up or not. Then launcher-util hooks
them up. On key hit, launcher-util gets called, tells shell to do
something, the shell tells launcher-util to switch. If launcher-util
could just switch, or shell hooked up the keys, the call chain would be
one-directional.

Setting up the keybindings and checking if you are already on the VT is
pretty trivial IMHO, so much that it does not pay for the effort to make
that code shared. If weston_compositor_activate_vt() handles the
already current VT just fine, there's not much the compositor could get
wrong.

But, if the above does not convince you, I am not going to argue about
it any further and will accept it like that. :-)

Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20161205/b70e199f/attachment.sig>