385829 - Search for bookmark queries by lastModified / dateDated

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Asaf Romano (gone)]

For the recently used tags query, we need support for something like
place:folder=[TAG_ROOT]&sort=[LAST_MODIFIED]&maxResults=10

Comment 1

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

accepting.  will start on this ASAP so that I can unblock Mano's other bugs.

Comment 2

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

Attachment: patch, v1

still need:

example urls:

// toolbar folder (for me) last modified descending
place:folder=6&queryType=1&sort=14&maxResults=5 

// toolbar folder (for me) last modified ascending 
place:folder=6&queryType=1&sort=13&maxResults=5 

1)  unit tests
2)  check that when changes are made to the modification date, changes are observed

Comment 3

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

Attachment: patch, v2

determine the proper update requirements for this type of query.

in the past, we have assumed that if you define the max results, it must be a history query, but that is no longer the case.

Comment 4

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

Attachment: patch, v3

add support for sorting by date added as well

Comment 5

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

with that you can do an interesting query like:

place:queryType=1&sort=12&maxResults=8&onlyBookmarked=1

"show me the top 8 bookmarks sorted by date added descending"

I'll go write some unit tests and then seek review.

Comment 6

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

Attachment: unit tests

Comment 7

[Asaf Romano (gone)]

Seth: wrong bug or wrong patch?

Comment 8

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

> Seth: wrong bug or wrong patch?

Actually, an incomplete patch.  I misunderstood the problem.

tags are folders (under the tag root, for example, folder=3), and a tagged uri is a bookmark under that folder.

I'll continue working on what you want, which is support for a query that results in the child folders of a folder, sorted by last modified.

I still think we want these changes, though.  Can you review them?

Comment 9

[Asaf Romano (gone)]

Comment on attachment 272354 [details] [diff] [review]
patch, v3

>Index: toolkit/components/places/src/nsNavHistory.cpp
>===================================================================

>+        NS_WARNING("SORT_BY_DATEADDED_DESCENDING only makes sense for bookmark queries");
>+        return NS_ERROR_UNEXPECTED;
>+      }
>+      break;

I think we better ignore the it here and not throw, the post-query sorting methods have their fallbacks for cases like this.

r=mano otherwise.

Comment 10

[Asaf Romano (gone)]

Comment on attachment 272364 [details] [diff] [review]
unit tests

Seth, any chance you can move this test to a separate file (test_bugXXX etc.)? This one is getting overloaded.

Comment 11

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

> I think we better ignore the it here and not throw, the post-query sorting
> methods have their fallbacks for cases like this.

I don't think the post-query sorting methods will handle it.  What are you thinking of?  I think we'll just proceed silently, and not sort at all.

Comment 12

[Asaf Romano (gone)]

http://lxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsNavHistoryResult.cpp#809

Comment 13

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

Attachment: break out unit test, per mano

Comment 14

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

morphing.  logged bug #388378 about what Mano needs for tagging.

Comment 15

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

Attachment: patch, v4 (as checked in)

Comment 16

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

fixed.

Checking in nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHis
tory.cpp
new revision: 1.143; previous revision: 1.142
done

awaiting review from mano before I check in the unit, so marking in-testsuite?

Comment 17

[Asaf Romano (gone)]

Comment on attachment 272575 [details] [diff] [review]
break out unit test, per mano

>Index: test_385829.js
>===================================================================

>+// get bookmarks root index
>+var root = bmsvc.bookmarksRoot;

id, not index.

>+// main
>+function run_test() {
>+  // test search on folder with various sorts and max results
>+  // see bug #385829
>+  try {

please remove the surrounding try block.

>+    var bug385829folder 
>+      = bmsvc.createFolder(root, "bug 385829 test", bmsvc.DEFAULT_INDEX);

you can use a shorter name now.


>+    var options = histsvc.getNewQueryOptions();
>+    var query = histsvc.getNewQuery();
>+    options.queryType = Ci.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS;
>+    options.maxResults = 3;
>+    query.onlyBookmarked = true;

onlyBookmarked is meaningless for QUERY_TYPE_BOOKMARKS

>+    // test SORT_BY_DATEADDED_ASCENDING
>+    options.sortingMode = options.SORT_BY_DATEADDED_ASCENDING;
>+    var result = histsvc.executeQuery(query, options);
>+    var rootNode = result.root;
>+    rootNode.containerOpen = true;
>+    var cc = rootNode.childCount;
>+    do_check_true(cc == 3);

nit: do_check_eq(cc, 3)



>+    rootNode.containerOpen = false;
>+    // test SORT_BY_...
>+    options.sortingMode = options.SORT_BY_...;
>+    result = histsvc.executeQuery(query, options);
>+    rootNode = result.root;
>+    rootNode.containerOpen = true;

hrm, i would rather set the new sortingMode on the result directly so we also test live update.

Comment 18

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

Attachment: unit tests v2, per mano

Comment 19

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

the fix landed on 7/16, so marking fixed.  but the unit test is still pending review.

Comment 20

[Asaf Romano (gone)]

Comment on attachment 273027 [details] [diff] [review]
unit tests v2, per mano

r=mano

Comment 21

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

unit tests checked in:

cvs commit: Examining .
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_385829
.js,v
done
Checking in test_385829.js;
/cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_385829.js,v  <--
  test_385829.js
initial revision: 1.1
done
Checking in test_bookmarks.js;
/cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_bookmarks.js,v
<--  test_bookmarks.js
new revision: 1.33; previous revision: 1.32
done

Comment 22

[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