[PATCH weston v2 1/2] launcher: don't try to switch to the current vt

[Giulio Camuffo]

2016-11-25 15:10 GMT+01:00 Pekka Paalanen <ppaalanen at gmail.com>:
> On Wed, 29 Jun 2016 11:57:11 +0200
> Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
>
>> Signed-off-by: Giulio Camuffo <giuliocamuffo at gmail.com>
>> ---
>>
>> v2: this was split from the following commit
>>
>>  libweston/launcher-direct.c        | 12 ++++++++++++
>>  libweston/launcher-impl.h          |  1 +
>>  libweston/launcher-logind.c        |  8 ++++++++
>>  libweston/launcher-util.c          |  7 ++++++-
>>  libweston/launcher-weston-launch.c | 12 ++++++++++++
>>  5 files changed, 39 insertions(+), 1 deletion(-)
>
> Hi,
>
> this patch is not actually checking the current VT, it is checking
> Weston's VT. That is, you want to avoid Weston switching to Weston's VT.
>
> Under normal circumstances Weston's VT is also the current VT when
> Weston is handling keyboard, but in faulty cases it might not be true.
> E.g. I think fbdev is still broken with VT switching, so can't say what
> would happen there.
>
> Hence I would change the summary to "don't try to switch to weston's VT".

That makes sense.

>
> I suppose this patch is needed for the following patch to not trigger
> no-op callbacks, and there was no other badness this would fix?

I think this makes sense on its own. It's not just to avoid no-ops,
the problem this solves is to block the screen (as i do in orbital
when switching tty) when hitting ctrl+alt+fN, with N being weston's
one, and leaving the user wondering why it's so slow to switch while
he hit the wrong key.


Cheers,
Giulio

>
>> diff --git a/libweston/launcher-direct.c b/libweston/launcher-direct.c
>> index 29d9c28..0ff99a4 100644
>> --- a/libweston/launcher-direct.c
>> +++ b/libweston/launcher-direct.c
>> @@ -305,6 +305,17 @@ launcher_direct_destroy(struct weston_launcher *launcher_base)
>>       free(launcher);
>>  }
>>
>> +static int
>> +launcher_direct_get_vt(struct weston_launcher *base)
>> +{
>> +     struct launcher_direct *launcher = wl_container_of(base, launcher, base);
>> +     struct stat s;
>> +     if (fstat(launcher->tty, &s) < 0)
>> +             return -1;
>> +
>> +     return minor(s.st_rdev);
>> +}
>
> This looks good.
>
>> +
>>  struct launcher_interface launcher_direct_iface = {
>>       launcher_direct_connect,
>>       launcher_direct_destroy,
>> @@ -312,4 +323,5 @@ struct launcher_interface launcher_direct_iface = {
>>       launcher_direct_close,
>>       launcher_direct_activate_vt,
>>       launcher_direct_restore,
>> +     launcher_direct_get_vt,
>>  };
>> diff --git a/libweston/launcher-impl.h b/libweston/launcher-impl.h
>> index 742721b..0bc3245 100644
>> --- a/libweston/launcher-impl.h
>> +++ b/libweston/launcher-impl.h
>> @@ -34,6 +34,7 @@ struct launcher_interface {
>>       void (* close) (struct weston_launcher *launcher, int fd);
>>       int (* activate_vt) (struct weston_launcher *launcher, int vt);
>>       void (* restore) (struct weston_launcher *launcher);
>> +     int (* get_vt) (struct weston_launcher *launcher);
>
> Documenting these would be nice...
>
>>  };
>>
>>  struct weston_launcher {
>> diff --git a/libweston/launcher-logind.c b/libweston/launcher-logind.c
>> index f755ec3..4680402 100644
>> --- a/libweston/launcher-logind.c
>> +++ b/libweston/launcher-logind.c
>> @@ -829,6 +829,13 @@ launcher_logind_destroy(struct weston_launcher *launcher)
>>       free(wl);
>>  }
>>
>> +static int
>> +launcher_logind_get_vt(struct weston_launcher *launcher)
>> +{
>> +     struct launcher_logind *wl = wl_container_of(launcher, wl, base);
>> +     return wl->vtnr;
>> +}
>
> Ok.
>
>> +
>>  struct launcher_interface launcher_logind_iface = {
>>       launcher_logind_connect,
>>       launcher_logind_destroy,
>> @@ -836,4 +843,5 @@ struct launcher_interface launcher_logind_iface = {
>>       launcher_logind_close,
>>       launcher_logind_activate_vt,
>>       launcher_logind_restore,
>> +     launcher_logind_get_vt,
>>  };
>> diff --git a/libweston/launcher-util.c b/libweston/launcher-util.c
>> index 03b9d63..5fbfdcf 100644
>> --- a/libweston/launcher-util.c
>> +++ b/libweston/launcher-util.c
>> @@ -97,8 +97,13 @@ switch_vt_binding(struct weston_keyboard *keyboard,
>>                 uint32_t time, uint32_t key, void *data)
>>  {
>>       struct weston_compositor *compositor = data;
>> +     struct weston_launcher *launcher = compositor->launcher;
>> +     int vt = key - KEY_F1 + 1;
>>
>> -     weston_launcher_activate_vt(compositor->launcher, key - KEY_F1 + 1);
>> +     if (vt == launcher->iface->get_vt(launcher))
>> +             return;
>> +
>> +     weston_launcher_activate_vt(launcher, vt);
>>  }
>
> Ok.
>
>>
>>  WL_EXPORT void
>> diff --git a/libweston/launcher-weston-launch.c b/libweston/launcher-weston-launch.c
>> index ad919f1..b74975a 100644
>> --- a/libweston/launcher-weston-launch.c
>> +++ b/libweston/launcher-weston-launch.c
>> @@ -290,6 +290,17 @@ launcher_weston_launch_destroy(struct weston_launcher *launcher_base)
>>       free(launcher);
>>  }
>>
>> +static int
>> +launcher_weston_launch_get_vt(struct weston_launcher *base)
>> +{
>> +     struct launcher_weston_launch *launcher = wl_container_of(base, launcher, base);
>> +     struct stat s;
>> +     if (fstat(launcher->tty, &s) < 0)
>> +             return -1;
>> +
>> +     return minor(s.st_rdev);
>> +}
>
> The same as for launcher_direct, ok.
>
>> +
>>  struct launcher_interface launcher_weston_launch_iface = {
>>       launcher_weston_launch_connect,
>>       launcher_weston_launch_destroy,
>> @@ -297,4 +308,5 @@ struct launcher_interface launcher_weston_launch_iface = {
>>       launcher_weston_launch_close,
>>       launcher_weston_launch_activate_vt,
>>       launcher_weston_launch_restore,
>> +     launcher_weston_launch_get_vt,
>>  };
>
> Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>
> Not sure if this needs rebasing, but since I think you would revise the
> second patch, please re-send this one too as rebased.
>
>
> Thanks,
> pq