Hi,
On Mon, Jan 28, 2019 at 1:23 PM Victor Toso <victort...@redhat.com> wrote:
>
> From: Victor Toso <m...@victortoso.com>
>
> * Attaching the 'guest_' prefix for actions that were started from
> guest agent, those renames are:
>
> clipboard_grab -> guest_clipboard_grab
> clipboard_release -> guest_clipboard_release
> clipboard_request -> guest_clipboard_request_data
> clipboard_request_send_data -> guest_clipboard_request_send_data
>
> * Attaching the 'client_' prefix for actions that were triggered by
> client and sent/received by guest agent, those renames are:
>
> clipboard_get -> client_clipboard_request_data
> clipboard_got_from_guest -> client_clipboard_received_data
>
> * Changed from 'clipboard_' to 'clipboard_handler' prefix for
> callbacks related to GtkClipboard, those renames are:
>
> clipboard_clear -> clipboard_handler_clear
> clipboard_owner_change -> clipboard_handler_owner_change_cb
> clipboard_received_text_cb -> clipboard_handler_received_text_cb
> clipboard_get_targets -> clipboard_handler_get_targets_cb
These 4 seem a bit weird.
I would either change them all to "clipboard_*_handler", or "clipboard_*_cb".
I wouldn't use "handler" together with "cb".
They could also bear the "client_" prefix as they're all concerning
clipboard on the client side.
>
> Signed-off-by: Victor Toso <m...@victortoso.com>
> ---
> src/spice-gtk-session.c | 123 ++++++++++++++++++++++++----------------
> 1 file changed, 75 insertions(+), 48 deletions(-)
>
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index 1a19bca..ac2ba0b 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -95,9 +95,9 @@ struct _SpiceGtkSessionPrivate {
>
> /* ------------------------------------------------------------------ */
> /* Prototypes for private functions */
> -static void clipboard_owner_change(GtkClipboard *clipboard,
> - GdkEventOwnerChange *event,
> - gpointer user_data);
> +static void clipboard_handler_owner_change_cb(GtkClipboard *clipboard,
> + GdkEventOwnerChange *event,
> + gpointer user_data);
> static void channel_new(SpiceSession *session, SpiceChannel *channel,
> gpointer user_data);
> static void channel_destroy(SpiceSession *session, SpiceChannel *channel,
> @@ -182,10 +182,10 @@ static void spice_gtk_session_init(SpiceGtkSession
> *self)
>
> s->clipboard = gtk_clipboard_get(GDK_SELECTION_CLIPBOARD);
> g_signal_connect(G_OBJECT(s->clipboard), "owner-change",
> - G_CALLBACK(clipboard_owner_change), self);
> + G_CALLBACK(clipboard_handler_owner_change_cb), self);
> s->clipboard_primary = gtk_clipboard_get(GDK_SELECTION_PRIMARY);
> g_signal_connect(G_OBJECT(s->clipboard_primary), "owner-change",
> - G_CALLBACK(clipboard_owner_change), self);
> + G_CALLBACK(clipboard_handler_owner_change_cb), self);
> spice_g_signal_connect_object(keymap, "state-changed",
> G_CALLBACK(keymap_modifiers_changed),
> self, 0);
> }
> @@ -222,13 +222,13 @@ static void spice_gtk_session_dispose(GObject *gobject)
> /* release stuff */
> if (s->clipboard) {
> g_signal_handlers_disconnect_by_func(s->clipboard,
> - G_CALLBACK(clipboard_owner_change), self);
> + G_CALLBACK(clipboard_handler_owner_change_cb), self);
> s->clipboard = NULL;
> }
>
> if (s->clipboard_primary) {
> g_signal_handlers_disconnect_by_func(s->clipboard_primary,
> - G_CALLBACK(clipboard_owner_change), self);
> + G_CALLBACK(clipboard_handler_owner_change_cb), self);
> s->clipboard_primary = NULL;
> }
>
> @@ -537,10 +537,10 @@ static gpointer free_weak_ref(gpointer data)
> return object;
> }
>
> -static void clipboard_get_targets(GtkClipboard *clipboard,
> - GdkAtom *atoms,
> - gint n_atoms,
> - gpointer user_data)
> +static void clipboard_handler_get_targets_cb(GtkClipboard *clipboard,
> + GdkAtom *atoms,
> + gint n_atoms,
> + gpointer user_data)
> {
> SpiceGtkSession *self = free_weak_ref(user_data);
>
> @@ -635,9 +635,10 @@ static void clipboard_get_targets(GtkClipboard
> *clipboard,
> * emits owner-change event; On Wayland that does not happen as spice-gtk
> still is
> * the owner of the clipboard.
> */
> -static void clipboard_owner_change(GtkClipboard *clipboard,
> - GdkEventOwnerChange *event,
> - gpointer user_data)
> +static void
> +clipboard_handler_owner_change_cb(GtkClipboard *clipboard,
> + GdkEventOwnerChange *event,
> + gpointer user_data)
> {
> g_return_if_fail(SPICE_IS_GTK_SESSION(user_data));
>
> @@ -676,7 +677,8 @@ static void clipboard_owner_change(GtkClipboard
> *clipboard,
> 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,
> + gtk_clipboard_request_targets(clipboard,
> + clipboard_handler_get_targets_cb,
> get_weak_ref(self));
> }
>
> @@ -689,9 +691,14 @@ typedef struct
> guint selection;
> } RunInfo;
>
> -static void clipboard_got_from_guest(SpiceMainChannel *main, guint selection,
> - guint type, const guchar *data, guint
> size,
> - gpointer user_data)
> +/* Only After client_clipboard_request_data */
lowercase a in "After"?
> +static void
> +client_clipboard_received_data(SpiceMainChannel *main,
> + guint selection,
> + guint type,
> + const guchar *data,
> + guint size,
> + gpointer user_data)
> {
> RunInfo *ri = user_data;
> SpiceGtkSessionPrivate *s = ri->self->priv;
> @@ -730,9 +737,11 @@ static void clipboard_agent_connected(RunInfo *ri)
> g_main_loop_quit(ri->loop);
> }
>
> -static void clipboard_get(GtkClipboard *clipboard,
> - GtkSelectionData *selection_data,
> - guint info, gpointer user_data)
> +static void
> +client_clipboard_request_data(GtkClipboard *clipboard,
> + GtkSelectionData *selection_data,
> + guint info,
> + gpointer user_data)
> {
> g_return_if_fail(SPICE_IS_GTK_SESSION(user_data));
>
> @@ -758,8 +767,7 @@ static void clipboard_get(GtkClipboard *clipboard,
> ri.self = self;
>
> clipboard_handler = g_signal_connect(s->main, "main-clipboard-selection",
> -
> G_CALLBACK(clipboard_got_from_guest),
> - &ri);
> +
> G_CALLBACK(client_clipboard_received_data), &ri);
> agent_handler = g_signal_connect_swapped(s->main,
> "notify::agent-connected",
> G_CALLBACK(clipboard_agent_connected),
> &ri);
> @@ -770,7 +778,7 @@ static void clipboard_get(GtkClipboard *clipboard,
>
> g_object_get(s->main, "agent-connected", &agent_connected, NULL);
> if (!agent_connected) {
> - SPICE_DEBUG("canceled clipboard_get, before running loop");
> + SPICE_DEBUG("canceled client_clipboard_request_data, before running
> loop");
replace hardcoded function name with "%s" and __func__?
> goto cleanup;
> }
>
> @@ -790,16 +798,21 @@ cleanup:
> g_signal_handler_disconnect(s->main, agent_handler);
> }
>
> -static void clipboard_clear(GtkClipboard *clipboard, gpointer user_data)
> +static void
> +clipboard_handler_clear(GtkClipboard *clipboard, gpointer user_data)
> {
> SPICE_DEBUG("clipboard_clear");
I think this should be updated in this patch too (instead of 2/2).
> /* We watch for clipboard ownership changes and act on those, so we
> don't need to do anything here */
> }
>
> -static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
> - guint32* types, guint32 ntypes,
> - gpointer user_data)
> +/* Guest agent is sending guest's clipboard metadata */
> +static gboolean
> +guest_clipboard_grab(SpiceMainChannel *main,
> + guint selection,
> + guint32* types,
> + guint32 ntypes,
> + gpointer user_data)
> {
> g_return_val_if_fail(SPICE_IS_GTK_SESSION(user_data), FALSE);
>
> @@ -848,8 +861,8 @@ static gboolean clipboard_grab(SpiceMainChannel *main,
> guint selection,
> if (!gtk_clipboard_set_with_owner(cb,
> targets,
> num_targets,
> - clipboard_get,
> - clipboard_clear,
> + client_clipboard_request_data,
> + clipboard_handler_clear,
> G_OBJECT(self))) {
> g_warning("clipboard grab failed");
> return FALSE;
> @@ -906,9 +919,9 @@ static char *fixup_clipboard_text(SpiceGtkSession *self,
> const char *text, int *
> return conv;
> }
>
> -static void clipboard_received_text_cb(GtkClipboard *clipboard,
> - const gchar *text,
> - gpointer user_data)
> +static void clipboard_handler_received_text_cb(GtkClipboard *clipboard,
> + const gchar *text,
> + gpointer user_data)
> {
> SpiceGtkSession *self = free_weak_ref(user_data);
> char *conv = NULL;
> @@ -951,7 +964,8 @@ notify_agent:
> g_free(conv);
> }
>
> -static void clipboard_received_cb(GtkClipboard *clipboard,
> +static void
> +guest_clipboard_request_send_data(GtkClipboard *clipboard,
> GtkSelectionData *selection_data,
> gpointer user_data)
This function contains the following line:
| g_warning("clipboard_received for unsupported type: %s", name);
Needs update too.
> {
> @@ -995,16 +1009,20 @@ static void clipboard_received_cb(GtkClipboard
> *clipboard,
>
> const guchar *data = gtk_selection_data_get_data(selection_data);
>
> - /* text should be handled through clipboard_received_text_cb(), not
> - * clipboard_received_cb().
> + /* text should be handled through clipboard_handler_received_text_cb(),
> not
> + * guest_clipboard_request_send_data()
> */
> g_warn_if_fail(type != VD_AGENT_CLIPBOARD_UTF8_TEXT);
>
> spice_main_channel_clipboard_selection_notify(s->main, selection, type,
> data, len);
> }
>
> -static gboolean clipboard_request(SpiceMainChannel *main, guint selection,
> - guint type, gpointer user_data)
> +/* Guest agent is requesting client's clipboard data */
> +static gboolean
> +guest_clipboard_request_data(SpiceMainChannel *main,
> + guint selection,
> + guint type,
> + gpointer user_data)
> {
> g_return_val_if_fail(SPICE_IS_GTK_SESSION(user_data), FALSE);
>
> @@ -1023,7 +1041,8 @@ static gboolean clipboard_request(SpiceMainChannel
> *main, guint selection,
> return FALSE;
>
> if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
> - gtk_clipboard_request_text(cb, clipboard_received_text_cb,
> + gtk_clipboard_request_text(cb,
> + clipboard_handler_received_text_cb,
> get_weak_ref(self));
> } else {
> for (m = 0; m < SPICE_N_ELEMENTS(atom2agent); m++) {
> @@ -1034,15 +1053,18 @@ static gboolean clipboard_request(SpiceMainChannel
> *main, guint selection,
> g_return_val_if_fail(m < SPICE_N_ELEMENTS(atom2agent), FALSE);
>
> atom = gdk_atom_intern_static_string(atom2agent[m].xatom);
> - gtk_clipboard_request_contents(cb, atom, clipboard_received_cb,
> + gtk_clipboard_request_contents(cb, atom,
> guest_clipboard_request_send_data,
> get_weak_ref(self));
> }
>
> return TRUE;
> }
>
> -static void clipboard_release(SpiceMainChannel *main, guint selection,
> - gpointer user_data)
> +/* Guest agent is clearing last guest's clipboard metadata */
> +static void
> +guest_clipboard_release(SpiceMainChannel *main,
> + guint selection,
> + gpointer user_data)
> {
> g_return_if_fail(SPICE_IS_GTK_SESSION(user_data));
>
> @@ -1073,11 +1095,11 @@ static void channel_new(SpiceSession *session,
> SpiceChannel *channel,
> SPICE_DEBUG("Changing main channel from %p to %p", s->main, channel);
> s->main = SPICE_MAIN_CHANNEL(channel);
> g_signal_connect(channel, "main-clipboard-selection-grab",
> - G_CALLBACK(clipboard_grab), self);
> + G_CALLBACK(guest_clipboard_grab), self);
> g_signal_connect(channel, "main-clipboard-selection-request",
> - G_CALLBACK(clipboard_request), self);
> + G_CALLBACK(guest_clipboard_request_data), self);
> g_signal_connect(channel, "main-clipboard-selection-release",
> - G_CALLBACK(clipboard_release), self);
> + G_CALLBACK(guest_clipboard_release), self);
> }
> if (SPICE_IS_INPUTS_CHANNEL(channel)) {
> spice_g_signal_connect_object(channel, "inputs-modifiers",
> @@ -1207,7 +1229,8 @@ void spice_gtk_session_copy_to_guest(SpiceGtkSession
> *self)
> int selection = VD_AGENT_CLIPBOARD_SELECTION_CLIPBOARD;
>
> if (s->clip_hasdata[selection] && !s->clip_grabbed[selection]) {
> - gtk_clipboard_request_targets(s->clipboard, clipboard_get_targets,
> + gtk_clipboard_request_targets(s->clipboard,
> + clipboard_handler_get_targets_cb,
> get_weak_ref(self));
> }
> }
> @@ -1233,8 +1256,12 @@ void
> spice_gtk_session_paste_from_guest(SpiceGtkSession *self)
> return;
> }
>
> - if (!gtk_clipboard_set_with_owner(s->clipboard,
> s->clip_targets[selection], s->nclip_targets[selection],
> - clipboard_get, clipboard_clear,
> G_OBJECT(self))) {
> + if (!gtk_clipboard_set_with_owner(s->clipboard,
> + s->clip_targets[selection],
> + s->nclip_targets[selection],
> + client_clipboard_request_data,
> + clipboard_handler_clear,
> + G_OBJECT(self))) {
> g_warning("Clipboard grab failed");
> return;
> }
> --
> 2.20.1
>
Otherwise seems good to me. But I would consider delaying this patch
until the bug is fixed. At the moment, it might result in a real mess
in the ongoing discussions.
Cheers,
Jakub
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
