[Spice-devel] [spice-gtk v3 1/3] gtk-session: clipboard: document owner-changed event

[Jakub Janku]

Hi,

On Mon, Jan 14, 2019 at 10:34 AM Victor Toso <victortoso at redhat.com> wrote:
>
> Hi,
>
> On Sun, Jan 13, 2019 at 09:59:11PM +0100, Jakub Janku wrote:
> > Hi,
> >
> > I see that I'm being late to the party as this has already been
> > pushed upstream, but I'd like to comment anyway:
> >
> > On Thu, Jan 10, 2019 at 1:47 PM Victor Toso <victortoso at redhat.com> wrote:
> > >
> > > From: Victor Toso <me at victortoso.com>
> > >
> > > This patch improves the code style by adding braces, removing the
> > > single case switch and add documentation to the handler of
> > > GtkClipboard::owner-changed.
> > >
> > > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > > ---
> > >  src/spice-gtk-session.c | 54 +++++++++++++++++++++++++++++------------
> > >  1 file changed, 39 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > > index 1ccae07..adc72a2 100644
> > > --- a/src/spice-gtk-session.c
> > > +++ b/src/spice-gtk-session.c
> > > @@ -618,6 +618,23 @@ static void clipboard_get_targets(GtkClipboard *clipboard,
> > >      s->nclip_targets[selection] = 0;
> > >  }
> > >
> > > +/* Callback for every owner-change event for given @clipboard.
> > > + * This event is triggered in different ways depending on the environment of
> > > + * the Client, some examples:
> > > + *
> > > + * Situation 1: When another application on the client machine is holding and
> > > + * changing the clipboard. If client is on Wayland, spice-gtk only receives the
> > > + * related GtkClipboard::owner-changed event after focus-in event on Spice
> > > + * widget; On X11, we will receive it at the moment the clipboard data has been
> > > + * changed in by other application.
> > > + *
> > > + * Situation 2: When spice-gtk holds the focus and is changing the clipboard by
> > > + * either setting new content information with gtk_clipboard_set_with_owner() or
> > > + * clearing up old content with gtk_clipboard_clear(). The main difference between
> > > + * Wayland and X11 is that on X11, gtk_clipboard_clear() set the owner to none, which
> >
> > "set" -> "sets" ?
>
> I think you are right :)
>
> > > + * emits owner-change event; On Wayland that does not happen as spice-gtk still is
> > > + * the owner of the clipboard.
> >
> > You must have come across this bug, since you commented on it:
> > https://gitlab.gnome.org/GNOME/gtk/issues/715
> >
> > I've created a simple test case which is attached.
> > It seems like no "owner-change" event is emitted after
> > gtk_clipboard_clear() is called,
> > but after gtk_clipboard_set_with_owner() the event is emitted twice.
> >
> > So the Wayland behavior you described here might just be caused by the bug...
>
> Could be.
>
> But just to be clear, what I tried to document was _when_ the
> callback is called. I just tried the test case under X11 and
> Wayland and the same thing happens as expected: On X11, the
> callback is called when the clipboard has changed by another
> application; On Wayland, the callback is called if the clipboard
> has changed and when we have the focus.

That's of course true.
Just though it might be worth mentioning.
>
> > > + */
> > >  static void clipboard_owner_change(GtkClipboard        *clipboard,
> > >                                     GdkEventOwnerChange *event,
> > >                                     gpointer            user_data)
> > > @@ -631,30 +648,37 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
> > >      selection = get_selection_from_clipboard(s, clipboard);
> > >      g_return_if_fail(selection != -1);
> > >
> > > -    if (s->main == NULL)
> > > +    if (s->main == NULL) {
> > >          return;
> > > +    }
> > >
> > > +    /* In case we sent a grab to the agent, we need to release it now as
> > > +     * previous clipboard data should not be reachable anymore */
> > >      if (s->clip_grabbed[selection]) {
> > >          s->clip_grabbed[selection] = FALSE;
> > > -        if (spice_main_channel_agent_test_capability(s->main, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
> > > +        if (spice_main_channel_agent_test_capability(s->main, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND)) {
> > >              spice_main_channel_clipboard_selection_release(s->main, selection);
> > > +        }
> > >      }
> > >
> > > -    switch (event->reason) {
> > > -    case GDK_OWNER_CHANGE_NEW_OWNER:
> > > -        if (gtk_clipboard_get_owner(clipboard) == G_OBJECT(self))
> > > -            break;
> > > -
> > > -        s->clipboard_by_guest[selection] = FALSE;
> > > -        s->clip_hasdata[selection] = TRUE;
> > > -        if (s->auto_clipboard_enable && !read_only(self))
> > > -            gtk_clipboard_request_targets(clipboard, clipboard_get_targets,
> > > -                                          get_weak_ref(self));
> > > -        break;
> > > -    default:
> > > +    /* We are mostly interested when owner has changed in which case
> > > +     * we would like to let agent know about new clipboard data. */
> > > +    if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER) {
> > >          s->clip_hasdata[selection] = FALSE;
> > > -        break;
> > > +        return;
> > >      }
> > > +
> > > +    /* This situation happens when clipboard is being cleared by us, when agent
> > > +     * sends a release-grab for instance */
> >
> > This is not true imho. It should only happen after a grab (after
> > gtk_clipboard_set_with_owner() is called).
> > After a release (gtk_clipboard_clear() call),
> > gtk_clipboard_get_owner(clipboard) should already return NULL.
>
> The comment refers to the gtk_clipboard_clear() not owner-changed
> event so you are right, of course. This comment was probably a
> left over from different interactions that I did in this code :(
>
> I'll send a fix for the documentation.
>
> For the other changes, I'd love to hear your opinion too. Did not
> get 2/3 and 3/3 patches acked but more keen to discuss the idea.
>
> I was wrong in regards to clipboard managers on Wayland, that is,
> clipboard managers on Wayland *can* get clipboard data of
> spice-gtk while spice-gtk's widgets are holding the focus :(

That's weird. I didn't think this was possible.
I've just had a very brief look at the code of GPaste:

https://github.com/Keruspe/GPaste/blob/master/src/daemon/gpaste-daemon.c

the main function includes the following lines:
    |    /* FIXME: remove this once gtk supports clipboard correctly
on wayland */
    |    gdk_set_allowed_backends ("x11");
This probably indicates that the clipboard manager actually runs on
XWayland, which would explain why it's able to eavesdrop on the
clipboard without having focus.
It is the same "workaround" that we use in vdagent.
>
> I didn't check how that happen but I'd rather avoid/deny that to
> happen..
>
> Thanks again Jakub,
>
> > > +    if (gtk_clipboard_get_owner(clipboard) == G_OBJECT(self)) {
> > > +        return;
> > > +    }
> > > +
> > > +    s->clipboard_by_guest[selection] = FALSE;
> > > +    s->clip_hasdata[selection] = TRUE;
> > > +    if (s->auto_clipboard_enable && !read_only(self))
> > > +        gtk_clipboard_request_targets(clipboard, clipboard_get_targets,
> > > +                                      get_weak_ref(self));
> > >  }
> > >
> > >  typedef struct
> > > --
> > > 2.20.1
> > >
> >
> > Cheers,
> > Jakub
>
>