Re: [Xfce4-commits] r30225 - in xfce4-appfinder/trunk: . src

[Brian J. Tarricone-3]

On 2009/07/08 17:01, Jannis Pohlmann wrote:
> Log:
> * src/xfce-appfinder-window.c: Don't use startup notification for now.
>  As we use exo-open to launch desktop entries, people usually end up
>  with annoying waiting cursors and a lot of temporary exo-open items
>  in the taskbar.

Doesn't appfinder launch apps based on their .desktop file?  That will
tell you if you should be using SN or not.  If the app has an incorrect
.desktop file, bug the app author...

        -b
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Nick Schermer]

2009/7/9 Jannis Pohlmann <jannis@...>:
> -  if (G_UNLIKELY (!xfce_exec_on_screen (screen, command, FALSE, TRUE, &error)))
> +  if (G_UNLIKELY (!xfce_exec_on_screen (screen, command, FALSE, FALSE, &error)))

Agree with Brian here.

> +  if (G_LIKELY (categories != NULL))
> +    {
> +      categories_array = g_new0 (const gchar *, g_list_length (categories) + 1);
> +
> +      for (lp = categories, n = 0; lp != NULL; lp = lp->next, ++n)
> +        categories_array[n] = lp->data;
> +
> +      categories_string = g_strjoinv (", ", (gchar **) categories_array);
> +      g_string_append_printf (tooltip_str, _("<b>Categories:</b> %s\n"), categories_string);
> +      g_free (categories_string);
> +
> +      g_free (categories_array);
> +    }

This code is looks crap, it uses a wrong type, creates an extra array
only because it wants to use g_strjoinv?

Nick
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Colin Leroy-2]

On Thu, 9 Jul 2009 08:25:01 +0200, Nick Schermer wrote:

Hi,

> > +      categories_array = g_new0 (const gchar *, g_list_length
> > (categories) + 1); +
> > +      for (lp = categories, n = 0; lp != NULL; lp = lp->next, ++n)
> > +        categories_array[n] = lp->data;

gchar *tmp = NULL, *categories_string = NULL;

for (lp = categories; lp != NULL; lp = lp->next)
  {
    tmp = g_strdup_printf("%s%s%s",
             categories_string ? categories_string : "",
             categories_string ? ", " : "",
             lp->data);
    g_free(categories_string);
    categories_string = tmp;
  }

(Yeah, I feel like coding these days :)
--
Colin
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Nick Schermer]

2009/7/9 Colin Leroy <colin@...>:

> On Thu, 9 Jul 2009 08:25:01 +0200, Nick Schermer wrote:
>
> Hi,
>
>> > +      categories_array = g_new0 (const gchar *, g_list_length
>> > (categories) + 1); +
>> > +      for (lp = categories, n = 0; lp != NULL; lp = lp->next, ++n)
>> > +        categories_array[n] = lp->data;
>
> gchar *tmp = NULL, *categories_string = NULL;
>
> for (lp = categories; lp != NULL; lp = lp->next)
>  {
>    tmp = g_strdup_printf("%s%s%s",
>             categories_string ? categories_string : "",
>             categories_string ? ", " : "",
>             lp->data);
>    g_free(categories_string);
>    categories_string = tmp;
>  }
>

That's even worse... Hint: eventually you append to a GString.

Nick
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Brian J. Tarricone-3]

On 2009/07/08 23:46, Colin Leroy wrote:

> On Thu, 9 Jul 2009 08:25:01 +0200, Nick Schermer wrote:
>
> Hi,
>
>>> +      categories_array = g_new0 (const gchar *, g_list_length
>>> (categories) + 1); +
>>> +      for (lp = categories, n = 0; lp != NULL; lp = lp->next, ++n)
>>> +        categories_array[n] = lp->data;
>
> gchar *tmp = NULL, *categories_string = NULL;
>
> for (lp = categories; lp != NULL; lp = lp->next)
>    {
>      tmp = g_strdup_printf("%s%s%s",
>               categories_string ? categories_string : "",
>               categories_string ? ", " : "",
>               lp->data);
>      g_free(categories_string);
>      categories_string = tmp;
>    }

An extra allocation for each list item?  Ick.  Better to just iterate
over the list twice; figure out the total strlen in the first pass,
allocate a buffer a single time, and then do the second pass and copy
the chars to the new buffer.  (Hell, the original code iterates over the
list twice [g_list_length() is O(n)] already anyway.)

Or just mostly use the original code and do g_string_append_printf() for
each list item.  Yah, you get more allocs that way, but GString does a
better job than just doing it manually, usually.

The original code, isn't actually that bad, depending on how
g_strjoinv() is implemented.  But a single malloc() for categories_array
is likely to be slower than a pass over the entire list with a strlen()
on each element.

Eh, I dunno.  Performance could go either way depending on the number of
list entries (which I'd figure would usually be 4-6 in the average case,
maybe worst case around 15).

        -b
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Jasper Huijsmans]

> Eh, I dunno.  Performance could go either way depending on the number of
> list entries (which I'd figure would usually be 4-6 in the average case,
> maybe worst case around 15).
>

Performance. Right. Might I suggest no-one will ever notice the difference? ;-)

Anyway, I think, I'd prefer red[1] for this particular bike shed...

cheers,
        Jasper

[1]  with red I mean Brian's solution of iterating the list twice in
order to allow allocating a buffer only once, which is obviously the
best solution ;-)
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Brian J. Tarricone-3]

On 2009/07/09 00:17, Jasper Huijsmans wrote:

> [1]  with red I mean Brian's solution of iterating the list twice in
> order to allow allocating a buffer only once, which is obviously the
> best solution ;-)

Do I get a cookie?

        -b
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Nick Schermer]

2009/7/9 Brian J. Tarricone <brian@...>:
> On 2009/07/09 00:17, Jasper Huijsmans wrote:
>
>> [1]  with red I mean Brian's solution of iterating the list twice in
>> order to allow allocating a buffer only once, which is obviously the
>> best solution ;-)
>
> Do I get a cookie?

/me gives Brian a cookie.

Nick
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Nick Schermer]

My take on this one:

tooltip_str = g_string_sized_new (512);

categories = xfce_menu_item_get_categories (item);
 if (categories != NULL)
   {
      g_string_append (tooltip_str, _("<b>Categories:</b> "));

      for (lp = categories; lp != NULL; lp = lp->next)
        {
          g_string_append (tooltip_str, (gchar *) lp->data);
          if (lp->next != NULL)
            g_string_append (tooltip_str, ", ");
        }
    }

command = xfce_menu_item_get_command (item);
if (command != NULL && *command != '\0')
  {
    if (categories != NULL)
       g_string_append_c (tooltip_str, '\n');

    g_string_append_printf (tooltip_str, _("<b>Command:</b> %s"), command);
  }

Nick
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Jannis Pohlmann-3]

On Wed, 08 Jul 2009 18:39:40 -0700
"Brian J. Tarricone" <brian@...> wrote:

> On 2009/07/08 17:01, Jannis Pohlmann wrote:
> > Log:
> > * src/xfce-appfinder-window.c: Don't use startup
> > notification for now. As we use exo-open to launch desktop entries,
> > people usually end up with annoying waiting cursors and a lot of
> > temporary exo-open items in the taskbar.
>
> Doesn't appfinder launch apps based on their .desktop file?  That
> will tell you if you should be using SN or not.  If the app has an
> incorrect .desktop file, bug the app author...

Not yet. It simply calls "exo-open /foo/bar/baz.desktop". I know this
is bad and should be changed. I don't even know why I didn't do that in
the first place. Maybe because it was so damn simple to just use
exo-open.

  - Jannis


_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Jannis Pohlmann-3]

On Thu, 9 Jul 2009 11:09:32 +0200
Nick Schermer <nickschermer@...> wrote:

> My take on this one:
>
> tooltip_str = g_string_sized_new (512);
>
> categories = xfce_menu_item_get_categories (item);
>  if (categories != NULL)
>    {
>       g_string_append (tooltip_str, _("<b>Categories:</b> "));

I used _("<b>Categories:</b> %s") in the original code because
translators might want to change the place where the categories list is
inserted.

> command = xfce_menu_item_get_command (item);
> if (command != NULL && *command != '\0')
>   {
>     if (categories != NULL)
>        g_string_append_c (tooltip_str, '\n');

Yeah, this is something I forgot about. Let me rework the code a little
bit.

  - Jannis


_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Jannis Pohlmann-3]

On Thu, 09 Jul 2009 00:01:56 -0700
"Brian J. Tarricone" <brian@...> wrote:

> On 2009/07/08 23:46, Colin Leroy wrote:
> > On Thu, 9 Jul 2009 08:25:01 +0200, Nick Schermer wrote:
> >
> > Hi,
> >
> >>> +      categories_array = g_new0 (const gchar *, g_list_length
> >>> (categories) + 1); +
> >>> +      for (lp = categories, n = 0; lp != NULL; lp = lp->next,
> >>> ++n)
> >>> +        categories_array[n] = lp->data;
> >
> > gchar *tmp = NULL, *categories_string = NULL;
> >
> > for (lp = categories; lp != NULL; lp = lp->next)
> >    {
> >      tmp = g_strdup_printf("%s%s%s",
> >               categories_string ? categories_string : "",
> >               categories_string ? ", " : "",
> >               lp->data);
> >      g_free(categories_string);
> >      categories_string = tmp;
> >    }
>
> An extra allocation for each list item?  Ick.  Better to just iterate
> over the list twice; figure out the total strlen in the first pass,
> allocate a buffer a single time, and then do the second pass and copy
> the chars to the new buffer.  (Hell, the original code iterates over
> the list twice [g_list_length() is O(n)] already anyway.)
>
> Or just mostly use the original code and do g_string_append_printf()
> for each list item.  Yah, you get more allocs that way, but GString
> does a better job than just doing it manually, usually.
>
> The original code, isn't actually that bad, depending on how
> g_strjoinv() is implemented.  But a single malloc() for
> categories_array is likely to be slower than a pass over the entire
> list with a strlen() on each element.

I've kept the original code with minor modifications. g_strjoinv() is
the most elegant way to join several strings using a delimiter.
Internally, it iterates over the string array twice doing a malloc only
once. It basically does what you described (iterate to get the total
buffer size needed, then copy all the strings with delimiters into the
buffer). No need to re-implement that ourselves.

Using g_strjoinv() also allows the "<b>Categories:</b> %s" string to be
translated rather than just "<b>Categories:</b>". I don't know how
important it is, but I think it's better this way.

I don't really see any reason to change that code more than I did now.
It's very simple, it's easy to understand (after all, it just builds an
array of pointers to pass to g_strjoinv) and with about 4-15
categories, do we really care so much about how often we iterate over
the list and how many memcpy/malloc calls we have?

(Side note: Most of this mail is not directed at you but rather at
 Nick who started this thread. Personall, I think the exo-open issue is
 far more important.)

  - Jannis


_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Nick Schermer]

2009/7/9 Jannis Pohlmann <jannis@...>:
> I don't really see any reason to change that code more than I did now.
> It's very simple, it's easy to understand (after all, it just builds an
> array of pointers to pass to g_strjoinv) and with about 4-15
> categories, do we really care so much about how often we iterate over
> the list and how many memcpy/malloc calls we have?

Fine by me, but when I read the code I was a bit disappointed this
enters trunk, that's all.

Nick
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev