Bug 444179 - Library>Views>Sort>Sort by Tags does nothing, r=mak77

browser/components/places/content/places.js

@@ -23,6 +23,7 @@
  *   Annie Sullivan <annie.sullivan@gmail.com>
  *   Asaf Romano <mano@mozilla.com>
  *   Ehsan Akhgari <ehsan.akhgari@gmail.com>
+ *   Drew Willcoxon <adw@mozilla.com>
  *
  * 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
@@ -1732,11 +1733,11 @@ var ViewMenu = {
 
     var columnId;
     if (aColumn) {
-      columnId = aColumn.getAttribute("anonid")
+      columnId = aColumn.getAttribute("anonid");
       if (!aDirection) {
         var sortColumn = this._getSortColumn();
-        aDirection = sortColumn ?
-                     sortColumn.getAttribute("sortDirection") : "descending";
+        if (sortColumn)
+          aDirection = sortColumn.getAttribute("sortDirection");
       }
     }
     else {
@@ -1744,47 +1745,38 @@ var ViewMenu = {
       columnId = sortColumn ? sortColumn.getAttribute("anonid") : "title";
     }
 
-    var sortingMode;
-    var sortingAnnotation = "";
-    const NHQO = Ci.nsINavHistoryQueryOptions;
-    switch (columnId) {
-      case "title":
-        sortingMode = aDirection == "descending" ?
-          NHQO.SORT_BY_TITLE_DESCENDING : NHQO.SORT_BY_TITLE_ASCENDING;
-        break;
-      case "url":
-        sortingMode = aDirection == "descending" ?
-          NHQO.SORT_BY_URI_DESCENDING : NHQO.SORT_BY_URI_ASCENDING;
-        break;
-      case "date":
-        sortingMode = aDirection == "descending" ?
-          NHQO.SORT_BY_DATE_DESCENDING : NHQO.SORT_BY_DATE_ASCENDING;
-        break;
-      case "visitCount":
-        sortingMode = aDirection == "descending" ?
-          NHQO.SORT_BY_VISITCOUNT_DESCENDING : NHQO.SORT_BY_VISITCOUNT_ASCENDING;
-        break;
-      case "keyword":
-        sortingMode = aDirection == "descending" ?
-          NHQO.SORT_BY_KEYWORD_DESCENDING : NHQO.SORT_BY_KEYWORD_ASCENDING;
-        break;
-      case "description":
-        sortingAnnotation = DESCRIPTION_ANNO;
-        sortingMode = aDirection == "descending" ?
-          NHQO.SORT_BY_ANNOTATION_DESCENDING : NHQO.SORT_BY_ANNOTATION_ASCENDING;
-        break;
-      case "dateAdded":
-        sortingMode = aDirection == "descending" ?
-          NHQO.SORT_BY_DATEADDED_DESCENDING : NHQO.SORT_BY_DATEADDED_ASCENDING;
-        break;
-      case "lastModified":
-        sortingMode = aDirection == "descending" ?
-          NHQO.SORT_BY_LASTMODIFIED_DESCENDING : NHQO.SORT_BY_LASTMODIFIED_ASCENDING;
-        break;
-      default:
-        throw("Invalid Column");
-    }
-    result.sortingAnnotation = sortingAnnotation;
-    result.sortingMode = sortingMode;
+    // This maps the possible values of columnId (i.e., anonid's of treecols in
+    // placeContent) to the default sortingMode and sortingAnnotation values for
+    // each column.
+    //   key:  Sort key in the name of one of the
+    //         nsINavHistoryQueryOptions.SORT_BY_* constants
+    //   dir:  Default sort direction to use if none has been specified
+    //   anno: The annotation to sort by, if key is "ANNOTATION"
+    var colLookupTable = {
+      title:        { key: "TITLE",        dir: "ascending"  },
+      tags:         { key: "TAGS",         dir: "ascending"  },
+      url:          { key: "URI",          dir: "ascending"  },
+      date:         { key: "DATE",         dir: "descending" },
+      visitCount:   { key: "VISITCOUNT",   dir: "descending" },
+      keyword:      { key: "KEYWORD",      dir: "ascending"  },
+      dateAdded:    { key: "DATEADDED",    dir: "descending" },
+      lastModified: { key: "LASTMODIFIED", dir: "descending" },
+      description:  { key: "ANNOTATION",
+                      dir: "ascending",
+                      anno: DESCRIPTION_ANNO }
+    };
+
+    // Make sure we have a valid column.
+    if (!colLookupTable.hasOwnProperty(columnId))
+      throw("Invalid column");
+
+    // Use a default sort direction if none has been specified.  If aDirection
+    // is invalid, result.sortingMode will be undefined, which has the effect
+    // of unsorting the tree.
+    aDirection = (aDirection || colLookupTable[columnId].dir).toUpperCase();
+
+    var sortConst = "SORT_BY_" + colLookupTable[columnId].key + "_" + aDirection;
+    result.sortingAnnotation = colLookupTable[columnId].anno || "";
+    result.sortingMode = Ci.nsINavHistoryQueryOptions[sortConst];
   }
 };

browser/components/places/tests/browser/Makefile.in

@@ -47,6 +47,7 @@ _BROWSER_TEST_FILES = \
 	browser_425884.js \
 	browser_423515.js \
 	browser_457473_no_copy_guid.js \
+	browser_sort_in_library.js \
 	$(NULL)
 
 libs:: $(_BROWSER_TEST_FILES)

browser/components/places/tests/browser/browser_sort_in_library.js

@@ -0,0 +1,295 @@
+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim:set ts=2 sw=2 sts=2 et: */
+/* ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
+ *
+ * The contents of this file are subject to the Mozilla Public License Version
+ * 1.1 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * http://www.mozilla.org/MPL/
+ *
+ * Software distributed under the License is distributed on an "AS IS" basis,
+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
+ * for the specific language governing rights and limitations under the
+ * License.
+ *
+ * The Original Code is Places test code.
+ *
+ * The Initial Developer of the Original Code is Mozilla Corp.
+ * Portions created by the Initial Developer are Copyright (C) 2009
+ * the Initial Developer. All Rights Reserved.
+ *
+ * Contributor(s):
+ *  Drew Willcoxon <adw@mozilla.com> (Original Author)
+ *
+ * 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
+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
+ * in which case the provisions of the GPL or the LGPL are applicable instead
+ * of those above. If you wish to allow use of your version of this file only
+ * under the terms of either the GPL or the LGPL, and not to allow others to
+ * use your version of this file under the terms of the MPL, indicate your
+ * decision by deleting the provisions above and replace them with the notice
+ * and other provisions required by the GPL or the LGPL. If you do not delete
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the MPL, the GPL or the LGPL.
+ *
+ * ***** END LICENSE BLOCK ***** */
+
+/**
+ * Tests the following bugs:
+ *
+ * Bug 443745 - View>Sort>of "alpha" sort items is default to Z>A instead of A>Z
+ * https://bugzilla.mozilla.org/show_bug.cgi?id=443745
+ *
+ * Bug 444179 - Library>Views>Sort>Sort by Tags does nothing
+ * https://bugzilla.mozilla.org/show_bug.cgi?id=444179
+ *
+ * Basically, fully tests sorting the placeContent tree in the Places Library
+ * window.  Sorting is verified by comparing the nsINavHistoryResult returned by
+ * placeContent.getResult to the expected sort values.
+ */
+
+// Two properties of nsINavHistoryResult control the sort of the tree:
+// sortingMode and sortingAnnotation.  sortingMode's value is one of the
+// nsINavHistoryQueryOptions.SORT_BY_* constants.  sortingAnnotation is the
+// annotation used to sort for SORT_BY_ANNOTATION_* mode.
+//
+// This lookup table maps the possible values of anonid's of the treecols to
+// objects that represent the treecols' correct state after the user sorts the
+// previously unsorted tree by selecting a column from the Views > Sort menu.
+// sortingMode is constructed from the key and dir properties (i.e.,
+// SORT_BY_<key>_<dir>) and sortingAnnotation is checked against anno.  anno
+// may be undefined if key is not "ANNOTATION".
+const SORT_LOOKUP_TABLE = {
+  title:        { key: "TITLE",        dir: "ASCENDING"  },
+  tags:         { key: "TAGS",         dir: "ASCENDING"  },
+  url:          { key: "URI",          dir: "ASCENDING"  },
+  date:         { key: "DATE",         dir: "DESCENDING" },
+  visitCount:   { key: "VISITCOUNT",   dir: "DESCENDING" },
+  keyword:      { key: "KEYWORD",      dir: "ASCENDING"  },
+  dateAdded:    { key: "DATEADDED",    dir: "DESCENDING" },
+  lastModified: { key: "LASTMODIFIED", dir: "DESCENDING" },
+  description:  { key:  "ANNOTATION",
+                  dir:  "ASCENDING",
+                  anno: "bookmarkProperties/description" }
+};
+
+// This is the column that's sorted if one is not specified and the tree is
+// currently unsorted.  Set it to a key substring in the name of one of the
+// nsINavHistoryQueryOptions.SORT_BY_* constants, e.g., "TITLE", "URI".
+// Method ViewMenu.setSortColumn in browser/components/places/content/places.js
+// determines this value.
+const DEFAULT_SORT_KEY = "TITLE";
+
+// Part of the test is checking that sorts stick, so each time we sort we need
+// to remember it.
+let prevSortDir = null;
+let prevSortKey = null;
+
+///////////////////////////////////////////////////////////////////////////////
+
+/**
+ * Ensures that the sort of aTree is aSortingMode and aSortingAnno.
+ *
+ * @param aTree
+ *        the tree to check
+ * @param aSortingMode
+ *        one of the Ci.nsINavHistoryQueryOptions.SORT_BY_* constants
+ * @param aSortingAnno
+ *        checked only if sorting mode is one of the
+ *        Ci.nsINavHistoryQueryOptions.SORT_BY_ANNOTATION_* constants
+ */
+function checkSort(aTree, aSortingMode, aSortingAnno) {
+  // The placeContent tree's sort is determined by the nsINavHistoryResult it
+  // stores.  Get it and check that the sort is what the caller expects.
+  let res = aTree.getResult();
+  isnot(res, null,
+        "sanity check: placeContent.getResult() should not return null");
+
+  // Check sortingMode.
+  is(res.sortingMode, aSortingMode,
+     "column should now have sortingMode " + aSortingMode);
+
+  // Check sortingAnnotation, but only if sortingMode is ANNOTATION.
+  if ([Ci.nsINavHistoryQueryOptions.SORT_BY_ANNOTATION_ASCENDING,
+       Ci.nsINavHistoryQueryOptions.SORT_BY_ANNOTATION_DESCENDING].
+      indexOf(aSortingMode) >= 0) {
+    is(res.sortingAnnotation, aSortingAnno,
+       "column should now have sorting annotation " + aSortingAnno);
+  }
+}
+
+/**
+ * Sets the sort of aTree.
+ *
+ * @param aOrganizerWin
+ *        the Places window
+ * @param aTree
+ *        the tree to sort
+ * @param aUnsortFirst
+ *        true if the sort should be set to SORT_BY_NONE before sorting by aCol
+ *        and aDir
+ * @param aShouldFail
+ *        true if setSortColumn should fail on aCol or aDir
+ * @param aCol
+ *        the column of aTree by which to sort
+ * @param aDir
+ *        either "ascending" or "descending"
+ */
+function setSort(aOrganizerWin, aTree, aUnsortFirst, aShouldFail, aCol, aDir) {
+  if (aUnsortFirst) {
+    aOrganizerWin.ViewMenu.setSortColumn();
+    checkSort(aTree, Ci.nsINavHistoryQueryOptions.SORT_BY_NONE, "");
+
+    // Remember the sort key and direction.
+    prevSortKey = null;
+    prevSortDir = null;
+  }
+
+  let failed = false;
+  try {
+    aOrganizerWin.ViewMenu.setSortColumn(aCol, aDir);
+
+    // Remember the sort key and direction.
+    if (!aCol && !aDir) {
+      prevSortKey = null;
+      prevSortDir = null;
+    }
+    else {
+      if (aCol)
+        prevSortKey = SORT_LOOKUP_TABLE[aCol.getAttribute("anonid")].key;
+      else if (prevSortKey === null)
+        prevSortKey = DEFAULT_SORT_KEY;
+
+      if (aDir)
+        prevSortDir = aDir.toUpperCase();
+      else if (prevSortDir === null)
+        prevSortDir = SORT_LOOKUP_TABLE[aCol.getAttribute("anonid")].dir;
+    }
+  } catch (exc) {
+    failed = true;
+  }
+
+  is(failed, !!aShouldFail,
+     "setSortColumn on column " +
+     (aCol ? aCol.getAttribute("anonid") : "(no column)") +
+     " with direction " + (aDir || "(no direction)") +
+     " and table previously " + (aUnsortFirst ? "unsorted" : "sorted") +
+     " should " + (aShouldFail ? "" : "not ") + "fail");
+}
+
+/**
+ * Tries sorting by an invalid column and sort direction.
+ *
+ * @param aOrganizerWin
+ *        the Places window
+ * @param aPlaceContentTree
+ *        the placeContent tree in aOrganizerWin
+ */
+function testInvalid(aOrganizerWin, aPlaceContentTree) {
+  // Invalid column should fail by throwing an exception.
+  let bogusCol = document.createElement("treecol");
+  bogusCol.setAttribute("anonid", "bogusColumn");
+  setSort(aOrganizerWin, aPlaceContentTree, true, true, bogusCol, "ascending");
+
+  // Invalid direction reverts to SORT_BY_NONE.
+  setSort(aOrganizerWin, aPlaceContentTree, false, false, null, "bogus dir");
+  checkSort(aPlaceContentTree, Ci.nsINavHistoryQueryOptions.SORT_BY_NONE, "");
+}
+
+/**
+ * Tests sorting aPlaceContentTree by column only and then by both column
+ * and direction.
+ *
+ * @param aOrganizerWin
+ *        the Places window
+ * @param aPlaceContentTree
+ *        the placeContent tree in aOrganizerWin
+ * @param aUnsortFirst
+ *        true if, before each sort we try, we should sort to SORT_BY_NONE
+ */
+function testSortByColAndDir(aOrganizerWin, aPlaceContentTree, aUnsortFirst) {
+  let cols = aPlaceContentTree.getElementsByTagName("treecol");
+  ok(cols.length > 0, "sanity check: placeContent should contain columns");
+
+  for (let i = 0; i < cols.length; i++) {
+    let col = cols.item(i);
+    ok(col.hasAttribute("anonid"),
+       "sanity check: column " + col.id + " should have anonid");
+
+    let colId = col.getAttribute("anonid");
+    ok(colId in SORT_LOOKUP_TABLE,
+       "sanity check: unexpected placeContent column anonid");
+
+    let sortConst =
+      "SORT_BY_" + SORT_LOOKUP_TABLE[colId].key + "_" +
+      (aUnsortFirst ? SORT_LOOKUP_TABLE[colId].dir : prevSortDir);
+    let expectedSortMode = Ci.nsINavHistoryQueryOptions[sortConst];
+    let expectedAnno = SORT_LOOKUP_TABLE[colId].anno || "";
+
+    // Test sorting by only a column.
+    setSort(aOrganizerWin, aPlaceContentTree, aUnsortFirst, false, col);
+    checkSort(aPlaceContentTree, expectedSortMode, expectedAnno);
+
+    // Test sorting by both a column and a direction.
+    ["ascending", "descending"].forEach(function (dir) {
+      let sortConst =
+        "SORT_BY_" + SORT_LOOKUP_TABLE[colId].key + "_" + dir.toUpperCase();
+      let expectedSortMode = Ci.nsINavHistoryQueryOptions[sortConst];
+      setSort(aOrganizerWin, aPlaceContentTree, aUnsortFirst, false, col, dir);
+      checkSort(aPlaceContentTree, expectedSortMode, expectedAnno);
+    });
+  }
+}
+
+/**
+ * Tests sorting aPlaceContentTree by direction only.
+ *
+ * @param aOrganizerWin
+ *        the Places window
+ * @param aPlaceContentTree
+ *        the placeContent tree in aOrganizerWin
+ * @param aUnsortFirst
+ *        true if, before each sort we try, we should sort to SORT_BY_NONE
+ */
+function testSortByDir(aOrganizerWin, aPlaceContentTree, aUnsortFirst) {
+  ["ascending", "descending"].forEach(function (dir) {
+    let key = (aUnsortFirst ? DEFAULT_SORT_KEY : prevSortKey);
+    let sortConst = "SORT_BY_" + key + "_" + dir.toUpperCase();
+    let expectedSortMode = Ci.nsINavHistoryQueryOptions[sortConst];
+    setSort(aOrganizerWin, aPlaceContentTree, aUnsortFirst, false, null, dir);
+    checkSort(aPlaceContentTree, expectedSortMode, "");
+  });
+}
+
+///////////////////////////////////////////////////////////////////////////////
+
+function test() {
+  waitForExplicitFinish();
+
+  // Open the Places Library window.
+  let win = window.openDialog("chrome://browser/content/places/places.xul",
+                              "",
+                              "chrome,toolbar=yes,dialog=no,resizable");
+
+  // Wait for it to load before we start.
+  win.addEventListener("load", function onload() {
+    win.removeEventListener("load", onload, false);
+    executeSoon(function () {
+      let tree = win.document.getElementById("placeContent");
+      isnot(tree, null, "sanity check: placeContent tree should exist");
+      // Run the tests.
+      testSortByColAndDir(win, tree, true);
+      testSortByColAndDir(win, tree, false);
+      testSortByDir(win, tree, true);
+      testSortByDir(win, tree, false);
+      testInvalid(win, tree);
+      // Reset the sort to SORT_BY_NONE.
+      setSort(win, tree, false, false);
+      // Close the window and finish.
+      win.close();
+      finish();
+    });
+  }, false);
+}

toolkit/components/places/src/nsNavHistoryResult.cpp

@@ -1126,19 +1126,19 @@ PRInt32 nsNavHistoryContainerResultNode::SortComparison_AnnotationLess(
         PRInt32 a_val = 0, b_val = 0;
         GET_ANNOTATIONS_VALUES(GetItemAnnotationInt32,
                                GetPageAnnotationInt32, &a_val, &b_val);
-        value = (a < b) ? -1 : (a > b) ? 1 : 0;
+        value = (a_val < b_val) ? -1 : (a_val > b_val) ? 1 : 0;
       }
       else if (annoType == nsIAnnotationService::TYPE_INT64) {
         PRInt64 a_val = 0, b_val = 0;
         GET_ANNOTATIONS_VALUES(GetItemAnnotationInt64,
                                GetPageAnnotationInt64, &a_val, &b_val);
-        value = (a < b) ? -1 : (a > b) ? 1 : 0;
+        value = (a_val < b_val) ? -1 : (a_val > b_val) ? 1 : 0;
       }
       else if (annoType == nsIAnnotationService::TYPE_DOUBLE) {
         double a_val = 0, b_val = 0;
         GET_ANNOTATIONS_VALUES(GetItemAnnotationDouble,
                                GetPageAnnotationDouble, &a_val, &b_val);
-        value = (a < b) ? -1 : (a > b) ? 1 : 0;
+        value = (a_val < b_val) ? -1 : (a_val > b_val) ? 1 : 0;
       }
     }
   }

toolkit/components/places/tests/queries/head_queries.js

@@ -311,9 +311,12 @@ function queryData(obj) {
   this.isLivemark = obj.isLivemark ? obj.isLivemark : false;
   this.parentFolder = obj.parentFolder ? obj.parentFolder : bmsvc.placesRoot;
   this.feedURI = obj.feedURI ? obj.feedURI : "";
-  this.bmIndex = obj.bmIndex ? obj.bmIndex : bmsvc.DEFAULT_INDEX;
+  this.index = obj.index ? obj.index : bmsvc.DEFAULT_INDEX;
   this.isFolder = obj.isFolder ? obj.isFolder : false;
   this.contractId = obj.contractId ? obj.contractId : "";
+  this.lastModified = obj.lastModified ? obj.lastModified : today;
+  this.dateAdded = obj.dateAdded ? obj.dateAdded : today;
+  this.keyword = obj.keyword ? obj.keyword : "";
 
   // And now, the attribute for whether or not this object should appear in the
   // resulting query
@@ -324,15 +327,16 @@ function queryData(obj) {
 queryData.prototype = { }
 
 /**
- * Helper function to compare an array of query objects with a result set
- * NOTE: It assumes the array of query objects contains the SAME SORT as the
- *       result set and only checks the URI and title of the results.
- *       For deeper checks, you'll need to write your own method.
+ * Helper function to compare an array of query objects with a result set.
+ * It assumes the array of query objects contains the SAME SORT as the result
+ * set.  It checks the the uri, title, time, and bookmarkIndex properties of
+ * the results, where appropriate.
  */
 function compareArrayToResult(aArray, aRoot) {
   LOG("Comparing Array to Results");
 
-  if (!aRoot.containerOpen)
+  var wasOpen = aRoot.containerOpen;
+  if (!wasOpen)
     aRoot.containerOpen = true;
 
   // check expected number of results against actual
@@ -340,16 +344,26 @@ function compareArrayToResult(aArray, aRoot) {
   do_check_eq(expectedResultCount, aRoot.childCount);
 
   var inQueryIndex = 0;
-  for (var i=0; i < aArray.length; i++) {
+  for (var i = 0; i < aArray.length; i++) {
     if (aArray[i].isInQuery) {
       var child = aRoot.getChild(inQueryIndex);
       LOG("testing testData[" + i + "] vs result[" + inQueryIndex + "]");
       LOG("testing testData[" + aArray[i].uri + "] vs result[" + child.uri + "]");
-      //do_check_eq(aArray[i].uri, child.uri);
-      //do_check_eq(aArray[i].title, child.title);
+      if (!aArray[i].isFolder)
+        do_check_eq(aArray[i].uri, child.uri);
+      do_check_eq(aArray[i].title, child.title);
+      if (aArray[i].hasOwnProperty("lastVisit"))
+        do_check_eq(aArray[i].lastVisit, child.time);
+      if (aArray[i].hasOwnProperty("index") &&
+          aArray[i].index != bmsvc.DEFAULT_INDEX)
+        do_check_eq(aArray[i].index, child.bookmarkIndex);
+
       inQueryIndex++;
     }
   }
+
+  if (!wasOpen)
+    aRoot.containerOpen = false;
   LOG("Comparing Array to Results passes");
 }
 

toolkit/components/places/tests/queries/test_onlyBookmarked.js

@@ -135,6 +135,9 @@ function run_test() {
   LOG("begin live-update test");
   compareArrayToResult(testData, root);
   LOG("end live-update test");
+/*
+  // we are actually not updating during a batch.
+  // see bug 432706 for details.
 
   // Here's a batch update
   var updateBatch = {
@@ -153,7 +156,7 @@ function run_test() {
   LOG("begin batched test");
   compareArrayToResult(testData, root);
   LOG("end batched test");
-
+*/
   // Close the container when finished
   root.containerOpen = false;
 }