Discussion:
[PATCH weston] libinput: Restore keyboard focus after VT switch
Jamey Sharp
2018-05-21 23:21:14 UTC
Permalink
Under Weston's drm-backend, after returning to Weston from another VT,
no window has focus.

There's already code in notify_keyboard_focus_out and
notify_keyboard_focus_in to save and restore focus, respectively; and
udev_input_enable eventually calls notify_keyboard_focus_in, by way of
evdev_notify_keyboard_focus. But udev_input_disable doesn't currently
call notify_keyboard_focus_out.

This patch makes udev_input_disable symmetric with udev_input_enable by
calling notify_keyboard_focus_out on every seat, to save focus before
suspending libinput. In my testing this successfully resolved my issue.
However, I don't have a multi-seat setup to verify that it works there
too.

Signed-off-by: Jamey Sharp <***@minilop.net>
---
I'm not subscribed to the list, so please Cc me on replies.

libweston/libinput-seat.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/libweston/libinput-seat.c b/libweston/libinput-seat.c
index ac1e8e99..ded6dd3c 100644
--- a/libweston/libinput-seat.c
+++ b/libweston/libinput-seat.c
@@ -158,9 +158,14 @@ udev_seat_remove_devices(struct udev_seat *seat)
void
udev_input_disable(struct udev_input *input)
{
+ struct udev_seat *seat;
+
if (input->suspended)
return;

+ wl_list_for_each(seat, &input->compositor->seat_list, base.link)
+ notify_keyboard_focus_out(&seat->base);
+
wl_event_source_remove(input->libinput_source);
input->libinput_source = NULL;
libinput_suspend(input->libinput);
--
2.16.2
Daniel Stone
2018-07-22 10:09:06 UTC
Permalink
Hi Jamey,
Post by Jamey Sharp
Under Weston's drm-backend, after returning to Weston from another VT,
no window has focus.
There's already code in notify_keyboard_focus_out and
notify_keyboard_focus_in to save and restore focus, respectively; and
udev_input_enable eventually calls notify_keyboard_focus_in, by way of
evdev_notify_keyboard_focus. But udev_input_disable doesn't currently
call notify_keyboard_focus_out.
This patch makes udev_input_disable symmetric with udev_input_enable by
calling notify_keyboard_focus_out on every seat, to save focus before
suspending libinput. In my testing this successfully resolved my issue.
However, I don't have a multi-seat setup to verify that it works there
too.
This seems correct to me. Peter, any thoughts/comments?

Cheers,
Daniel
Jamey Sharp
2018-07-22 13:53:56 UTC
Permalink
Hey Daniel, thanks for taking a look! It turns out that this patch worked
in Weston 3, but in Weston 4, commit
85d55540cb64bf97a08b40f79dc66843f8295d3 broke it. I didn't investigate
carefully enough to understand what that commit was for or why it broke
this, but my patch plus a revert of that commit makes Weston 4 work without
side effects we've been able to observe, so that combo is what we've been
using for the last month.

I hope that's useful information

Jamey
Post by Daniel Stone
Hi Jamey,
Post by Jamey Sharp
Under Weston's drm-backend, after returning to Weston from another VT,
no window has focus.
There's already code in notify_keyboard_focus_out and
notify_keyboard_focus_in to save and restore focus, respectively; and
udev_input_enable eventually calls notify_keyboard_focus_in, by way of
evdev_notify_keyboard_focus. But udev_input_disable doesn't currently
call notify_keyboard_focus_out.
This patch makes udev_input_disable symmetric with udev_input_enable by
calling notify_keyboard_focus_out on every seat, to save focus before
suspending libinput. In my testing this successfully resolved my issue.
However, I don't have a multi-seat setup to verify that it works there
too.
This seems correct to me. Peter, any thoughts/comments?
Cheers,
Daniel
Peter Hutterer
2018-08-02 05:39:54 UTC
Permalink
for the archives, this had a short discusson on IRC but then fell under the
radar again
Post by Jamey Sharp
Hey Daniel, thanks for taking a look! It turns out that this patch worked
in Weston 3, but in Weston 4, commit
85d55540cb64bf97a08b40f79dc66843f8295d3 broke it. I didn't investigate
carefully enough to understand what that commit was for or why it broke
this, but my patch plus a revert of that commit makes Weston 4 work without
side effects we've been able to observe, so that combo is what we've been
using for the last month.
20:41 < whot> SardemFF7: afaict, the problem is that vt switching causes a
libinput_suspend() call. that generates DEVICE_REMOVED events which
eventually trigger weston_seat_release_keyboard(). That calls
weston_keyboard_set_focus() and because of 85d55540cb6 we're guaranteed
that the saved_kbd_focus is now NULL
20:41 < whot> so on vt switch back, we don't know what to restore the focus
to
20:45 < SardemFF7> I see
20:46 < ongy> does weston remove the seat when the last device got removed?
I had that issue before when working on waymonad.
20:47 < whot> I don't think so, from a quick check
20:53 < SardemFF7> I think I see another solution to my issue that would
(hopefully) not conflict like the current one
20:54 < SardemFF7> in set_focus(), early: if (saved != NULL && new != NULL)
{ saved = new; return; /* let set_focus do its thing when weston is back
on focus */}
20:56 < SardemFF7> so if you change focus when weston-not-focused, it’s
updated, if you reset focus/unfocus (set to NULL), saved focus is not
touched, and in other cases saved should be NULL I think…
[...]
21:02 < whot> SardemFF7: I think that should work, yes

Cheers,
Peter
Post by Jamey Sharp
I hope that's useful information…
Jamey
Post by Daniel Stone
Hi Jamey,
Post by Jamey Sharp
Under Weston's drm-backend, after returning to Weston from another VT,
no window has focus.
There's already code in notify_keyboard_focus_out and
notify_keyboard_focus_in to save and restore focus, respectively; and
udev_input_enable eventually calls notify_keyboard_focus_in, by way of
evdev_notify_keyboard_focus. But udev_input_disable doesn't currently
call notify_keyboard_focus_out.
This patch makes udev_input_disable symmetric with udev_input_enable by
calling notify_keyboard_focus_out on every seat, to save focus before
suspending libinput. In my testing this successfully resolved my issue.
However, I don't have a multi-seat setup to verify that it works there
too.
This seems correct to me. Peter, any thoughts/comments?
Cheers,
Daniel
Quentin Glidic
2018-08-02 08:29:53 UTC
Permalink
From: Quentin Glidic <sardemff7+***@sardemff7.net>

If we start a special (grabbing) client when Weston is unfocused, it
would lose focus when coming back to Weston.

A first attempt to fix this was 85d55540cb64bf97a08b40f79dc66843f8295d3b
but it messed with VT switching.

This fix just updates the saved focus, so when Weston gets focused back,
it will focus the correct client.

Signed-off-by: Quentin Glidic <sardemff7+***@sardemff7.net>
---

Sorry for the delay, I hoped I could make a Gitlab MR but sadly it
didn’t happen yet. :-)

I think this patch won’t conflict with VT switching, and it does fix the
issue I had initially.

Cheers,

libweston/input.c | 38 +++++++++++++++++++++++++-------------
1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/libweston/input.c b/libweston/input.c
index f1017dc1b..6a7f584fd 100644
--- a/libweston/input.c
+++ b/libweston/input.c
@@ -1507,6 +1507,17 @@ weston_pointer_set_focus(struct weston_pointer *pointer,
wl_signal_emit(&pointer->focus_signal, pointer);
}

+static void
+destroy_device_saved_kbd_focus(struct wl_listener *listener, void *data)
+{
+ struct weston_seat *ws;
+
+ ws = container_of(listener, struct weston_seat,
+ saved_kbd_focus_listener);
+
+ ws->saved_kbd_focus = NULL;
+}
+
static void
send_enter_to_resource_list(struct wl_list *list,
struct weston_keyboard *keyboard,
@@ -1528,7 +1539,8 @@ weston_keyboard_set_focus(struct weston_keyboard *keyboard,
struct weston_surface *surface)
{
struct wl_resource *resource;
- struct wl_display *display = keyboard->seat->compositor->wl_display;
+ struct weston_seat *seat = keyboard->seat;
+ struct wl_display *display = seat->compositor->wl_display;
uint32_t serial;
struct wl_list *focus_resource_list;

@@ -1540,6 +1552,17 @@ weston_keyboard_set_focus(struct weston_keyboard *keyboard,
if (surface && !surface->resource)
surface = NULL;

+ /* If we have a saved focus, this means Weston itself is unfocused.
+ * In this case, we just want to update our to-be-restored focus.
+ */
+ if (seat->saved_kbd_focus != NULL && surface != NULL) {
+ wl_list_remove(&seat->saved_kbd_focus_listener.link);
+ seat->saved_kbd_focus = surface;
+ wl_signal_add(&surface->destroy_signal,
+ &seat->saved_kbd_focus_listener);
+ return;
+ }
+
focus_resource_list = &keyboard->focus_resource_list;

if (!wl_list_empty(focus_resource_list) && keyboard->focus != surface) {
@@ -2229,17 +2252,6 @@ notify_pointer_focus(struct weston_seat *seat, struct weston_output *output,
}
}

-static void
-destroy_device_saved_kbd_focus(struct wl_listener *listener, void *data)
-{
- struct weston_seat *ws;
-
- ws = container_of(listener, struct weston_seat,
- saved_kbd_focus_listener);
-
- ws->saved_kbd_focus = NULL;
-}
-
WL_EXPORT void
notify_keyboard_focus_in(struct weston_seat *seat, struct wl_array *keys,
enum weston_key_state_update update_state)
@@ -2262,8 +2274,8 @@ notify_keyboard_focus_in(struct weston_seat *seat, struct wl_array *keys,

if (surface) {
wl_list_remove(&seat->saved_kbd_focus_listener.link);
- weston_keyboard_set_focus(keyboard, surface);
seat->saved_kbd_focus = NULL;
+ weston_keyboard_set_focus(keyboard, surface);
}
}
--
2.18.0
Quentin Glidic
2018-08-02 08:32:24 UTC
Permalink
Post by Quentin Glidic
If we start a special (grabbing) client when Weston is unfocused, it
would lose focus when coming back to Weston.
A first attempt to fix this was 85d55540cb64bf97a08b40f79dc66843f8295d3b
but it messed with VT switching.
This fix just updates the saved focus, so when Weston gets focused back,
it will focus the correct client.
---
Sorry for the delay, I hoped I could make a Gitlab MR but sadly it
didn’t happen yet. :-)
I think this patch won’t conflict with VT switching, and it does fix the
issue I had initially.
Cheers,
Forgot to CC everyone.
Post by Quentin Glidic
libweston/input.c | 38 +++++++++++++++++++++++++-------------
1 file changed, 25 insertions(+), 13 deletions(-)
diff --git a/libweston/input.c b/libweston/input.c
index f1017dc1b..6a7f584fd 100644
--- a/libweston/input.c
+++ b/libweston/input.c
@@ -1507,6 +1507,17 @@ weston_pointer_set_focus(struct weston_pointer *pointer,
wl_signal_emit(&pointer->focus_signal, pointer);
}
+static void
+destroy_device_saved_kbd_focus(struct wl_listener *listener, void *data)
+{
+ struct weston_seat *ws;
+
+ ws = container_of(listener, struct weston_seat,
+ saved_kbd_focus_listener);
+
+ ws->saved_kbd_focus = NULL;
+}
+
static void
send_enter_to_resource_list(struct wl_list *list,
struct weston_keyboard *keyboard,
@@ -1528,7 +1539,8 @@ weston_keyboard_set_focus(struct weston_keyboard *keyboard,
struct weston_surface *surface)
{
struct wl_resource *resource;
- struct wl_display *display = keyboard->seat->compositor->wl_display;
+ struct weston_seat *seat = keyboard->seat;
+ struct wl_display *display = seat->compositor->wl_display;
uint32_t serial;
struct wl_list *focus_resource_list;
@@ -1540,6 +1552,17 @@ weston_keyboard_set_focus(struct weston_keyboard *keyboard,
if (surface && !surface->resource)
surface = NULL;
+ /* If we have a saved focus, this means Weston itself is unfocused.
+ * In this case, we just want to update our to-be-restored focus.
+ */
+ if (seat->saved_kbd_focus != NULL && surface != NULL) {
+ wl_list_remove(&seat->saved_kbd_focus_listener.link);
+ seat->saved_kbd_focus = surface;
+ wl_signal_add(&surface->destroy_signal,
+ &seat->saved_kbd_focus_listener);
+ return;
+ }
+
focus_resource_list = &keyboard->focus_resource_list;
if (!wl_list_empty(focus_resource_list) && keyboard->focus != surface) {
@@ -2229,17 +2252,6 @@ notify_pointer_focus(struct weston_seat *seat, struct weston_output *output,
}
}
-static void
-destroy_device_saved_kbd_focus(struct wl_listener *listener, void *data)
-{
- struct weston_seat *ws;
-
- ws = container_of(listener, struct weston_seat,
- saved_kbd_focus_listener);
-
- ws->saved_kbd_focus = NULL;
-}
-
WL_EXPORT void
notify_keyboard_focus_in(struct weston_seat *seat, struct wl_array *keys,
enum weston_key_state_update update_state)
@@ -2262,8 +2274,8 @@ notify_keyboard_focus_in(struct weston_seat *seat, struct wl_array *keys,
if (surface) {
wl_list_remove(&seat->saved_kbd_focus_listener.link);
- weston_keyboard_set_focus(keyboard, surface);
seat->saved_kbd_focus = NULL;
+ weston_keyboard_set_focus(keyboard, surface);
}
}
--
Quentin “Sardem FF7” Glidic
Derek Foreman
2018-08-10 17:55:42 UTC
Permalink
Post by Quentin Glidic
Post by Quentin Glidic
If we start a special (grabbing) client when Weston is unfocused, it
would lose focus when coming back to Weston.
A first attempt to fix this was 85d55540cb64bf97a08b40f79dc66843f8295d3b
but it messed with VT switching.
This fix just updates the saved focus, so when Weston gets focused back,
it will focus the correct client.
---
Sorry for the delay, I hoped I could make a Gitlab MR but sadly it
didn’t happen yet. :-)
I think this patch won’t conflict with VT switching, and it does fix the
issue I had initially.
I'm a bit confused as to where we're at with this.

How did the reverted patch "mess with" or "conflict with" VT switching?

Is it intended that these two patches be applied, and then Jamey's patch
(marked as "superseded" in patchwork) be applied on top to resolve the
loss of focus on VT switch away/back?

Thought this might be important to land before the release, but it's not
terribly clear what it actually fixes. I'd assumed it was the VT switch
thing, but that remains unresolved.

Help? :)

Thanks,
Derek
Post by Quentin Glidic
Post by Quentin Glidic
Cheers,
Forgot to CC everyone.
Post by Quentin Glidic
  libweston/input.c | 38 +++++++++++++++++++++++++-------------
  1 file changed, 25 insertions(+), 13 deletions(-)
diff --git a/libweston/input.c b/libweston/input.c
index f1017dc1b..6a7f584fd 100644
--- a/libweston/input.c
+++ b/libweston/input.c
@@ -1507,6 +1507,17 @@ weston_pointer_set_focus(struct weston_pointer *pointer,
      wl_signal_emit(&pointer->focus_signal, pointer);
  }
  +static void
+destroy_device_saved_kbd_focus(struct wl_listener *listener, void *data)
+{
+    struct weston_seat *ws;
+
+    ws = container_of(listener, struct weston_seat,
+              saved_kbd_focus_listener);
+
+    ws->saved_kbd_focus = NULL;
+}
+
  static void
  send_enter_to_resource_list(struct wl_list *list,
                  struct weston_keyboard *keyboard,
@@ -1528,7 +1539,8 @@ weston_keyboard_set_focus(struct weston_keyboard *keyboard,
                struct weston_surface *surface)
  {
      struct wl_resource *resource;
-    struct wl_display *display = keyboard->seat->compositor->wl_display;
+    struct weston_seat *seat = keyboard->seat;
+    struct wl_display *display = seat->compositor->wl_display;
      uint32_t serial;
      struct wl_list *focus_resource_list;
weston_keyboard *keyboard,
      if (surface && !surface->resource)
          surface = NULL;
  +    /* If we have a saved focus, this means Weston itself is
unfocused.
+     * In this case, we just want to update our to-be-restored focus.
+     */
+    if (seat->saved_kbd_focus != NULL && surface != NULL) {
+        wl_list_remove(&seat->saved_kbd_focus_listener.link);
+        seat->saved_kbd_focus = surface;
+        wl_signal_add(&surface->destroy_signal,
+                  &seat->saved_kbd_focus_listener);
+        return;
+    }
+
      focus_resource_list = &keyboard->focus_resource_list;
        if (!wl_list_empty(focus_resource_list) && keyboard->focus !=
surface) {
@@ -2229,17 +2252,6 @@ notify_pointer_focus(struct weston_seat *seat,
struct weston_output *output,
      }
  }
  -static void
-destroy_device_saved_kbd_focus(struct wl_listener *listener, void *data)
-{
-    struct weston_seat *ws;
-
-    ws = container_of(listener, struct weston_seat,
-              saved_kbd_focus_listener);
-
-    ws->saved_kbd_focus = NULL;
-}
-
  WL_EXPORT void
  notify_keyboard_focus_in(struct weston_seat *seat, struct wl_array
*keys,
               enum weston_key_state_update update_state)
@@ -2262,8 +2274,8 @@ notify_keyboard_focus_in(struct weston_seat
*seat, struct wl_array *keys,
        if (surface) {
          wl_list_remove(&seat->saved_kbd_focus_listener.link);
-        weston_keyboard_set_focus(keyboard, surface);
          seat->saved_kbd_focus = NULL;
+        weston_keyboard_set_focus(keyboard, surface);
      }
  }
 
Peter Hutterer
2018-08-16 03:24:38 UTC
Permalink
Post by Derek Foreman
Post by Quentin Glidic
Post by Quentin Glidic
If we start a special (grabbing) client when Weston is unfocused, it
would lose focus when coming back to Weston.
A first attempt to fix this was 85d55540cb64bf97a08b40f79dc66843f8295d3b
but it messed with VT switching.
This fix just updates the saved focus, so when Weston gets focused back,
it will focus the correct client.
---
Sorry for the delay, I hoped I could make a Gitlab MR but sadly it
didn’t happen yet. :-)
I think this patch won’t conflict with VT switching, and it does fix the
issue I had initially.
I'm a bit confused as to where we're at with this.
How did the reverted patch "mess with" or "conflict with" VT switching?
it ended up always setting the keyboard focus to NULL on VT switch (due to
how libinput devices are handled), so on vt switch back you had no focus.
Post by Derek Foreman
Is it intended that these two patches be applied, and then Jamey's patch
(marked as "superseded" in patchwork) be applied on top to resolve the
loss of focus on VT switch away/back?
AIUI, these two need supersede Jamey's patchl but I'm not 100% sure on that,
sorry.

Cheers,
Peter
Post by Derek Foreman
Thought this might be important to land before the release, but it's not
terribly clear what it actually fixes. I'd assumed it was the VT switch
thing, but that remains unresolved.
Help? :)
Thanks,
Derek
Post by Quentin Glidic
Post by Quentin Glidic
Cheers,
Forgot to CC everyone.
Post by Quentin Glidic
  libweston/input.c | 38 +++++++++++++++++++++++++-------------
  1 file changed, 25 insertions(+), 13 deletions(-)
diff --git a/libweston/input.c b/libweston/input.c
index f1017dc1b..6a7f584fd 100644
--- a/libweston/input.c
+++ b/libweston/input.c
@@ -1507,6 +1507,17 @@ weston_pointer_set_focus(struct weston_pointer *pointer,
      wl_signal_emit(&pointer->focus_signal, pointer);
  }
  +static void
+destroy_device_saved_kbd_focus(struct wl_listener *listener, void *data)
+{
+    struct weston_seat *ws;
+
+    ws = container_of(listener, struct weston_seat,
+              saved_kbd_focus_listener);
+
+    ws->saved_kbd_focus = NULL;
+}
+
  static void
  send_enter_to_resource_list(struct wl_list *list,
                  struct weston_keyboard *keyboard,
@@ -1528,7 +1539,8 @@ weston_keyboard_set_focus(struct weston_keyboard *keyboard,
                struct weston_surface *surface)
  {
      struct wl_resource *resource;
-    struct wl_display *display = keyboard->seat->compositor->wl_display;
+    struct weston_seat *seat = keyboard->seat;
+    struct wl_display *display = seat->compositor->wl_display;
      uint32_t serial;
      struct wl_list *focus_resource_list;
weston_keyboard *keyboard,
      if (surface && !surface->resource)
          surface = NULL;
  +    /* If we have a saved focus, this means Weston itself is
unfocused.
+     * In this case, we just want to update our to-be-restored focus.
+     */
+    if (seat->saved_kbd_focus != NULL && surface != NULL) {
+        wl_list_remove(&seat->saved_kbd_focus_listener.link);
+        seat->saved_kbd_focus = surface;
+        wl_signal_add(&surface->destroy_signal,
+                  &seat->saved_kbd_focus_listener);
+        return;
+    }
+
      focus_resource_list = &keyboard->focus_resource_list;
        if (!wl_list_empty(focus_resource_list) && keyboard->focus !=
surface) {
@@ -2229,17 +2252,6 @@ notify_pointer_focus(struct weston_seat *seat,
struct weston_output *output,
      }
  }
  -static void
-destroy_device_saved_kbd_focus(struct wl_listener *listener, void *data)
-{
-    struct weston_seat *ws;
-
-    ws = container_of(listener, struct weston_seat,
-              saved_kbd_focus_listener);
-
-    ws->saved_kbd_focus = NULL;
-}
-
  WL_EXPORT void
  notify_keyboard_focus_in(struct weston_seat *seat, struct wl_array
*keys,
               enum weston_key_state_update update_state)
@@ -2262,8 +2274,8 @@ notify_keyboard_focus_in(struct weston_seat
*seat, struct wl_array *keys,
        if (surface) {
          wl_list_remove(&seat->saved_kbd_focus_listener.link);
-        weston_keyboard_set_focus(keyboard, surface);
          seat->saved_kbd_focus = NULL;
+        weston_keyboard_set_focus(keyboard, surface);
      }
  }
 
Quentin Glidic
2018-08-16 07:33:34 UTC
Permalink
Post by Peter Hutterer
Post by Derek Foreman
Post by Quentin Glidic
If we start a special (grabbing) client when Weston is unfocused, it
would lose focus when coming back to Weston.
A first attempt to fix this was 85d55540cb64bf97a08b40f79dc66843f8295d3b
but it messed with VT switching.
This fix just updates the saved focus, so when Weston gets focused back,
it will focus the correct client.
---
Sorry for the delay, I hoped I could make a Gitlab MR but sadly it
didn’t happen yet. :-)
I think this patch won’t conflict with VT switching, and it does fix the
issue I had initially.
I'm a bit confused as to where we're at with this.
How did the reverted patch "mess with" or "conflict with" VT switching?
it ended up always setting the keyboard focus to NULL on VT switch (due to
how libinput devices are handled), so on vt switch back you had no focus.
Post by Derek Foreman
Is it intended that these two patches be applied, and then Jamey's patch
(marked as "superseded" in patchwork) be applied on top to resolve the
loss of focus on VT switch away/back?
AIUI, these two need supersede Jamey's patchl but I'm not 100% sure on that,
sorry.
Cheers,
Peter
Post by Derek Foreman
Thought this might be important to land before the release, but it's not
terribly clear what it actually fixes. I'd assumed it was the VT switch
thing, but that remains unresolved.
Help? :)
Sorry for the confusion. This (second) patch is a cleaner fix of the
issue that was “fixed” by the reverted commit. Then on top of it, you’ll
have to apply Jamey’s patch, which is an independent issue+fix (which
the old fix conflicted with). I’m not sure why it was marked
superseeded, maybe Patchwork detecting my patch as a reply?
--
Quentin “Sardem FF7” Glidic
Derek Foreman
2018-08-24 14:11:07 UTC
Permalink
Post by Quentin Glidic
Post by Peter Hutterer
Post by Derek Foreman
Post by Quentin Glidic
If we start a special (grabbing) client when Weston is unfocused, it
would lose focus when coming back to Weston.
A first attempt to fix this was
85d55540cb64bf97a08b40f79dc66843f8295d3b
but it messed with VT switching.
This fix just updates the saved focus, so when Weston gets focused back,
it will focus the correct client.
---
Sorry for the delay, I hoped I could make a Gitlab MR but sadly it
didn’t happen yet. :-)
I think this patch won’t conflict with VT switching, and it does fix the
issue I had initially.
I'm a bit confused as to where we're at with this.
How did the reverted patch "mess with" or "conflict with" VT switching?
it ended up always setting the keyboard focus to NULL on VT switch (due to
how libinput devices are handled), so on vt switch back you had no focus.
 
Post by Derek Foreman
Is it intended that these two patches be applied, and then Jamey's patch
(marked as "superseded" in patchwork) be applied on top to resolve the
loss of focus on VT switch away/back?
AIUI, these two need supersede Jamey's patchl but I'm not 100% sure on that,
sorry.
Cheers,
    Peter
Post by Derek Foreman
Thought this might be important to land before the release, but it's not
terribly clear what it actually fixes.  I'd assumed it was the VT switch
thing, but that remains unresolved.
Help? :)
Sorry for the confusion. This (second) patch is a cleaner fix of the
issue that was “fixed” by the reverted commit. Then on top of it, you’ll
have to apply Jamey’s patch, which is an independent issue+fix (which
the old fix conflicted with). I’m not sure why it was marked
superseeded, maybe Patchwork detecting my patch as a reply?
Thanks guys. Due to hilariously misconfigured inbox filters I didn't
catch these replies until today. Sorry.

I think the VT switch problem has been around for at least 1 release
now, possibly a few more, so I think it's ok to release with the long
standing (mostly cosmetic) bug, and deal with these fixes shortly after.

Thanks again,
Derek
Jamey Sharp
2018-08-24 17:23:12 UTC
Permalink
For what it's worth, I'm happy to use backported patches. I just hope this
gets addressed upstream eventually.

It's a little more than just cosmetic if you have a graphical application
that can be driven purely by keyboard, and sometimes you don't have a
working pointer input device so you can't get focus back after a VT switch.
I grant that's a somewhat niche use case, but it's the one I'm dealing
with... :-)

I was confused about the state of these patches too, because I didn't see
the original mails. Hopefully next week I can test the combination of
Quentin's revert+fix pair with my patch and make sure it passes the tests I
set up.

On that note, I would offer my test framework upstream, except I set up an
entire qemu image using NixOS to test this, and that seems a little
heavyweight. I can't think of an easier way to test drm-backend stuff
though...

Jamey

On Fri, Aug 24, 2018 at 7:11 AM, Derek Foreman <
Post by Jamey Sharp
Post by Quentin Glidic
Post by Peter Hutterer
Post by Derek Foreman
Post by Quentin Glidic
If we start a special (grabbing) client when Weston is unfocused, it
would lose focus when coming back to Weston.
A first attempt to fix this was
85d55540cb64bf97a08b40f79dc66843f8295d3b
but it messed with VT switching.
This fix just updates the saved focus, so when Weston gets focused back,
it will focus the correct client.
---
Sorry for the delay, I hoped I could make a Gitlab MR but sadly it
didn’t happen yet. :-)
I think this patch won’t conflict with VT switching, and it does
fix the
issue I had initially.
I'm a bit confused as to where we're at with this.
How did the reverted patch "mess with" or "conflict with" VT switching?
it ended up always setting the keyboard focus to NULL on VT switch (due to
how libinput devices are handled), so on vt switch back you had no
focus.
Post by Quentin Glidic
Post by Peter Hutterer
Post by Derek Foreman
Is it intended that these two patches be applied, and then Jamey's
patch
Post by Quentin Glidic
Post by Peter Hutterer
Post by Derek Foreman
(marked as "superseded" in patchwork) be applied on top to resolve the
loss of focus on VT switch away/back?
AIUI, these two need supersede Jamey's patchl but I'm not 100% sure on that,
sorry.
Cheers,
Peter
Post by Derek Foreman
Thought this might be important to land before the release, but it's
not
Post by Quentin Glidic
Post by Peter Hutterer
Post by Derek Foreman
terribly clear what it actually fixes. I'd assumed it was the VT
switch
Post by Quentin Glidic
Post by Peter Hutterer
Post by Derek Foreman
thing, but that remains unresolved.
Help? :)
Sorry for the confusion. This (second) patch is a cleaner fix of the
issue that was “fixed” by the reverted commit. Then on top of it, you’ll
have to apply Jamey’s patch, which is an independent issue+fix (which
the old fix conflicted with). I’m not sure why it was marked
superseeded, maybe Patchwork detecting my patch as a reply?
Thanks guys. Due to hilariously misconfigured inbox filters I didn't
catch these replies until today. Sorry.
I think the VT switch problem has been around for at least 1 release
now, possibly a few more, so I think it's ok to release with the long
standing (mostly cosmetic) bug, and deal with these fixes shortly after.
Thanks again,
Derek
Derek Foreman
2018-08-24 17:52:33 UTC
Permalink
Post by Jamey Sharp
For what it's worth, I'm happy to use backported patches. I just hope
this gets addressed upstream eventually.
It's a little more than just cosmetic if you have a graphical
application that can be driven purely by keyboard, and sometimes you
don't have a working pointer input device so you can't get focus back
after a VT switch. I grant that's a somewhat niche use case, but it's
the one I'm dealing with... :-)
Sorry if it seemed I was dismissing this work entirely. This bug has
been annoying me for quite some time and I hadn't looked into it at all
myself. For my use case, I can use the keyboard focus switch shortcut to
focus something, so it hasn't been a showstopper.

I think we're going to do a weston 5.0.1 in a month or so, and this will
be among the fixes it contains. :)
Post by Jamey Sharp
I was confused about the state of these patches too, because I didn't
see the original mails. Hopefully next week I can test the combination
of Quentin's revert+fix pair with my patch and make sure it passes the
tests I set up.
That would be great!
Post by Jamey Sharp
On that note, I would offer my test framework upstream, except I set up
an entire qemu image using NixOS to test this, and that seems a little
heavyweight. I can't think of an easier way to test drm-backend stuff
though...
Would still be interesting to take a look at, I think.

Thanks,
Derek
Post by Jamey Sharp
Jamey
On Fri, Aug 24, 2018 at 7:11 AM, Derek Foreman
Post by Quentin Glidic
Post by Peter Hutterer
Post by Derek Foreman
Post by Quentin Glidic
If we start a special (grabbing) client when Weston is
unfocused, it
Post by Quentin Glidic
Post by Peter Hutterer
Post by Derek Foreman
Post by Quentin Glidic
would lose focus when coming back to Weston.
A first attempt to fix this was
85d55540cb64bf97a08b40f79dc66843f8295d3b
but it messed with VT switching.
This fix just updates the saved focus, so when Weston gets focused
back,
it will focus the correct client.
---
Sorry for the delay, I hoped I could make a Gitlab MR but sadly it
didn’t happen yet. :-)
I think this patch won’t conflict with VT switching, and it does
fix the
issue I had initially.
I'm a bit confused as to where we're at with this.
How did the reverted patch "mess with" or "conflict with" VT
switching?
Post by Quentin Glidic
Post by Peter Hutterer
it ended up always setting the keyboard focus to NULL on VT switch (due to
how libinput devices are handled), so on vt switch back you had
no focus.
Post by Quentin Glidic
Post by Peter Hutterer
 
Post by Derek Foreman
Is it intended that these two patches be applied, and then
Jamey's patch
Post by Quentin Glidic
Post by Peter Hutterer
Post by Derek Foreman
(marked as "superseded" in patchwork) be applied on top to
resolve the
Post by Quentin Glidic
Post by Peter Hutterer
Post by Derek Foreman
loss of focus on VT switch away/back?
AIUI, these two need supersede Jamey's patchl but I'm not 100%
sure on
Post by Quentin Glidic
Post by Peter Hutterer
that,
sorry.
Cheers,
    Peter
Post by Derek Foreman
Thought this might be important to land before the release, but
it's not
Post by Quentin Glidic
Post by Peter Hutterer
Post by Derek Foreman
terribly clear what it actually fixes.  I'd assumed it was the
VT switch
Post by Quentin Glidic
Post by Peter Hutterer
Post by Derek Foreman
thing, but that remains unresolved.
Help? :)
Sorry for the confusion. This (second) patch is a cleaner fix of the
issue that was “fixed” by the reverted commit. Then on top of it,
you’ll
Post by Quentin Glidic
have to apply Jamey’s patch, which is an independent issue+fix (which
the old fix conflicted with). I’m not sure why it was marked
superseeded, maybe Patchwork detecting my patch as a reply?
Thanks guys.  Due to hilariously misconfigured inbox filters I didn't
catch these replies until today.  Sorry.
I think the VT switch problem has been around for at least 1 release
now, possibly a few more, so I think it's ok to release with the long
standing (mostly cosmetic) bug, and deal with these fixes shortly after.
Thanks again,
Derek
Jamey Sharp
2018-09-18 00:04:16 UTC
Permalink
It took me longer than I hoped to get back to this, but I have now tested
Quentin Glidic's two patches (the revert followed by the revised fix) plus
my "Restore keyboard focus after VT switch", applied to Weston 4. With all
three patches applied, Weston passes my tests. Hooray!

And just to be sure, I verified that applying Quentin's patches without
mine do _not_ pass my tests. So Quentin's patches don't interfere with mine
(good!) but also don't supersede mine, despite how Patchwork apparently got
set.

Thanks everyone for keeping this going and working out the right details!
Jamey

On Fri, Aug 24, 2018 at 10:52 AM Derek Foreman <
Post by Derek Foreman
Post by Jamey Sharp
For what it's worth, I'm happy to use backported patches. I just hope
this gets addressed upstream eventually.
It's a little more than just cosmetic if you have a graphical
application that can be driven purely by keyboard, and sometimes you
don't have a working pointer input device so you can't get focus back
after a VT switch. I grant that's a somewhat niche use case, but it's
the one I'm dealing with... :-)
Sorry if it seemed I was dismissing this work entirely. This bug has
been annoying me for quite some time and I hadn't looked into it at all
myself. For my use case, I can use the keyboard focus switch shortcut to
focus something, so it hasn't been a showstopper.
I think we're going to do a weston 5.0.1 in a month or so, and this will
be among the fixes it contains. :)
Post by Jamey Sharp
I was confused about the state of these patches too, because I didn't
see the original mails. Hopefully next week I can test the combination
of Quentin's revert+fix pair with my patch and make sure it passes the
tests I set up.
That would be great!
Post by Jamey Sharp
On that note, I would offer my test framework upstream, except I set up
an entire qemu image using NixOS to test this, and that seems a little
heavyweight. I can't think of an easier way to test drm-backend stuff
though...
Would still be interesting to take a look at, I think.
Thanks,
Derek
Post by Jamey Sharp
Jamey
On Fri, Aug 24, 2018 at 7:11 AM, Derek Foreman
Post by Quentin Glidic
Post by Peter Hutterer
Post by Derek Foreman
Post by Quentin Glidic
If we start a special (grabbing) client when Weston is
unfocused, it
Post by Quentin Glidic
Post by Peter Hutterer
Post by Derek Foreman
Post by Quentin Glidic
would lose focus when coming back to Weston.
A first attempt to fix this was
85d55540cb64bf97a08b40f79dc66843f8295d3b
but it messed with VT switching.
This fix just updates the saved focus, so when Weston gets
focused
Post by Jamey Sharp
Post by Quentin Glidic
Post by Peter Hutterer
Post by Derek Foreman
Post by Quentin Glidic
back,
it will focus the correct client.
---
Sorry for the delay, I hoped I could make a Gitlab MR but
sadly it
Post by Jamey Sharp
Post by Quentin Glidic
Post by Peter Hutterer
Post by Derek Foreman
Post by Quentin Glidic
didn’t happen yet. :-)
I think this patch won’t conflict with VT switching, and it
does
Post by Jamey Sharp
Post by Quentin Glidic
Post by Peter Hutterer
Post by Derek Foreman
Post by Quentin Glidic
fix the
issue I had initially.
I'm a bit confused as to where we're at with this.
How did the reverted patch "mess with" or "conflict with" VT
switching?
Post by Quentin Glidic
Post by Peter Hutterer
it ended up always setting the keyboard focus to NULL on VT switch
(due to
how libinput devices are handled), so on vt switch back you had
no focus.
Post by Quentin Glidic
Post by Peter Hutterer
Post by Derek Foreman
Is it intended that these two patches be applied, and then
Jamey's patch
Post by Quentin Glidic
Post by Peter Hutterer
Post by Derek Foreman
(marked as "superseded" in patchwork) be applied on top to
resolve the
Post by Quentin Glidic
Post by Peter Hutterer
Post by Derek Foreman
loss of focus on VT switch away/back?
AIUI, these two need supersede Jamey's patchl but I'm not 100%
sure on
Post by Quentin Glidic
Post by Peter Hutterer
that,
sorry.
Cheers,
Peter
Post by Derek Foreman
Thought this might be important to land before the release, but
it's not
Post by Quentin Glidic
Post by Peter Hutterer
Post by Derek Foreman
terribly clear what it actually fixes. I'd assumed it was the
VT switch
Post by Quentin Glidic
Post by Peter Hutterer
Post by Derek Foreman
thing, but that remains unresolved.
Help? :)
Sorry for the confusion. This (second) patch is a cleaner fix of
the
Post by Jamey Sharp
Post by Quentin Glidic
issue that was “fixed” by the reverted commit. Then on top of it,
you’ll
Post by Quentin Glidic
have to apply Jamey’s patch, which is an independent issue+fix
(which
Post by Jamey Sharp
Post by Quentin Glidic
the old fix conflicted with). I’m not sure why it was marked
superseeded, maybe Patchwork detecting my patch as a reply?
Thanks guys. Due to hilariously misconfigured inbox filters I didn't
catch these replies until today. Sorry.
I think the VT switch problem has been around for at least 1 release
now, possibly a few more, so I think it's ok to release with the long
standing (mostly cosmetic) bug, and deal with these fixes shortly
after.
Post by Jamey Sharp
Thanks again,
Derek
Peter Hutterer
2018-08-03 04:15:01 UTC
Permalink
Post by Quentin Glidic
If we start a special (grabbing) client when Weston is unfocused, it
would lose focus when coming back to Weston.
A first attempt to fix this was 85d55540cb64bf97a08b40f79dc66843f8295d3b
but it messed with VT switching.
This fix just updates the saved focus, so when Weston gets focused back,
it will focus the correct client.
---
Sorry for the delay, I hoped I could make a Gitlab MR but sadly it
didn’t happen yet. :-)
I think this patch won’t conflict with VT switching, and it does fix the
issue I had initially.
this seems like it should do the thing, thanks.

Acked-by: Peter Hutterer <***@who-t.net>

Cheers,
Peter
Post by Quentin Glidic
libweston/input.c | 38 +++++++++++++++++++++++++-------------
1 file changed, 25 insertions(+), 13 deletions(-)
diff --git a/libweston/input.c b/libweston/input.c
index f1017dc1b..6a7f584fd 100644
--- a/libweston/input.c
+++ b/libweston/input.c
@@ -1507,6 +1507,17 @@ weston_pointer_set_focus(struct weston_pointer *pointer,
wl_signal_emit(&pointer->focus_signal, pointer);
}
+static void
+destroy_device_saved_kbd_focus(struct wl_listener *listener, void *data)
+{
+ struct weston_seat *ws;
+
+ ws = container_of(listener, struct weston_seat,
+ saved_kbd_focus_listener);
+
+ ws->saved_kbd_focus = NULL;
+}
+
static void
send_enter_to_resource_list(struct wl_list *list,
struct weston_keyboard *keyboard,
@@ -1528,7 +1539,8 @@ weston_keyboard_set_focus(struct weston_keyboard *keyboard,
struct weston_surface *surface)
{
struct wl_resource *resource;
- struct wl_display *display = keyboard->seat->compositor->wl_display;
+ struct weston_seat *seat = keyboard->seat;
+ struct wl_display *display = seat->compositor->wl_display;
uint32_t serial;
struct wl_list *focus_resource_list;
@@ -1540,6 +1552,17 @@ weston_keyboard_set_focus(struct weston_keyboard *keyboard,
if (surface && !surface->resource)
surface = NULL;
+ /* If we have a saved focus, this means Weston itself is unfocused.
+ * In this case, we just want to update our to-be-restored focus.
+ */
+ if (seat->saved_kbd_focus != NULL && surface != NULL) {
+ wl_list_remove(&seat->saved_kbd_focus_listener.link);
+ seat->saved_kbd_focus = surface;
+ wl_signal_add(&surface->destroy_signal,
+ &seat->saved_kbd_focus_listener);
+ return;
+ }
+
focus_resource_list = &keyboard->focus_resource_list;
if (!wl_list_empty(focus_resource_list) && keyboard->focus != surface) {
@@ -2229,17 +2252,6 @@ notify_pointer_focus(struct weston_seat *seat, struct weston_output *output,
}
}
-static void
-destroy_device_saved_kbd_focus(struct wl_listener *listener, void *data)
-{
- struct weston_seat *ws;
-
- ws = container_of(listener, struct weston_seat,
- saved_kbd_focus_listener);
-
- ws->saved_kbd_focus = NULL;
-}
-
WL_EXPORT void
notify_keyboard_focus_in(struct weston_seat *seat, struct wl_array *keys,
enum weston_key_state_update update_state)
@@ -2262,8 +2274,8 @@ notify_keyboard_focus_in(struct weston_seat *seat, struct wl_array *keys,
if (surface) {
wl_list_remove(&seat->saved_kbd_focus_listener.link);
- weston_keyboard_set_focus(keyboard, surface);
seat->saved_kbd_focus = NULL;
+ weston_keyboard_set_focus(keyboard, surface);
}
}
--
2.18.0
_______________________________________________
wayland-devel mailing list
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Loading...