384228 - Implement Search-In-Folder(s)

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Asaf Romano (gone)]

The query API allows one to set multiple folders for a query (via setFolders). The implementation in nsNavHistory only looks at the first folder in the passed-in folders-array though.

Comment 1

[Dietrich Ayala (:dietrich)]

mano, do you need this for anything in M8?

Comment 2

[Dietrich Ayala (:dietrich)]

note: fixing this will require explicit declaration of GROUP_BY_FOLDER in queries that are relying on the current implicit grouping behaviour.

Comment 3

[Dietrich Ayala (:dietrich)]

Attachment: fix v1

Comment 4

[Dietrich Ayala (:dietrich)]

Attachment: fix v1 (with tests)

Comment 5

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

Comment on attachment 279673 [details] [diff] [review]
fix v1 (with tests)

r=sspitzer (but please list yourself as the contributor for the test, not me)

Comment 6

[Asaf Romano (gone)]

Comment on attachment 279673 [details] [diff] [review]
fix v1 (with tests)

Dietrich, I think we better fix this at the same time we make "Search In Folder" search _recursively_, the current code is hardly a work-around.

Comment 7

[Dietrich Ayala (:dietrich)]

yes, agreed, this is probably more useful if it recurses

Comment 8

[Asaf Romano (gone)]

Attachment: patch

* multiple folders support
 * recursive search
 * bug 395593.
 * tests

Comment 9

[Dietrich Ayala (:dietrich)]

Comment on attachment 280280 [details] [diff] [review]
patch

a few nits below, but r=me otherwise.

however, i'm worried about the long-term performance of post-query filtering for folders because the scope of bookmarks searches, at the SQL level, includes everything in moz_bookmarks. as tag usage proliferates, this may become problematic. right now, perf of search on large bookmarks sets is not stellar, so this argument is not intrinsic to this change, but it may exacerbate the problem.

one optimization might be to restrict bookmark queries to a set of known folders, eg: bookmarks root and "unfiled", and {insert extension hook folder list here}.

>@@ -2379,59 +2384,63 @@ nsNavHistory::GetQueryResults(nsNavHisto
> 
>   // create statement
>   nsCOMPtr<mozIStorageStatement> statement;
>   rv = mDBConn->CreateStatement(queryString, getter_AddRefs(statement));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // bind parameters
>   PRInt32 numParameters = 0;
>+
>+  // optimize the case where we just want a list with no grouping: this
>+  // directly fills in the results and we avoid a copy of the whole list
>+  PRBool resultAsList = PR_TRUE;
>+  PRUint32 groupCount;
>+  const PRUint16 *groupings = aOptions->GroupingMode(&groupCount);
>+
>+  if (groupCount != 0 || aOptions->ExcludeQueries()) {
>+    resultAsList = PR_FALSE;
>+  }
>+
>   PRInt32 i;
>   for (i = 0; i < aQueries.Count(); i ++) {
>     PRInt32 clauseParameters = 0;
>     rv = BindQueryClauseParameters(statement, numParameters,
>                                    aQueries[i], aOptions, &clauseParameters);
>     NS_ENSURE_SUCCESS(rv, rv);
>     numParameters += clauseParameters;
>+    if (resultAsList) {
>+      if (aQueries[i]->Folders().Length() != 0) {
>+        resultAsList = PR_FALSE;
>+      } else {
>+        PRBool hasSearchTerms;
>+        rv = aQueries[i]->GetHasSearchTerms(&hasSearchTerms);
>+        if (hasSearchTerms)
>+          resultAsList = PR_FALSE;
>+        NS_ENSURE_SUCCESS(rv, rv);
>+      }
>+    }

when bug 331487 is fixed (i have a wip patch), bookmark queries can group by folder or not regardless of specified folders or search terms. please add a note about this.

>+          if (belongs) {
>+            includeFolders[queryIndex]->AppendElement(lastAncestor);
>+          } else {
>+            excludeFolders[queryIndex]->AppendElement(lastAncestor);
>+            continue;
>+          }
>+        }
>+      }
> 
>-      for (PRInt32 termIndex = 0; termIndex < terms.Count(); termIndex ++) {
>+      // serach terms

s/serach/search/

>+      PRBool allTermsFound = PR_TRUE;
>+      for (PRInt32 termIndex = 0; termIndex < terms[queryIndex]->Count() && allTermsFound; termIndex ++) {

nit: line length

Comment 10

[Asaf Romano (gone)]

Attachment: as checked in

mozilla/toolkit/components/places/src/nsNavHistory.cpp 1.171
mozilla/toolkit/components/places/src/nsNavHistory.h 1.101
mozilla/toolkit/components/places/tests/bookmarks/test_384228.js
initial revision: 1.1
mozilla/toolkit/components/places/tests/bookmarks/test_395593.js
initial revision: 1.1

Comment 11

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b5pre) Gecko/2008032504 Minefield/3.0b5pre

Comment 12

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv