Closed Bug 384228 Opened 13 years ago Closed 12 years ago

Implement Search-In-Folder(s)

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3 beta1

People

(Reporter: mano, Assigned: mano)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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.
Flags: blocking-firefox3?
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: --- → Firefox 3 M8
Assignee: nobody → dietrich
mano, do you need this for anything in M8?
note: fixing this will require explicit declaration of GROUP_BY_FOLDER in queries that are relying on the current implicit grouping behaviour.
OS: Mac OS X → All
Hardware: PC → All
Attached patch fix v1 (obsolete) — Splinter Review
Attachment #279672 - Flags: review?(sspitzer)
Attached patch fix v1 (with tests) (obsolete) — Splinter Review
Attachment #279672 - Attachment is obsolete: true
Attachment #279673 - Flags: review?(sspitzer)
Attachment #279672 - Flags: review?(sspitzer)
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)
Attachment #279673 - Flags: review?(sspitzer) → review+
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.
yes, agreed, this is probably more useful if it recurses
Target Milestone: Firefox 3 M8 → Firefox 3 M9
Attachment #279673 - Attachment is obsolete: true
Assignee: dietrich → mano
Priority: -- → P2
Summary: multiple-folders in complex queries are ignored → Implement Search-In-Folder(s)
Target Milestone: Firefox 3 M9 → Firefox 3 M8
Blocks: 393464
Status: NEW → ASSIGNED
Target Milestone: Firefox 3 M8 → Firefox 3 M9
Attached patch patch (obsolete) — Splinter Review
* multiple folders support
 * recursive search
 * bug 395593.
 * tests
Attachment #280280 - Flags: review?(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
Attachment #280280 - Flags: review?(dietrich) → review+
Blocks: 378799
Attached patch as checked inSplinter Review
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
Attachment #280280 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b5pre) Gecko/2008032504 Minefield/3.0b5pre
Status: RESOLVED → VERIFIED
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.