[Spice-devel] [PATCH] Grab keyboard based on focus in windows client

[Victor Toso]

Hey,

On Mon, Jun 05, 2017 at 04:00:32PM +0300, Snir Sheriber wrote:
> Hi,
> 
> 
> On 06/05/2017 02:11 PM, Victor Toso wrote:
> > Hi,
> > 
> > Small note, I would for 72 of length in commit message. Seems to be 55.
> > 
> > On Thu, Jun 01, 2017 at 06:44:18PM +0300, Snir Sheriber wrote:
> > > Currently the client grabs keyboard based on session
> > > focus, on windows client it generates grab_broken
> > > event.
> > > This patch expands the solution presented in 143ebfd
> > > to work also on windows client without causing
> > > grab_broken event.
> > > This is implemented a bit differently from linux, if
> > > on mouse entrance session already has focus, focus
> > > will be set to the current window, this will generate
> > > focus events that will release and grab the focus again
> > > without grab_broken event.
> > I would also add the note you did in the cover-letter about it:
> > 
> >   | [2-more-info] The issues is that on windows gtk will generate grab
> >   | broken if wm_killfocus received (when focus is changed) while the
> >   | application has the grab. which it means that if we have the grab on
> >   | hover without getting the focus first (i.e by clicking) it will
> >   | cause grab_broken which follows by no grab at all, gtk does this on
> >   | purpose, i'm not sure why it is good for.
> > 
> > Btw, it might be good to file a bug against gtk about this, no?
> 
> Yes, maybe, i guess sometimes this grab_broken is needed, just not sure
> when..
> 
> > 
> > > Resolves: rhbz#1429611
> > > Related: rhbz#1275231
> > > ---
> > >   src/spice-widget.c | 17 ++++++++++++++++-
> > >   1 file changed, 16 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/spice-widget.c b/src/spice-widget.c
> > > index 1a1d5a6..b3407b5 100644
> > > --- a/src/spice-widget.c
> > > +++ b/src/spice-widget.c
> > > @@ -850,10 +850,17 @@ static void try_keyboard_grab(SpiceDisplay *display)
> > >           return;
> > >       if (d->keyboard_grab_active)
> > >           return;
> > > +#ifdef G_OS_WIN32
> > > +    if (!d->keyboard_have_focus)
> > > +        return;
> > > +    if (!d->mouse_have_pointer)
> > > +        return;
> > > +#else
> > >       if (!spice_gtk_session_get_keyboard_has_focus(d->gtk_session))
> > >           return;
> > >       if (!spice_gtk_session_get_mouse_has_pointer(d->gtk_session))
> > >           return;
> > > +#endif
> > >       if (d->keyboard_grab_released)
> > >           return;
> > > 
> > > @@ -1543,6 +1550,12 @@ static void update_display(SpiceDisplay *display)
> > >       win32_window = display ?
> > >                           gdk_win32_window_get_impl_hwnd(gtk_widget_get_window(GTK_WIDGET(display))) :
> > >                           NULL;
> > > +    if(win32_window) {
> > > +        SpiceDisplayPrivate *d = display->priv;
> > > +        if(spice_gtk_session_get_keyboard_has_focus(d->gtk_session) &&
> > > +           spice_gtk_session_get_mouse_has_pointer(d->gtk_session))
> > > +            SetFocus(win32_window);
> > > +    }
> > Hmm, I put a SPICE_DEBUG() after SetFocus() but the debug messages was
> > never printed. Windows 7.
> >
> > That makes me think we don't really need to set the focus or it might
> > depend on gtk/windows version?
>
> That's weird, works on my win7 client, if it doesn't set the focus this
> solution shouldn't work at all.
> See if mouse hovering between windows changes the focus.

Oh, my bad. SPICE_DEBUG() is at fault. Changing to fprintf(stderr,...)
works;

With the extra bit in commit log,
Acked-by: Victor Toso <victortoso at redhat.com>

>
> >
> > >   #endif
> > >   }
> > > 
> > > @@ -1862,7 +1875,6 @@ static gboolean focus_in_event(GtkWidget *widget, GdkEventFocus *focus G_GNUC_UN
> > >   static gboolean focus_out_event(GtkWidget *widget, GdkEventFocus *focus G_GNUC_UNUSED)
> > >   {
> > >       SpiceDisplay *display = SPICE_DISPLAY(widget);
> > > -    SpiceDisplayPrivate *d = display->priv;
> > > 
> > >       DISPLAY_DEBUG(display, "%s", __FUNCTION__);
> > >       update_display(NULL);
> > > @@ -1871,8 +1883,11 @@ static gboolean focus_out_event(GtkWidget *widget, GdkEventFocus *focus G_GNUC_U
> > >        * Ignore focus out after a keyboard grab
> > >        * (this happens when doing the grab from the enter_event callback).
> > >        */
> > > +#ifndef G_OS_WIN32
> > > +    SpiceDisplayPrivate *d = display->priv;
> > >       if (d->keyboard_grab_active)
> > >           return true;
> > > +#endif
> > Your solution works fine.
> > 
> > Reviewed-by: Victor Toso <victortoso at redhat.com>
> 
> Thanks for your review :)
> 
> > 
> > >       release_keys(display);
> > >       update_keyboard_focus(display, false);
> > > -- 
> > > 2.9.3
> > > 
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170605/c69dae9f/attachment.sig>