Closed Bug 452000 Opened 12 years ago Closed 11 years ago

Sort by keyword in Library does not work.

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: paul, Assigned: mak)

References

Details

(Keywords: regression, verified1.9.1)

Attachments

(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1

In Firefox 2, sort by keyword in the bookmarks manager by clicking its column worked properly, but in Firefox 3, it does not.

Reproducible: Always

Steps to Reproduce:
1. Open Library.
2. Ensure Keyword column is visible in right pane.
3. Click on the column header.
Actual Results:  
Random sorting--at least I can't figure out the sort algorithm. And if you click on the column header twice more (to go to descending then back to ascending order), the ordering is different! And doing that again and again, can give different ordering each time.

Expected Results:  
Ascending alphabetical order by keyword, with no-keyword bookmarks all at the top or bottom.
Can't reproduce in Mozilla/5.0 (Windows; U; Windows NT 6.0; de; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1 (Vista). 
Sorting bookmarks per keywords works fine for me. 
I should add that I get similar results if I try sorting via the View menu.
This problem is also referred to in bug 424076, comment #8 (https://bugzilla.mozilla.org/show_bug.cgi?id=424076#c8).
Flags: blocking-firefox3.1?
This is not going to block the 3.1 release. Marking wanted+.
Flags: wanted-firefox3.1+
Flags: blocking-firefox3.1?
Flags: blocking-firefox3.1-
Keywords: regression
Priority: -- → P2
Target Milestone: --- → Firefox 3.1
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
Version: unspecified → Trunk
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Flags: in-testsuite?
Attached patch patch v1.0 (obsolete) — Splinter Review
the bug is due to the fact we are comparing aKeyword with aKeyword (the same!)

So while there i added a global query sorting test, tag sorting test do not exists obviously (bug 444179)
Attachment #356285 - Flags: review?(dietrich)
Comment on attachment 356285 [details] [diff] [review]
patch v1.0


>@@ -64,16 +66,21 @@ if (!profileDir) {
> if (!profileDir) {
>  // Register our own provider for the profile directory.
>  // It will simply return the current directory.
>  var provider = {
>    getFile: function(prop, persistent) {
>      persistent.value = true;
>      if (prop == NS_APP_USER_PROFILE_50_DIR) {
>        return dirSvc.get("CurProcD", Ci.nsIFile);
>+     }
>+     if (prop == NS_APP_HISTORY_50_FILE) {
>+       var histFile = dirSvc.get("CurProcD", Ci.nsIFile);
>+       histFile.append("history.dat");
>+       return histFile;

what's this for?

>+////////////////////////////////////////////////////////////////////////////////
>+
>+tests.push({
>+  _sortingMode: histsvc.SORT_BY_TITLE_ASCENDING,
>+
>+  setup: function() {
>+    LOG("Sorting test 2: SORT BY TITLE");
>+
>+    this._unsortedData = [
>+      { isBookmark: true,
>+        uri: "http://urlB.com/",
>+        parentFolder: bmsvc.toolbarFolder,
>+        index: bmsvc.DEFAULT_INDEX,
>+        title: "titleB",
>+        isInQuery: true },
>+
>+      { isBookmark: true,
>+        uri: "http://urlA.com/",
>+        parentFolder: bmsvc.toolbarFolder,
>+        index: bmsvc.DEFAULT_INDEX,
>+        title: "titleA",
>+        isInQuery: true },
>+
>+      { isBookmark: true,
>+        uri: "http://urlC.com/",
>+        parentFolder: bmsvc.toolbarFolder,
>+        index: bmsvc.DEFAULT_INDEX,
>+        title: "titleC",
>+        isInQuery: true },
>+    ];
>+
>+    this._sortedData = this._unsortedData = [
>+      this._unsortedData[1],
>+      this._unsortedData[0],
>+      this._unsortedData[2],
>+    ];
>+
>+    // This function in head_queries.js creates our database with the above data
>+    populateDB(this._unsortedData);
>+  },
>+
>+  check: function() {
>+    // Query
>+    var query = histsvc.getNewQuery();
>+    query.setFolders([bmsvc.toolbarFolder], 1);
>+    query.onlyBookmarked = true;
>+    
>+    // query options
>+    var options = histsvc.getNewQueryOptions();
>+    options.sortingMode = this._sortingMode;
>+
>+    // Results - this gets the result set and opens it for reading and modification.
>+    var result = histsvc.executeQuery(query, options);
>+    var root = result.root;
>+    root.containerOpen = true;
>+    compareArrayToResult(this._sortedData, root);
>+    root.containerOpen = false;
>+  },
>+
>+  check_reverse: function() {
>+    this._sortingMode = histsvc.SORT_BY_TITLE_DESCENDING;
>+    this._sortedData.reverse();
>+    return;
>+  }
>+});

maybe i'm missing something, but i don't see how check_reverse() is checking anything. you reverse the sorted data, but don't actually re-run check() anywhere.
(In reply to comment #6)
> (From update of attachment 356285 [details] [diff] [review])
> 
> >@@ -64,16 +66,21 @@ if (!profileDir) {
> > if (!profileDir) {
> >  // Register our own provider for the profile directory.
> >  // It will simply return the current directory.
> >  var provider = {
> >    getFile: function(prop, persistent) {
> >      persistent.value = true;
> >      if (prop == NS_APP_USER_PROFILE_50_DIR) {
> >        return dirSvc.get("CurProcD", Ci.nsIFile);
> >+     }
> >+     if (prop == NS_APP_HISTORY_50_FILE) {
> >+       var histFile = dirSvc.get("CurProcD", Ci.nsIFile);
> >+       histFile.append("history.dat");
> >+       return histFile;
> 
> what's this for?

that's required by nsnavhistory::RemoveAllPages
http://mxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsNavHistory.cpp#4009
we have the same code in toolkit tests header, even if probably returning NS_OK there could be still ok.


> 
> maybe i'm missing something, but i don't see how check_reverse() is checking
> anything. you reverse the sorted data, but don't actually re-run check()
> anywhere.

uh, that was the change i made at last minute to reduce test size, i must remember that after the midnight i have to triple check :) should call check() clearly
Attached patch patch v1.1Splinter Review
Attachment #356285 - Attachment is obsolete: true
Attachment #356312 - Flags: review?(dietrich)
Attachment #356285 - Flags: review?(dietrich)
Comment on attachment 356312 [details] [diff] [review]
patch v1.1

r=me
Attachment #356312 - Flags: review?(dietrich) → review+
http://hg.mozilla.org/mozilla-central/rev/7c2dc72c7ace
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment on attachment 356312 [details] [diff] [review]
patch v1.1

asking approval for 1.9.1, with test for all sortings
Attachment #356312 - Flags: approval1.9.1?
Flags: in-testsuite? → in-testsuite+
Target Milestone: Firefox 3.1 → Firefox 3.2a1
Comment on attachment 356312 [details] [diff] [review]
patch v1.1

a191=beltzner
Attachment #356312 - Flags: approval1.9.1? → approval1.9.1+
Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090312 Shiretoko/3.1b4pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090312 Shiretoko/3.1b4pre.

Verified fixed on the trunk using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090312 Minefield/3.2a1pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090312 Minefield/3.2a1pre
Status: RESOLVED → VERIFIED
Duplicate of this bug: 490291
Duplicate of this bug: 498879
You need to log in before you can comment on or make changes to this bug.