Hi, On Mon, Jan 14, 2019 at 1:41 PM Marc-André Lureau <marcandre.lureau@xxxxxxxxx> wrote: > > Hi > > On Mon, Jan 14, 2019 at 4:34 PM Victor Toso <victortoso@xxxxxxxxxx> wrote: > > > > From: Victor Toso <me@xxxxxxxxxxxxxx> > > > > 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 What's so weird about that? It's also X11-specific that we can receive these "owner-change" events while spice-gtk is holding the focus since on Wayland, apps need to gain focus first to be able to set the clipboard, afaik. The clipboard protocols on X11 and Wayland have simply been designed in different ways. So although GTK tries to unify the behavior and provide the same experience in both environments, some limitations just cannot be overcome. > - 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. Sure, another app can update the clipboard, but imagine the following situation: 1) user copies something in the guest 2) an app in the client's system grabs the clipboard without having keyboard focus -- this means that the grab likely wasn't directly initiated by the user 3) spice-gtk receives "owner-change" event and subsequently sends grab to the agent 4) based on the grab message from spice-gtk in step 3), vdagent grabs the clipboard in the guest system -- and by that, it overwrites the clipboard contents copied in step 1) So the clipboard is suddenly changed, although the user didn't take any action. I think this is unwanted and this patch solves it, so I'm all for ack-ing this just for this reason. If this also solves the bug, it's even better. Nevertheless, I would be really happy if we could track down what exactly causes the issues that have been reported and describe it in more detail. > > 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". > > > > > > 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@xxxxxxxxxx> > > Tested-by: James Harvey @jamespharvey20 Does this mean it fixes the issues for him? > > --- > > 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. */ For completeness, it might be good to mention that the event can be caused by other applications too (as Marc-André pointed out), even though spice-gtk is holding the keyboard-grab. > > + 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@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/spice-devel > > Cheers, Jakub _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel