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

[Jonathon Jongsma]

On Wed, 2017-05-24 at 18:54 +0200, Victor Toso wrote:
> Hi,
> 
> On Wed, May 24, 2017 at 11:36:55AM -0500, Jonathon Jongsma wrote:
> > 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?
> 
> Yes!
> 
> > 
> > > 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"
> 
> Sure
> 
> > 
> > > - 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?
> 
> Yes!
> 
> Does it look better?
> 
> "
> gtk-session: use clear variable for array's size
> 
> By using 'num_types' variables, we have a clear variable with a clear
> purpose: It will track the number of VD_AGENT_CLIPBOARD types we are
> storing in types[] array.
> 
> This new variable helps:
> - removing one 'for' loop which was counting the number stored types;
> - doing fewer iterations in one 'for' loop
> 
> 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>
> Acked-by: Frediano Ziglio <fziglio at redhat.com>
> "
> 
> Many thanks!


Looks good to me!

Jonathon


> 
> > 
> > 
> > > 
> > > 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;
> > >      }