show tag results in url bar autocomplete

VERIFIED FIXED in Firefox 3 alpha8

Status

()

VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: moco, Assigned: moco)

Tracking

Trunk
Firefox 3 alpha8
Points:
---
Bug Flags:
blocking-firefox3 -
wanted-firefox3 +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

show tag results in url bar autocomplete

As an intermediate step between where I am with it right now (https://bugzilla.mozilla.org/show_bug.cgi?id=395161) and faaborg's mockup (http://people.mozilla.com/~faaborg/files/granParadisoUI/places_UnifyingSearch_i1.png), here's what I'd like to propose for m8 (meaning, something I think I can finish and get reviewed today.)

Upon typing in the url bar:

    * prioritize exact (case insensitive) tag matches to the top, and use the tag icon instead of the star.  among the tag matches, sort by frecency.  The reasoning here is to promote tagging, to make finding tagged items easier, and now users can use tags for the additional purpose of affecting search results.  Say I want g to be google.  I can tag google.com with g and if that is the only page tagged with g, it will always be first.  I'd rather users use tags to do this, than just bookmarking.

    * after the tag matches, the frecency results for bookmarks + the first chunk of history.  the reason for this is so that unvisited bookmarks will be "near the top", yet not completely outweighing unbookmarked visits.

    * after all that, continue on with the rest of history (in the current asynchronous fashion). 

Note, this should be as fast as things currently are, as tag searching and bookmark searching fast as those are typicallly several orders of magnitude less than history.
Flags: in-litmus?
Flags: blocking-firefox3?
taking, patch in hand.
Assignee: nobody → sspitzer
Target Milestone: --- → Firefox 3 M8
Comment on attachment 279981 [details] [diff] [review]
patch, v1

note, some of the changes to nsNavHistory::StartSearch() are not part of this fix, but are problems I spotted while working on this code.

for example, we were searching previous results when doing a "typed only" search, getting the match count of the previous results (for no reason), a warning fix PRInt32 -> PRUint32, and save calls to PR_Now() and avoid a query to determine the earliest visit if we are doing a "typed only" search
Attachment #279981 - Flags: review?(dietrich)
Created attachment 279989 [details] [diff] [review]
patch, v2.  my bookmark query is flooding results and is too slow, I'll take showing unvisited bookmarks off to another bug
Attachment #279981 - Attachment is obsolete: true
Attachment #279989 - Flags: review?(dietrich)
Attachment #279981 - Flags: review?(dietrich)
Comment on attachment 279989 [details] [diff] [review]
patch, v2.  my bookmark query is flooding results and is too slow, I'll take showing unvisited bookmarks off to another bug

>-//    The auto complete query we use depends on options, so we have it in
>+//    The auto complete queries we use depends on options, so we have it in

nits:

s/depends/depend/
s/it/them/

>+
>+  sql = NS_LITERAL_CSTRING(
>+    "SELECT h.url, h.title, f.url, b.id, b.parent "
>+    "FROM moz_places h "
>+    "JOIN moz_bookmarks b ON b.fk = h.id "
>+    "LEFT OUTER JOIN moz_historyvisits v ON h.id = v.place_id "
>+    "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id "
>+    "WHERE "
>+    "(b.parent = (SELECT t.id FROM moz_bookmarks t WHERE t.parent = ?1 and t.title LIKE ?2 ESCAPE '/')) "
>+    "GROUP BY h.id ORDER BY h.visit_count DESC, MAX(v.visit_date) DESC;");
>+  rv = mDBConn->CreateStatement(sql, getter_AddRefs(mDBTagAutoCompleteQuery));
>+  NS_ENSURE_SUCCESS(rv, rv);
> 
>-  return mDBConn->CreateStatement(sql, getter_AddRefs(mDBAutoCompleteQuery));
>+  return NS_OK;
> }

> 
>@@ -234,47 +259,25 @@ nsNavHistory::StartSearch(const nsAStrin
>   nsresult rv;
> 
>   // determine if we can start by searching through the previous search results.
>   // if we can't, we need to reset mCurrentChunkEndTime and mCurrentOldestVisit.
>   // if we can, we will search through our previous search results and then resume 
>   // searching using the previous mCurrentChunkEndTime and mCurrentOldestVisit values.
>   PRBool searchPrevious = PR_FALSE;
>   if (aPreviousResult) {

>+    // if search string begins with the previous search string, it's a go.
>+    // but don't search previous results if the previous search string was empty
>+    // or if the current serach string is empty.  (an empty search string is a "typed only"

s/serach/search/

> 
>       PRBool isMatch = CaseInsensitiveFindInReadable(mCurrentSearchString, url);
>       if (!isMatch)
>         isMatch = CaseInsensitiveFindInReadable(mCurrentSearchString, title);
> 
>       if (isMatch) {
>-        nsAutoString image, style;
>-        aPreviousResult->GetImageAt(i, image);
>+        nsAutoString style;
>         aPreviousResult->GetStyleAt(i, style);
>- 
>-        mCurrentResultURLs.Put(url, PR_TRUE);
>+        
>+        // if the previous result item was a tagged item, and it matches
>+        // the current search term (due to url or title) don't include it.
>+        // find it "naturally" with a history or bookmark search.
>+        // 
>+        // note, if the previous result item is tagged with the current search term,
>+        // our call to AutoCompleteTagsSearch() above will find it.
>+        if (!style.Equals(NS_LITERAL_STRING("tag"))) {

could you do this check before the matching? don't you want to throw out all previous tag entries in the previous result?

>+  // Determine the result of the search
>+  while (NS_SUCCEEDED(mDBTagAutoCompleteQuery->ExecuteStep(&hasMore)) && hasMore) {
>+    nsAutoString entryURL, entryTitle, entryFavicon;
>+    mDBTagAutoCompleteQuery->GetString(kAutoCompleteIndex_URL, entryURL);
>+    mDBTagAutoCompleteQuery->GetString(kAutoCompleteIndex_Title, entryTitle);
>+    mDBTagAutoCompleteQuery->GetString(kAutoCompleteIndex_FaviconURL, entryFavicon);
>+    PRInt64 itemId = 0;
>+    mDBTagAutoCompleteQuery->GetInt64(kAutoCompleteIndex_ItemId, &itemId);
>+    PRInt64 parentId = 0;
>+    mDBTagAutoCompleteQuery->GetInt64(kAutoCompleteIndex_ParentId, &parentId);

it's pretty standard in the rest of places code to check the rv of mozStorage's Get* value accessors, would that be overkill here?
Created attachment 280011 [details] [diff] [review]
patch v3, address dietrich's review comments

1)

nits:

s/depends/depend/
s/it/them/

fixed, thanks.

2)


s/serach/search/

fixed, thanks.

3)


could you do this check before the matching? don't you want to throw out all
previous tag entries in the previous result?

yes, good catch!  fixed, thanks.

4)

it's pretty standard in the rest of places code to check the rv of mozStorage's
Get* value accessors, would that be overkill here?

fixed, thanks.
Attachment #279989 - Attachment is obsolete: true
Attachment #280011 - Flags: review?(dietrich)
Attachment #279989 - Flags: review?(dietrich)
Comment on attachment 280011 [details] [diff] [review]
patch v3, address dietrich's review comments

r=me, thanks.
Attachment #280011 - Flags: review?(dietrich) → review+
Comment on attachment 280011 [details] [diff] [review]
patch v3, address dietrich's review comments

seeking approval for m8
Attachment #280011 - Flags: approval1.9?

Updated

11 years ago
Attachment #280011 - Flags: approval1.9? → approval1.9+
fixed.

Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHis
tory.cpp
new revision: 1.168; previous revision: 1.167
done
Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHisto
ry.h
new revision: 1.97; previous revision: 1.96
done
Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v  <
--  nsNavHistoryAutoComplete.cpp
new revision: 1.18; previous revision: 1.17
done
Checking in browser/themes/pinstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/browser.css,v  <--  browser.cs
s
new revision: 1.76; previous revision: 1.75
done
Checking in browser/themes/pinstripe/browser/jar.mn;
/cvsroot/mozilla/browser/themes/pinstripe/browser/jar.mn,v  <--  jar.mn
new revision: 1.53; previous revision: 1.52
done
RCS file: /cvsroot/mozilla/browser/themes/pinstripe/browser/places/tag.png,v
done
Checking in browser/themes/pinstripe/browser/places/tag.png;
/cvsroot/mozilla/browser/themes/pinstripe/browser/places/tag.png,v  <--  tag.png

initial revision: 1.1
done
Checking in browser/themes/winstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v  <--  browser.cs
s
new revision: 1.91; previous revision: 1.90
done
Checking in browser/themes/winstripe/browser/jar.mn;
/cvsroot/mozilla/browser/themes/winstripe/browser/jar.mn,v  <--  jar.mn
new revision: 1.49; previous revision: 1.48
done
RCS file: /cvsroot/mozilla/browser/themes/winstripe/browser/places/tag.png,v
done
Checking in browser/themes/winstripe/browser/places/tag.png;
/cvsroot/mozilla/browser/themes/winstripe/browser/places/tag.png,v  <--  tag.png

initial revision: 1.1
done
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Seth: Using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007090704 Minefield/3.0a8pre, I don't see my locations chunked out like I do in your screenshot (Locations, Tags, Bookmarks): https://bugzilla.mozilla.org/attachment.cgi?id=279966.

Another issue I noticed: I get two instances of the site that I tagged in the URL bar - one that has a star and one that has the tag icon. I can grab a screenshot if you need it.
ignore Comment 11 - sorry, it looks as if this landed late in the cycle and did not make the first build out the door, which is the one I am testing.
Created attachment 280107 [details]
correct screen shot (sorry marcia!)
Attachment #279966 - Attachment is obsolete: true
sorry marcia, my original screen shot was for https://bugzilla.mozilla.org/show_bug.cgi?id=395161, not this bug.

I've replaced the screen shot.
> Another issue I noticed: I get two instances of the site that I tagged in the
> URL bar - one that has a star and one that has the tag icon. I can grab a
> screenshot if you need it.

I have code in there that is supposed to prevent that from happening.  can you log a spin off bug with a screen shot?  I'll try to reproduce that bug here as well.
Bug 395445 has been filed to cover the spin off issue.

(In reply to comment #15)
> > Another issue I noticed: I get two instances of the site that I tagged in the
> > URL bar - one that has a star and one that has the tag icon. I can grab a
> > screenshot if you need it.
> 
> I have code in there that is supposed to prevent that from happening.  can you
> log a spin off bug with a screen shot?  I'll try to reproduce that bug here as
> well.
> 

Litmus Triage Team: stephend, will you cover this one?
(In reply to comment #17)
> Litmus Triage Team: stephend, will you cover this one?

Sure; http://litmus.mozilla.org/show_test.cgi?id=4680

in-litmus+
Flags: in-litmus? → in-litmus+
Verified FIXED using:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007091204 Minefield/3.0a8pre

and Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8pre) Gecko/2007091204 Minefield/3.0a8pre
Status: RESOLVED → VERIFIED

Updated

11 years ago
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [wanted-firefox3]
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3]
You need to log in before you can comment on or make changes to this bug.