Created attachment 144618 [details] [review]
Match on menu category during application search

Being able to display all applications in a category based on the search string with a category name is generaly useful.
    
Remove unused this._appCategories and a call to non-existing itemInfo.get_categories()

Comment on attachment 144618 [details] [review]
Match on menu category during application search

>-            if (category.indexOf(search) >= 0)
>+        for (let i = 0; i < this._menus.length; i++) {
>+            let menuItem = this._menus[i];
>+            // Match only on the beginning of the words in category names,
>+            // because otherwise it introduces unnecessary noise in the results.
>+            if ((menuItem.name.toLowerCase().indexOf(search) == 0 ||
>+                 menuItem.name.toLowerCase().indexOf(" " + search) > 0)
>+                && this._isInfoMatchingMenu(itemInfo, this._appSystem.get_applications_for_menu(menuItem.id)))
>                 return true;

I am confused; get_applications_for_menu returns an array, but _isInfoMatchingMenu expects a string.  Moreover, as I mentioned in the other bug get_applications_for_menu is not cached, and calling it once per item (as _isInfoMatchingSearch is used) is going to be **extremely** expensive.

The way I'd expect this to work is that we keep a cache of the menu->[applications] somewhere (ShellAppSystem would be good).  Next we have a function _getMatchingMenusForSearch(search) which returns a list of menus that match that search string (really this should only be computed once at the start of a search, not per-item).

Then inside _is_infoMatchingSearch, we can just check whether the app's id is inside the list of applications for any of the menus which are matched.

(In reply to comment #2)
>> The way I'd expect this to work is that we keep a cache of the
> menu->[applications] somewhere (ShellAppSystem would be good).  

I'm workign on this now

Created attachment 144651 [details] [review]
Match on menu category during application search

The new approach prepares the list of applications that match the search term based on their menu name up-front. This is done in the setSearch() override in AppDisplay so that to not add an extra method to the GenericDisplay interface.

Review of attachment 144651 [details] [review]:

I think there are more efficient and slightly simpler ways to do this.

::: js/ui/appDisplay.js
@@ +179,3 @@
         this._menuDisplays = [];
+        // map<search term, applications from menus that match the search term>
+        this._menuSearchAppMatches = {};

I think it would make more sense to do this as a map from 

 applicationId => true

that is a "set" of application IDs - so in setSearch(), you'd just loop over the menus, see if they match the set of terms, and if so add all the items in the menu to the set.

The algorithm you are using has complexity (# of apps in matched categories) x (total # of apps)

@@ +271,3 @@
+                if (menuItem.name.toLowerCase().indexOf(term) == 0 ||
+                    menuItem.name.toLowerCase().indexOf(" " + term) > 0)
+                    this._menuSearchAppMatches[term] = this._menuSearchAppMatches[term].concat(this._appSystem.get_applications_for_menu(menuItem.id));

It's probably somewhat theoretically better to do:

 this._menuSearchMatches[term].push.apply(this._menuSearchMatches[term], 
                                          this._appSystem.get_applications_for_menu(menuItem.id));

though I can't say that's any prettier to read. I might actually add 

 function _pushItems(array, items) {
    array.push.apply(array, items);
 }

as a helper at global scope, even if it's just used in one place. (But not needed if you take my approach above.)

Review of attachment 144651 [details] [review]:

In terms of how this fits into the overall search framework (which is "term matches items"), two options:

 A) You could refactor the outer code to allow appDisplay to modify that (and either give a score bump of one for any term match, or to get really fancy, make the map I suggested earlier a map not to => true but to => [matched terms count]. But do we even use the score for sorting? I only have one multiword category anyways "Sound & Video" so I don't think trying to get the details of that right is worth code,)

 B) You could use a map of maps

Created attachment 144928 [details] [review]
Match on menu category during application search

The new patch uses map of maps that contain application ids that match a given search term to improve the lookup in _isInfoMatchingSearch()

Review of attachment 144928 [details] [review]:

Looks good to me