[spice-gtk] widget: avoid gdk_seat_grab/ungrab() API temporarily

Submitted by Victor Toso on May 22, 2018, 6:42 a.m.

Details

Message ID 20180522064245.8439-1-victortoso@redhat.com
State New
Headers show
Series "widget: avoid gdk_seat_grab/ungrab() API temporarily" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso May 22, 2018, 6:42 a.m.
From: Victor Toso <me@victortoso.com>

We are using those APIs to avoid the deprecated ones from GTK but
unexpected events are handled different by them introducing a
regression that is easy to reproduce when Spice client holds the
focus and user locks the screen.

We need to better handle the situation where gdk_seat_grab() fails,
specifically with GDK_GRAB_ALREADY_GRABBED which is the situation
for the bugs mentioned below. The grab does not fail with older API
in the same situation.

Note that gdk_seat_grab() returning GDK_GRAB_ALREADY_GRABBED is due
the locking screen holding the keyboard device which also means that
remote-viewer isn't visible/show but gdk_window_is_visible() returns
true till gdk_seat_grab() is called. The failure in gtk+ function
call sets the visibility to false and the next gdk_seat_grab() call
does return GDK_GRAB_NOT_VIEWABLE and gdk_window_is_visible() false
as it should.

So, gtk+ needs to fix some behavior and spice-gtk improve error
handle but till then, let's avoid the regression for users.

This patch basically avoid the changes introduced by following
commits:

 | commit 283f41cd289084689fbdf1151da55aa451ba343c
 | gtk: Use gdk_window_get_device_position
 |
 | commit a718aec66658ba6bb3bb45a9af81a3aa2a652d18
 | widget: Use deprecated function to ungrab device
 |
 | commit 3f4c5bcc88ca5db125ec48ebf696cb23a8e6339a
 | gtk: Avoid deprecated gdk_keyboard_grab
 |
 | commit ef7a6fa1798c8e53c0b77330b398a78181cf90e5
 | gtk: Avoid deprecated gdk_pointer_grab

Related: https://bugzilla.redhat.com/show_bug.cgi?id=1485968
Related: https://bugzilla.redhat.com/show_bug.cgi?id=1571422

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 src/spice-widget.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/spice-widget.c b/src/spice-widget.c
index 72f5334..ec6a197 100644
--- a/src/spice-widget.c
+++ b/src/spice-widget.c
@@ -811,7 +811,7 @@  SpiceGrabSequence *spice_display_get_grab_keys(SpiceDisplay *display)
     return d->grabseq;
 }
 
-#if GTK_CHECK_VERSION(3, 20, 0)
+#if 0 && GTK_CHECK_VERSION(3, 20, 0)
 static GdkSeat *spice_display_get_default_seat(SpiceDisplay *display)
 {
     GdkWindow *window = gtk_widget_get_window(GTK_WIDGET(display));
@@ -867,7 +867,7 @@  static void try_keyboard_grab(SpiceDisplay *display)
                                             GetModuleHandle(NULL), 0);
     g_warn_if_fail(d->keyboard_hook != NULL);
 #endif
-#if GTK_CHECK_VERSION(3, 20, 0)
+#if 0 && GTK_CHECK_VERSION(3, 20, 0)
     status = gdk_seat_grab(spice_display_get_default_seat(display),
                            gtk_widget_get_window(widget),
                            GDK_SEAT_CAPABILITY_KEYBOARD,
@@ -892,7 +892,7 @@  static void try_keyboard_grab(SpiceDisplay *display)
 static void ungrab_keyboard(G_GNUC_UNUSED SpiceDisplay *display)
 {
     G_GNUC_BEGIN_IGNORE_DEPRECATIONS
-#if GTK_CHECK_VERSION(3, 20, 0)
+#if 0 && GTK_CHECK_VERSION(3, 20, 0)
     /* we want to ungrab just the keyboard - it is not possible using gdk_seat_ungrab().
        See also https://bugzilla.gnome.org/show_bug.cgi?id=780133 */
     GdkDevice *keyboard = gdk_seat_get_keyboard(spice_display_get_default_seat(display));
@@ -1042,7 +1042,7 @@  static gboolean do_pointer_grab(SpiceDisplay *display)
 
     try_keyboard_grab(display);
     G_GNUC_BEGIN_IGNORE_DEPRECATIONS
-#if GTK_CHECK_VERSION(3, 20, 0)
+#if 0 && GTK_CHECK_VERSION(3, 20, 0)
     status = gdk_seat_grab(spice_display_get_default_seat(display),
                            window,
                            GDK_SEAT_CAPABILITY_ALL_POINTING,
@@ -1174,7 +1174,7 @@  static void mouse_wrap(SpiceDisplay *display, GdkEventMotion *motion)
 static void ungrab_pointer(G_GNUC_UNUSED SpiceDisplay *display)
 {
     G_GNUC_BEGIN_IGNORE_DEPRECATIONS
-#if GTK_CHECK_VERSION(3, 20, 0)
+#if 0 && GTK_CHECK_VERSION(3, 20, 0)
     /* we want to ungrab just the pointer - it is not possible using gdk_seat_ungrab().
        See also https://bugzilla.gnome.org/show_bug.cgi?id=780133 */
     GdkDevice *pointer = gdk_seat_get_pointer(spice_display_get_default_seat(display));
@@ -2431,7 +2431,7 @@  static GdkDevice *spice_gdk_window_get_pointing_device(GdkWindow *window)
 {
     GdkDisplay *gdk_display = gdk_window_get_display(window);
     G_GNUC_BEGIN_IGNORE_DEPRECATIONS
-#if GTK_CHECK_VERSION(3, 20, 0)
+#if 0 && GTK_CHECK_VERSION(3, 20, 0)
     return gdk_seat_get_pointer(gdk_display_get_default_seat(gdk_display));
 #else
     return gdk_device_manager_get_client_pointer(gdk_display_get_device_manager(gdk_display));

Comments

The intention of those commits was to be ready for new things (wayland,
gtk4... wait, actually gtk3). That side effect was not desired...
I'm not sure whether is better to revert it completely and go back to gtk2
api.

Ack,
Pavel

If there is a bug/regression in gtk+, it'd be good to have reference to it
in the commit ;)

Maybe it's easier for you to come up with a simple gtk reproducer since you
analyzed the issue properly.
If the comment is still valid, pity that the gtk+ bug did not get any
reaction (in the bug report).


2018-05-22 8:42 GMT+02:00 Victor Toso <victortoso@redhat.com>:

> From: Victor Toso <me@victortoso.com>
>
> We are using those APIs to avoid the deprecated ones from GTK but
> unexpected events are handled different by them introducing a
> regression that is easy to reproduce when Spice client holds the
> focus and user locks the screen.
>
> We need to better handle the situation where gdk_seat_grab() fails,
> specifically with GDK_GRAB_ALREADY_GRABBED which is the situation
> for the bugs mentioned below. The grab does not fail with older API
> in the same situation.
>
> Note that gdk_seat_grab() returning GDK_GRAB_ALREADY_GRABBED is due
> the locking screen holding the keyboard device which also means that
> remote-viewer isn't visible/show but gdk_window_is_visible() returns
> true till gdk_seat_grab() is called. The failure in gtk+ function
> call sets the visibility to false and the next gdk_seat_grab() call
> does return GDK_GRAB_NOT_VIEWABLE and gdk_window_is_visible() false
> as it should.
>
> So, gtk+ needs to fix some behavior and spice-gtk improve error
> handle but till then, let's avoid the regression for users.
>
> This patch basically avoid the changes introduced by following
> commits:
>
>  | commit 283f41cd289084689fbdf1151da55aa451ba343c
>  | gtk: Use gdk_window_get_device_position
>  |
>  | commit a718aec66658ba6bb3bb45a9af81a3aa2a652d18
>  | widget: Use deprecated function to ungrab device
>  |
>  | commit 3f4c5bcc88ca5db125ec48ebf696cb23a8e6339a
>  | gtk: Avoid deprecated gdk_keyboard_grab
>  |
>  | commit ef7a6fa1798c8e53c0b77330b398a78181cf90e5
>  | gtk: Avoid deprecated gdk_pointer_grab
>
> Related: https://bugzilla.redhat.com/show_bug.cgi?id=1485968
> Related: https://bugzilla.redhat.com/show_bug.cgi?id=1571422
>
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  src/spice-widget.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/src/spice-widget.c b/src/spice-widget.c
> index 72f5334..ec6a197 100644
> --- a/src/spice-widget.c
> +++ b/src/spice-widget.c
> @@ -811,7 +811,7 @@ SpiceGrabSequence *spice_display_get_grab_keys(SpiceDisplay
> *display)
>      return d->grabseq;
>  }
>
> -#if GTK_CHECK_VERSION(3, 20, 0)
> +#if 0 && GTK_CHECK_VERSION(3, 20, 0)
>  static GdkSeat *spice_display_get_default_seat(SpiceDisplay *display)
>  {
>      GdkWindow *window = gtk_widget_get_window(GTK_WIDGET(display));
> @@ -867,7 +867,7 @@ static void try_keyboard_grab(SpiceDisplay *display)
>                                              GetModuleHandle(NULL), 0);
>      g_warn_if_fail(d->keyboard_hook != NULL);
>  #endif
> -#if GTK_CHECK_VERSION(3, 20, 0)
> +#if 0 && GTK_CHECK_VERSION(3, 20, 0)
>      status = gdk_seat_grab(spice_display_get_default_seat(display),
>                             gtk_widget_get_window(widget),
>                             GDK_SEAT_CAPABILITY_KEYBOARD,
> @@ -892,7 +892,7 @@ static void try_keyboard_grab(SpiceDisplay *display)
>  static void ungrab_keyboard(G_GNUC_UNUSED SpiceDisplay *display)
>  {
>      G_GNUC_BEGIN_IGNORE_DEPRECATIONS
> -#if GTK_CHECK_VERSION(3, 20, 0)
> +#if 0 && GTK_CHECK_VERSION(3, 20, 0)
>      /* we want to ungrab just the keyboard - it is not possible using
> gdk_seat_ungrab().
>         See also https://bugzilla.gnome.org/show_bug.cgi?id=780133 */
>      GdkDevice *keyboard = gdk_seat_get_keyboard(spice_
> display_get_default_seat(display));
> @@ -1042,7 +1042,7 @@ static gboolean do_pointer_grab(SpiceDisplay
> *display)
>
>      try_keyboard_grab(display);
>      G_GNUC_BEGIN_IGNORE_DEPRECATIONS
> -#if GTK_CHECK_VERSION(3, 20, 0)
> +#if 0 && GTK_CHECK_VERSION(3, 20, 0)
>      status = gdk_seat_grab(spice_display_get_default_seat(display),
>                             window,
>                             GDK_SEAT_CAPABILITY_ALL_POINTING,
> @@ -1174,7 +1174,7 @@ static void mouse_wrap(SpiceDisplay *display,
> GdkEventMotion *motion)
>  static void ungrab_pointer(G_GNUC_UNUSED SpiceDisplay *display)
>  {
>      G_GNUC_BEGIN_IGNORE_DEPRECATIONS
> -#if GTK_CHECK_VERSION(3, 20, 0)
> +#if 0 && GTK_CHECK_VERSION(3, 20, 0)
>      /* we want to ungrab just the pointer - it is not possible using
> gdk_seat_ungrab().
>         See also https://bugzilla.gnome.org/show_bug.cgi?id=780133 */
>      GdkDevice *pointer = gdk_seat_get_pointer(spice_
> display_get_default_seat(display));
> @@ -2431,7 +2431,7 @@ static GdkDevice *spice_gdk_window_get_pointing_device(GdkWindow
> *window)
>  {
>      GdkDisplay *gdk_display = gdk_window_get_display(window);
>      G_GNUC_BEGIN_IGNORE_DEPRECATIONS
> -#if GTK_CHECK_VERSION(3, 20, 0)
> +#if 0 && GTK_CHECK_VERSION(3, 20, 0)
>      return gdk_seat_get_pointer(gdk_display_get_default_seat(gdk_
> display));
>  #else
>      return gdk_device_manager_get_client_pointer(gdk_display_get_
> device_manager(gdk_display));
> --
> 2.17.0
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
Hi Pavel,

Thanks for taking time to review this one.

On Tue, May 22, 2018 at 08:44:16PM +0200, Pavel Grunt wrote:
> The intention of those commits was to be ready for new things
> (wayland, gtk4... wait, actually gtk3). That side effect was
> not desired...  I'm not sure whether is better to revert it
> completely and go back to gtk2 api.

The change to get into gtk3 functions is good IMHO as we should
be getting better support for wayland. I can only reproduce this
bug on X11 so there is that :)

> Ack,

Thanks, I don't really plan to apply this patch upstream unless
we are about to do a release. I hope to have time to handle
gdk_seat_grab failures soonish.

> Pavel
>
> If there is a bug/regression in gtk+, it'd be good to have
> reference to it in the commit ;)

I have both bugs in the bottom of the commit log, they are:

Fedora: https://bugzilla.redhat.com/show_bug.cgi?id=1485968
RHEL7: https://bugzilla.redhat.com/show_bug.cgi?id=1571422

> Maybe it's easier for you to come up with a simple gtk
> reproducer since you analyzed the issue properly.

Yes, I should have done that already indeed, thanks for the
suggestion.

> If the comment is still valid, pity that the gtk+ bug did not
> get any reaction (in the bug report).

Indeed.

Cheers,
        toso
>
> 2018-05-22 8:42 GMT+02:00 Victor Toso <victortoso@redhat.com>:
>
> > From: Victor Toso <me@victortoso.com>
> >
> > We are using those APIs to avoid the deprecated ones from GTK but
> > unexpected events are handled different by them introducing a
> > regression that is easy to reproduce when Spice client holds the
> > focus and user locks the screen.
> >
> > We need to better handle the situation where gdk_seat_grab() fails,
> > specifically with GDK_GRAB_ALREADY_GRABBED which is the situation
> > for the bugs mentioned below. The grab does not fail with older API
> > in the same situation.
> >
> > Note that gdk_seat_grab() returning GDK_GRAB_ALREADY_GRABBED is due
> > the locking screen holding the keyboard device which also means that
> > remote-viewer isn't visible/show but gdk_window_is_visible() returns
> > true till gdk_seat_grab() is called. The failure in gtk+ function
> > call sets the visibility to false and the next gdk_seat_grab() call
> > does return GDK_GRAB_NOT_VIEWABLE and gdk_window_is_visible() false
> > as it should.
> >
> > So, gtk+ needs to fix some behavior and spice-gtk improve error
> > handle but till then, let's avoid the regression for users.
> >
> > This patch basically avoid the changes introduced by following
> > commits:
> >
> >  | commit 283f41cd289084689fbdf1151da55aa451ba343c
> >  | gtk: Use gdk_window_get_device_position
> >  |
> >  | commit a718aec66658ba6bb3bb45a9af81a3aa2a652d18
> >  | widget: Use deprecated function to ungrab device
> >  |
> >  | commit 3f4c5bcc88ca5db125ec48ebf696cb23a8e6339a
> >  | gtk: Avoid deprecated gdk_keyboard_grab
> >  |
> >  | commit ef7a6fa1798c8e53c0b77330b398a78181cf90e5
> >  | gtk: Avoid deprecated gdk_pointer_grab
> >
> > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1485968
> > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1571422
> >
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > ---
> >  src/spice-widget.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/spice-widget.c b/src/spice-widget.c
> > index 72f5334..ec6a197 100644
> > --- a/src/spice-widget.c
> > +++ b/src/spice-widget.c
> > @@ -811,7 +811,7 @@ SpiceGrabSequence *spice_display_get_grab_keys(SpiceDisplay
> > *display)
> >      return d->grabseq;
> >  }
> >
> > -#if GTK_CHECK_VERSION(3, 20, 0)
> > +#if 0 && GTK_CHECK_VERSION(3, 20, 0)
> >  static GdkSeat *spice_display_get_default_seat(SpiceDisplay *display)
> >  {
> >      GdkWindow *window = gtk_widget_get_window(GTK_WIDGET(display));
> > @@ -867,7 +867,7 @@ static void try_keyboard_grab(SpiceDisplay *display)
> >                                              GetModuleHandle(NULL), 0);
> >      g_warn_if_fail(d->keyboard_hook != NULL);
> >  #endif
> > -#if GTK_CHECK_VERSION(3, 20, 0)
> > +#if 0 && GTK_CHECK_VERSION(3, 20, 0)
> >      status = gdk_seat_grab(spice_display_get_default_seat(display),
> >                             gtk_widget_get_window(widget),
> >                             GDK_SEAT_CAPABILITY_KEYBOARD,
> > @@ -892,7 +892,7 @@ static void try_keyboard_grab(SpiceDisplay *display)
> >  static void ungrab_keyboard(G_GNUC_UNUSED SpiceDisplay *display)
> >  {
> >      G_GNUC_BEGIN_IGNORE_DEPRECATIONS
> > -#if GTK_CHECK_VERSION(3, 20, 0)
> > +#if 0 && GTK_CHECK_VERSION(3, 20, 0)
> >      /* we want to ungrab just the keyboard - it is not possible using
> > gdk_seat_ungrab().
> >         See also https://bugzilla.gnome.org/show_bug.cgi?id=780133 */
> >      GdkDevice *keyboard = gdk_seat_get_keyboard(spice_
> > display_get_default_seat(display));
> > @@ -1042,7 +1042,7 @@ static gboolean do_pointer_grab(SpiceDisplay
> > *display)
> >
> >      try_keyboard_grab(display);
> >      G_GNUC_BEGIN_IGNORE_DEPRECATIONS
> > -#if GTK_CHECK_VERSION(3, 20, 0)
> > +#if 0 && GTK_CHECK_VERSION(3, 20, 0)
> >      status = gdk_seat_grab(spice_display_get_default_seat(display),
> >                             window,
> >                             GDK_SEAT_CAPABILITY_ALL_POINTING,
> > @@ -1174,7 +1174,7 @@ static void mouse_wrap(SpiceDisplay *display,
> > GdkEventMotion *motion)
> >  static void ungrab_pointer(G_GNUC_UNUSED SpiceDisplay *display)
> >  {
> >      G_GNUC_BEGIN_IGNORE_DEPRECATIONS
> > -#if GTK_CHECK_VERSION(3, 20, 0)
> > +#if 0 && GTK_CHECK_VERSION(3, 20, 0)
> >      /* we want to ungrab just the pointer - it is not possible using
> > gdk_seat_ungrab().
> >         See also https://bugzilla.gnome.org/show_bug.cgi?id=780133 */
> >      GdkDevice *pointer = gdk_seat_get_pointer(spice_
> > display_get_default_seat(display));
> > @@ -2431,7 +2431,7 @@ static GdkDevice *spice_gdk_window_get_pointing_device(GdkWindow
> > *window)
> >  {
> >      GdkDisplay *gdk_display = gdk_window_get_display(window);
> >      G_GNUC_BEGIN_IGNORE_DEPRECATIONS
> > -#if GTK_CHECK_VERSION(3, 20, 0)
> > +#if 0 && GTK_CHECK_VERSION(3, 20, 0)
> >      return gdk_seat_get_pointer(gdk_display_get_default_seat(gdk_
> > display));
> >  #else
> >      return gdk_device_manager_get_client_pointer(gdk_display_get_
> > device_manager(gdk_display));
> > --
> > 2.17.0
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
Hi,

Quick update, this will not be needed anymore, yay.
https://gitlab.gnome.org/GNOME/gtk/merge_requests/166

Upstream bug with lots of information:
https://gitlab.gnome.org/GNOME/gtk/issues/1073

Cheers,
        toso

On Thu, May 24, 2018 at 08:41:02AM +0200, Victor Toso wrote:
> Hi Pavel,
> 
> Thanks for taking time to review this one.
> 
> On Tue, May 22, 2018 at 08:44:16PM +0200, Pavel Grunt wrote:
> > The intention of those commits was to be ready for new things
> > (wayland, gtk4... wait, actually gtk3). That side effect was
> > not desired...  I'm not sure whether is better to revert it
> > completely and go back to gtk2 api.
> 
> The change to get into gtk3 functions is good IMHO as we should
> be getting better support for wayland. I can only reproduce this
> bug on X11 so there is that :)
> 
> > Ack,
> 
> Thanks, I don't really plan to apply this patch upstream unless
> we are about to do a release. I hope to have time to handle
> gdk_seat_grab failures soonish.
> 
> > Pavel
> >
> > If there is a bug/regression in gtk+, it'd be good to have
> > reference to it in the commit ;)
> 
> I have both bugs in the bottom of the commit log, they are:
> 
> Fedora: https://bugzilla.redhat.com/show_bug.cgi?id=1485968
> RHEL7: https://bugzilla.redhat.com/show_bug.cgi?id=1571422
> 
> > Maybe it's easier for you to come up with a simple gtk
> > reproducer since you analyzed the issue properly.
> 
> Yes, I should have done that already indeed, thanks for the
> suggestion.
> 
> > If the comment is still valid, pity that the gtk+ bug did not
> > get any reaction (in the bug report).
> 
> Indeed.
> 
> Cheers,
>         toso
> >
> > 2018-05-22 8:42 GMT+02:00 Victor Toso <victortoso@redhat.com>:
> >
> > > From: Victor Toso <me@victortoso.com>
> > >
> > > We are using those APIs to avoid the deprecated ones from GTK but
> > > unexpected events are handled different by them introducing a
> > > regression that is easy to reproduce when Spice client holds the
> > > focus and user locks the screen.
> > >
> > > We need to better handle the situation where gdk_seat_grab() fails,
> > > specifically with GDK_GRAB_ALREADY_GRABBED which is the situation
> > > for the bugs mentioned below. The grab does not fail with older API
> > > in the same situation.
> > >
> > > Note that gdk_seat_grab() returning GDK_GRAB_ALREADY_GRABBED is due
> > > the locking screen holding the keyboard device which also means that
> > > remote-viewer isn't visible/show but gdk_window_is_visible() returns
> > > true till gdk_seat_grab() is called. The failure in gtk+ function
> > > call sets the visibility to false and the next gdk_seat_grab() call
> > > does return GDK_GRAB_NOT_VIEWABLE and gdk_window_is_visible() false
> > > as it should.
> > >
> > > So, gtk+ needs to fix some behavior and spice-gtk improve error
> > > handle but till then, let's avoid the regression for users.
> > >
> > > This patch basically avoid the changes introduced by following
> > > commits:
> > >
> > >  | commit 283f41cd289084689fbdf1151da55aa451ba343c
> > >  | gtk: Use gdk_window_get_device_position
> > >  |
> > >  | commit a718aec66658ba6bb3bb45a9af81a3aa2a652d18
> > >  | widget: Use deprecated function to ungrab device
> > >  |
> > >  | commit 3f4c5bcc88ca5db125ec48ebf696cb23a8e6339a
> > >  | gtk: Avoid deprecated gdk_keyboard_grab
> > >  |
> > >  | commit ef7a6fa1798c8e53c0b77330b398a78181cf90e5
> > >  | gtk: Avoid deprecated gdk_pointer_grab
> > >
> > > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1485968
> > > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1571422
> > >
> > > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > > ---
> > >  src/spice-widget.c | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/src/spice-widget.c b/src/spice-widget.c
> > > index 72f5334..ec6a197 100644
> > > --- a/src/spice-widget.c
> > > +++ b/src/spice-widget.c
> > > @@ -811,7 +811,7 @@ SpiceGrabSequence *spice_display_get_grab_keys(SpiceDisplay
> > > *display)
> > >      return d->grabseq;
> > >  }
> > >
> > > -#if GTK_CHECK_VERSION(3, 20, 0)
> > > +#if 0 && GTK_CHECK_VERSION(3, 20, 0)
> > >  static GdkSeat *spice_display_get_default_seat(SpiceDisplay *display)
> > >  {
> > >      GdkWindow *window = gtk_widget_get_window(GTK_WIDGET(display));
> > > @@ -867,7 +867,7 @@ static void try_keyboard_grab(SpiceDisplay *display)
> > >                                              GetModuleHandle(NULL), 0);
> > >      g_warn_if_fail(d->keyboard_hook != NULL);
> > >  #endif
> > > -#if GTK_CHECK_VERSION(3, 20, 0)
> > > +#if 0 && GTK_CHECK_VERSION(3, 20, 0)
> > >      status = gdk_seat_grab(spice_display_get_default_seat(display),
> > >                             gtk_widget_get_window(widget),
> > >                             GDK_SEAT_CAPABILITY_KEYBOARD,
> > > @@ -892,7 +892,7 @@ static void try_keyboard_grab(SpiceDisplay *display)
> > >  static void ungrab_keyboard(G_GNUC_UNUSED SpiceDisplay *display)
> > >  {
> > >      G_GNUC_BEGIN_IGNORE_DEPRECATIONS
> > > -#if GTK_CHECK_VERSION(3, 20, 0)
> > > +#if 0 && GTK_CHECK_VERSION(3, 20, 0)
> > >      /* we want to ungrab just the keyboard - it is not possible using
> > > gdk_seat_ungrab().
> > >         See also https://bugzilla.gnome.org/show_bug.cgi?id=780133 */
> > >      GdkDevice *keyboard = gdk_seat_get_keyboard(spice_
> > > display_get_default_seat(display));
> > > @@ -1042,7 +1042,7 @@ static gboolean do_pointer_grab(SpiceDisplay
> > > *display)
> > >
> > >      try_keyboard_grab(display);
> > >      G_GNUC_BEGIN_IGNORE_DEPRECATIONS
> > > -#if GTK_CHECK_VERSION(3, 20, 0)
> > > +#if 0 && GTK_CHECK_VERSION(3, 20, 0)
> > >      status = gdk_seat_grab(spice_display_get_default_seat(display),
> > >                             window,
> > >                             GDK_SEAT_CAPABILITY_ALL_POINTING,
> > > @@ -1174,7 +1174,7 @@ static void mouse_wrap(SpiceDisplay *display,
> > > GdkEventMotion *motion)
> > >  static void ungrab_pointer(G_GNUC_UNUSED SpiceDisplay *display)
> > >  {
> > >      G_GNUC_BEGIN_IGNORE_DEPRECATIONS
> > > -#if GTK_CHECK_VERSION(3, 20, 0)
> > > +#if 0 && GTK_CHECK_VERSION(3, 20, 0)
> > >      /* we want to ungrab just the pointer - it is not possible using
> > > gdk_seat_ungrab().
> > >         See also https://bugzilla.gnome.org/show_bug.cgi?id=780133 */
> > >      GdkDevice *pointer = gdk_seat_get_pointer(spice_
> > > display_get_default_seat(display));
> > > @@ -2431,7 +2431,7 @@ static GdkDevice *spice_gdk_window_get_pointing_device(GdkWindow
> > > *window)
> > >  {
> > >      GdkDisplay *gdk_display = gdk_window_get_display(window);
> > >      G_GNUC_BEGIN_IGNORE_DEPRECATIONS
> > > -#if GTK_CHECK_VERSION(3, 20, 0)
> > > +#if 0 && GTK_CHECK_VERSION(3, 20, 0)
> > >      return gdk_seat_get_pointer(gdk_display_get_default_seat(gdk_
> > > display));
> > >  #else
> > >      return gdk_device_manager_get_client_pointer(gdk_display_get_
> > > device_manager(gdk_display));
> > > --
> > > 2.17.0
> > >
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > >



> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Fri, May 25, 2018 at 04:00:43PM +0200, Victor Toso wrote:
> Hi,
> 
> Quick update, this will not be needed anymore, yay.
> https://gitlab.gnome.org/GNOME/gtk/merge_requests/166
> 
> Upstream bug with lots of information:
> https://gitlab.gnome.org/GNOME/gtk/issues/1073

Ah great to read!

Christophe

> 
> Cheers,
>         toso
> 
> On Thu, May 24, 2018 at 08:41:02AM +0200, Victor Toso wrote:
> > Hi Pavel,
> > 
> > Thanks for taking time to review this one.
> > 
> > On Tue, May 22, 2018 at 08:44:16PM +0200, Pavel Grunt wrote:
> > > The intention of those commits was to be ready for new things
> > > (wayland, gtk4... wait, actually gtk3). That side effect was
> > > not desired...  I'm not sure whether is better to revert it
> > > completely and go back to gtk2 api.
> > 
> > The change to get into gtk3 functions is good IMHO as we should
> > be getting better support for wayland. I can only reproduce this
> > bug on X11 so there is that :)
> > 
> > > Ack,
> > 
> > Thanks, I don't really plan to apply this patch upstream unless
> > we are about to do a release. I hope to have time to handle
> > gdk_seat_grab failures soonish.
> > 
> > > Pavel
> > >
> > > If there is a bug/regression in gtk+, it'd be good to have
> > > reference to it in the commit ;)
> > 
> > I have both bugs in the bottom of the commit log, they are:
> > 
> > Fedora: https://bugzilla.redhat.com/show_bug.cgi?id=1485968
> > RHEL7: https://bugzilla.redhat.com/show_bug.cgi?id=1571422
> > 
> > > Maybe it's easier for you to come up with a simple gtk
> > > reproducer since you analyzed the issue properly.
> > 
> > Yes, I should have done that already indeed, thanks for the
> > suggestion.
> > 
> > > If the comment is still valid, pity that the gtk+ bug did not
> > > get any reaction (in the bug report).
> > 
> > Indeed.
> > 
> > Cheers,
> >         toso
> > >
> > > 2018-05-22 8:42 GMT+02:00 Victor Toso <victortoso@redhat.com>:
> > >
> > > > From: Victor Toso <me@victortoso.com>
> > > >
> > > > We are using those APIs to avoid the deprecated ones from GTK but
> > > > unexpected events are handled different by them introducing a
> > > > regression that is easy to reproduce when Spice client holds the
> > > > focus and user locks the screen.
> > > >
> > > > We need to better handle the situation where gdk_seat_grab() fails,
> > > > specifically with GDK_GRAB_ALREADY_GRABBED which is the situation
> > > > for the bugs mentioned below. The grab does not fail with older API
> > > > in the same situation.
> > > >
> > > > Note that gdk_seat_grab() returning GDK_GRAB_ALREADY_GRABBED is due
> > > > the locking screen holding the keyboard device which also means that
> > > > remote-viewer isn't visible/show but gdk_window_is_visible() returns
> > > > true till gdk_seat_grab() is called. The failure in gtk+ function
> > > > call sets the visibility to false and the next gdk_seat_grab() call
> > > > does return GDK_GRAB_NOT_VIEWABLE and gdk_window_is_visible() false
> > > > as it should.
> > > >
> > > > So, gtk+ needs to fix some behavior and spice-gtk improve error
> > > > handle but till then, let's avoid the regression for users.
> > > >
> > > > This patch basically avoid the changes introduced by following
> > > > commits:
> > > >
> > > >  | commit 283f41cd289084689fbdf1151da55aa451ba343c
> > > >  | gtk: Use gdk_window_get_device_position
> > > >  |
> > > >  | commit a718aec66658ba6bb3bb45a9af81a3aa2a652d18
> > > >  | widget: Use deprecated function to ungrab device
> > > >  |
> > > >  | commit 3f4c5bcc88ca5db125ec48ebf696cb23a8e6339a
> > > >  | gtk: Avoid deprecated gdk_keyboard_grab
> > > >  |
> > > >  | commit ef7a6fa1798c8e53c0b77330b398a78181cf90e5
> > > >  | gtk: Avoid deprecated gdk_pointer_grab
> > > >
> > > > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1485968
> > > > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1571422
> > > >
> > > > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > > > ---
> > > >  src/spice-widget.c | 12 ++++++------
> > > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/src/spice-widget.c b/src/spice-widget.c
> > > > index 72f5334..ec6a197 100644
> > > > --- a/src/spice-widget.c
> > > > +++ b/src/spice-widget.c
> > > > @@ -811,7 +811,7 @@ SpiceGrabSequence *spice_display_get_grab_keys(SpiceDisplay
> > > > *display)
> > > >      return d->grabseq;
> > > >  }
> > > >
> > > > -#if GTK_CHECK_VERSION(3, 20, 0)
> > > > +#if 0 && GTK_CHECK_VERSION(3, 20, 0)
> > > >  static GdkSeat *spice_display_get_default_seat(SpiceDisplay *display)
> > > >  {
> > > >      GdkWindow *window = gtk_widget_get_window(GTK_WIDGET(display));
> > > > @@ -867,7 +867,7 @@ static void try_keyboard_grab(SpiceDisplay *display)
> > > >                                              GetModuleHandle(NULL), 0);
> > > >      g_warn_if_fail(d->keyboard_hook != NULL);
> > > >  #endif
> > > > -#if GTK_CHECK_VERSION(3, 20, 0)
> > > > +#if 0 && GTK_CHECK_VERSION(3, 20, 0)
> > > >      status = gdk_seat_grab(spice_display_get_default_seat(display),
> > > >                             gtk_widget_get_window(widget),
> > > >                             GDK_SEAT_CAPABILITY_KEYBOARD,
> > > > @@ -892,7 +892,7 @@ static void try_keyboard_grab(SpiceDisplay *display)
> > > >  static void ungrab_keyboard(G_GNUC_UNUSED SpiceDisplay *display)
> > > >  {
> > > >      G_GNUC_BEGIN_IGNORE_DEPRECATIONS
> > > > -#if GTK_CHECK_VERSION(3, 20, 0)
> > > > +#if 0 && GTK_CHECK_VERSION(3, 20, 0)
> > > >      /* we want to ungrab just the keyboard - it is not possible using
> > > > gdk_seat_ungrab().
> > > >         See also https://bugzilla.gnome.org/show_bug.cgi?id=780133 */
> > > >      GdkDevice *keyboard = gdk_seat_get_keyboard(spice_
> > > > display_get_default_seat(display));
> > > > @@ -1042,7 +1042,7 @@ static gboolean do_pointer_grab(SpiceDisplay
> > > > *display)
> > > >
> > > >      try_keyboard_grab(display);
> > > >      G_GNUC_BEGIN_IGNORE_DEPRECATIONS
> > > > -#if GTK_CHECK_VERSION(3, 20, 0)
> > > > +#if 0 && GTK_CHECK_VERSION(3, 20, 0)
> > > >      status = gdk_seat_grab(spice_display_get_default_seat(display),
> > > >                             window,
> > > >                             GDK_SEAT_CAPABILITY_ALL_POINTING,
> > > > @@ -1174,7 +1174,7 @@ static void mouse_wrap(SpiceDisplay *display,
> > > > GdkEventMotion *motion)
> > > >  static void ungrab_pointer(G_GNUC_UNUSED SpiceDisplay *display)
> > > >  {
> > > >      G_GNUC_BEGIN_IGNORE_DEPRECATIONS
> > > > -#if GTK_CHECK_VERSION(3, 20, 0)
> > > > +#if 0 && GTK_CHECK_VERSION(3, 20, 0)
> > > >      /* we want to ungrab just the pointer - it is not possible using
> > > > gdk_seat_ungrab().
> > > >         See also https://bugzilla.gnome.org/show_bug.cgi?id=780133 */
> > > >      GdkDevice *pointer = gdk_seat_get_pointer(spice_
> > > > display_get_default_seat(display));
> > > > @@ -2431,7 +2431,7 @@ static GdkDevice *spice_gdk_window_get_pointing_device(GdkWindow
> > > > *window)
> > > >  {
> > > >      GdkDisplay *gdk_display = gdk_window_get_display(window);
> > > >      G_GNUC_BEGIN_IGNORE_DEPRECATIONS
> > > > -#if GTK_CHECK_VERSION(3, 20, 0)
> > > > +#if 0 && GTK_CHECK_VERSION(3, 20, 0)
> > > >      return gdk_seat_get_pointer(gdk_display_get_default_seat(gdk_
> > > > display));
> > > >  #else
> > > >      return gdk_device_manager_get_client_pointer(gdk_display_get_
> > > > device_manager(gdk_display));
> > > > --
> > > > 2.17.0
> > > >
> > > > _______________________________________________
> > > > Spice-devel mailing list
> > > > Spice-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > > >
> 
> 
> 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 



> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel