[Spice-devel] [spice-gtk v4 2/2] gtk-session: clipboard: x11: owner-change improvement

[Victor Toso]

Hi,

On Mon, Jan 14, 2019 at 04:40:35PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Jan 14, 2019 at 4:34 PM Victor Toso <victortoso at redhat.com> wrote:
> >
> > From: Victor Toso <me at victortoso.com>
> >
> > On X11, the release-grab message might end up clearing the
> > GtkClipboard which triggers the owner-changed callback having the
> > event owner as NULL.
> >
> > We should not be calling gtk_clipboard_request_targets() in this
> > situation as is prone to errors as the intention here is request
> > clipboard information from changes made by client OS.
> >
> > The fix is to avoid any such request while spice client is holding the
> > keyboard grab.
> 
> Two things that bug me about this:
> - it's x11 specific, weird

You mean that GtkClipboard event's should behave the same way in
Wayland?

> - the condition seems wrong: if an application has the keyboard grab,
> that doesn't mean that another cannot update the clipboard. For
> example, I suppose this can easily happen with multiple
> pointers/inputs or some automation.

Yes but, do we wan't to support that? This is application-level
tweak on clipboard behavior so I don't think controlling it a bit
is bad if we agree that the rationale behind it is correct.

The other way to fix this is likely a major design in the code to
not touch the clipboard on grab/release (from guest messages)
while we have the focus. IMHO, also easy to get wrong.

> Other than that, if it fixes or workaround real clipboard
> issues, I am not against it, but I think we should add more
> comments about the "hack".

Calling it a hack is the same as saying you have a problem with
the rationale behind it, no?

I don't mind calling it a hack but I'm in favor of not having a
grab send by client to the guest by 3rd application running on
client OS *while* spice-gtk is holding the focus. Keeping the
behavior is bad, IMHO.

Not sure what you mean with multiple inputs. Are we targeting
something else other than Desktop clients?

> > Related: https://gitlab.freedesktop.org/spice/win32/vd_agent/issues/6
> > Related: https://gitlab.freedesktop.org/spice/linux/vd_agent/issues/9
> > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
> >
> > Changed in v4:
> > - Updated commit log
> >
> > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > Tested-by: James Harvey @jamespharvey20
> > ---
> >  src/spice-gtk-session.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > index abce43f..20df70d 100644
> > --- a/src/spice-gtk-session.c
> > +++ b/src/spice-gtk-session.c
> > @@ -674,6 +674,19 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
> >          return;
> >      }
> >
> > +#ifdef GDK_WINDOWING_X11
> > +    /* In X11, while holding the keyboard-grab we are not interested in this
> > +     * event as it either came by grab or release messages from agent.  */

Should I add:

    +   /* TODO: X11 clipboard specifics should be removed in the future */


Cheers,

> > +    if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
> > +        spice_gtk_session_get_keyboard_has_focus(self)) {
> > +        SPICE_DEBUG("clipboard: owner-changed event: not requesting client's target "
> > +                    "while holding focus");
> > +        return;
> > +    }
> > +#endif
> > +    SPICE_DEBUG("clipboard: owner-changed event: has-focus=%d",
> > +                spice_gtk_session_get_keyboard_has_focus(self));
> > +
> >      s->clipboard_by_guest[selection] = FALSE;
> >      s->clip_hasdata[selection] = TRUE;
> >      if (s->auto_clipboard_enable && !read_only(self))
> > --
> > 2.20.1
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> 
> 
> --
> Marc-André Lureau
-------------- 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/20190114/68868068/attachment.sig>