Created attachment 151253 [details] [review]
[search] Match on executable names as well

Include matches on desktop entries' exec property in addition to the
name and comment ones.

Review of attachment 151253 [details] [review]:

One comment:

::: src/shell-app-system.c
@@ +629,3 @@
   comment = gmenu_tree_entry_get_comment ((GMenuTreeEntry*)info->entry);
+  exec = gmenu_tree_entry_get_exec ((GMenuTreeEntry*)info->entry);
+

A question to me here is whether we want to do any trimming; for example in my /usr/share/applications I see a few:

/usr/share/applications/manage-print-jobs.desktop:Exec=system-config-printer-applet --no-tray-icon
/usr/share/applications/chromium-browser.desktop:Exec=chromium-browser --enable-plugins --enable-extensions --enable-user-scripts --enable-printing --enable-sync --auto-ssl-client-auth

I don't think we want the first to match "icon" or the second to match "printing" for example.

How about taking everything before the first space, then inside that after '/' (if exists, otherwise all).

Created attachment 151293 [details] [review]
Match on executable names as well

Good point. Updated patch to options and pathnames from the exec line.

Review of attachment 151293 [details] [review]:

::: src/shell-app-system.c
@@ +632,3 @@
+  if ((pos = strchr (exec, ' ')))
+  exec = gmenu_tree_entry_get_exec ((GMenuTreeEntry*)info->entry);
+

You can't do that on a "const char *"; basically need to strdup here.

Created attachment 151463 [details] [review]
Match on executable names as well

Changes from last patch:

* don't mess with const strings

* use g_utf8_* instead of standard string functions

* copy only the chunk of interest instead of modifying
  the string in place

Review of attachment 151463 [details] [review]:

::: src/shell-app-system.c
@@ +625,3 @@
+{
+trim_exec_line (const char *str)
+static char *

Consider the case of:

Exec=/usr/bin/foo --data=/usr/lib/foodata

then slash_pos will be greater than space_pos.

@@ +629,3 @@
+{
+trim_exec_line (const char *str)
+static char *

And then this will potentially overflow the string.

@@ +631,3 @@
+{
+trim_exec_line (const char *str)
+static char *

How about (not tested):

static char *
trim_exec_line (char *str)
{
  char *start, *end;
  long len;

  end = g_utf8_strchr (str, -1, ' ');
  if (end == NULL)
    end = str + strlen (str);

  len = g_utf8_strlen (str, end - str);

  start = g_utf8_strrchr (str, len, '/');
  if (start == NULL)
    start = str;

  if (end < start)
    return g_strndup (str, end - str);

  return g_strndup (start, end - start);
}

If this is wrong maybe we should break down and use GRegex...

(In reply to comment #6)
> Review of attachment 151463 [details] [review]:
> 
> Consider the case of:
> 
> Exec=/usr/bin/foo --data=/usr/lib/foodata
> 
> then slash_pos will be greater than space_pos.

No, it won't. By passing an appropriate length argument to g_utf8_strrchr(), the original string is searched for the last occurrence of a slash within *length* characters.

So while this case works correctly, I'll try to make the code clearer (or at least add a comment).


> How about (not tested):
> 
> static char *
> trim_exec_line (char *str)
> {
>   char *start, *end;
>   long len;
> 
>   end = g_utf8_strchr (str, -1, ' ');
>   if (end == NULL)
>     end = str + strlen (str);
> 
>   len = g_utf8_strlen (str, end - str);
> 
>   start = g_utf8_strrchr (str, len, '/');

As len is the string length up to end, start is either NULL or points to the last '/' between str and end ...


>   if (end < start)
>     return g_strndup (str, end - str);

... this condition never evaluates to TRUE.


> If this is wrong maybe we should break down and use GRegex...

Yeah, we could - though I'd guess that GRegex will be a little slower than direct string manipulation ...

(In reply to comment #7)
> (In reply to comment #6)
> > Review of attachment 151463 [details] [review] [details]:
> > 
> > Consider the case of:
> > 
> > Exec=/usr/bin/foo --data=/usr/lib/foodata
> > 
> > then slash_pos will be greater than space_pos.
> 
> No, it won't. By passing an appropriate length argument to g_utf8_strrchr(),
> the original string is searched for the last occurrence of a slash within
> *length* characters.

Ah, OK, right.  Sorry for the bad review then.

(In reply to comment #6)

>   end = g_utf8_strchr (str, -1, ' ');

Without actually reviewing the code, let me just comment here that in general I wouldn't using g_utf8_strchr() unless I was searching for a non-ASCII character.

Created attachment 151501 [details] [review]
Match on executable names as well

(In reply to comment #8)
> Ah, OK, right.  Sorry for the bad review then.

Hey, no offense taken - just shows that the code was not quite clear enough,
right?

So here's a rewrite of trim_exec_line(), which is a whole lot simpler. While
the previous version did work, this is a solution I actually like - I hope
you agree ;)

I also replaced all g_utf8_*() with standard string functions - it should
work, but then my knowledge of utf8 is quite limited ...

Review of attachment 151501 [details] [review]:

One small comment, otherwise looks good, please commit this now =)

::: src/shell-app-system.c
@@ +617,3 @@
+{
+trim_exec_line (const char *str)
+static char *

Why not just define all of these as "const char *"?  Then you can avoid the casts.

Attachment 151501 [details] pushed as f8acd0f - Match on executable names as well