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

[Victor Toso]

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!

>
>
> > 
> > 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;
> >      }
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170524/2e12c29b/attachment.sig>