[Spice-devel] [PATCH spice-protocol 3/3] vdagent: introduce VD_AGENT_CAP_CLIPBOARD_GRAB_SERIAL

[Jakub Janku]

Hi,

On Sun, Mar 24, 2019 at 7:40 PM Marc-André Lureau
<marcandre.lureau at gmail.com> wrote:
>
> Hi
>
> On Sun, Mar 24, 2019 at 6:49 PM Jakub Janku <jjanku at redhat.com> wrote:
> >
> > On Fri, Mar 22, 2019 at 2:57 PM <marcandre.lureau at redhat.com> wrote:
> > >
> > > From: Marc-André Lureau <marcandre.lureau at redhat.com>
> > >
> > > When this capability is negoticated by both the client & the agent,
> > > the clipboard grab messages have an associated serial counter.
> > >
> > > The serial is reset to 0 upon client connection.
> > >
> > > The counter is increment by 1 on each grab message, by both sides.
> > >
> > > The sender of the message with the highest serial should be the
> > > clipboard grab owner, and the current session serial should be
> > > updated.
> > >
> > > If a lower serial than the current session serial is received, the
> > > grab should be discarded.
> > >
> > > Whenever two grabs share the same serial, the one coming from the
> > > client should have a higher priority and the client should gain the
> > > clipboard ownership.
> >
> > Why should the client be the one with a higher priority?
>
> No strong reason, there has to be a winner in this case: both side are
> taking the grab simultaneously, we have to decide which one actually
> holds it in the end.  Other ideas?

As I said - depending on the keyboard focus. However, this would
probably further complicate the protocol and so I have doubts whether
it's worth it, since this already handles a rather uncommon scenario.
Would love to hear opinions on who should be the "winner" in this case
from others too.

Apart from that, would be great if James tested it to see how his
environment behaves with these patches.
>
> >
> > If you look at James' case again:
> > with you proposed change, the clipboard manager would take over if a
> > race occurs, but the clipboard manager usually supports only a limited
> > number of targets.
> > For example, you copy a graph in Excel, (regrab and race occurs),
> > clipboard manager from the client wins and sets the clipboard, now
> > you're able to paste the graph only as a non-editable image.
>
> If there is a clipboard manager on the client side, it's the client
> desire to do such transformation, isn't it?

That's hard to say. I can imagine it could confuse some users while
others would appreciate it (e.g. in the case when they'd run some
automation systems in the client).
>
> >
> > So to provide an "uninterrupted" experience, I think that the
> > component with keyboard focus should actually get the priority.
>
> You are working against the clipboard manager in this case, which may
> do as much harm as good.

Correct, but you can still run another clipboard manager in the guest system.
>
> > >
> > > No special treatement is done for the unlikely case of overflowing the
> > > counter. It may temporarily inverse the priority, until both side have
> > > overflown and/or synchronized.
> > >
> > > Note: this mechanism isn't aiming at making "the most recent" (as in
> > > time) side gaining the ownership. One side sending subsequent grab
> > > messages earlier will likely take the ownership over a side sending a
> > > single message simultaneously the other way. It only clears the
> > > situation where both side believe that the other is the current
> > > clipboard owner, by having a global ordering and priority in case of
> > > serial conflict.
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> > > ---
> > >  spice/vd_agent.h | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/spice/vd_agent.h b/spice/vd_agent.h
> > > index 862cb5c..9ae3cc7 100644
> > > --- a/spice/vd_agent.h
> > > +++ b/spice/vd_agent.h
> > > @@ -218,6 +218,9 @@ typedef struct SPICE_ATTR_PACKED VDAgentClipboardGrab {
> > >  #if 0 /* VD_AGENT_CAP_CLIPBOARD_SELECTION */
> > >      uint8_t selection;
> > >      uint8_t __reserved[sizeof(uint32_t) - 1 * sizeof(uint8_t)];
> > > +#endif
> > > +#if 0 /* VD_AGENT_CAP_CLIPBOARD_GRAB_SERIAL */
> > > +    uint32_t serial;
> > >  #endif
> > >      uint32_t types[0];
> > >  } VDAgentClipboardGrab;
> > > @@ -288,6 +291,7 @@ enum {
> > >      VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS,
> > >      VD_AGENT_CAP_GRAPHICS_DEVICE_INFO,
> > >      VD_AGENT_CAP_CLIPBOARD_NO_RELEASE_ON_REGRAB,
> > > +    VD_AGENT_CAP_CLIPBOARD_GRAB_SERIAL,
> > >      VD_AGENT_END_CAP,
> > >  };
> > >
> > > --
> > > 2.21.0.4.g36eb1cb9cf
> > >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
>
> --
> Marc-André Lureau

Overall, I'm not against this implementation, but testing from someone
who's experienced the issues is a must, imho.

Cheers,
Jakub