Closed
Bug 395267
Opened 17 years ago
Closed 17 years ago
show tag results in url bar autocomplete
Categories
(Firefox :: Address Bar, defect)
Firefox
Address Bar
Tracking
()
VERIFIED
FIXED
Firefox 3 alpha8
People
(Reporter: moco, Assigned: moco)
Details
Attachments
(2 files, 3 obsolete files)
26.55 KB,
patch
|
dietrich
:
review+
mconnor
:
approval1.9+
|
Details | Diff | Splinter Review |
16.93 KB,
image/png
|
Details |
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?
Assignee | ||
Comment 1•17 years ago
|
||
taking, patch in hand.
Assignee: nobody → sspitzer
Target Milestone: --- → Firefox 3 M8
Assignee | ||
Comment 2•17 years ago
|
||
Assignee | ||
Comment 3•17 years ago
|
||
Assignee | ||
Comment 4•17 years ago
|
||
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)
Assignee | ||
Comment 5•17 years ago
|
||
Attachment #279981 -
Attachment is obsolete: true
Attachment #279989 -
Flags: review?(dietrich)
Attachment #279981 -
Flags: review?(dietrich)
Comment 6•17 years ago
|
||
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?
Assignee | ||
Comment 7•17 years ago
|
||
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 8•17 years ago
|
||
Comment on attachment 280011 [details] [diff] [review]
patch v3, address dietrich's review comments
r=me, thanks.
Attachment #280011 -
Flags: review?(dietrich) → review+
Assignee | ||
Comment 9•17 years ago
|
||
Comment on attachment 280011 [details] [diff] [review]
patch v3, address dietrich's review comments
seeking approval for m8
Attachment #280011 -
Flags: approval1.9?
Updated•17 years ago
|
Attachment #280011 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 10•17 years ago
|
||
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
Closed: 17 years ago
Resolution: --- → FIXED
Comment 11•17 years ago
|
||
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.
Comment 12•17 years ago
|
||
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.
Assignee | ||
Comment 13•17 years ago
|
||
Attachment #279966 -
Attachment is obsolete: true
Assignee | ||
Comment 14•17 years ago
|
||
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.
Assignee | ||
Comment 15•17 years ago
|
||
> 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.
Comment 16•17 years ago
|
||
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.
>
Comment 17•17 years ago
|
||
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•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [wanted-firefox3]
Updated•17 years ago
|
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3]
You need to log in
before you can comment on or make changes to this bug.
Description
•