[Spice-devel] [spice-gtk v1 0/4] gtk-session misc changes

[Victor Toso]

From: Victor Toso <***@victortoso.com>

Hi,

Small code improvements.

Victor Toso (4):
gtk-session: remove extra clipboard selection check
gtk-session: prefer early check to agent connectivity
gtk-session: remove single goto usage
gtk-session: better variable name

src/spice-gtk-session.c | 51 ++++++++++++++++++++---------------------
1 file changed, 25 insertions(+), 26 deletions(-)

[Victor Toso]

From: Victor Toso <***@victortoso.com>

Commit 284c1f2d switched to
spice_main_channel_clipboard_selection_release() which does check if
agent is connected and does the right thing (expected) in regards to
releasing the clipboard by calling agent_clipboard_release() which
does check VD_AGENT_CAP_CLIPBOARD_SELECTION (like current removed
code).

So this patch removes redundant check.

Same goes for spice_main_channel_clipboard_selection_grab() function.

Signed-off-by: Victor Toso <***@redhat.com>
---
src/spice-gtk-session.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 20a4060..8d31045 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -610,9 +610,7 @@ static void clipboard_get_targets(GtkClipboard *clipboard,
}

s->clip_grabbed[selection] = TRUE;
-
- if (spice_main_channel_agent_test_capability(s->main, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
- spice_main_channel_clipboard_selection_grab(s->main, selection, types, num_types);
+ spice_main_channel_clipboard_selection_grab(s->main, selection, types, num_types);

/* Sending a grab causes the agent to do an implicit release */
s->nclip_targets[selection] = 0;
@@ -636,8 +634,7 @@ static void clipboard_owner_change(GtkClipboard *clipboard,

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))
- spice_main_channel_clipboard_selection_release(s->main, selection);
+ spice_main_channel_clipboard_selection_release(s->main, selection);
}

switch (event->reason) {

[Frediano Ziglio]

Post by Victor Toso
Commit 284c1f2d switched to
spice_main_channel_clipboard_selection_release() which does check if
agent is connected and does the right thing (expected) in regards to
releasing the clipboard by calling agent_clipboard_release() which
does check VD_AGENT_CAP_CLIPBOARD_SELECTION (like current removed
code).
So this patch removes redundant check.
Same goes for spice_main_channel_clipboard_selection_grab() function.

This is not the same. The test in agent_clipboard_grab is done with
g_return_if_fail which emits a warning while current code does not.
Also spice_main_channel_clipboard_selection_grab always calls
spice_channel_wakeup which current code does not.

Post by Victor Toso
---
src/spice-gtk-session.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 20a4060..8d31045 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -610,9 +610,7 @@ static void clipboard_get_targets(GtkClipboard *clipboard,
}
s->clip_grabbed[selection] = TRUE;
-
- if (spice_main_channel_agent_test_capability(s->main,
VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
- spice_main_channel_clipboard_selection_grab(s->main, selection, types, num_types);
+ spice_main_channel_clipboard_selection_grab(s->main, selection, types, num_types);
/* Sending a grab causes the agent to do an implicit release */
s->nclip_targets[selection] = 0;
@@ -636,8 +634,7 @@ static void clipboard_owner_change(GtkClipboard
*clipboard,
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))
- spice_main_channel_clipboard_selection_release(s->main, selection);
+ spice_main_channel_clipboard_selection_release(s->main, selection);
}
switch (event->reason) {

Frediano

[Victor Toso]

Hi,

Post by Frediano Ziglio

Post by Victor Toso
Commit 284c1f2d switched to
spice_main_channel_clipboard_selection_release() which does check if
agent is connected and does the right thing (expected) in regards to
releasing the clipboard by calling agent_clipboard_release() which
does check VD_AGENT_CAP_CLIPBOARD_SELECTION (like current removed
code).
So this patch removes redundant check.
Same goes for spice_main_channel_clipboard_selection_grab() function.

This is not the same. The test in agent_clipboard_grab is done
with g_return_if_fail which emits a warning while current code
does not.

Which probably should

Post by Frediano Ziglio
Also spice_main_channel_clipboard_selection_grab always calls
spice_channel_wakeup which current code does not.

Which is probably a bug there (should call spice_channel_wakeup
if message was queued.

What do you think?

Post by Frediano Ziglio

Post by Victor Toso
---
src/spice-gtk-session.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 20a4060..8d31045 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -610,9 +610,7 @@ static void clipboard_get_targets(GtkClipboard *clipboard,
}
s->clip_grabbed[selection] = TRUE;
-
- if (spice_main_channel_agent_test_capability(s->main,
VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
- spice_main_channel_clipboard_selection_grab(s->main, selection,
types, num_types);
+ spice_main_channel_clipboard_selection_grab(s->main, selection, types, num_types);
/* Sending a grab causes the agent to do an implicit release */
s->nclip_targets[selection] = 0;
@@ -636,8 +634,7 @@ static void clipboard_owner_change(GtkClipboard
*clipboard,
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))
- spice_main_channel_clipboard_selection_release(s->main, selection);
+ spice_main_channel_clipboard_selection_release(s->main, selection);
}
switch (event->reason) {

Frediano

[Frediano Ziglio]

Post by Victor Toso
Hi,

Post by Frediano Ziglio

Post by Victor Toso
Commit 284c1f2d switched to
spice_main_channel_clipboard_selection_release() which does check if
agent is connected and does the right thing (expected) in regards to
releasing the clipboard by calling agent_clipboard_release() which
does check VD_AGENT_CAP_CLIPBOARD_SELECTION (like current removed
code).
So this patch removes redundant check.
Same goes for spice_main_channel_clipboard_selection_grab() function.

This is not the same. The test in agent_clipboard_grab is done
with g_return_if_fail which emits a warning while current code
does not.

Which probably should

IMHO is the opposite, as we supports agents that does not support
VD_AGENT_CAP_CLIPBOARD_BY_DEMAND why giving warning every time?
If we would like to warn the user that guest is using a very old
agent would not be better to warn once when the capabilities are
received? Maybe if it's really important this could be also done
with a GUI message (maybe with a persistent tick "don't warn
about it next time" like for the exit message).

Post by Victor Toso

Post by Frediano Ziglio
Also spice_main_channel_clipboard_selection_grab always calls
spice_channel_wakeup which current code does not.

Which is probably a bug there (should call spice_channel_wakeup
if message was queued.
What do you think?

Not sure if this function is also expected to handle pending
events and what would happen if this is removed.
If it's safe to do so then I would move spice_channel_wakeup
call inside spice_main_channel_clipboard_selection_grab at
the end of agent_clipboard_grab (which is only called by
spice_main_channel_clipboard_selection_grab).

Post by Victor Toso

Post by Frediano Ziglio

Post by Victor Toso
---
src/spice-gtk-session.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 20a4060..8d31045 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -610,9 +610,7 @@ static void clipboard_get_targets(GtkClipboard *clipboard,
}
s->clip_grabbed[selection] = TRUE;
-
- if (spice_main_channel_agent_test_capability(s->main,
VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
- spice_main_channel_clipboard_selection_grab(s->main, selection,
types, num_types);
+ spice_main_channel_clipboard_selection_grab(s->main, selection,
types,
num_types);
/* Sending a grab causes the agent to do an implicit release */
s->nclip_targets[selection] = 0;
@@ -636,8 +634,7 @@ static void clipboard_owner_change(GtkClipboard
*clipboard,
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))
- spice_main_channel_clipboard_selection_release(s->main, selection);
+ spice_main_channel_clipboard_selection_release(s->main, selection);
}
switch (event->reason) {

Frediano

[Victor Toso]

Hi,

Post by Frediano Ziglio

Post by Victor Toso
Hi,

Post by Frediano Ziglio

Post by Victor Toso
Commit 284c1f2d switched to
spice_main_channel_clipboard_selection_release() which does check if
agent is connected and does the right thing (expected) in regards to
releasing the clipboard by calling agent_clipboard_release() which
does check VD_AGENT_CAP_CLIPBOARD_SELECTION (like current removed
code).
So this patch removes redundant check.
Same goes for spice_main_channel_clipboard_selection_grab() function.

This is not the same. The test in agent_clipboard_grab is done
with g_return_if_fail which emits a warning while current code
does not.

Which probably should

IMHO is the opposite, as we supports agents that does not
support VD_AGENT_CAP_CLIPBOARD_BY_DEMAND why giving warning
every time?

Hm, that was added in 2010 [0], spice-vdagent-0.6.3. Why should
we support supper earlier versions of the agent anyway?

https://gitlab.freedesktop.org/spice/linux/vd_agent/commit/653a2ac191dd699de7e9dd322e4b#cbc5e2eb746c0c8aaa6d4629e43dfb9da6b0c945_94_67

Post by Frediano Ziglio
If we would like to warn the user that guest is using a very old
agent would not be better to warn once when the capabilities are
received? Maybe if it's really important this could be also done
with a GUI message (maybe with a persistent tick "don't warn
about it next time" like for the exit message).

I'm pretty sure nobody tests such old agent... I'm in favor of
warning as it should help find bugs instead of silently not doing
it like current code.

Post by Frediano Ziglio

Post by Victor Toso

Post by Frediano Ziglio
Also spice_main_channel_clipboard_selection_grab always calls
spice_channel_wakeup which current code does not.

Which is probably a bug there (should call spice_channel_wakeup
if message was queued.
What do you think?

Not sure if this function is also expected to handle pending
events and what would happen if this is removed.

No, the wakeup is after adding the message to the queue with the
purpose of not delaying that message to be sent. Some code in
client could block while waiting the clipboard request and that's
why the wakeup can be helpful.

Post by Frediano Ziglio
If it's safe to do so then I would move spice_channel_wakeup
call inside spice_main_channel_clipboard_selection_grab at
the end of agent_clipboard_grab (which is only called by
spice_main_channel_clipboard_selection_grab).

I would rather make agent_clipboard_* to return true if
succeed in adding the message to the queue. That's implementation
detail, We/I can look later on.. Important is that we agree
wakeup shouldn't be called on failure.

Post by Frediano Ziglio

Post by Victor Toso

Post by Frediano Ziglio

Post by Victor Toso
---
src/spice-gtk-session.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 20a4060..8d31045 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -610,9 +610,7 @@ static void clipboard_get_targets(GtkClipboard *clipboard,
}
s->clip_grabbed[selection] = TRUE;
-
- if (spice_main_channel_agent_test_capability(s->main,
VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
- spice_main_channel_clipboard_selection_grab(s->main, selection,
types, num_types);
+ spice_main_channel_clipboard_selection_grab(s->main, selection,
types,
num_types);
/* Sending a grab causes the agent to do an implicit release */
s->nclip_targets[selection] = 0;
@@ -636,8 +634,7 @@ static void clipboard_owner_change(GtkClipboard
*clipboard,
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))
- spice_main_channel_clipboard_selection_release(s->main,
selection);
+ spice_main_channel_clipboard_selection_release(s->main, selection);
}
switch (event->reason) {

Frediano

Thanks again :)

[Victor Toso]

From: Victor Toso <***@victortoso.com>

In case the agent is disconnected, we we don't need to create the
struct RunInfo, GMainLoop and add handlers to some signals.

This patch also removes one goto and related cleanup label.

Signed-off-by: Victor Toso <***@redhat.com>
---
src/spice-gtk-session.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 8d31045..1615172 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -725,6 +725,12 @@ static void clipboard_get(GtkClipboard *clipboard,
g_return_if_fail(info < SPICE_N_ELEMENTS(atom2agent));
g_return_if_fail(s->main != NULL);

+ g_object_get(s->main, "agent-connected", &agent_connected, NULL);
+ if (!agent_connected) {
+ SPICE_DEBUG("Request to guest failed as agent is not running");
+ return;
+ }
+
ri.selection_data = selection_data;
ri.info = info;
ri.loop = g_main_loop_new(NULL, FALSE);
@@ -741,13 +747,6 @@ static void clipboard_get(GtkClipboard *clipboard,
spice_main_channel_clipboard_selection_request(s->main, selection,
atom2agent[info].vdagent);

-
- g_object_get(s->main, "agent-connected", &agent_connected, NULL);
- if (!agent_connected) {
- SPICE_DEBUG("canceled clipboard_get, before running loop");
- goto cleanup;
- }
-
/* This is modeled on the implementation of gtk_dialog_run() even though
* these thread functions are deprecated and appears to be needed to avoid
* dead-lock from gtk_dialog_run().
@@ -758,7 +757,6 @@ static void clipboard_get(GtkClipboard *clipboard,
gdk_threads_enter();
G_GNUC_END_IGNORE_DEPRECATIONS

-cleanup:
g_clear_pointer(&ri.loop, g_main_loop_unref);
g_signal_handler_disconnect(s->main, clipboard_handler);
g_signal_handler_disconnect(s->main, agent_handler);

[Frediano Ziglio]

Post by Victor Toso
In case the agent is disconnected, we we don't need to create the
struct RunInfo, GMainLoop and add handlers to some signals.
This patch also removes one goto and related cleanup label.
---
src/spice-gtk-session.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 8d31045..1615172 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -725,6 +725,12 @@ static void clipboard_get(GtkClipboard *clipboard,
g_return_if_fail(info < SPICE_N_ELEMENTS(atom2agent));
g_return_if_fail(s->main != NULL);
+ g_object_get(s->main, "agent-connected", &agent_connected, NULL);
+ if (!agent_connected) {
+ SPICE_DEBUG("Request to guest failed as agent is not running");
+ return;
+ }
+
ri.selection_data = selection_data;
ri.info = info;
ri.loop = g_main_loop_new(NULL, FALSE);
@@ -741,13 +747,6 @@ static void clipboard_get(GtkClipboard *clipboard,
spice_main_channel_clipboard_selection_request(s->main, selection,
atom2agent[info].vdagent);
-
- g_object_get(s->main, "agent-connected", &agent_connected, NULL);
- if (!agent_connected) {
- SPICE_DEBUG("canceled clipboard_get, before running loop");
- goto cleanup;
- }
-
/* This is modeled on the implementation of gtk_dialog_run() even though
* these thread functions are deprecated and appears to be needed to avoid
* dead-lock from gtk_dialog_run().
@@ -758,7 +757,6 @@ static void clipboard_get(GtkClipboard *clipboard,
gdk_threads_enter();
G_GNUC_END_IGNORE_DEPRECATIONS
g_clear_pointer(&ri.loop, g_main_loop_unref);
g_signal_handler_disconnect(s->main, clipboard_handler);
g_signal_handler_disconnect(s->main, agent_handler);

Before spice_main_channel_clipboard_selection_request was always called so
spice_channel_wakeup was also called. Honestly I don't have enough knowledge
to understand if this could be an issue or not. Not clear which events
could be possible pending here, the function looks quite an hack, looking
at the signals set there's "notify::agent-connected" which seems to mean
that meanwhile the "agent-connected" value is expected to change during this
function.

Frediano

[Victor Toso]

Hi,

Post by Frediano Ziglio

Post by Victor Toso
In case the agent is disconnected, we we don't need to create the
struct RunInfo, GMainLoop and add handlers to some signals.
This patch also removes one goto and related cleanup label.
---
src/spice-gtk-session.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 8d31045..1615172 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -725,6 +725,12 @@ static void clipboard_get(GtkClipboard *clipboard,
g_return_if_fail(info < SPICE_N_ELEMENTS(atom2agent));
g_return_if_fail(s->main != NULL);
+ g_object_get(s->main, "agent-connected", &agent_connected, NULL);
+ if (!agent_connected) {
+ SPICE_DEBUG("Request to guest failed as agent is not running");
+ return;
+ }
+
ri.selection_data = selection_data;
ri.info = info;
ri.loop = g_main_loop_new(NULL, FALSE);
@@ -741,13 +747,6 @@ static void clipboard_get(GtkClipboard *clipboard,
spice_main_channel_clipboard_selection_request(s->main, selection,
atom2agent[info].vdagent);
-
- g_object_get(s->main, "agent-connected", &agent_connected, NULL);
- if (!agent_connected) {
- SPICE_DEBUG("canceled clipboard_get, before running loop");
- goto cleanup;
- }
-
/* This is modeled on the implementation of gtk_dialog_run() even though
* these thread functions are deprecated and appears to be needed to avoid
* dead-lock from gtk_dialog_run().
@@ -758,7 +757,6 @@ static void clipboard_get(GtkClipboard *clipboard,
gdk_threads_enter();
G_GNUC_END_IGNORE_DEPRECATIONS
g_clear_pointer(&ri.loop, g_main_loop_unref);
g_signal_handler_disconnect(s->main, clipboard_handler);
g_signal_handler_disconnect(s->main, agent_handler);

Before spice_main_channel_clipboard_selection_request was
always called so spice_channel_wakeup was also called. Honestly
I don't have enough knowledge to understand if this could be an
issue or not.

Ah, you are right. Well:
fn spice_main_channel_clipboard_selection_request() calls
fn agent_clipboard_request() which will fail on check
g_return_if_fail(c->agent_connected)

So, if we don't want to have the failure (we want to send the
agent message only if agent is connected) moving this check up is
not just a cleanup but should avoid this kind of issue.

At this moment, we don't have this check so _should_ be possible
to triggered this critical by calling
spice_gtk_session_paste_from_guest while agent is not
connected. Spicy tool listens to agent-connect and does not allow
it to happen but we shouldn't rely on the library's user, I
think.

Post by Frediano Ziglio
Not clear which events could be possible pending here, the
function looks quite an hack, looking
at the signals set there's "notify::agent-connected" which
seems to mean that meanwhile the "agent-connected" value is
expected to change during this function.
Frediano

Yeah, perhaps utility function would be better
(..)_agent_is_connected();

Victor

[Victor Toso]

From: Victor Toso <***@victortoso.com>

Returning TRUE here should be fine. Not much error handling around the
label.

Signed-off-by: Victor Toso <***@redhat.com>
---
src/spice-gtk-session.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 1615172..bf3c1fb 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -813,8 +813,9 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,

if (read_only(self) ||
!s->auto_clipboard_enable ||
- s->nclip_targets[selection] == 0)
- goto skip_grab_clipboard;
+ s->nclip_targets[selection] == 0) {
+ return TRUE;
+ }

if (!gtk_clipboard_set_with_owner(cb, targets, i,
clipboard_get, clipboard_clear, G_OBJECT(self))) {
@@ -824,7 +825,6 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
s->clipboard_by_guest[selection] = TRUE;
s->clip_hasdata[selection] = FALSE;

-skip_grab_clipboard:
return TRUE;
}

[Frediano Ziglio]

Post by Victor Toso
Returning TRUE here should be fine. Not much error handling around the
label.

About "Returning TRUE here should be fine" not sure why you used the "should",
any doubt? Is it like "maybe in the future there could be a need for a
label to do more clean up" ?
Otherwise,

Post by Victor Toso
---
src/spice-gtk-session.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 1615172..bf3c1fb 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -813,8 +813,9 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
if (read_only(self) ||
!s->auto_clipboard_enable ||
- s->nclip_targets[selection] == 0)
- goto skip_grab_clipboard;
+ s->nclip_targets[selection] == 0) {
+ return TRUE;
+ }
if (!gtk_clipboard_set_with_owner(cb, targets, i,
clipboard_get, clipboard_clear,
G_OBJECT(self))) {
@@ -824,7 +825,6 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
s->clipboard_by_guest[selection] = TRUE;
s->clip_hasdata[selection] = FALSE;
return TRUE;
}

Frediano

[Victor Toso]

Post by Frediano Ziglio

Post by Victor Toso
Returning TRUE here should be fine. Not much error handling around the
label.

About "Returning TRUE here should be fine" not sure why you
used the "should", any doubt? Is it like "maybe in the future
there could be a need for a label to do more clean up" ?

I'm not native speaker. I meant that I don't expect problems in
returning TRUE instead of being unsure that this change is fine.

Post by Frediano Ziglio
Otherwise,

Let me know if I should change the commit log.
Thanks for review,
Victor

Post by Frediano Ziglio

Post by Victor Toso
---
src/spice-gtk-session.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 1615172..bf3c1fb 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -813,8 +813,9 @@ static gboolean clipboard_grab(SpiceMainChannel *main,
guint selection,
if (read_only(self) ||
!s->auto_clipboard_enable ||
- s->nclip_targets[selection] == 0)
- goto skip_grab_clipboard;
+ s->nclip_targets[selection] == 0) {
+ return TRUE;
+ }
if (!gtk_clipboard_set_with_owner(cb, targets, i,
clipboard_get, clipboard_clear,
G_OBJECT(self))) {
@@ -824,7 +825,6 @@ static gboolean clipboard_grab(SpiceMainChannel *main,
guint selection,
s->clipboard_by_guest[selection] = TRUE;
s->clip_hasdata[selection] = FALSE;
return TRUE;
}

Frediano

[Frediano Ziglio]

Post by Victor Toso

Post by Frediano Ziglio

Post by Victor Toso
Returning TRUE here should be fine. Not much error handling around the
label.

About "Returning TRUE here should be fine" not sure why you
used the "should", any doubt? Is it like "maybe in the future
there could be a need for a label to do more clean up" ?

I'm not native speaker. I meant that I don't expect problems in
returning TRUE instead of being unsure that this change is fine.

Not native speaker too :-)
It's fine, is doing the same thing at C level.

Post by Victor Toso

Post by Frediano Ziglio
Otherwise,

Let me know if I should change the commit log.
Thanks for review,
Victor

Fine for me, just curious about your doubts.

Post by Victor Toso

Post by Frediano Ziglio

Post by Victor Toso
---
src/spice-gtk-session.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 1615172..bf3c1fb 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -813,8 +813,9 @@ static gboolean clipboard_grab(SpiceMainChannel *main,
guint selection,
if (read_only(self) ||
!s->auto_clipboard_enable ||
- s->nclip_targets[selection] == 0)
- goto skip_grab_clipboard;
+ s->nclip_targets[selection] == 0) {
+ return TRUE;
+ }
if (!gtk_clipboard_set_with_owner(cb, targets, i,
clipboard_get, clipboard_clear,
G_OBJECT(self))) {
@@ -824,7 +825,6 @@ static gboolean clipboard_grab(SpiceMainChannel *main,
guint selection,
s->clipboard_by_guest[selection] = TRUE;
s->clip_hasdata[selection] = FALSE;
return TRUE;
}

Frediano

[Victor Toso]

From: Victor Toso <***@victortoso.com>

Not saying it is perfect name but 'i' as index does not state much
after the for loop in g_memdup() and gtk_clipboard_set_with_owner().

Using number of elements as indexes is far from unusual so let's
rename it and reduce by one the single letter vars.

Also note that the change in indentation on arguments of
gtk_clipboard_set_with_owner() while renaming 'i' -> 'num_targets'.
Follow up patch will rename some callbacks, so keeping one argument
per line would reduce slightly the change set of patch set.

Signed-off-by: Victor Toso <***@redhat.com>
---
src/spice-gtk-session.c | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index bf3c1fb..ef3faea 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -781,22 +781,22 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
gboolean target_selected[SPICE_N_ELEMENTS(atom2agent)] = { FALSE, };
gboolean found;
GtkClipboard* cb;
- int m, n, i;
+ int m, n;
+ int num_targets = 0;

cb = get_clipboard_from_selection(s, selection);
g_return_val_if_fail(cb != NULL, FALSE);

- i = 0;
for (n = 0; n < ntypes; ++n) {
found = FALSE;
for (m = 0; m < SPICE_N_ELEMENTS(atom2agent); m++) {
if (atom2agent[m].vdagent == types[n] && !target_selected[m]) {
found = TRUE;
- g_return_val_if_fail(i < SPICE_N_ELEMENTS(atom2agent), FALSE);
- targets[i].target = (gchar*)atom2agent[m].xatom;
- targets[i].info = m;
+ g_return_val_if_fail(num_targets < SPICE_N_ELEMENTS(atom2agent), FALSE);
+ targets[num_targets].target = (gchar*)atom2agent[m].xatom;
+ targets[num_targets].info = m;
target_selected[m] = TRUE;
- i += 1;
+ num_targets += 1;
}
}
if (!found) {
@@ -806,8 +806,8 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
}

g_free(s->clip_targets[selection]);
- s->nclip_targets[selection] = i;
- s->clip_targets[selection] = g_memdup(targets, sizeof(GtkTargetEntry) * i);
+ s->nclip_targets[selection] = num_targets;
+ s->clip_targets[selection] = g_memdup(targets, sizeof(GtkTargetEntry) * num_targets);
/* Receiving a grab implies we've released our own grab */
s->clip_grabbed[selection] = FALSE;

@@ -817,8 +817,12 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
return TRUE;
}

- if (!gtk_clipboard_set_with_owner(cb, targets, i,
- clipboard_get, clipboard_clear, G_OBJECT(self))) {
+ if (!gtk_clipboard_set_with_owner(cb,
+ targets,
+ num_targets,
+ clipboard_get,
+ clipboard_clear,
+ G_OBJECT(self))) {
g_warning("clipboard grab failed");
return FALSE;
}

[Frediano Ziglio]

Post by Victor Toso
Not saying it is perfect name but 'i' as index does not state much
after the for loop in g_memdup() and gtk_clipboard_set_with_owner().
Using number of elements as indexes is far from unusual so let's
rename it and reduce by one the single letter vars.
Also note that the change in indentation on arguments of
gtk_clipboard_set_with_owner() while renaming 'i' -> 'num_targets'.
Follow up patch will rename some callbacks, so keeping one argument
per line would reduce slightly the change set of patch set.
---
src/spice-gtk-session.c | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index bf3c1fb..ef3faea 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -781,22 +781,22 @@ static gboolean clipboard_grab(SpiceMainChannel *main,
guint selection,
gboolean target_selected[SPICE_N_ELEMENTS(atom2agent)] = { FALSE, };
gboolean found;
GtkClipboard* cb;
- int m, n, i;
+ int m, n;
+ int num_targets = 0;
cb = get_clipboard_from_selection(s, selection);
g_return_val_if_fail(cb != NULL, FALSE);
- i = 0;
for (n = 0; n < ntypes; ++n) {
found = FALSE;
for (m = 0; m < SPICE_N_ELEMENTS(atom2agent); m++) {
if (atom2agent[m].vdagent == types[n] && !target_selected[m]) {
found = TRUE;
- g_return_val_if_fail(i < SPICE_N_ELEMENTS(atom2agent), FALSE);
- targets[i].target = (gchar*)atom2agent[m].xatom;
- targets[i].info = m;
+ g_return_val_if_fail(num_targets <
SPICE_N_ELEMENTS(atom2agent), FALSE);
+ targets[num_targets].target = (gchar*)atom2agent[m].xatom;
+ targets[num_targets].info = m;
target_selected[m] = TRUE;
- i += 1;
+ num_targets += 1;

Really minor, why not "num_targets++;" ?

Very OT: sometimes I miss the "with" statement from Pascal!

Post by Victor Toso
}
}
if (!found) {
@@ -806,8 +806,8 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
}
g_free(s->clip_targets[selection]);
- s->nclip_targets[selection] = i;
- s->clip_targets[selection] = g_memdup(targets, sizeof(GtkTargetEntry) * i);
+ s->nclip_targets[selection] = num_targets;
+ s->clip_targets[selection] = g_memdup(targets, sizeof(GtkTargetEntry) * num_targets);
/* Receiving a grab implies we've released our own grab */
s->clip_grabbed[selection] = FALSE;
@@ -817,8 +817,12 @@ static gboolean clipboard_grab(SpiceMainChannel *main,
guint selection,
return TRUE;
}
- if (!gtk_clipboard_set_with_owner(cb, targets, i,
- clipboard_get, clipboard_clear, G_OBJECT(self))) {
+ if (!gtk_clipboard_set_with_owner(cb,
+ targets,
+ num_targets,
+ clipboard_get,
+ clipboard_clear,
+ G_OBJECT(self))) {
g_warning("clipboard grab failed");
return FALSE;
}

Otherwise,
Acked-by: Frediano Ziglio <***@redhat.com>

Frediano

[Victor Toso]

Hi,

Post by Frediano Ziglio

Post by Victor Toso
for (m = 0; m < SPICE_N_ELEMENTS(atom2agent); m++) {
if (atom2agent[m].vdagent == types[n] && !target_selected[m]) {
found = TRUE;
- g_return_val_if_fail(i < SPICE_N_ELEMENTS(atom2agent), FALSE);
- targets[i].target = (gchar*)atom2agent[m].xatom;
- targets[i].info = m;
+ g_return_val_if_fail(num_targets <
SPICE_N_ELEMENTS(atom2agent), FALSE);
+ targets[num_targets].target = (gchar*)atom2agent[m].xatom;
+ targets[num_targets].info = m;
target_selected[m] = TRUE;
- i += 1;
+ num_targets += 1;

Really minor, why not "num_targets++;" ?

Why not indeed. I'll change it.

Post by Frediano Ziglio
Very OT: sometimes I miss the "with" statement from Pascal!

:)

Post by Frediano Ziglio

Post by Victor Toso
}
}
if (!found) {
@@ -806,8 +806,8 @@ static gboolean clipboard_grab(SpiceMainChannel *main,
guint selection,
}
g_free(s->clip_targets[selection]);
- s->nclip_targets[selection] = i;
- s->clip_targets[selection] = g_memdup(targets, sizeof(GtkTargetEntry) * i);
+ s->nclip_targets[selection] = num_targets;
+ s->clip_targets[selection] = g_memdup(targets, sizeof(GtkTargetEntry) *
num_targets);
/* Receiving a grab implies we've released our own grab */
s->clip_grabbed[selection] = FALSE;
@@ -817,8 +817,12 @@ static gboolean clipboard_grab(SpiceMainChannel *main,
guint selection,
return TRUE;
}
- if (!gtk_clipboard_set_with_owner(cb, targets, i,
- clipboard_get, clipboard_clear,
G_OBJECT(self))) {
+ if (!gtk_clipboard_set_with_owner(cb,
+ targets,
+ num_targets,
+ clipboard_get,
+ clipboard_clear,
+ G_OBJECT(self))) {
g_warning("clipboard grab failed");
return FALSE;
}

Otherwise,
Frediano

Thanks,
Victor