[Spice-devel] [PATCH spice-gtk v3 1/4] gtk-session: use clear variable for array's size

[Jonathon Jongsma]

I see Frediano acked this, but there are a couple issues with the
commit log if you haven't pushed yet.

On Mon, 2017-05-22 at 20:08 +0200, Victor Toso wrote:
> From: Victor Toso <me at victortoso.com>
> 
> By using 'num_types' variables, we have a clear variable with a clear
> propose: It will track the number of VD_AGENT_CLIPBOARD types we are

I think you mean purpose instead of propose?

> storing in types[] array.
> 
> This new variable helps:
> - removing one for that was counting the number stored types;

"for that was" is a very confusing phrase. It took me about 3 re-
readings (and a look at the patch) to decipher that the word "for" here
referred to the C loop keyword. Perhaps consider "loop that was" or
"for loop that was"

> - reducing one for to the size of 'num_types'

I guess here you're trying to say that you're doing fewer iterations in
a 'for' loop, right? 


> 
> A few extra comments were included to clarify what the logic should
> be
> doing and a extra debug was included to point out situations where
> the
> desktop has sent us valid clipboard data but none will be sent the
> guest.
> 
> Signed-off-by: Victor Toso <victortoso at redhat.com>
> Signed-off-by: Victor Toso <me at victortoso.com>
> ---
>  src/spice-gtk-session.c | 34 +++++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index cfdfc4c..83eaa3e 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -616,6 +616,7 @@ static void clipboard_get_targets(GtkClipboard
> *clipboard,
>  
>      SpiceGtkSessionPrivate *s = self->priv;
>      guint32 types[SPICE_N_ELEMENTS(atom2agent)] = { 0 };
> +    gint num_types;
>      char *name;
>      int a, m, t;
>      int selection;
> @@ -635,38 +636,41 @@ static void clipboard_get_targets(GtkClipboard
> *clipboard,
>          }
>      }
>  
> +    /* Set all Atoms that matches our current protocol
> implementation */
> +    num_types = 0;
>      for (a = 0; a < n_atoms; a++) {
>          name = gdk_atom_name(atoms[a]);
>          for (m = 0; m < SPICE_N_ELEMENTS(atom2agent); m++) {
>              if (strcasecmp(name, atom2agent[m].xatom) != 0) {
>                  continue;
>              }
> -            /* found match */
> -            for (t = 0; t < SPICE_N_ELEMENTS(atom2agent); t++) {
> +
> +            /* check if type is already in list */
> +            for (t = 0; t < num_types; t++) {
>                  if (types[t] == atom2agent[m].vdagent) {
> -                    /* type already in list */
> -                    break;
> -                }
> -                if (types[t] == 0) {
> -                    /* add type to empty slot */
> -                    types[t] = atom2agent[m].vdagent;
>                      break;
>                  }
>              }
> -            break;
> +
> +            if (types[t] == 0) {
> +                /* add type to empty slot */
> +                types[t] = atom2agent[m].vdagent;
> +                num_types++;
> +            }
>          }
>          g_free(name);
>      }
> -    for (t = 0; t < SPICE_N_ELEMENTS(atom2agent); t++) {
> -        if (types[t] == 0) {
> -            break;
> -        }
> +
> +    if (num_types == 0) {
> +        SPICE_DEBUG("No GdkAtoms will be sent from %d", n_atoms);
> +        return;
>      }
> -    if (!s->clip_grabbed[selection] && t > 0) {
> +
> +    if (!s->clip_grabbed[selection]) {
>          s->clip_grabbed[selection] = TRUE;
>  
>          if (spice_main_agent_test_capability(s->main,
> VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
> -            spice_main_clipboard_selection_grab(s->main, selection,
> types, t);
> +            spice_main_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;
>      }