Created attachment 145456 [details] [review]
Indicate matching strings in search results

Make matching strings bold in search results so that the user can tell why
certain items are showing up as matches.

Make sure that the details pane displayed also has the matching strings in bold,
which is particularly useful if the matching section is truncated in the regular
view of the item.

Review of attachment 145456 [details] [review]:

::: js/ui/docDisplay.js
@@ +42,3 @@
+        // At the moment, document description only has the time since last visited,
+        // so the text matches within it should not be highlighted.
+        this._indicateMatchesInDescription = false;

It feels messy to me to have the subclass just assign to this private variable. If I was reading genericDisplay.js, I would not expect to have code in another file changing the variable. Probably better to add a setter on GenericDisplay. Or maybe just add this to setItemInfo()/setDescriptionText(), since that's really the point at which the we know whether we want to highlight the matches in the description or not.

::: js/ui/genericDisplay.js
@@ +57,3 @@
+// Returns a markup string for the text that has all the places that match
+// the search argument in bold.
+function getMarkupWithMatches(text, search) {

What's the handling of search terms here? It seems that you are handling the search as a single string instead of a set of search terms as it is handled in other places?

@@ +64,3 @@
+     let markup = '';
+     let endOfMatchIndex = 0;
+     let matchIndex = text.toLowerCase().indexOf(search);

Better to do lowerText = text.toLowerCase() once than on everytime you find the next match.

Don't you want to lowercase the search as well in case the user actually types a lowercase search?

@@ +68,3 @@
+         if (matchIndex > 0) {
+            let nonMatchingText = text.substring(endOfMatchIndex, matchIndex);
+            markup = markup + GLib.markup_escape_text(nonMatchingText, -1);

I'd probably write:

 markup += GLib.markup_escape_text(...);

Might give Javascript the opportunity for some more optimization and in any case reads clearer as "append".

@@ +74,3 @@
+         endOfMatchIndex = matchIndex + search.length;
+         if (endOfMatchIndex < text.length)
+             matchIndex = text.toLowerCase().indexOf(search, endOfMatchIndex);

Don't think you need the special case

gjs> "abc".indexOf("a", 3)
-1

@@ +79,3 @@
+     }
+     if (endOfMatchIndex < text.length)
+         markup = markup + GLib.markup_escape_text(text.substring(endOfMatchIndex, text.length), -1);

Since you didn't special case the "first match at beginning", might as well not special case this either - this works fine when endOfMatchIndex == text.length.

@@ +181,1 @@
         this._detailsDescriptions = [];

Hmm, I don't see what ever removes things from these arrays, even when the names/details are destroyed. (And once that is fixed, isn't there only ever one detailName/detailDescription at a time?)

@@ +248,3 @@
                                                     line_wrap: true,
+                                                    use_markup: true,
+                                                    text: getMarkupWithMatches(this._descriptionText, this._search) });

this doesn't seem to honor this._indicateMachesInDescription?

@@ +346,3 @@
     // Sets the description text for the item, including the description text
     // in the details actors that have been created for the item.
     _setDescriptionText: function(text) {

The two code paths for setting the description text - here and in setItemInfo - feel awkward to me.

@@ +667,3 @@
         for (let itemId in this._displayedItems) {
             let item = this._displayedItems[itemId];
+            item.setSearch(this._search);

this is just as comment-worthy (or unworthy) as the place where you are setting the search on not-visible items below. If you want to comment it in only one place, better to do it on the first place and then say // see comment in redisplaySubSearch()

@@ +680,3 @@
     },
 
     _redisplayReordering: function() {

The function name isn't very good here - obviously not a new problem, but gets worse when the first comment in the function is about something that is unrelated to what the function is about. I think the best thing to do is to just move the loop-and-set-search into the caller; the efficiency win of reusing the loop in redisplaySubSearch() isn't that big. And that solves the double comment problem as well.

@@ +683,3 @@
+        // We need to be sure to update the search on all display items, even the ones we are not
+        // going to be displaying, because they might still have the details associated with them
+        // showing in a separate pane.

Hmm, is that the right behavior? should we ever be showing details for an item that isn't visible? I guess that might be useful for comparison.

Created attachment 145735 [details] [review]
Indicate matching strings in search results

We currently don't remove the details pane when the item is hidden because we are replacing the results pane with the details pane when browsing. This doesn't work well because there is no way to return to the results you've been browsing (other than clicking More again), but we should wait for a better design for the browse mode and details display before making more changes to it. We don't remove the details pane when hiding a search result either, but it possibly makes sense for comparison and can be decided on later based on the overall design for viewing details.

The details actor is destroyed in destroyContent() in dash.js We do that when the content is no longer displayed because we don't want to keep large image previews around. We might also need to do that after the item is destroyed if the details pane was open at that time. This could happen if the cache was refreshed and _recreateDisplayItems() in the GenericDisplay was called. However, this doesn't happen in practice because updates to the recent documents list or applications list don't typically happen while in the overview. This might change if say Firefox was adding downloads to the recent documents list as they finished downloading.

I updated the details code to ensure that there is at most one details actor around at a time.


Indicate matching strings in search results

Make matching strings bold in search results so that the user can tell why certain items are showing up as matches. Indicate all matching search terms in the results which reflects that we are applying OR to the search terms when displaying the results.

Make sure that the details pane displayed also has the matching strings in bold, which is particularly useful if the matching section is truncated in the regular view of the item.

Move details into a separate class so that it is easier to update details text and description and ensure that there is at most one details object associated with a given item.

Remove the unused resetState() function and reset _openDetailIndex when redisaplaying items in GenericItemDisplay.

Review of attachment 145735 [details] [review]:

Generally seems like a nice improvement over the last class, various comments in detail below. I think splitting out the Details class works well to make the structure of handling clearer.

::: js/ui/genericDisplay.js
@@ +67,3 @@
+        let term = terms[i];
+        if (i < terms.length - 1)
+            regExpString += term + '|';

What you've written here is equivalent to terms.join('|'); But there's something else you need to be careful of - term could potentially contain \ characters, which would cause compiling the regular expression to throw or behave weirdly.

Something like:

terms.map(function (x) { 
              return x.replace('\\', '\\\\')
          }).join("|")

should work.

@@ +75,3 @@
+    // We'll need the matches to be in lower case anyway,
+    // so it makes sense to use lowerCaseText rather than
+    // 'i' flag on regexp.

See comment below about looping over regexp matches - I don't think you actually need the lower case matches.

@@ +86,3 @@
+    for (let i = 0; i < matches.length; i++) {
+        let match = matches[i];
+        matchStartIndex = lowerCaseText.indexOf(match, matchEndIndex);

See:

https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Global_Objects/RegExp/exec

for a better way to loop over matches;

while ((match = regExp.exec(lowerCaseText)) != null)
{
  /* Do something with match.index, etc. */
}

@@ +105,3 @@
+
+        this.actor.connect('destroy', Lang.bind(this, function () {
+            this.actor = null;

It would make more sense for me for GenericDisplayItem to connect to _details.actor.destroy and null out _details, it seems awkward to me to leave around a "broken" shell Details, and have to check 'details && details.actor' in a bunch of places.

@@ +377,3 @@
     _setDescriptionText: function(text) {
+        this._descriptionText = text;
+        let markup = getMarkupWithMatches(text, this._search);

Shouldn't this check _getIndicateMatchesIndescription?

@@ +390,3 @@
     },
 
+    _getIndicateMatchesInDescription: function() {

Could use a short comment

@@ +741,3 @@
+        // It's ok to reset the open details index even if we are still showing details
+        // for some item because we only use this index to decide if we should show or
+        // hide details when the information icon is clicked. Resetting this index

Hmm, this strikes me as doing something confusing in the code then documenting why its OK. Could it just be openDetailsItem that holds a reference to a GenericDisplayItem?

@@ +743,3 @@
+        // hide details when the information icon is clicked. Resetting this index
+        // ensures that we will show the details for the new item at that index when the 
+        // information icon is clicked, rather than hide the details pane.

But it means that if the same item is still showing and you click on its (i), it doesn't hide, right?

*** Bug 596439 has been marked as a duplicate of this bug. ***

Ping. :)

I sometimes am surprised by some of the results, and knowing what the match is would be helpful.

Created attachment 240340 [details] [review]
searchDisplay: Highlight search terms in search result description

The consensus on IRC seemed to be that if we are going to highlight terms at
all, it will only be in the description, so that's what this patch does.

regexEscape() could be moved to the utils if it would be useful elsewhere.

Created attachment 242957 [details]
Example highlight when searching for a whole word in 3.8

Created attachment 242958 [details]
Example highlight when searching for a single character in 3.8

I think this example illustrates that highlighting a single character is useless. So we need to determine at what point highlighting becomes useful. It might make sense to highlight the search terms in cases of:

 - A single word that is longer than two characters
 - Multiple words

In other cases, I don't know if it would be useful at all. Of course, by the time these cases are satisfied, the result are probably pretty small already.

Furthermore, the mockups[1] show highlighting only in the (currently unimplemented) chat search provider where context would be very useful in picking the right conversation. For other situations, maybe it doesn't make as much sense.

[1] https://git.gnome.org/browse/gnome-shell-design/plain/mockups/static/search-results.png

hmm, this would be nice to land for 3.10

It would still be nice to have this. There are some updated designs in bug 749957.

(In reply to Allan Day from comment #11)
> It would still be nice to have this. There are some updated designs in bug
> 749957.

Those updated designs were implemented for 3.26.

*** This bug has been marked as a duplicate of bug 749957 ***