GNOME Bugzilla – Bug 711557
Add g_desktop_app_info_search()
Last modified: 2013-11-07 17:41:55 UTC
The long-awaited. See patches.
Created attachment 259094 [details] [review] Add g_desktop_app_info_search() The first time this function is called we load all of the keyfiles in the directory, ignoring the 'Hidden' ones and build an index out of the interesting fields using g_str_tokenize_and_fold(). We do prefix matching on the tokens to find relevent desktop files. Right now this is implemented as a hashtable that we iterate over, checking prefixes on each token. This could possibly be sped up by creating an array, but it's already pretty fast...
Created attachment 259095 [details] [review] appinfo: Add some testcases for searching
Review of attachment 259095 [details] [review]: If this is going to be an automated / installed test, we need to dist the desktop files, right?
Review of attachment 259094 [details] [review]: ::: gio/gdesktopappinfo.c @@ +250,3 @@ +enum +{ + DESKTOP_KEY_nil, What's the point of nil here? @@ +274,3 @@ + DESKTOP_KEY_Type, + DESKTOP_KEY_Version, + DESKTOP_KEY_X_GNOME_FullName, So, it doesn't seem we use any of the items in this array unless they're a searchable category. I assume this is for the index eventually, but can we reduce this list to the five used below, and expand it when we add the index? @@ +282,3 @@ + +const gchar desktop_key_match_category[N_DESKTOP_KEYS] = { + /* NB: no harm in repeating numbers in case of a tie */ I don't understand this comment. @@ +342,3 @@ + * each directory) + */ +struct result I know it's internal, but "search_result" ?
(In reply to comment #5) > + DESKTOP_KEY_nil, > > What's the point of nil here? Good question -- I don't remember. I wanted to have 0 mean that there was no match, but this is handled in the match categories below; shouldn't be any need for it in the list of keys. > @@ +274,3 @@ > + DESKTOP_KEY_Type, > + DESKTOP_KEY_Version, > + DESKTOP_KEY_X_GNOME_FullName, > > So, it doesn't seem we use any of the items in this array unless they're a > searchable category. I assume this is for the index eventually, but can we > reduce this list to the five used below, and expand it when we add the index? OK. > > @@ +282,3 @@ > + > +const gchar desktop_key_match_category[N_DESKTOP_KEYS] = { > + /* NB: no harm in repeating numbers in case of a tie */ > > I don't understand this comment. Basically, if we consider that two keys are equivalently good for matching on, we can repeat the score for them. I'll elaborate a bit in the comment. > > @@ +342,3 @@ > + * each directory) > + */ > +struct result > > I know it's internal, but "search_result" ? Sure...
Created attachment 259200 [details] [review] Add g_desktop_app_info_search() The first time this function is called we load all of the keyfiles in the directory, ignoring the 'Hidden' ones and build an index out of the interesting fields using g_str_tokenize_and_fold(). We do prefix matching on the tokens to find relevent desktop files. Right now this is implemented as a hashtable that we iterate over, checking prefixes on each token. This could possibly be sped up by creating an array, but it's already pretty fast...
Review of attachment 259200 [details] [review]: This is reads nicely with all the comments. Thanks. ::: gio/gdesktopappinfo.c @@ +30,3 @@ #include <string.h> #include <unistd.h> +#include <locale.h> I don't think you use this anywhere. @@ +259,3 @@ +}; + +const gchar desktop_key_match_category[N_DESKTOP_KEYS] = { This seems to be more complex than it needs to be. Why aren't you using a strv sorted by level? Do you think we'll ever have the case where two keys have the same level? @@ +395,3 @@ + if G_UNLIKELY (static_search_results_allocated < static_token_results_size) + { + static_search_results_allocated = static_token_results_allocated; static_search_results_allocated = static_token_results_size; @@ +478,3 @@ + +static void +merge_directory_results (void) This doesn't merge, it appends. @@ +693,3 @@ + + if (!desktop_key_match_category[i]) + continue; This can never happen. @@ +3485,3 @@ + * applications that matched @search_string with an equal score. The + * outer list is sorted by score so that the first strv contains the + * best-matching applications, and so on. Since we're not telling consumers of this API what those scores represent and how they relate to each other, wouldn't it be enough to just return a gchar** sorted by score? It would certainly simplify using this function a bit.
(In reply to comment #8) > +#include <locale.h> > > I don't think you use this anywhere. setlocale() in True. This only gets used later, once the desktop index stuff is added in. Removed for now. > @@ +259,3 @@ > +}; > + > +const gchar desktop_key_match_category[N_DESKTOP_KEYS] = { > > This seems to be more complex than it needs to be. Why aren't you using a strv > sorted by level? Do you think we'll ever have the case where two keys have the > same level? This will make more sense once the desktop file index code shows up. Once we have that, we need a fast way to go from key ID (as found in the file) to priority. Having the array map in this direction will allow that. > +static void > +merge_directory_results (void) > > This doesn't merge, it appends. It's equivalent -- due to the masking we will never have the same result appearing in multiple directories, so no actively merging needs to take place. > @@ +693,3 @@ > + > + if (!desktop_key_match_category[i]) > + continue; > > This can never happen. This is a result of me removing the unused entries because of Jasper's comment above. I want to leave this in for when I re-add them. > @@ +3485,3 @@ > + * applications that matched @search_string with an equal score. The > + * outer list is sorted by score so that the first strv contains the > + * best-matching applications, and so on. > > Since we're not telling consumers of this API what those scores represent and > how they relate to each other, wouldn't it be enough to just return a gchar** > sorted by score? > > It would certainly simplify using this function a bit. The reason that it's done this way is because gnome-shell wants to sort the results by most-used, but it doesn't want to do that across categories (ie: a match on the name should still be more powerful than a match on keywords, despite mostusedness).
Attachment 259095 [details] pushed as 6e0bbd8 - appinfo: Add some testcases for searching Attachment 259200 [details] pushed as 3d32d53 - Add g_desktop_app_info_search()