Hi, On Mon, Jan 14, 2019 at 04:40:35PM +0400, Marc-André Lureau 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 You mean that GtkClipboard event's should behave the same way in Wayland? > - 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. Yes but, do we wan't to support that? This is application-level tweak on clipboard behavior so I don't think controlling it a bit is bad if we agree that the rationale behind it is correct. The other way to fix this is likely a major design in the code to not touch the clipboard on grab/release (from guest messages) while we have the focus. IMHO, also easy to get wrong. > 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". Calling it a hack is the same as saying you have a problem with the rationale behind it, no? I don't mind calling it a hack but I'm in favor of not having a grab send by client to the guest by 3rd application running on client OS *while* spice-gtk is holding the focus. Keeping the behavior is bad, IMHO. Not sure what you mean with multiple inputs. Are we targeting something else other than Desktop clients? > > 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 > > --- > > 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. */ Should I add: + /* TODO: X11 clipboard specifics should be removed in the future */ Cheers, > > + 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 > > > > -- > Marc-André Lureau
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel