[Spice-devel] [PATCH linux/vd-agent 10/11] clipboard: only send release when no immediate grab

[Marc-André Lureau]

Hi

On Fri, Mar 29, 2019 at 10:17 AM Frediano Ziglio <fziglio at redhat.com> wrote:
>
> >
> > Hi
> >
> > On Thu, Mar 28, 2019 at 5:30 PM Frediano Ziglio <fziglio at redhat.com> wrote:
> > >
> > > >
> > > > From: Marc-André Lureau <marcandre.lureau at redhat.com>
> > > >
> > > > Do not send a release event between two grabs, this helps with window
> > > > manager interaction issues on peer side.
> > > >
> > >
> > > I would explain which kind of issue this is supposed to fix.
> >
> > They react to clipboard events in different ways. The point is to
> > minimize the amount of events to avoid extra unnecessary interactions.
> >
> > In particular, on release event, some clipboard managers take the
> > grab. This creates a race with Spice which does a release between
> > grabs currently.
> >
>
> As clipboard managers are not using SPICE I suppose here you
> are talking about desktop clipboard release, not agent RELEASE
> message. But on the previous sentence we were speaking about
> agent RELEASE.

Yes, when a peer receives RELEASE, it will effectively release the
grab on its desktop, and this may make the desktop clipboard agent
react.

>
> > >
> > > > Advertise this behaviour via a capability introduced in spice-protocol
> > > > 0.12.16, so the client doesn't need to do some time-based filtering.
> > > >
> > > > (the capability shouldn't need to be negotiated, a client shouldn't
> > > > expect a release between two grabs)
> > > >
> > > > Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> > >
> > > I suppose Jakub/Victor could do a much better review/test than me.
> > >
> > > > ---
> > > >  src/vdagent/clipboard.c | 12 ++++++------
> > > >  src/vdagent/x11.c       |  7 +++----
> > > >  src/vdagentd/vdagentd.c |  1 +
> > > >  3 files changed, 10 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/src/vdagent/clipboard.c b/src/vdagent/clipboard.c
> > > > index 9fb87e3..097c6ee 100644
> > > > --- a/src/vdagent/clipboard.c
> > > > +++ b/src/vdagent/clipboard.c
> > > > @@ -242,13 +242,13 @@ static void clipboard_owner_change_cb(GtkClipboard
> > > > *clipboard,
> > > >          return;
> > > >      }
> > > >
> > > > -    if (sel->owner == OWNER_GUEST) {
> > > > -        clipboard_new_owner(c, sel_id, OWNER_NONE);
> > > > -        udscs_write(c->conn, VDAGENTD_CLIPBOARD_RELEASE, sel_id, 0,
> > > > NULL,
> > > > 0);
> > > > -    }
> > > > -
> > > > -    if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER)
> > > > +    if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER) {
> > > > +        if (sel->owner == OWNER_GUEST) {
> > > > +            clipboard_new_owner(c, sel_id, OWNER_NONE);
> > > > +            udscs_write(c->conn, VDAGENTD_CLIPBOARD_RELEASE, sel_id, 0,
> > > > NULL, 0);
> > > > +        }
> > > >          return;
> > > > +    }
> > > >
> > > >      /* if there's a pending request for clipboard targets, cancel it */
> > > >      if (sel->last_targets_req)
> > > > diff --git a/src/vdagent/x11.c b/src/vdagent/x11.c
> > > > index c2515a8..9409b53 100644
> > > > --- a/src/vdagent/x11.c
> > > > +++ b/src/vdagent/x11.c
> > > > @@ -530,11 +530,10 @@ static void vdagent_x11_handle_event(struct
> > > > vdagent_x11
> > > > *x11, XEvent event)
> > > >          if (ev.xfev.owner == x11->selection_window)
> > > >              return;
> > > >
> > > > -        /* If the clipboard owner is changed we no longer own it */
> > >
> > > Why not keeping this comment?
> >
> > The comment is no longer valid with this change: if the clipboard
> > owner is changed, the agent may still own the grab (at the protocol
> > level).
> >
>
> Yes, this prove what I was saying in the previous e-mail, you are
> mixing desktop ownership and SPICE grab, if you consider from
> desktop prospective (which is using owner definition like in
> "ev.xfev.owner") the comment is still valid, but you are reading
> it with different definitions so it became invalid.
>
> > As you can see with the following line being removed:
> > >
> > > > -        vdagent_x11_set_clipboard_owner(x11, selection, owner_none);
> > > > -
> > > > -        if (ev.xfev.owner == None)
> > > > +        if (ev.xfev.owner == None) {
> > > > +            vdagent_x11_set_clipboard_owner(x11, selection, owner_none);
> > > >              return;
> > > > +        }
> > > >
> > > >          /* Request the supported targets from the new owner */
> > > >          XConvertSelection(x11->display, ev.xfev.selection,
> > > >          x11->targets_atom,
> > > > diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> > > > index 72a3e13..683e5d3 100644
> > > > --- a/src/vdagentd/vdagentd.c
> > > > +++ b/src/vdagentd/vdagentd.c
> > > > @@ -133,6 +133,7 @@ static void send_capabilities(struct
> > > > vdagent_virtio_port
> > > > *vport,
> > > >      VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MAX_CLIPBOARD);
> > > >      VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_AUDIO_VOLUME_SYNC);
> > > >      VD_AGENT_SET_CAPABILITY(caps->caps,
> > > >      VD_AGENT_CAP_GRAPHICS_DEVICE_INFO);
> > > > +    VD_AGENT_SET_CAPABILITY(caps->caps,
> > > > VD_AGENT_CAP_CLIPBOARD_NO_RELEASE_ON_REGRAB);
> > > >      virtio_msg_uint32_to_le((uint8_t *)caps, size, 0);
> > > >
> > > >      vdagent_virtio_port_write(vport, VDP_CLIENT_PORT,
> > >



-- 
Marc-André Lureau