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

[Snir Sheriber]

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.

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