Makes sense to me.  I think we want to encourage the idea that further typing increases refinement.

Note that the way it works (or is supposed to work anyways) is that hits with more matches sort before other hits, so in the example, the result will be:

 ab.txt
 a.txt
 b.txt

The idea was to get AND behavior, but to be forgiving if some of the terms are wrong.

(The division into sections may obscure this a bit, since we sort individually within each section)

Created attachment 163437 [details] [review]
[search] Use 'AND' instead of 'OR' on search terms

The current search system uses the OR operator to concatenate search
terms. While results which are matched multiple times sort before
other matches, it is almost guaranteed that adding an additional term
to the search increments the number of results, which is rather
surprising.

The patch does only change the actual matching, the sorting of search results including the ordering by multiple, prefix and substring matches is left as-is. As AND implies that a result has to match every term, the signification of MULTIPLE is changed to refer to multiple matches of a single term - currently this is only meaningful for applications, where we match name, description and executable.

Works here.  Marina?

Review of attachment 163437 [details] [review]:

The way you use MATCH_MULTIPLE right now is that it will only be the case if the name and the executable *start* with the search term. I think it is an ok way to use it, but it requires a comment about it in shell-app-system.c

I also looked over the search functions in placeDisplay.js, docDisplay.js, and docInfo.js and think there is a lot of duplicated code there.

At the very least we can merge initialSearch() and subsearch() in docInfo.js into searchDocs() similar to _searchPlaces() in placeDisplay.js If we definitely know that multipleResults array will be empty, I think it is best to remove it in both places.

We can also create a static function matchTerms(string, terms) in search.js that is used from matchTerms() in placeDisplay.js and in docInfo.js

It seems that creating a static function like getMatchingItems(items, stringToMatchFunction, terms) that can be used from getInitialResultSet() and getSubserachResultSet() in placeDisplay.js and in docDisplay.js might be a little tricky (because we'd need to define stringToMatchFunction that returns a string we want to match for an item and because we'd need to later map the item results to ids/uris respectively). It's up to you if you think it makes sense to do that too to remove duplication in _searchPlaces() and searchDocs().

::: src/shell-app-system.c
@@ +751,3 @@
         {
+          if (current_match != MATCH_NONE)
+            current_match = MATCH_MULTIPLE;

current_match is always MATCH_NONE at this point

@@ +767,2 @@
          }
       else if (p != NULL)

&& current_match == MATCH_NONE

otherwise it can potentially be a downgrade from MATCH_PREFIX

@@ +772,3 @@
+        {
+          p = strstr (info->casefolded_description, term);
+          if (p != NULL)

&& current_match == MATCH_NONE

otherwise it can potentially be a downgrade from MATCH_PREFIX or MATCH_MULTIPLE

@@ +777,3 @@
+
+      if (current_match == MATCH_NONE)
+          return current_match;

indentation

@@ +779,3 @@
+          return current_match;
+
+      if (match == MATCH_NONE || match > current_match)

Why would you only downgrade here? Can it be "if (match < current_match)" instead?

Created attachment 165576 [details] [review]
[search] Use 'AND' instead of 'OR' on search terms

(In reply to comment #5)
> The way you use MATCH_MULTIPLE right now is that it will only be the case if
> the name and the executable *start* with the search term. I think it is an ok
> way to use it, but it requires a comment about it in shell-app-system.c

Ugh. No, it should really mean "matching more than one property" now. Fixed (hopefully).


> At the very least we can merge initialSearch() and subsearch() in docInfo.js
> into searchDocs() similar to _searchPlaces() in placeDisplay.js

Done.


> If we definitely know that multipleResults array will be empty, I think it
> is best to remove it in both places.

I'd like to keep it, because it is in line with the documentation in search.js and shell-app-system.c. Right now it is always empty because we are matching on a single property, but that might change - if it does, I think it is nice to have a reminder in place for how it's supposed to work.


> We can also create a static function matchTerms(string, terms) in search.js
> that is used from matchTerms() in placeDisplay.js and in docInfo.js

Not sure if we should - the functions are identical, but I'd say that's rather a coincidence. If we start to use something more sophisticated for files (tracker/zeitgeist/whatever), this is likely to change. Even if we stay with gtk-recent, we could start using matching more stuff, e.g. the url as well as the display name for places, or the last application using a file for recent items.


> Why would you only downgrade here? Can it be "if (match < current_match)"
> instead?

It's not a downgrade, it's the weird enum ordering - I changed the enum to be in priority order now (both C and js).

Created attachment 165588 [details] [review]
[search] Use 'AND' instead of 'OR' on search terms

Some forgotten commata;
Also the simple code sharing in docInfo was wrong - fixed the function call.

Review of attachment 165588 [details] [review]:

The code looks good, except that it fails the Firefox test! :) Which means that Firefox is not the first option when I type "f" or even "fi". This happens because we value MATCH_MULTIPLE over MATCH_PREFIX and a combination of "ConFIguration Editor", "Directly edit your entire conFIguration database" gets MATCH_MULTIPLE, while "FIrefox", "Browse the web" gets MATCH_PREFIX.

Let's value MATCH_PREFIX over MATCH_MULTIPLE and use MATCH_MULTIPLE to mean multiple substring matches across different criteria. That means that we'll need to update the logic in shell-app-system.c and concatenation order in all the relevant places.

Another thing I noticed is that we assign MATCH_PREFIX on either name match or on executable name match for applications, and there is no way to differentiate between the two. The results might be confusing to the user in the case when the first result matched on the executable, but other application names match the search string better. I think that's ok for now.

::: js/ui/search.js
@@ +125,1 @@
      *  * Put items which match on a prefix before non-prefix substring matches

The comment will need to be updated here if we prefer items that match on a prefix to the ones that match on substrings from multiple criteria.

Created attachment 168177 [details] [review]
[search] Use 'AND' instead of 'OR' on search terms

Split MATCH_MULTIPLE into MATCH_MULTIPLE_PREFIX and MATCH_MULTIPLE_SUBTRING, where the former translates to multiple matches including at least one MATCH_PREFIX and the latter to multiple MATCH_SUBTRING matches. The resulting order becomes then MATCH_NONE, MATCH_SUBTRING, MATCH_MULTIPLE_SUBTRING, MATCH_PREFIX, MATCH_MULTIPLE_PREFIX - that way multiple matches still get higher priority, while making the preference of prefix matches over subtring matches clearer. The net result is that the firefox test works again!

Review of attachment 168177 [details] [review]:

Woohoo! The Firefox test works again :)!

Now that you've implemented it, I really like how you have both MATCH_MULTIPLE_PREFIX and MATCH_MULTIPLE_SUBTRING.

Good to commit with one small fix below.

::: src/shell-app-system.c
@@ +760,1 @@
+      if (info->casefolded_description)

This should be
if (info->casefolded_description && current_match < MATCH_PREFIX)
otherwise this can downgrade.
You can also add a comment that matching on the prefix of the description is not meaningful enough, so we don't check if the term matches the prefix of the description.

Attachment 168177 [details] pushed as a9c0dcb - [search] Use 'AND' instead of 'OR' on search terms