Hi, On Sun, Jan 13, 2019 at 09:59:11PM +0100, Jakub Janku wrote: > Hi, > > I see that I'm being late to the party as this has already been > pushed upstream, but I'd like to comment anyway: > > On Thu, Jan 10, 2019 at 1:47 PM Victor Toso <victortoso@xxxxxxxxxx> wrote: > > > > From: Victor Toso <me@xxxxxxxxxxxxxx> > > > > This patch improves the code style by adding braces, removing the > > single case switch and add documentation to the handler of > > GtkClipboard::owner-changed. > > > > Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx> > > --- > > src/spice-gtk-session.c | 54 +++++++++++++++++++++++++++++------------ > > 1 file changed, 39 insertions(+), 15 deletions(-) > > > > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c > > index 1ccae07..adc72a2 100644 > > --- a/src/spice-gtk-session.c > > +++ b/src/spice-gtk-session.c > > @@ -618,6 +618,23 @@ static void clipboard_get_targets(GtkClipboard *clipboard, > > s->nclip_targets[selection] = 0; > > } > > > > +/* Callback for every owner-change event for given @clipboard. > > + * This event is triggered in different ways depending on the environment of > > + * the Client, some examples: > > + * > > + * Situation 1: When another application on the client machine is holding and > > + * changing the clipboard. If client is on Wayland, spice-gtk only receives the > > + * related GtkClipboard::owner-changed event after focus-in event on Spice > > + * widget; On X11, we will receive it at the moment the clipboard data has been > > + * changed in by other application. > > + * > > + * Situation 2: When spice-gtk holds the focus and is changing the clipboard by > > + * either setting new content information with gtk_clipboard_set_with_owner() or > > + * clearing up old content with gtk_clipboard_clear(). The main difference between > > + * Wayland and X11 is that on X11, gtk_clipboard_clear() set the owner to none, which > > "set" -> "sets" ? I think you are right :) > > + * emits owner-change event; On Wayland that does not happen as spice-gtk still is > > + * the owner of the clipboard. > > You must have come across this bug, since you commented on it: > https://gitlab.gnome.org/GNOME/gtk/issues/715 > > I've created a simple test case which is attached. > It seems like no "owner-change" event is emitted after > gtk_clipboard_clear() is called, > but after gtk_clipboard_set_with_owner() the event is emitted twice. > > So the Wayland behavior you described here might just be caused by the bug... Could be. But just to be clear, what I tried to document was _when_ the callback is called. I just tried the test case under X11 and Wayland and the same thing happens as expected: On X11, the callback is called when the clipboard has changed by another application; On Wayland, the callback is called if the clipboard has changed and when we have the focus. > > + */ > > static void clipboard_owner_change(GtkClipboard *clipboard, > > GdkEventOwnerChange *event, > > gpointer user_data) > > @@ -631,30 +648,37 @@ static void clipboard_owner_change(GtkClipboard *clipboard, > > selection = get_selection_from_clipboard(s, clipboard); > > g_return_if_fail(selection != -1); > > > > - if (s->main == NULL) > > + if (s->main == NULL) { > > return; > > + } > > > > + /* In case we sent a grab to the agent, we need to release it now as > > + * previous clipboard data should not be reachable anymore */ > > if (s->clip_grabbed[selection]) { > > s->clip_grabbed[selection] = FALSE; > > - if (spice_main_channel_agent_test_capability(s->main, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND)) > > + if (spice_main_channel_agent_test_capability(s->main, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND)) { > > spice_main_channel_clipboard_selection_release(s->main, selection); > > + } > > } > > > > - switch (event->reason) { > > - case GDK_OWNER_CHANGE_NEW_OWNER: > > - if (gtk_clipboard_get_owner(clipboard) == G_OBJECT(self)) > > - break; > > - > > - s->clipboard_by_guest[selection] = FALSE; > > - s->clip_hasdata[selection] = TRUE; > > - if (s->auto_clipboard_enable && !read_only(self)) > > - gtk_clipboard_request_targets(clipboard, clipboard_get_targets, > > - get_weak_ref(self)); > > - break; > > - default: > > + /* We are mostly interested when owner has changed in which case > > + * we would like to let agent know about new clipboard data. */ > > + if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER) { > > s->clip_hasdata[selection] = FALSE; > > - break; > > + return; > > } > > + > > + /* This situation happens when clipboard is being cleared by us, when agent > > + * sends a release-grab for instance */ > > This is not true imho. It should only happen after a grab (after > gtk_clipboard_set_with_owner() is called). > After a release (gtk_clipboard_clear() call), > gtk_clipboard_get_owner(clipboard) should already return NULL. The comment refers to the gtk_clipboard_clear() not owner-changed event so you are right, of course. This comment was probably a left over from different interactions that I did in this code :( I'll send a fix for the documentation. For the other changes, I'd love to hear your opinion too. Did not get 2/3 and 3/3 patches acked but more keen to discuss the idea. I was wrong in regards to clipboard managers on Wayland, that is, clipboard managers on Wayland *can* get clipboard data of spice-gtk while spice-gtk's widgets are holding the focus :( I didn't check how that happen but I'd rather avoid/deny that to happen.. Thanks again Jakub, > > + if (gtk_clipboard_get_owner(clipboard) == G_OBJECT(self)) { > > + return; > > + } > > + > > + s->clipboard_by_guest[selection] = FALSE; > > + s->clip_hasdata[selection] = TRUE; > > + if (s->auto_clipboard_enable && !read_only(self)) > > + gtk_clipboard_request_targets(clipboard, clipboard_get_targets, > > + get_weak_ref(self)); > > } > > > > typedef struct > > -- > > 2.20.1 > > > > Cheers, > Jakub
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel