Closed Bug 395161 Opened 17 years ago Closed 16 years ago

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

Categories

(Firefox :: Address Bar, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 3.1a1

People

(Reporter: moco, Assigned: Mardak)

References

Details

(Keywords: user-doc-complete)

Attachments

(7 files, 8 obsolete files)

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
Flags: blocking-firefox3?
For the record, I've written in bug 393508 comment 20 what I think about grouping.
-> beltzner to help sort out this stuff
Assignee: sspitzer → beltzner
I think this bug is effectively wontfix'd by Alex' second and upcoming third iteration in bug 393508.
yes, Dão is right.

this is wontfixed unless we want to morph it to cover alex's new mockups in bug #393508.
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" 
Flags: blocking-firefox3? → blocking-firefox3-
Keywords: uiwanted
Summary: add grouping to the url bar autocomplete results for locations, bookmarks, tags → make it possible to group to the url bar autocomplete results by locations, bookmarks, tags
Target Milestone: --- → Future
Blocks: 393508
No longer depends on: 393508
Flags: wanted-firefox3+
Attached patch v1 (obsolete) — Splinter Review
Stealing bug for beltzner's comment #8

* star (bookmark and tags)
# history (non-star)
~ tags
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
Summary: make it possible to group to the url bar autocomplete results by locations, bookmarks, tags → make it possible to restrict the url bar autocomplete results to bookmarks, tags, or history entries
Attached patch v1.1 (obsolete) — Splinter Review
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.
Assignee: beltzner → edilee
Attachment #306406 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #306454 - Flags: review?
Attached image screenshot of v1.1
Attachment #306455 - Flags: ui-review?(beltzner)
Comment on attachment 306455 [details]
screenshot of v1.1

Awesome. ui-r=beltzner
Attachment #306455 - Flags: ui-review?(beltzner) → ui-review+
Comment on attachment 306454 [details] [diff] [review]
v1.1

Dietrich: who's a good person to review this?
Attachment #306454 - Flags: review? → review?(dietrich)
Attached patch v1.2 (obsolete) — Splinter Review
Don't do adaptive (or keyword) search nor feedback stuff when we're doing a special search.
Attachment #306454 - Attachment is obsolete: true
Attachment #306580 - Flags: review?(dietrich)
Attachment #306454 - Flags: review?(dietrich)
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)?
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.
Blocks: 405745
Attached patch v1.3 (obsolete) — Splinter Review
Now with customizable special search characters/words.
Attachment #306580 - Attachment is obsolete: true
Attachment #306968 - Flags: review?(dietrich)
Attachment #306580 - Flags: review?(dietrich)
Attached image screenshot of v1.3
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
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]
Attached patch v2 (obsolete) — Splinter Review
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.
Attachment #306968 - Attachment is obsolete: true
Attachment #308118 - Flags: review?
Attachment #306968 - Flags: review?(dietrich)
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.
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
Depends on: 406257
Attached patch v2.1 (obsolete) — Splinter Review
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! (^ *, ^ #, ^ @, ^ +, * #, * @, * +, # @, # +, @ +)
Attachment #308118 - Attachment is obsolete: true
Attachment #308515 - Flags: review?(dietrich)
Attachment #308118 - Flags: review?
Blocks: 422490
No longer blocks: 422490
Depends on: 422869
Blocks: 422944
Blocks: 424557
Keywords: uiwanted
Summary: make it possible to restrict the url bar autocomplete results to bookmarks, tags, or history entries → Make it possible to restrict the url bar autocomplete results to bookmarks/history entries and match only url/title/tags
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/
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)
Blocks: 428008
Attached patch v2.2 (obsolete) — Splinter Review
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.
Attachment #308515 - Attachment is obsolete: true
Attachment #314622 - Flags: review?(dietrich)
Attachment #308515 - Flags: review?(dietrich)
(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.
Flags: blocking-firefox3- → blocking-firefox3?
Flags: blocking-firefox3?
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
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.
The old behaviour is based off of a different set of data, populated a different way, and uses a different widget.
Priority: -- → P1
Target Milestone: Future → Firefox 3.1
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
Attachment #314622 - Flags: review?(dietrich) → review+
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 $.  
(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.)
(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.
Attached patch v2.3Splinter Review
(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.
Attachment #314622 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/index.cgi/rev/90de3fdd3a16
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Flags: in-litmus?
Flags: in-litmus-
Resolution: --- → FIXED
Summary: Make it possible to restrict the url bar autocomplete results to bookmarks/history entries and match only url/title/tags → Make it possible to restrict the url bar autocomplete results to bookmarks/tagged/history entries and match only url/title
Target Milestone: Firefox 3.1 → Firefox 3.1a1
Blocks: 446161
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, ++)
Status: RESOLVED → VERIFIED
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)
Blocks: 434267
Blocks: 455561
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);
(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.
(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
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.
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.
You need to log in before you can comment on or make changes to this bug.