From bb3a5576104c05f416a58f63af54fa6806469017 Mon Sep 17 00:00:00 2001 From: "edward.lee@engineering.uiuc.edu" Date: Mon, 25 Feb 2008 15:52:14 -0800 Subject: [PATCH] Bug 401660 - when showing autocomplete result, show tags that partially match. r=dietrich, a1.9=damons --- .../components/places/src/nsNavHistory.cpp | 6 +- toolkit/components/places/src/nsNavHistory.h | 7 +- .../places/src/nsNavHistoryAutoComplete.cpp | 167 ++++++++---------- .../places/tests/unit/test_000_frecency.js | 16 +- .../places/tests/unit/test_408221.js | 7 +- .../unit/test_history_autocomplete_tags.js | 39 ++-- 6 files changed, 120 insertions(+), 122 deletions(-) diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp index a28bdf299e63..5447d7678890 100644 --- a/toolkit/components/places/src/nsNavHistory.cpp +++ b/toolkit/components/places/src/nsNavHistory.cpp @@ -283,9 +283,9 @@ const PRInt32 nsNavHistory::kGetInfoIndex_ItemLastModified = 10; const PRInt32 nsNavHistory::kAutoCompleteIndex_URL = 0; const PRInt32 nsNavHistory::kAutoCompleteIndex_Title = 1; const PRInt32 nsNavHistory::kAutoCompleteIndex_FaviconURL = 2; -const PRInt32 nsNavHistory::kAutoCompleteIndex_ItemId = 3; -const PRInt32 nsNavHistory::kAutoCompleteIndex_ParentId = 4; -const PRInt32 nsNavHistory::kAutoCompleteIndex_BookmarkTitle = 5; +const PRInt32 nsNavHistory::kAutoCompleteIndex_ParentId = 3; +const PRInt32 nsNavHistory::kAutoCompleteIndex_BookmarkTitle = 4; +const PRInt32 nsNavHistory::kAutoCompleteIndex_Tags = 5; static const char* gQuitApplicationMessage = "quit-application"; static const char* gXpcomShutdown = "xpcom-shutdown"; diff --git a/toolkit/components/places/src/nsNavHistory.h b/toolkit/components/places/src/nsNavHistory.h index 3b46c1c08da9..8d0b8660db8c 100644 --- a/toolkit/components/places/src/nsNavHistory.h +++ b/toolkit/components/places/src/nsNavHistory.h @@ -662,9 +662,9 @@ protected: static const PRInt32 kAutoCompleteIndex_URL; static const PRInt32 kAutoCompleteIndex_Title; static const PRInt32 kAutoCompleteIndex_FaviconURL; - static const PRInt32 kAutoCompleteIndex_ItemId; static const PRInt32 kAutoCompleteIndex_ParentId; static const PRInt32 kAutoCompleteIndex_BookmarkTitle; + static const PRInt32 kAutoCompleteIndex_Tags; nsCOMPtr mDBAutoCompleteQuery; // kAutoCompleteIndex_* results nsCOMPtr mDBFeedbackIncrease; @@ -698,13 +698,14 @@ protected: nsDataHashtable mLivemarkFeedURIs; nsresult AutoCompleteFullHistorySearch(PRBool* aHasMoreResults); - nsresult AutoCompleteTagsSearch(); /** * Query type passed to AutoCompleteProcessSearch to determine what style to * use and if results should be filtered */ - enum QueryType { QUERY_TAGS, QUERY_FULL }; + enum QueryType { + QUERY_FULL + }; nsresult AutoCompleteProcessSearch(mozIStorageStatement* aQuery, const QueryType aType, PRBool *aHasMoreResults = nsnull); diff --git a/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp b/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp index 3148482690b5..19817bf3c841 100644 --- a/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp +++ b/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp @@ -106,11 +106,30 @@ nsNavHistory::InitAutoComplete() nsresult nsNavHistory::CreateAutoCompleteQueries() { + // Helper to get a particular column from the bookmark/tags table based on if + // we want to include tags or not +#define SQL_STR_FRAGMENT_GET_BOOK_TAG(column, comparison, getMostRecent) \ + NS_LITERAL_CSTRING( \ + "(SELECT " column " " \ + "FROM moz_bookmarks b " \ + "JOIN moz_bookmarks t ON t.id = b.parent AND t.parent " comparison " ?1 " \ + "WHERE b.type = ") + nsPrintfCString("%d", \ + nsINavBookmarksService::TYPE_BOOKMARK) + \ + NS_LITERAL_CSTRING(" AND b.fk = h.id") + \ + (getMostRecent ? NS_LITERAL_CSTRING(" " \ + "ORDER BY b.lastModified DESC LIMIT 1), ") : NS_LITERAL_CSTRING("), ")) + + // This gets three columns from the bookmarks and tags table followed by ", " + const nsCString &bookTag = + SQL_STR_FRAGMENT_GET_BOOK_TAG("b.parent", "!=", PR_TRUE) + + SQL_STR_FRAGMENT_GET_BOOK_TAG("b.title", "!=", PR_TRUE) + + SQL_STR_FRAGMENT_GET_BOOK_TAG("GROUP_CONCAT(t.title, ' ')", "=", PR_FALSE); + nsCString sql = NS_LITERAL_CSTRING( - "SELECT h.url, h.title, f.url, b.id, b.parent, b.title " + "SELECT h.url, h.title, f.url, ") + bookTag + + NS_LITERAL_CSTRING("NULL " "FROM moz_places h " - "LEFT OUTER JOIN moz_bookmarks b ON b.fk = h.id " - "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id " + "LEFT OUTER JOIN moz_favicons f ON f.id = h.favicon_id " "WHERE h.frecency <> 0 "); if (mAutoCompleteOnlyTyped) @@ -124,7 +143,7 @@ nsNavHistory::CreateAutoCompleteQueries() // in the case of a frecency tie, break it with h.typed and h.visit_count // which is better than nothing. but this is slow, so not doing it yet. sql += NS_LITERAL_CSTRING( - "ORDER BY h.frecency DESC LIMIT ?1 OFFSET ?2"); + "ORDER BY h.frecency DESC LIMIT ?2 OFFSET ?3"); nsresult rv = mDBConn->CreateStatement(sql, getter_AddRefs(mDBAutoCompleteQuery)); @@ -184,11 +203,6 @@ nsNavHistory::PerformAutoComplete() nsresult rv; // Only do some extra searches on the first chunk if (!mCurrentChunkOffset) { - // No need to search for tags if there's nothing to search - if (!mCurrentSearchString.IsEmpty()) { - rv = AutoCompleteTagsSearch(); - NS_ENSURE_SUCCESS(rv, rv); - } } PRBool moreChunksToSearch = PR_FALSE; @@ -348,59 +362,6 @@ nsNavHistory::AddSearchToken(nsAutoString &aToken) mCurrentSearchTokens.AppendString(aToken); } -nsresult -nsNavHistory::AutoCompleteTagsSearch() -{ - // NOTE: - // after migration or clear all private data, we might end up with - // a lot of places with frecency = -1 (until idle) - // - // XXX bug 412736 - // in the case of a frecency tie, break it with h.typed and h.visit_count - // which is better than nothing. but this is slow, so not doing it yet. - nsCString sql = NS_LITERAL_CSTRING( - "SELECT h.url, h.title, f.url, b.id, b.parent, b.title " - "FROM moz_places h " - "JOIN moz_bookmarks b ON b.fk = h.id " - "LEFT OUTER JOIN moz_favicons f ON f.id = h.favicon_id " - "WHERE h.frecency <> 0 AND (b.parent IN " - "(SELECT t.id FROM moz_bookmarks t WHERE t.parent = ?1 AND ("); - - nsStringArray terms; - CreateTermsFromTokens(mCurrentSearchTokens, terms); - - for (PRInt32 i = 0; i < terms.Count(); i++) { - if (i) - sql += NS_LITERAL_CSTRING(" OR"); - - // +2 to skip over the "?1", which is the tag root parameter - sql += NS_LITERAL_CSTRING(" LOWER(t.title) = ") + - nsPrintfCString("LOWER(?%d)", i + 2); - } - - sql += NS_LITERAL_CSTRING("))) " - "ORDER BY h.frecency DESC"); - - nsCOMPtr tagAutoCompleteQuery; - nsresult rv = mDBConn->CreateStatement(sql, - getter_AddRefs(tagAutoCompleteQuery)); - NS_ENSURE_SUCCESS(rv, rv); - - rv = tagAutoCompleteQuery->BindInt64Parameter(0, GetTagsFolder()); - NS_ENSURE_SUCCESS(rv, rv); - - for (PRInt32 i = 0; i < terms.Count(); i++) { - // +1 to skip over the "?1", which is the tag root parameter - rv = tagAutoCompleteQuery->BindStringParameter(i + 1, *(terms.StringAt(i))); - NS_ENSURE_SUCCESS(rv, rv); - } - - rv = AutoCompleteProcessSearch(tagAutoCompleteQuery, QUERY_TAGS); - NS_ENSURE_SUCCESS(rv, rv); - - return NS_OK; -} - // nsNavHistory::AutoCompleteFullHistorySearch // // Search for places that have a title, url, @@ -415,10 +376,13 @@ nsNavHistory::AutoCompleteFullHistorySearch(PRBool* aHasMoreResults) { mozStorageStatementScoper scope(mDBAutoCompleteQuery); - nsresult rv = mDBAutoCompleteQuery->BindInt32Parameter(0, mAutoCompleteSearchChunkSize); + nsresult rv = mDBAutoCompleteQuery->BindInt32Parameter(0, GetTagsFolder()); NS_ENSURE_SUCCESS(rv, rv); - rv = mDBAutoCompleteQuery->BindInt32Parameter(1, mCurrentChunkOffset); + rv = mDBAutoCompleteQuery->BindInt32Parameter(1, mAutoCompleteSearchChunkSize); + NS_ENSURE_SUCCESS(rv, rv); + + rv = mDBAutoCompleteQuery->BindInt32Parameter(2, mCurrentChunkOffset); NS_ENSURE_SUCCESS(rv, rv); rv = AutoCompleteProcessSearch(mDBAutoCompleteQuery, QUERY_FULL, aHasMoreResults); @@ -468,83 +432,92 @@ nsNavHistory::AutoCompleteProcessSearch(mozIStorageStatement* aQuery, NS_UnescapeURL(cEntryURL); NS_ConvertUTF8toUTF16 entryURL(cEntryURL); + PRInt64 parentId = 0; nsAutoString entryTitle, entryFavicon, entryBookmarkTitle; rv = aQuery->GetString(kAutoCompleteIndex_Title, entryTitle); NS_ENSURE_SUCCESS(rv, rv); rv = aQuery->GetString(kAutoCompleteIndex_FaviconURL, entryFavicon); NS_ENSURE_SUCCESS(rv, rv); - PRInt64 itemId = 0; - rv = aQuery->GetInt64(kAutoCompleteIndex_ItemId, &itemId); + rv = aQuery->GetInt64(kAutoCompleteIndex_ParentId, &parentId); NS_ENSURE_SUCCESS(rv, rv); - - PRInt64 parentId = 0; - // Only bother to fetch parent id and bookmark title if we have a bookmark - if (itemId) { - rv = aQuery->GetInt64(kAutoCompleteIndex_ParentId, &parentId); - NS_ENSURE_SUCCESS(rv, rv); + + // Only fetch the bookmark title if we have a bookmark + if (parentId) { rv = aQuery->GetString(kAutoCompleteIndex_BookmarkTitle, entryBookmarkTitle); NS_ENSURE_SUCCESS(rv, rv); } + nsAutoString entryTags; + rv = aQuery->GetString(kAutoCompleteIndex_Tags, entryTags); + NS_ENSURE_SUCCESS(rv, rv); + PRBool useBookmark = PR_FALSE; + PRBool showTags = PR_FALSE; nsString style; switch (aType) { - case QUERY_TAGS: { - // Always prefer the bookmark title unless we don't have one - useBookmark = !entryBookmarkTitle.IsEmpty(); - - // Tags have a special stype to show a tag icon - style = NS_LITERAL_STRING("tag"); - - break; - } case QUERY_FULL: { // If we get any results, there's potentially another chunk to proces if (aHasMoreResults) *aHasMoreResults = PR_TRUE; - // Determine if every token matches either the bookmark, title, or url + // 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 current token matches the bookmark - PRBool bookmarkMatch = itemId && + PRBool bookmarkMatch = parentId && CaseInsensitiveFindInReadable(*token, entryBookmarkTitle); // If any part of the search string is in the bookmark title, show // that in the result instead of the page title useBookmark |= bookmarkMatch; + // If the token is in any of the tags, remember to show tags + PRBool tagsMatch = CaseInsensitiveFindInReadable(*token, entryTags); + showTags |= tagsMatch; + // True if any of them match; false makes us quit the loop - matchAll = bookmarkMatch || + matchAll = bookmarkMatch || tagsMatch || CaseInsensitiveFindInReadable(*token, entryTitle) || CaseInsensitiveFindInReadable(*token, entryURL); } - // Skip if we don't match all terms in the bookmark, title or url + // Skip if we don't match all terms in the bookmark, tag, title or url if (!matchAll) continue; - // Style bookmarks that aren't feed items and feed URIs as bookmark - style = (itemId && !mLivemarkFeedItemIds.Get(parentId, &dummy)) || - mLivemarkFeedURIs.Get(escapedEntryURL, &dummy) ? - NS_LITERAL_STRING("bookmark") : NS_LITERAL_STRING("favicon"); - break; } default: { - // Always prefer the bookmark title unless we don't have one - useBookmark = !entryBookmarkTitle.IsEmpty(); - - // Style bookmarks that aren't feed items and feed URIs as bookmark - style = (itemId && !mLivemarkFeedItemIds.Get(parentId, &dummy)) || - mLivemarkFeedURIs.Get(escapedEntryURL, &dummy) ? - NS_LITERAL_STRING("bookmark") : NS_LITERAL_STRING("favicon"); + // Always prefer to show tags if we have them; otherwise, prefer the + // bookmark title if we have it + if (!entryTags.IsEmpty()) + showTags = PR_TRUE; + else + useBookmark = !entryBookmarkTitle.IsEmpty(); break; } } + // Add the tags to the title if necessary + if (showTags) { + // Always show the bookmark if possible when we have tags + useBookmark = !entryBookmarkTitle.IsEmpty(); + /* XXX bug 418257 to look at RTL issues of appending tags + (useBookmark ? entryBookmarkTitle : entryTitle) + += NS_LITERAL_STRING(" (") + entryTags + NS_LITERAL_STRING(")"); + */ + } + + // Tags have a special style to show a tag icon; otherwise, style the + // bookmarks that aren't feed items and feed URIs as bookmark + style = showTags ? NS_LITERAL_STRING("tag") : (parentId && + !mLivemarkFeedItemIds.Get(parentId, &dummy)) || + mLivemarkFeedURIs.Get(escapedEntryURL, &dummy) ? + NS_LITERAL_STRING("bookmark") : NS_LITERAL_STRING("favicon"); + // Pick the right title to show based on the result of the query type const nsAString &title = useBookmark ? entryBookmarkTitle : entryTitle; diff --git a/toolkit/components/places/tests/unit/test_000_frecency.js b/toolkit/components/places/tests/unit/test_000_frecency.js index 4f7f4b8700e3..f90149cf58e8 100644 --- a/toolkit/components/places/tests/unit/test_000_frecency.js +++ b/toolkit/components/places/tests/unit/test_000_frecency.js @@ -24,6 +24,7 @@ version(180); * Matt Crocker * Seth Spitzer * Dietrich Ayala + * Edward Lee * * Alternatively, the contents of this file may be used under the terms of * either the GNU General Public License Version 2 or later (the "GPL"), or @@ -253,8 +254,19 @@ function run_test() { // test that matches are sorted by frecency for (var i = 0; i < controller.matchCount; i++) { - do_check_eq(controller.getValueAt(i), results[i][0].spec); - do_check_eq(controller.getCommentAt(i), results[i][2]); + let searchURL = controller.getValueAt(i); + let expectURL = results[i][0].spec; + if (searchURL == expectURL) { + do_check_eq(controller.getValueAt(i), results[i][0].spec); + do_check_eq(controller.getCommentAt(i), results[i][2]); + } else { + // If the results didn't match exactly, perhaps it's still the right + // frecency just in the wrong "order" (order of same frecency is + // undefined), so check if frecency matches. This is okay because we + // can still ensure the correct number of expected frecencies. + let getFrecency = function(aURL) aURL.match(/frecency:(-?\d+)$/)[1]; + do_check_eq(getFrecency(searchURL), getFrecency(expectURL)); + } } do_test_finished(); diff --git a/toolkit/components/places/tests/unit/test_408221.js b/toolkit/components/places/tests/unit/test_408221.js index 36fef3f5d0f2..db1dcba6491f 100755 --- a/toolkit/components/places/tests/unit/test_408221.js +++ b/toolkit/components/places/tests/unit/test_408221.js @@ -22,6 +22,7 @@ * Contributor(s): * Matt Crocker * Seth Spitzer + * Edward Lee * * Alternatively, the contents of this file may be used under the terms of * either the GNU General Public License Version 2 or later (the "GPL"), or @@ -124,10 +125,14 @@ function ensure_tag_results(uris, searchTerm) do_check_eq(controller.searchStatus, Ci.nsIAutoCompleteController.STATUS_COMPLETE_MATCH); do_check_eq(controller.matchCount, uris.length); + let vals = []; for (var i=0; i * Seth Spitzer + * Edward Lee * * Alternatively, the contents of this file may be used under the terms of * either the GNU General Public License Version 2 or later (the "GPL"), or @@ -101,6 +102,7 @@ try { function ensure_tag_results(uris, searchTerm) { + print("Searching for '" + searchTerm + "'"); var controller = Components.classes["@mozilla.org/autocomplete/controller;1"]. getService(Components.interfaces.nsIAutoCompleteController); @@ -122,12 +124,18 @@ function ensure_tag_results(uris, searchTerm) input.onSearchComplete = function() { do_check_eq(numSearchesStarted, 1); do_check_eq(controller.searchStatus, - Ci.nsIAutoCompleteController.STATUS_COMPLETE_MATCH); + uris.length ? + Ci.nsIAutoCompleteController.STATUS_COMPLETE_MATCH : + Ci.nsIAutoCompleteController.STATUS_COMPLETE_NO_MATCH); do_check_eq(controller.matchCount, uris.length); + let vals = []; for (var i=0; i