<div dir="auto">Hi, On Sun, Mar 24, 2019 at 7:45 PM Marc-André Lureau <<a href="mailto:marcandre.lureau@gmail.com" target="_blank" rel="noreferrer">marcandre.lureau@gmail.com</a>> wrote: > > Hi > > On Sun, Mar 24, 2019 at 6:49 PM Jakub Janku <<a href="mailto:jjanku@redhat.com" target="_blank" rel="noreferrer">jjanku@redhat.com</a>> wrote: > > > > Hi, > > > > On Thu, Mar 21, 2019 at 1:21 PM <<a href="mailto:marcandre.lureau@redhat.com" target="_blank" rel="noreferrer">marcandre.lureau@redhat.com</a>> wrote: > > > > > > From: Marc-André Lureau <<a href="mailto:marcandre.lureau@redhat.com" target="_blank" rel="noreferrer">marcandre.lureau@redhat.com</a>> > > > > > > Delay the release events for 0.5 sec. If no further grab comes in, > > > then release the grab. Otherwise, let's skip the release. This avoids > > > some races with clipboard managers. > > > > > > Related to: > > > <a href="https://gitlab.freedesktop.org/spice/spice-gtk/issues/82" rel="noreferrer noreferrer" target="_blank">https://gitlab.freedesktop.org/spice/spice-gtk/issues/82</a> > > > > > > Signed-off-by: Marc-André Lureau <<a href="mailto:marcandre.lureau@redhat.com" target="_blank" rel="noreferrer">marcandre.lureau@redhat.com</a>> > > > > In the 0.5 second period, any requests from apps in the client for > > clipboard data should be ignored since the vdagent can't provide the > > data any more, so we shouldn't request it. > > So I would add a condition to clipboard_get(): > > if (s->clipboard_release_delay[selection]) { > > SPICE_DEBUG("..."); > > return; > > } > > yes, thanks > > > > --- > > > src/spice-gtk-session.c | 80 ++++++++++++++++++++++++++++++++++++----- > > > 1 file changed, 72 insertions(+), 8 deletions(-) > > > > > > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c > > > index 0e748b6..d73a44b 100644 > > > --- a/src/spice-gtk-session.c > > > +++ b/src/spice-gtk-session.c > > > @@ -59,6 +59,7 @@ struct _SpiceGtkSessionPrivate { > > > gboolean clip_hasdata[CLIPBOARD_LAST]; > > > gboolean clip_grabbed[CLIPBOARD_LAST]; > > > gboolean clipboard_by_guest[CLIPBOARD_LAST]; > > > + guint clipboard_release_delay[CLIPBOARD_LAST]; > > > /* auto-usbredir related */ > > > gboolean auto_usbredir_enable; > > > int auto_usbredir_reqs; > > > @@ -95,6 +96,7 @@ struct _SpiceGtkSessionPrivate { > > > > > > /* ------------------------------------------------------------------ */ > > > /* Prototypes for private functions */ > > > +static void clipboard_release(SpiceGtkSession *self, guint selection); > > > static void clipboard_owner_change(GtkClipboard *clipboard, > > > GdkEventOwnerChange *event, > > > gpointer user_data); > > > @@ -247,6 +249,23 @@ static void spice_gtk_session_dispose(GObject *gobject) > > > G_OBJECT_CLASS(spice_gtk_session_parent_class)->dispose(gobject); > > > } > > > > > > +static void clipboard_release_delay_remove(SpiceGtkSession *self, guint selection, > > > + gboolean release_if_delayed) > > > +{ > > > + SpiceGtkSessionPrivate *s = self->priv; > > > + > > > + if (!s->clipboard_release_delay[selection]) > > > + return; > > > + > > > + if (release_if_delayed) { > > > + SPICE_DEBUG("delayed clipboard release, sel:%u", selection); > > > + clipboard_release(self, selection); > > > + } > > > + > > > + g_source_remove(s->clipboard_release_delay[selection]); > > > + s->clipboard_release_delay[selection] = 0; > > > +} > > > + > > > static void spice_gtk_session_finalize(GObject *gobject) > > > { > > > SpiceGtkSession *self = SPICE_GTK_SESSION(gobject); > > > @@ -256,6 +275,7 @@ static void spice_gtk_session_finalize(GObject *gobject) > > > /* release stuff */ > > > for (i = 0; i < CLIPBOARD_LAST; ++i) { > > > g_clear_pointer(&s->clip_targets[i], g_free); > > > + clipboard_release_delay_remove(self, i, true); > > > } > > > > > > /* Chain up to the parent class */ > > > @@ -815,6 +835,8 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection, > > > int m, n; > > > int num_targets = 0; > > > > > > + clipboard_release_delay_remove(self, selection, false); > > > + > > > cb = get_clipboard_from_selection(s, selection); > > > g_return_val_if_fail(cb != NULL, FALSE); > > > > > > @@ -1044,17 +1066,12 @@ static gboolean clipboard_request(SpiceMainChannel *main, guint selection, > > > return TRUE; > > > } > > > > > > -static void clipboard_release(SpiceMainChannel *main, guint selection, > > > - gpointer user_data) > > > +static void clipboard_release(SpiceGtkSession *self, guint selection) > > > { > > > - g_return_if_fail(SPICE_IS_GTK_SESSION(user_data)); > > > - > > > - SpiceGtkSession *self = user_data; > > > SpiceGtkSessionPrivate *s = self->priv; > > > GtkClipboard* clipboard = get_clipboard_from_selection(s, selection); > > > > > > - if (!clipboard) > > > - return; > > > + g_return_if_fail(clipboard != NULL); > > > > > > s->nclip_targets[selection] = 0; > > > > > > @@ -1064,6 +1081,53 @@ static void clipboard_release(SpiceMainChannel *main, guint selection, > > > s->clipboard_by_guest[selection] = FALSE; > > > } > > > > > > +typedef struct SpiceGtkClipboardRelease { > > > + SpiceGtkSession *self; > > > + guint selection; > > > +} SpiceGtkClipboardRelease; > > > + > > > +static gboolean clipboard_release_timeout(gpointer user_data) > > > +{ > > > + SpiceGtkClipboardRelease *rel = user_data; > > > + > > > + clipboard_release_delay_remove(rel->self, rel->selection, true); > > > + > > > + return G_SOURCE_REMOVE; > > > +} > > > + > > > +/* > > > + * The agents send release between two grabs. This may trigger > > > + * clipboard managers trying to grab the clipboard. We end up with two > > > + * sides, client and remote, racing for the clipboard grab, and > > > + * believing each other is the owner. > > > + * > > > + * Workaround this problem by delaying the release event by 0.5 sec. > > > + * FIXME: protocol change to solve the conflict and set client priority. > > > + */ > > > +#define CLIPBOARD_RELEASE_DELAY 500 /* ms */ > > > + > > > +static void clipboard_release_delay(SpiceMainChannel *main, guint selection, > > > + gpointer user_data) > > > +{ > > > + SpiceGtkSession *self = SPICE_GTK_SESSION(user_data); > > > + SpiceGtkSessionPrivate *s = self->priv; > > > + GtkClipboard* clipboard = get_clipboard_from_selection(s, selection); > > > + SpiceGtkClipboardRelease *rel; > > > + > > > + if (!clipboard) > > > + return; > > > + > > > + clipboard_release_delay_remove(self, selection, true); > > > + > > > + rel = g_new0(SpiceGtkClipboardRelease, 1); > > > + rel->self = self; > > > + rel->selection = selection; > > > + s->clipboard_release_delay[selection] = > > > + g_timeout_add_full(G_PRIORITY_DEFAULT, CLIPBOARD_RELEASE_DELAY, > > > + clipboard_release_timeout, rel, g_free); > > > + > > > +} > > > + > > > static void channel_new(SpiceSession *session, SpiceChannel *channel, > > > gpointer user_data) > > > { > > > @@ -1080,7 +1144,7 @@ static void channel_new(SpiceSession *session, SpiceChannel *channel, > > > g_signal_connect(channel, "main-clipboard-selection-request", > > > G_CALLBACK(clipboard_request), self); > > > g_signal_connect(channel, "main-clipboard-selection-release", > > > - G_CALLBACK(clipboard_release), self); > > > + G_CALLBACK(clipboard_release_delay), self); > > > > I find the naming you're introducing here rather confusing. > > I wouldn't change the signal handler name to "clipboard_release_delay". > > yes, let's not rename it. With > VD_AGENT_CAP_CLIPBOARD_NO_RELEASE_ON_REGRAB implementation, it won't > be delayed either. > > > What about using the word "pending" instead of "delay"? > > So: > > clipboard_release_delay --> clipboard_release_pending > > Delay or pending doesn't make much difference to me. > > > clipboard_release_delay_remove --> clipboard_release_pending_clear > > Again I don't care much. I prefer "remove", since that's the term used > for GSources. But if you feel strongly about "clear", that works for > me too. I prefer "pending" a bit more. Both "clear" and "remove" seem fine, up to you which one you use.<div dir="auto"> </div><div dir="auto">Thanks,</div><div dir="auto">Jakub > > > > > > > } > > > if (SPICE_IS_INPUTS_CHANNEL(channel)) { > > > spice_g_signal_connect_object(channel, "inputs-modifiers", > > > -- > > > 2.21.0.4.g36eb1cb9cf > > > > > _______________________________________________ > > Spice-devel mailing list > > <a href="mailto:Spice-devel@lists.freedesktop.org" target="_blank" rel="noreferrer">Spice-devel@lists.freedesktop.org</a> > > <a href="https://lists.freedesktop.org/mailman/listinfo/spice-devel" rel="noreferrer noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/spice-devel</a> > > > > -- > Marc-André Lureau </div></div>