395161 - Make it possible to restrict the url bar autocomplete results to bookmarks/tagged/history entries and match only url/title

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[(not reading, please use [e-mail] instead)]

add grouping to the url bar autocomplete results for locations, bookmarks, tags

this bug covers the implementation of what faaborg is designing in bug #393508

screen shot and rough patch coming

Comment 1

[(not reading, please use [e-mail] instead)]

Attachment: screen shot

Comment 2

[(not reading, please use [e-mail] instead)]

Attachment: draft patch, v1

Comment 3

[(not reading, please use [e-mail] instead)]

Attachment: draft patch, v2 (tag search should be case insensitive exact match)

Comment 4

[Dão Gottwald [:dao]]

For the record, I've written in bug 393508 comment 20 what I think about grouping.

Comment 5

[Mike Connor [:mconnor]]

-> beltzner to help sort out this stuff

Comment 6

[Dão Gottwald [:dao]]

I think this bug is effectively wontfix'd by Alex' second and upcoming third iteration in bug 393508.

Comment 7

[(not reading, please use [e-mail] instead)]

yes, Dão is right.

this is wontfixed unless we want to morph it to cover alex's new mockups in bug #393508.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

I think we've decided that the grouping UI, while interesting, isn't as
powerful as predicting what it is that users want when they type.

This bug isn't yet WONTFIX, though, as I think there are times when it could be
useful for the user to explicitly ask for it, so this rough patch could be
really useful.

Another thing I was thinking about (this is probably another bug) is allowing
power-users some quick keyword-like filters for location bar autocomplete, so
things like:

 * foo         would return only starred entries matching against "foo"
 # foo         would return only history entries matching against "foo"
 ~ foo         would return only tagged entries matching against "foo" 

Comment 9

[Ed Lee :Mardak]

Attachment: v1

Stealing bug for beltzner's comment #8

* star (bookmark and tags)
# history (non-star)
~ tags

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

For some reason when I tried to apply that patch, I wasn't able to see any bookmarks (in the menu or in the toolbar) nor create new ones. The star button was also missing. Tried with a clean profile. Backing out the patch restored the functionality.

Entirely likely that was my error not building in the right places, though :(

Anyway, super excited to see this moving along. I think I might have new ideas on the shortcut keys:

 * foo = search for bookmark titles with foo in bookmark title or URL
 + bar = search for tags with bar in tagname
 ^ baz = search for non bookmarked history entries with baz in page title or URL

Comment 11

[Ed Lee :Mardak]

Attachment: v1.1

Cleaned up the queries and optimized them with suggestions from Marco. Also, I've made it so that we always show tags if the page has tags.

I suppose ideally these 3 filters can be set in prefs.js. Especially for keyboards that can't easily type ^ + *

They don't even need to be a single character either. But I suppose you could even make it something like "b" or "t" or "h".. Or maybe a letter that's unused like z or [ etc.

Comment 12

[Ed Lee :Mardak]

Attachment: screenshot of v1.1

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 306455 [details]
screenshot of v1.1

Awesome. ui-r=beltzner

Comment 14

[Ed Lee :Mardak]

Attachment: screenshot of v1.1 "* w"

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 306454 [details] [diff] [review]
v1.1

Dietrich: who's a good person to review this?

Comment 16

[Ed Lee :Mardak]

Attachment: v1.2

Don't do adaptive (or keyword) search nor feedback stuff when we're doing a special search.

Comment 17

[bomfog]

It seems to me comment #8 and comment #10 propose contradictory behavior.

Comment #8, if I understand it, says always search titles AND urls AND tags, and then filter on whether the result is bookmarked/tagged.

Comment #10 seems to say only search titles and urls for "*", and only tags for "+", then filter.

So does the operator govern only the type of results to return, or also which info to search? I'd prefer the latter (comment 10 behavior) but the tryserver build seems to implement the former (comment 8).

Or maybe I'm just confused.

Also, should "*" search bookmark names as well as page titles (or does it already)?

Comment 18

[Ed Lee :Mardak]

Almost. The tryserver builds with this patch will select a subset of pages then do the normal filtering.

Meaning * will only search bookmarked pages, + searches only tagged pages, ^ searches non-bookmarked pages.

So performance-wise, it's better than what you said earlier where we search through all pages, filter on search terms then filter on the special criteria.

Comment 19

[Ed Lee :Mardak]

Attachment: v1.3

Now with customizable special search characters/words.

Comment 20

[Ed Lee :Mardak]

Attachment: screenshot of v1.3

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

adding shaver, as he was really one of the inspirations for this bug

Couple of quick interaction thoughts:
 
 - should we have another selector (@) which restricts the search only to the URL field? this would address a pretty common interaction complaint which some people see as a regression over the Fx2 autocomplete behaviour

 - should the special characters work only as prefixes, or if I type them as a suffix should that also work? that way if I'm typing "foo" and realize that I wanted to restrict my search, I can do so easily

Comment 22

[Mike Shaver (:shaver emeritus)]

Suffix probably not as good as "word"; the use case I'm thinking of:

. o O ( where is that page about how to put Stuff in Things I saw earlier? )

[things stuff]

      ( those are a lot of results, including my extensive collection of )
. o O ( bookmarks about the Stuff Things Dynasty; I don't want those now )

[things stuff ^]

. o O ( ah, much better; now let's narrow a bit more )

[things stuff ^ insertion]

. o O ( ah, there is the page; sweet awesomebar, how I love you )

[awesomebar mardak paypal]

Comment 23

[Ed Lee :Mardak]

Attachment: v2

Add the "force match" special searches:

pref("browser.urlbar.restrict.history", "^");
pref("browser.urlbar.restrict.bookmark", "*");
pref("browser.urlbar.match.title", "!");
pref("browser.urlbar.match.url", "@");
pref("browser.urlbar.match.tag", "+");

"restrict" cause only those types of things to match. "match" forces those fields to be matched.

Comment 24

[Ed Lee :Mardak]

Oh, I suppose I forgot to mention that you can use these special search words anywhere as shaver suggested.

FYI: A combination of ! @ + causes those extra fields to be matched as well. So "p + @ !" means there must be a "p" in the tag, url, AND tag.

Right now, the behavior of combining + * ^ will prefer the most "strict" rule.

+ * ^ -> +
+ ^ -> +
+ * -> +
+ -> +
* ^ -> *
* -> *
^ -> ^

Again, order doesn't matter.

Comment 25

[Ed Lee :Mardak]

Here's a build with this patch and some others..

https://build.mozilla.org/tryserver-builds/2008-03-08_01:[e-mail]/

Comment 26

[Ed Lee :Mardak]

I ran some timing tests with and without the latest patch..

"p" without patch:
8 ms 9 ms 7 ms 7 ms 8 ms 7 ms 7 ms 11 ms 8 ms 6 ms

"p" with patch:
7 ms 6 ms 7 ms 9 ms 6 ms 10 ms 6 ms 7 ms 7 ms 7 ms

"^" with patch:
21 ms 21 ms 20 ms 22 ms 19 ms 20 ms 19 ms 19 ms 23 ms 21 ms

"*" with patch:
26 ms 19 ms 20 ms 19 ms 21 ms 20 ms 18 ms 24 ms 20 ms 25 ms

"+" with patch:
18 ms 17 ms 21 ms 21 ms 16 ms 16 ms 16 ms 17 ms 16 ms 16 ms

Things also go *much* faster if we do this change along with reducing maxRichResults to something like 12, if we only show 6 rows by default -> scrollbar will only ever be half the height of the autocomplete. See bug 406257.

"^" with 12 results instead of 25:
12 ms 14 ms 12 ms 12 ms 12 ms 12 ms 12 ms 12 ms 15 ms 13 ms

"*" with 12 results instead of 25:
9 ms 9 ms 9 ms 11 ms 10 ms 11 ms 9 ms 11 ms 9 ms 10 ms

"+" with 12 results instead of 25:
12 ms 8 ms 8 ms 8 ms 11 ms 11 ms 10 ms 12 ms 11 ms 13 ms

Comment 27

[Ed Lee :Mardak]

Attachment: v2.1

Switched "!" to "#" for matching against page titles.

SUPER SPECIAL TESTCASE BLOWOUT! 21 tests in 1!!! ;) ;) ;)

What's included in this super test package?

3 resticting filters (history, bookmark, tag)
3 for special term as a word (at the beginning, in the middle, at the end)
5 restrict/match searches with a term (history, bookmark, title, url, tag)
10 various combinations of specials! (^ *, ^ #, ^ @, ^ +, * #, * @, * +, # @, # +, @ +)

Comment 28

[GaneshKB]

Hi all,
        This story is going on in different thread. I personally feel, you could create sub modules for each tab that you have, So that people can select them from the list and then you can add them as a bug. So it would still be under one list and it will be easy for you to pick it up.

Thank you
Ganesh.KB
http://dexter-laboratory.blogspot.com/

Comment 29

[d]

there is another switch needed: search only from the start of the url/title (starting from the hostname, with or without "www", as in previous versions)

Comment 30

[Ed Lee :Mardak]

Attachment: v2.2

Unbitrot but still on top of bug 392143 and change "match tag" to "restrict tag" because restricting on tags isn't useful for narrowing searches when using multiple words.

Comment 31

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #29)
> there is another switch needed: search only from the start of the url/title
> (starting from the hostname, with or without "www", as in previous versions)

Indeed - that should probably be a separate bug, though, and something to the effect of "http" or "www" being the "switch". Note that we already do this, but we order the results based on our weighting algorithms. I've filed bug 430355 to track the request to abandon those weightings and revert to the old style when we detect that the user is going down that path.

Comment 32

[Ryan A. C.]

This bug being 'future' I would like to suggest we don't hide the functionality within about:config so much. In options we have the odd one out "Tabs" which I think would do better renamed to "Interface" or something along those lines. This could contain "Tabs" and "Smart Location Bar" which has things like
Search these items:
[ ]URLs
[ ]Titles
[ ]Tags
And possibly even a ( )smart or ( )old behavior for those that might want to use the old one.

Of course the more advanced things like the restrict keywords and such are more suited for about:config of course

Comment 34

[Jez]

Here's what I don't understand.  If this is an acceptable amount of code to maintain, why is it too much work to maintain code that just does what the old behaviour did?  This seems like much more work.

Comment 35

[Mike Beltzner [:beltzner, not reading bugmail]]

The old behaviour is based off of a different set of data, populated a different way, and uses a different widget.

Comment 36

[Dietrich Ayala (:dietrich)]

Comment on attachment 314622 [details] [diff] [review]
v2.2


> // the maximum number of results to show in autocomplete when doing richResults
> pref("browser.urlbar.maxRichResults", 12);
> // Size of "chunks" affects the number of places to process between each search
> // timeout (ms). Too big and the UI will be unresponsive; too small and we'll
> // be waiting on the timeout too often without many results.
> pref("browser.urlbar.search.chunkSize", 1000);
> pref("browser.urlbar.search.timeout", 100);
>+pref("browser.urlbar.restrict.history", "^");
>+pref("browser.urlbar.restrict.bookmark", "*");
>+pref("browser.urlbar.restrict.tag", "+");
>+pref("browser.urlbar.match.title", "#");
>+pref("browser.urlbar.match.url", "@");

please add a comment about these

>@@ -464,32 +479,37 @@ nsNavHistory::Init()
>    ***
>    *** Nothing after these add observer calls should return anything but NS_OK.
>    *** If a failure code is returned, this nsNavHistory object will be held onto
>    *** by the observer service and the preference service. 
>    ****************************************************************************/
> 
>   nsCOMPtr<nsIObserverService> observerService =
>     do_GetService("@mozilla.org/observer-service;1", &rv);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   nsCOMPtr<nsIPrefBranch2> pbi = do_QueryInterface(mPrefBranch);
>   if (pbi) {
>     pbi->AddObserver(PREF_AUTOCOMPLETE_ONLY_TYPED, this, PR_FALSE);
>     pbi->AddObserver(PREF_AUTOCOMPLETE_ON_WORD_BOUNDARY, this, PR_FALSE);
>     pbi->AddObserver(PREF_AUTOCOMPLETE_FILTER_JAVASCRIPT, this, PR_FALSE);
>     pbi->AddObserver(PREF_AUTOCOMPLETE_MAX_RICH_RESULTS, this, PR_FALSE);
>+    pbi->AddObserver(PREF_AUTOCOMPLETE_RESTRICT_HISTORY, this, PR_FALSE);
>+    pbi->AddObserver(PREF_AUTOCOMPLETE_RESTRICT_BOOKMARK, this, PR_FALSE);
>+    pbi->AddObserver(PREF_AUTOCOMPLETE_RESTRICT_TAG, this, PR_FALSE);
>+    pbi->AddObserver(PREF_AUTOCOMPLETE_MATCH_TITLE, this, PR_FALSE);
>+    pbi->AddObserver(PREF_AUTOCOMPLETE_MATCH_URL, this, PR_FALSE);
>     pbi->AddObserver(PREF_AUTOCOMPLETE_SEARCH_CHUNK_SIZE, this, PR_FALSE);
>     pbi->AddObserver(PREF_AUTOCOMPLETE_SEARCH_TIMEOUT, this, PR_FALSE);

hrm, at some point we should just use a single observer of browser.urlbar.* (iirc that's possible)

>+
>+  sql = NS_LITERAL_CSTRING(
>+    "SELECT h.url, h.title, f.url") + BOOK_TAG_SQL + NS_LITERAL_CSTRING(" "
>+    "FROM moz_places h "
>+    "LEFT OUTER JOIN moz_favicons f ON f.id = h.favicon_id "
>+    "WHERE h.frecency <> 0 AND parent IS NULL "
>+    "ORDER BY h.frecency DESC LIMIT ?2 OFFSET ?3");
>+  rv = mDBConn->CreateStatement(sql,
>+    getter_AddRefs(mDBAutoCompleteHistoryQuery));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  sql = NS_LITERAL_CSTRING(
>+    "SELECT h.url, h.title, f.url") + BOOK_TAG_SQL + NS_LITERAL_CSTRING(" "
>+    "FROM moz_places h "
>+    "LEFT OUTER JOIN moz_favicons f ON f.id = h.favicon_id "
>+    "WHERE h.frecency <> 0 AND bookmark IS NOT NULL "
>+    "ORDER BY h.frecency DESC LIMIT ?2 OFFSET ?3");
>+  rv = mDBConn->CreateStatement(sql,
>+    getter_AddRefs(mDBAutoCompleteStarQuery));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  sql = NS_LITERAL_CSTRING(
>+    "SELECT h.url, h.title, f.url") + BOOK_TAG_SQL + NS_LITERAL_CSTRING(" "
>+    "FROM moz_places h "
>+    "LEFT OUTER JOIN moz_favicons f ON f.id = h.favicon_id "
>+    "WHERE h.frecency <> 0 AND tags IS NOT NULL "
>+    "ORDER BY h.frecency DESC LIMIT ?2 OFFSET ?3");
>+  rv = mDBConn->CreateStatement(sql,
>+    getter_AddRefs(mDBAutoCompleteTagsQuery));
>   NS_ENSURE_SUCCESS(rv, rv);

there's a lot of replicated SQL here. maybe create shared prefix and suffix SQL fragment strings?

>           // Determine if every token matches either the bookmark title, tags,
>           // page title, or page url
>-          PRBool matchAll = PR_TRUE;
>           for (PRInt32 i = 0; i < mCurrentSearchTokens.Count() && matchAll; i++) {
>             const nsString *token = mCurrentSearchTokens.StringAt(i);
> 
>             // Check if the tags match the search term
>             PRBool matchTags = (*tokenMatchesTarget)(*token, entryTags);
>             // Check if the title matches the search term
>             PRBool matchTitle = (*tokenMatchesTarget)(*token, title);
>+
>+            // Make sure we match something in the title if we have to
>+            matchAll = matchTags || matchTitle;
>+            if (mMatchTitle && !matchAll)
>+              break;

iiuc, this allows a match if tags match, but without testing the title, though the user has forced a title match.

r=me w/ these comments fixed

Comment 37

[Brian Polidoro]

I see there is browser.urlbar.matchOnlyTyped.  Is it worth having a browser.urlbar.match.OnlyTyped (Note the extra period) that uses another character? Maybe use % or $.  

Comment 39

[Ed Lee :Mardak]

(In reply to comment #36)
> iiuc, this allows a match if tags match, but without testing the title, though
> the user has forced a title match.
That was on purpose after changing matchTags back into restrictTags because matchTags has little power in reducing results after you already have all results showing that tag. Treating the tag as part of the title provides more keywords to filter results, but yeah arguably it's not really part of the title.

In the an updated patch, I'll change the "match history" be more "match history" (visit count > 0) than "match non-bookmark" as that's actually what people were looking for with the Hide Unvisited add-on. (Treating non-bookmark pages as "history" would never find visited bookmarks that /are/ in your history.)

Comment 40

[Ed Lee :Mardak]

(In reply to comment #37)
> I see there is browser.urlbar.matchOnlyTyped.  Is it worth having a
> browser.urlbar.match.OnlyTyped (Note the extra period) that uses another
> character? Maybe use % or $.  
Potentially we could have a browser.urlbar.match.typed and together with bug 424557, it would provide the same functionality as matchOnlyTyped when match.typed is set to "", so we could remove the original pref.

Comment 41

[Ed Lee :Mardak]

Attachment: v2.3

(In reply to comment #36)
> >+pref("browser.urlbar.restrict.history", "^");
> please add a comment about these
Commented.

> >+  sql = NS_LITERAL_CSTRING(
> >+    "SELECT h.url, h.title, f.url") + BOOK_TAG_SQL + NS_LITERAL_CSTRING(" "
> >+    "FROM moz_places h "
> >+    "LEFT OUTER JOIN moz_favicons f ON f.id = h.favicon_id "
> >+    "WHERE h.frecency <> 0 AND parent IS NULL "
> >+    "ORDER BY h.frecency DESC LIMIT ?2 OFFSET ?3");
> >+  rv = mDBConn->CreateStatement(sql,
> >+    getter_AddRefs(mDBAutoCompleteHistoryQuery));
> >+  NS_ENSURE_SUCCESS(rv, rv);
> there's a lot of replicated SQL here. maybe create shared prefix and suffix SQL
> fragment strings?
Split into a head and tail segment and updated the original query plus the 3 new queries.

Also added visit_count so that it can be filtered out if necessary.

Updated the testcase to use the new tests/autocomplete/ style of tests and changed the history checks to look for unvisited pages instead of non-bookmarked pages.

Comment 42

[Ed Lee :Mardak]

http://hg.mozilla.org/mozilla-central/index.cgi/rev/90de3fdd3a16

Comment 44

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: Manual testcases

Comment 45

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using the manual testcases attaches in comment 44, as well as ad-hoc testing by Marcia and me:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1a1) Gecko/2008072306 Shiretoko/3.1a1

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1a1) Gecko/2008072310 Shiretoko/3.1a1

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1a1) Gecko/2008072306 Shiretoko/3.1a1

(Plus Mardak's awesome unit tests, ++)

Comment 46

[FortMax]

Dos this fix allow users to change default trigger through Boolean variables in about:config? (i.e, match_URL false only requires a URL match if the @ trigger is used, and match_URL true requires a URL match unless the @ trigger is used)

Comment 47

[Chris Ilias [:cilias]]

user-doc-complete
http://support.mozilla.com/kb/Smart+Location+Bar?bl=n#On_the_fly

Comment 48

[[e-mail]]

Comment on attachment 330354 [details] [diff] [review]
v2.3

>+  nsXPIDLCString prefStr;
>+  mPrefBranch->GetCharPref(PREF_AUTOCOMPLETE_RESTRICT_HISTORY,
>+                           getter_Copies(prefStr));
>+  mAutoCompleteRestrictHistory = NS_ConvertUTF8toUTF16(prefStr);
>+  mPrefBranch->GetCharPref(PREF_AUTOCOMPLETE_RESTRICT_BOOKMARK,
>+                           getter_Copies(prefStr));
>+  mAutoCompleteRestrictBookmark = NS_ConvertUTF8toUTF16(prefStr);
>+  mPrefBranch->GetCharPref(PREF_AUTOCOMPLETE_RESTRICT_TAG,
>+                           getter_Copies(prefStr));
>+  mAutoCompleteRestrictTag = NS_ConvertUTF8toUTF16(prefStr);
>+  mPrefBranch->GetCharPref(PREF_AUTOCOMPLETE_MATCH_TITLE,
>+                           getter_Copies(prefStr));
>+  mAutoCompleteMatchTitle = NS_ConvertUTF8toUTF16(prefStr);
>+  mPrefBranch->GetCharPref(PREF_AUTOCOMPLETE_MATCH_URL,
>+                           getter_Copies(prefStr));
>+  mAutoCompleteMatchUrl = NS_ConvertUTF8toUTF16(prefStr);
This is inefficient coding practice. NS_ConvertUTF8toUTF16 creates an nsAutoString, with the converted string in its auto buffer, which then has to be copied into the member nsString's buffer. Instead, use CopyUTF8toUTF16(prefStr, mAutoCompleteRestrictHistory);

Comment 50

[Pardal Freudenthal (:ShareBird)]

(In reply to comment #47)
> user-doc-complete
> http://support.mozilla.com/kb/Smart+Location+Bar?bl=n#On_the_fly

This page is redirecting to another site, which contains no information about the search characters introduced by this bug. This is a very nice and useful feature, too bad that this information is not easily available.

Comment 51

[Kris Shannon]

(In reply to comment #50)
> (In reply to comment #47)
> > user-doc-complete
> > http://support.mozilla.com/kb/Smart+Location+Bar?bl=n#On_the_fly
> 
> This page is redirecting to another site, which contains no information about
> the search characters introduced by this bug. This is a very nice and useful
> feature, too bad that this information is not easily available.

http://support.mozilla.com/en-US/kb/Location%20bar%20autocomplete?bl=n#w_on-the-fly

Comment 52

[Pardal Freudenthal (:ShareBird)]

I'm still not seeing the information I want. Because... I'm using Firefox 4.0...
The div containing the information has the attribute data-for="fx35" and is hidden for Firefox 4.0 users.

Comment 53

[Chris Ilias [:cilias]]

Hi Pardal,
I've just removed the div tag for that section.
If you see anything else that needs updating, just post them in the support.mozilla.com contributors forum at <https://support.mozilla.com/en-US/forums/contributors>.

The knowledge base is also a wiki, so if you'd like to edit articles yourself, just click on the "Edit Article" tab in the sidebar of the article.