gnome-shell itself only does search for applications (but even that implementation mostly lives in GIO nowadays). What is shown in the video is nautilus' search provider, so reassigning there.

I'm inclined to agree - Files should do full text search and not just match file names. We have all this search power available to us, and we're not using it.

Imagine a situation where someone is searching for a term that appears inside their files, but isn't in the file names. Right now we wouldn't present any results at all, even though we have the capacity to return something.

As an optional extra, an extract from the file could be shown in the results list, in order to give a hint at why some files are being returned:

https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/nautilus/nautilus-next/wire-search.png

However, I don't think that enabling full text search is dependent on this.

> Imagine a situation where someone is searching for a term that appears inside
> their files, but isn't in the file names. Right now we wouldn't present any
> results at all, even though we have the capacity to return something.

Yes, that's exactly my typical use case. I have to use tracker-needle instead of Nautilus or the Activities Overview search, which is a less satisfying user experience.

Created attachment 349350 [details] [review]
search-engine-tracker: Don't limit results to filename matches

The filter SPARQL query was matching the query string to the
parent AND to the filename.

With this patch, the results are not limited to filename matches,
causing the fts:match search criteria to search within file
contents.

(In reply to Felipe Borges from comment #4)
> Created attachment 349350 [details] [review] [review]
> search-engine-tracker: Don't limit results to filename matches
> 

Of course just showing the results is not enough, it is just a beginning, but I would rather present the results anyway.

For the Full text search feature, we should have UI and better ranking capabilities IMO.

I was informed that there will be a GSoC/Outreachy project for this round about "Improving the Nautilus search" see https://wiki.gnome.org/Outreach/SummerOfCode/2017/Ideas

Review of attachment 349350 [details] [review]:

Hey Felipe,

Thanks for the patch! As you could imagine, the problem is that I'm not sure we want to show files that only matches text inside without any visual clue. This is even more important if we want to keep in mind the use cases of type-ahead for our search, as I have been trying in the past. I believe at some point we will need some way to limit results to only the file name, or give priority to those (even more than the current search ranking we have). This requires experimenting and tweaking, and for that I added it as task for GSoC. But if you feel like going ahead with it, please go :)

In any case, the patch would need work, so marking as such,

Review of attachment 349350 [details] [review]:

::: src/nautilus-search-engine-tracker.c
@@ +346,3 @@
     else
     {
+        g_string_append_printf (sparql, "tracker:uri-is-descendant('%s', ?url) || ", location_uri);

Shouldn't non-recursive search in the branch above get the same treatment?

I was envisioning the full-text functionality being limited to just the global Activities Overview search, where it's clear that you're looking for things across the whole system. In Nautilus itself, this is problematic because of how search replaced type-ahead. If type-ahead was brought back, it would be a lot less problematic...

(In reply to Nate Graham from comment #9)
> I was envisioning the full-text functionality being limited to just the
> global Activities Overview search, where it's clear that you're looking for
> things across the whole system.
Search in Nautilus is across the system, all the times (except if you turn it off in preferences)

> In Nautilus itself, this is problematic
> because of how search replaced type-ahead. If type-ahead was brought back,
> it would be a lot less problematic...

Yeah, we want the current search to also cover part of the type-ahead use case. type-ahead itself clashes with the search, so we cannot include in the way that was previously done. You can take a look at our discussion in https://bugzilla.gnome.org/show_bug.cgi?id=681871

https://bugzilla.gnome.org/show_bug.cgi?id=681871 definitely seems like the right approach to me. Performing a search  when typing starts in a Nautilus window violates the principle of least astonishment (https://en.wikipedia.org/wiki/Principle_of_least_astonishment) and also makes the actual search UI invisible. Consider what would happen if a user starts typing accidentally; the whole view is replaced with a search view that they may not know how to exit from.

I think novice users would be much better served by having the search UI be clear and visible--invoked through a search box--and expert users would be better served by bringing back type-ahead.

And as a happy side effect, this would instantly resolve the thorny issue we're grappling with in this bug.

Hey Nate,

The type ahead vs search was thoughtfully discussed in the bug I commented and in the past. This is probably not a good bug report to discuss this.

Regarding principle of least astonishment, I see where are you coming from, but I don't think it applies that much in here - We use this pattern all through GNOME, and we give enough feedback that search is active, with the button active too, so the user can guess how to exit. Also regular shortcuts like <esc> continue working in there.

Also, I don't think this has much to do with this bug report, not having UI for FTS is the issue, nothing else, and has nothing to do with type-ahead, even if having that separation in the UI having a FTS without UI would be confusing.

Created attachment 354031 [details] [review]
implement fts

Review of attachment 354031 [details] [review]:

It's shaping up well! I've found a few things to fix, mostly nits :).

::: data/org.gnome.nautilus.gschema.xml
@@ +220,3 @@
+    <key type="b" name="fts-default">
+      <default>true</default>
+      <summary>Whether to have full text search as default when opening a new window/tab</summary>

I suggest s/as default/enabled by default/ here

@@ +221,3 @@
+      <default>true</default>
+      <summary>Whether to have full text search as default when opening a new window/tab</summary>
+      <description>If set to true, then Nautilus will also match the file contents besides the name</description>

This just toggles the checkbutton default active state, right? Would be nice to mention it, and that users can still override it in the UI.

::: src/nautilus-file.c
@@ +925,3 @@
+    if (file->details->fts_snippet)
+    {
+        g_free (file->details->fts_snippet);

free/g_free are already NULL aware, you can remove the if() check

@@ +6157,3 @@
+void
+nautilus_file_set_search_fts_snippet (NautilusFile *file,
+                                      gchar        *fts_snippet)

Since you're copying this string and leaving it unmodified, it's better to pass it as const gchar*

@@ +6165,3 @@
+nautilus_file_get_search_fts_snippet (NautilusFile *file)
+{
+    return file->details->fts_snippet;

And here you're returning the internal copy which is not for the caller to modify, so better make this function return const gchar* as well.

::: src/nautilus-list-view.c
@@ +1570,3 @@
                         -1);
 
+    escaped_name = g_markup_escape_text (text, g_utf8_strlen (text, -1));

Using g_utf8_strlen here is subtly wrong, as it returns the count of utf8 characters, whereas basically all glib functions that take a "len" argument expect byte length as returned by strlen().

Taking the word "ñoño" as an example, g_utf8_strlen would return 4 while strlen would return 6 (because each 'ñ' takes 2 bytes). It would effectively mean that you're asking g_markup_escape_text to process text until the middle of the second 'ñ', which yields invalid utf8.

In this case you can simply pass -1 here to g_markup_escape_text, since the string is NUL-terminated

@@ +1616,3 @@
+        {
+            replaced_text = eel_str_replace_substring (snippet, "\n", "");
+            escaped_text = g_markup_escape_text (replaced_text, g_utf8_strlen (replaced_text, -1));

Same here, just pass -1.

::: src/nautilus-search-engine-tracker.c
@@ +333,3 @@
     mime_count = g_list_length (mimetypes);
 
+    sparql = g_string_new ("SELECT DISTINCT nie:url(?urn) fts:rank(?urn) nfo:fileLastModified(?urn) nfo:fileLastAccessed(?urn) fts:snippet(?urn)\n"

Would maybe be nicer to make the fts:snippet argument optional depending on fts_enabled? that'd also mean that you have to optionally get the result through tracker_sparql_cursor_get_string(cursor,4,NULL);

::: src/nautilus-search-hit.c
@@ +49,3 @@
     PROP_ACCESS_TIME,
     PROP_FTS_RANK,
+    PROP_FTS_SNIPPET,

Hmm, you added this but didn't do g_object_class_install_property(), I see you handle it in set_property(), but didn't do the same with get_property(). I think that makes sense since you have a getter function.

@@ +162,3 @@
 
+gchar *
+nautilus_search_hit_get_fts_snippet (NautilusSearchHit *hit)

This function should return const gchar* too :)

@@ +224,3 @@
+    if (hit->fts_snippet)
+    {
+        g_free (hit->fts_snippet);

No need for NULL checks before g_free

::: src/nautilus-search-popover.c
@@ +803,3 @@
+                                        NULL,
+                                        NULL,
+                                        g_cclosure_marshal_generic,

This is not wrong per se, but IMHO g_cclosure_marshal_VOID__BOOLEAN is better to use here. The generic marshaler is considerably slower, which I guess doesn't matter a lot here, but glib already provides us the better fit (i.e. no need to generate it).

...And on second thought, you have a signal communicate updates to the fts_enabled boolean, that's basically a pseudoproperty. IMHO it seems nicer to actually make this a property, do g_object_notify() and g_signal_connect(...,"notify::fts-enabled") on the other end.

Created attachment 354125 [details] [review]
implement fts

The search text can now also match the contents of a file, besides
the file name.

This is done with the help of a Tracker query, using fts:match, which
matces both the contents of a file and the filename.

The user also has the option to choose whether to use or not the
Full Text Search. This can be done with a preference, which represents
the default option when opening a new tab/window or from the search
popover.

Review of attachment 354125 [details] [review]:

Looks good! just minor 2 nits I found, I don't think another review round is necessary after those, so feel free to push after fixing locally.

::: src/nautilus-search-hit.c
@@ +321,3 @@
+        case PROP_FTS_SNIPPET:
+        {
+            g_value_set_string (value, hit->fts_snippet);

You do handle get_property() here and added the property for real. Good :), but the set_property() counterpart is still missing.

@@ +350,3 @@
 
+    if (hit->fts_snippet)
+        g_free (hit->fts_snippet);

No need for NULL check

Created attachment 354249 [details] [review]
implement fts

The search text can now also match the contents of a file, besides
the file name.

This is done with the help of a Tracker query, using fts:match, which
matces both the contents of a file and the filename.

The user also has the option to choose whether to use or not the
Full Text Search. This can be done with a preference, which represents
the default option when opening a new tab/window or from the search
popover.

Attachment 354249 [details] pushed as 11cbe22 - implement fts

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