Last Comment Bug 275223 - Page Info columns should be sortable
: Page Info columns should be sortable
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: Page Info Window (show other bugs)
: Trunk
: All All
: -- enhancement with 16 votes (vote)
: Firefox 12
Assigned To: Felix Fung (:felix)
:
Mentors:
: 317675 416556 416573 426875 569768 614590 619616 (view as bug list)
Depends on: 482890 727910
Blocks: cuts-control 736572 736574
  Show dependency treegraph
 
Reported: 2004-12-18 16:30 PST by hmikron
Modified: 2012-08-31 12:55 PDT (History)
29 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
added non-persistent sorting to pageinfo, took tree utils from preferences (18.75 KB, patch)
2010-11-08 11:51 PST, Felix Fung (:felix)
db48x: review-
Details | Diff | Splinter Review
Give all tree views in page info sortable columns (22.47 KB, patch)
2011-09-07 00:52 PDT, Felix Fung (:felix)
db48x: review-
Details | Diff | Splinter Review
Give all tree views in page info sortable columns (27.06 KB, patch)
2012-01-25 01:06 PST, Felix Fung (:felix)
db48x: review+
Details | Diff | Splinter Review

Description hmikron 2004-12-18 16:30:49 PST
While in Pag Info, in the "Media" tab.
Pushing the "Address" or "Type" headlines does NOT sort by address/type.
Comment 1 flock31070 2005-11-14 12:48:36 PST
I thought this was a bug...Didn't it used to work in past versions?
Comment 2 Erik Fabert 2005-11-24 05:16:09 PST
*** Bug 317675 has been marked as a duplicate of this bug. ***
Comment 3 Asher Levy 2006-01-07 07:14:35 PST
I don't recall this ever working.
Also, the location of the horizontal sperator below the media list
should be saved together with the window geometry.
Comment 4 Henrik Skupin (:whimboo) 2006-11-19 06:24:18 PST
The same issue for SeaMonkey is covered on bug 76170.
Comment 5 Andrés Delfino 2008-02-09 09:39:53 PST
*** Bug 416556 has been marked as a duplicate of this bug. ***
Comment 6 Florian Quèze [:florian] [:flo] (PTO until August 29th) 2008-02-09 15:35:42 PST
*** Bug 416573 has been marked as a duplicate of this bug. ***
Comment 7 Henrik Skupin (:whimboo) 2009-07-20 09:05:16 PDT
*** Bug 426875 has been marked as a duplicate of this bug. ***
Comment 8 Philip Chee 2010-06-02 18:22:44 PDT
*** Bug 569768 has been marked as a duplicate of this bug. ***
Comment 9 Philip Chee 2010-06-02 18:42:27 PDT
> Tanner M. Young [:tmyoung] <mozilla.bugs@alyoung.com> changed:
>
>           What    |Removed                     |Added
> ----------------------------------------------------------------------------
>           See Also|                            |https://bugzilla.mozilla.or
>                   |                            |g/show_bug.cgi?id=76170

The last time I put an internal b.m.o bug url in See Also it was moved to Blocks. I disagreed but was overruled.
Comment 10 Ciprian 2010-07-13 23:31:30 PDT
This simple thing, sorting by columns is very important. Trying to sort the media by type or by size in order to find a certain image for example is not possible now so you have to scroll over and over the long list ... It's time consuming, painful on the eyes and irritating. Please fix it soon. Thanks.
Comment 11 Felix Fung (:felix) 2010-11-08 11:51:02 PST
Created attachment 488927 [details] [diff] [review]
added non-persistent sorting to pageinfo, took tree utils from preferences
Comment 12 Philip Chee 2010-11-08 20:28:40 PST
> -    var sizeText;
> +    var kbSize;
>      if (cacheEntryDescriptor) {
>        var pageSize = cacheEntryDescriptor.dataSize;
> -      var kbSize = formatNumber(Math.round(pageSize / 1024 * 100) / 100);
> -      sizeText = gBundle.getFormattedString("mediaFileSize", [kbSize]);
> +      kbSize = Number(Math.round(pageSize / 1024 * 100) / 100);
>      }
>      else
> -      sizeText = gStrings.unknown;
> -    gImageView.addRow([url, type, sizeText, alt, 1, elem, isBg]);
> +      kbSize = -1
> +
> +    gImageView.addRow([url, type, kbSize, alt, 1, elem, isBg]);

Part of some other bug?
Comment 13 Felix Fung (:felix) 2010-11-08 21:00:48 PST
Summary of Changes

- Moved permissionutils.js which contained a generic tree sort/delete functions to treeUtils.js in global
- Changed treeSort function to do a check to see whether we want a lexicographical or numeric sort
- gImageView was storing the size of the files as text as in "1.23 kB", so that wouldn't sort correctly. Changed it to store as floats and changed getCellValue to produce the correct output
Comment 14 Philip Chee 2010-11-24 20:30:58 PST
*** Bug 614590 has been marked as a duplicate of this bug. ***
Comment 15 Philip Chee 2010-11-24 20:36:39 PST
Comment on attachment 488927 [details] [diff] [review]
added non-persistent sorting to pageinfo, took tree utils from preferences

>diff --git a/browser/base/content/pageinfo/pageInfo.js b/browser/base/content/pageinfo/pageInfo.js
>--- a/browser/base/content/pageinfo/pageInfo.js
>+++ b/browser/base/content/pageinfo/pageInfo.js
>@@ -65,17 +65,25 @@ pageInfoTreeView.prototype = {
>   },
> 
>   getCellText: function(row, column)
>   {
>     // row can be null, but js arrays are 0-indexed.
>     // colidx cannot be null, but can be larger than the number
>     // of columns in the array. In this case it's the fault of
>     // whoever typoed while calling this function.
>-    return this.data[row][column.index] || "";
>+    var text = this.data[row][column.index];
>+    if (column.index == COL_IMAGE_SIZE) {
>+      if (text == -1) {
>+        return gStrings.unknown;
>+      } else {
>+        return gBundle.getFormattedString("mediaFileSize", [text]);
>+      }
>+    }
>+    return text || "";
>   },
> 
>   setCellValue: function(row, column, value)
>   {
>   },
> 
>   setCellText: function(row, column, value)
>   {
>@@ -580,25 +588,25 @@ function addImage(url, type, alt, elem, 
>     catch(ex) {
>       try {
>         // open for READ, in non-blocking mode
>         cacheEntryDescriptor = ftpCacheSession.openCacheEntry(url, ACCESS_READ, false);
>       }
>       catch(ex2) { }
>     }
> 
>-    var sizeText;
>+    var kbSize;
>     if (cacheEntryDescriptor) {
>       var pageSize = cacheEntryDescriptor.dataSize;
>-      var kbSize = formatNumber(Math.round(pageSize / 1024 * 100) / 100);
>-      sizeText = gBundle.getFormattedString("mediaFileSize", [kbSize]);
>+      kbSize = Number(Math.round(pageSize / 1024 * 100) / 100);
>     }
>     else
>-      sizeText = gStrings.unknown;
>-    gImageView.addRow([url, type, sizeText, alt, 1, elem, isBg]);
>+      kbSize = -1
>+
>+    gImageView.addRow([url, type, kbSize, alt, 1, elem, isBg]);
> 
>     // Add the observer, only once.
>     if (gImageView.data.length == 1) {
>       document.getElementById("mediaTab").hidden = false;
>       Components.classes["@mozilla.org/observer-service;1"]
>                 .getService(Components.interfaces.nsIObserverService)
>                 .addObserver(imagePermissionObserver, "perm-changed", false);
>     }
>@@ -1236,8 +1244,31 @@ function selectImage()
> function checkProtocol(img)
> {
>   var url = img[COL_IMAGE_ADDRESS];
>   if (/^data:/.test(url) && /^image\//.test(img[COL_IMAGE_NODE].type))
>     return true;
>   const regex = /^(https?|ftp|file|about|chrome|resource):/;
>   return regex.test(url);
> }
>+
>+var gPageMediaInfo = {
>+  _lastSortColumn : -1,
>+  _lastSortAscending : false,
>+
>+  onPageMediaSort : function (aColumn)
>+  {
>+    var tree = document.getElementById("imagetree");
>+    var column = tree.columns.getNamedColumn(aColumn);
>+
>+    this._lastSortAscending = 
>+      gTreeUtils.sort(
>+        tree,
>+        gImageView,
>+        gImageView.data,
>+        column.index,
>+        this._lastSortColumn,
>+        this._lastSortAscending
>+      );
>+
>+    this._lastSortColumn = column.index;
>+  }
>+};
>diff --git a/browser/base/content/pageinfo/pageInfo.xul b/browser/base/content/pageinfo/pageInfo.xul
>--- a/browser/base/content/pageinfo/pageInfo.xul
>+++ b/browser/base/content/pageinfo/pageInfo.xul
>@@ -61,16 +61,17 @@
>   onunload="onUnloadPageInfo()"
>   align="stretch"
>   screenX="10" screenY="10"
>   width="&pageInfoWindow.width;" height="&pageInfoWindow.height;"
>   persist="screenX screenY width height sizemode">
> 
>   <script type="application/javascript" src="chrome://global/content/globalOverlay.js"/>
>   <script type="application/javascript" src="chrome://global/content/contentAreaUtils.js"/>
>+  <script type="application/javascript" src="chrome://global/content/treeUtils.js"/>
>   <script type="application/javascript" src="chrome://browser/content/pageinfo/pageInfo.js"/>
>   <script type="application/javascript" src="chrome://browser/content/pageinfo/feeds.js"/>
>   <script type="application/javascript" src="chrome://browser/content/pageinfo/permissions.js"/>
>   <script type="application/javascript" src="chrome://browser/content/pageinfo/security.js"/>
>   <script type="application/javascript" src="chrome://browser/content/utilityOverlay.js"/>
> 
>   <stringbundleset id="pageinfobundleset">
>     <stringbundle id="pageinfobundle" src="chrome://browser/locale/pageInfo.properties"/>
>@@ -213,29 +214,34 @@
>     </vbox>
> 
>     <!-- Media information -->
>     <vbox id="mediaPanel">
>       <tree id="imagetree" onselect="onImageSelect();" contextmenu="picontext"
>             ondragstart="onBeginLinkDrag(event,'image-address','image-alt')">
>         <treecols>
>           <treecol sortSeparators="true" primary="true" persist="width" flex="10"
>-                        width="10" id="image-address" label="&mediaAddress;"/>
>+                        width="10" id="image-address" label="&mediaAddress;"
>+                        onclick="gPageMediaInfo.onPageMediaSort('image-address');"/>
>           <splitter class="tree-splitter"/>
>           <treecol sortSeparators="true" persist="hidden width" flex="2"
>-                        width="2"  id="image-type"    label="&mediaType;"/>
>+                        width="2"  id="image-type"    label="&mediaType;"
>+                        onclick="gPageMediaInfo.onPageMediaSort('image-type');"/>
>           <splitter class="tree-splitter"/>
>           <treecol sortSeparators="true" hidden="true" persist="hidden width" flex="2"
>-                        width="2"  id="image-size"    label="&mediaSize;"/>
>+                        width="2"  id="image-size"  label="&mediaSize;" value="size"
>+                        onclick="gPageMediaInfo.onPageMediaSort('image-size');"/>
>           <splitter class="tree-splitter"/>
>           <treecol sortSeparators="true" hidden="true" persist="hidden width" flex="4"
>-                        width="4"  id="image-alt"    label="&mediaAltHeader;"/>
>+                        width="4"  id="image-alt"    label="&mediaAltHeader;"
>+                        onclick="gPageMediaInfo.onPageMediaSort('image-alt');"/>
>           <splitter class="tree-splitter"/>
>           <treecol sortSeparators="true" hidden="true" persist="hidden width" flex="1"
>-                        width="1"  id="image-count"    label="&mediaCount;"/>
>+                        width="1"  id="image-count"    label="&mediaCount;"
>+                        onclick="gPageMediaInfo.onPageMediaSort('image-count');"/>
>         </treecols>
>         <treechildren flex="1"/>
>       </tree>
>       <splitter orient="vertical" id="mediaSplitter"/>
>       <vbox flex="1" id="mediaPreviewBox" collapsed="true">
>         <grid id="mediaGrid">
>           <columns>
>             <column id="mediaLabelColumn"/>
>diff --git a/browser/components/preferences/cookies.xul b/browser/components/preferences/cookies.xul
>--- a/browser/components/preferences/cookies.xul
>+++ b/browser/components/preferences/cookies.xul
>@@ -46,17 +46,17 @@
> <window id="CookiesDialog" windowtype="Browser:Cookies"
>         class="windowDialog" title="&window.title;"
>         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>         style="width: &window.width;;"
>         onload="gCookiesWindow.init();"
>         onunload="gCookiesWindow.uninit();"
>         persist="screenX screenY width height">
> 
>-  <script src="chrome://browser/content/preferences/permissionsutils.js"/>
>+  <script src="chrome://global/content/treeUtils.js"/>
>   <script src="chrome://browser/content/preferences/cookies.js"/>
> 
>   <stringbundle id="bundlePreferences"
>                 src="chrome://browser/locale/preferences/preferences.properties"/>
> 
>   <keyset>
>     <key key="&windowClose.key;" modifiers="accel" oncommand="window.close();"/>
>     <key key="&focusSearch1.key;" modifiers="accel" oncommand="gCookiesWindow.focusFilterBox();"/>
>diff --git a/browser/components/preferences/jar.mn b/browser/components/preferences/jar.mn
>--- a/browser/components/preferences/jar.mn
>+++ b/browser/components/preferences/jar.mn
>@@ -18,17 +18,16 @@ browser.jar:
> *   content/browser/preferences/handlers.xml
> *   content/browser/preferences/handlers.css
> *   content/browser/preferences/languages.xul
> *   content/browser/preferences/languages.js
> *   content/browser/preferences/main.xul
> *   content/browser/preferences/main.js
> *   content/browser/preferences/permissions.xul
> *   content/browser/preferences/permissions.js
>-*   content/browser/preferences/permissionsutils.js
> *   content/browser/preferences/preferences.xul
> *   content/browser/preferences/privacy.xul
> *   content/browser/preferences/privacy.js
> *   content/browser/preferences/sanitize.xul
> *   content/browser/preferences/security.xul
> *   content/browser/preferences/security.js
> *   content/browser/preferences/selectBookmark.xul
> *   content/browser/preferences/selectBookmark.js
>diff --git a/browser/components/preferences/permissions.xul b/browser/components/preferences/permissions.xul
>--- a/browser/components/preferences/permissions.xul
>+++ b/browser/components/preferences/permissions.xul
>@@ -48,17 +48,17 @@
>         windowtype="Browser:Permissions"
>         title="&window.title;"
>         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>         style="width: &window.width;;"
>         onload="gPermissionManager.onLoad();"
>         onunload="gPermissionManager.uninit();"
>         persist="screenX screenY width height">
> 
>-  <script src="chrome://browser/content/preferences/permissionsutils.js"/>
>+  <script src="chrome://global/content/treeUtils.js"/>
>   <script src="chrome://browser/content/preferences/permissions.js"/>
> 
>   <stringbundle id="bundlePreferences"
>                 src="chrome://browser/locale/preferences/preferences.properties"/>
> 
>   <keyset>
>     <key key="&windowClose.key;" modifiers="accel" oncommand="window.close();"/>
>   </keyset>
>diff --git a/browser/components/preferences/permissionsutils.js b/browser/components/preferences/permissionsutils.js
>deleted file mode 100644
>--- a/browser/components/preferences/permissionsutils.js
>+++ /dev/null
>@@ -1,102 +0,0 @@
>-# -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
>-# ***** 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 the Firefox Preferences System.
>-#
>-# The Initial Developer of the Original Code is
>-# Ben Goodger.
>-# Portions created by the Initial Developer are Copyright (C) 2005
>-# the Initial Developer. All Rights Reserved.
>-#
>-# Contributor(s):
>-#   Ben Goodger <ben@mozilla.org>
>-#
>-# 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 *****
>-
>-var gTreeUtils = {
>-  deleteAll: function (aTree, aView, aItems, aDeletedItems)
>-  {
>-    for (var i = 0; i < aItems.length; ++i)
>-      aDeletedItems.push(aItems[i]);
>-    aItems.splice(0);
>-    var oldCount = aView.rowCount;
>-    aView._rowCount = 0;
>-    aTree.treeBoxObject.rowCountChanged(0, -oldCount);
>-  },
>-  
>-  deleteSelectedItems: function (aTree, aView, aItems, aDeletedItems)
>-  {
>-    var selection = aTree.view.selection;
>-    selection.selectEventsSuppressed = true;
>-    
>-    var rc = selection.getRangeCount();
>-    for (var i = 0; i < rc; ++i) {
>-      var min = { }; var max = { };
>-      selection.getRangeAt(i, min, max);
>-      for (var j = min.value; j <= max.value; ++j) {
>-        aDeletedItems.push(aItems[j]);
>-        aItems[j] = null;
>-      }
>-    }
>-    
>-    var nextSelection = 0;
>-    for (i = 0; i < aItems.length; ++i) {
>-      if (!aItems[i]) {
>-        var j = i;
>-        while (j < aItems.length && !aItems[j])
>-          ++j;
>-        aItems.splice(i, j - i);
>-        nextSelection = j < aView.rowCount ? j - 1 : j - 2;
>-        aView._rowCount -= j - i;
>-        aTree.treeBoxObject.rowCountChanged(i, i - j);
>-      }
>-    }
>-
>-    if (aItems.length) {
>-      selection.select(nextSelection);
>-      aTree.treeBoxObject.ensureRowIsVisible(nextSelection);
>-      aTree.focus();
>-    }
>-    selection.selectEventsSuppressed = false;
>-  },
>-  
>-  sort: function (aTree, aView, aDataSet, aColumn, 
>-                  aLastSortColumn, aLastSortAscending) 
>-  {
>-    var ascending = (aColumn == aLastSortColumn) ? !aLastSortAscending : true;
>-    aDataSet.sort(function (a, b) { return a[aColumn].toLowerCase().localeCompare(b[aColumn].toLowerCase()); });
>-    if (!ascending)
>-      aDataSet.reverse();
>-    
>-    aTree.view.selection.select(-1);
>-    aTree.view.selection.select(0);
>-    aTree.treeBoxObject.invalidate();
>-    aTree.treeBoxObject.ensureRowIsVisible(0);
>-    
>-    return ascending;
>-  }
>-};
>-
>diff --git a/toolkit/content/jar.mn b/toolkit/content/jar.mn
>--- a/toolkit/content/jar.mn
>+++ b/toolkit/content/jar.mn
>@@ -32,16 +32,17 @@ toolkit.jar:
> *+ content/global/editMenuOverlay.xul         (editMenuOverlay.xul)
> *+ content/global/finddialog.js               (finddialog.js)
> *+ content/global/finddialog.xul              (finddialog.xul)
> *+ content/global/findUtils.js                (findUtils.js)
>    content/global/filepicker.properties       (filepicker.properties)
> *+ content/global/globalOverlay.js            (globalOverlay.js)
> +  content/global/mozilla.xhtml               (mozilla.xhtml)
> *+ content/global/nsDragAndDrop.js            (nsDragAndDrop.js)
>+*  content/global/treeUtils.js                (treeUtils.js)
> *+ content/global/viewZoomOverlay.js          (viewZoomOverlay.js)
> *+ content/global/bindings/autocomplete.xml    (widgets/autocomplete.xml)
> *+ content/global/bindings/browser.xml         (widgets/browser.xml)
> *+ content/global/bindings/button.xml          (widgets/button.xml)
> *+ content/global/bindings/checkbox.xml        (widgets/checkbox.xml)
> *+ content/global/bindings/colorpicker.xml     (widgets/colorpicker.xml)
> *+ content/global/bindings/datetimepicker.xml  (widgets/datetimepicker.xml)
> *+ content/global/bindings/dialog.xml          (widgets/dialog.xml)
>diff --git a/toolkit/content/treeUtils.js b/toolkit/content/treeUtils.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/content/treeUtils.js
>@@ -0,0 +1,112 @@
>+# -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
>+# ***** 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 the Firefox Preferences System.
>+#
>+# The Initial Developer of the Original Code is
>+# Ben Goodger.
>+# Portions created by the Initial Developer are Copyright (C) 2005
>+# the Initial Developer. All Rights Reserved.
>+#
>+# Contributor(s):
>+#   Ben Goodger <ben@mozilla.org>
>+#
>+# 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 *****
>+
>+var gTreeUtils = {
>+  deleteAll: function (aTree, aView, aItems, aDeletedItems)
>+  {
>+    for (var i = 0; i < aItems.length; ++i)
>+      aDeletedItems.push(aItems[i]);
>+    aItems.splice(0);
>+    var oldCount = aView.rowCount;
>+    aView._rowCount = 0;
>+    aTree.treeBoxObject.rowCountChanged(0, -oldCount);
>+  },
>+  
>+  deleteSelectedItems: function (aTree, aView, aItems, aDeletedItems)
>+  {
>+    var selection = aTree.view.selection;
>+    selection.selectEventsSuppressed = true;
>+    
>+    var rc = selection.getRangeCount();
>+    for (var i = 0; i < rc; ++i) {
>+      var min = { }; var max = { };
>+      selection.getRangeAt(i, min, max);
>+      for (var j = min.value; j <= max.value; ++j) {
>+        aDeletedItems.push(aItems[j]);
>+        aItems[j] = null;
>+      }
>+    }
>+    
>+    var nextSelection = 0;
>+    for (i = 0; i < aItems.length; ++i) {
>+      if (!aItems[i]) {
>+        var j = i;
>+        while (j < aItems.length && !aItems[j])
>+          ++j;
>+        aItems.splice(i, j - i);
>+        nextSelection = j < aView.rowCount ? j - 1 : j - 2;
>+        aView._rowCount -= j - i;
>+        aTree.treeBoxObject.rowCountChanged(i, i - j);
>+      }
>+    }
>+
>+    if (aItems.length) {
>+      selection.select(nextSelection);
>+      aTree.treeBoxObject.ensureRowIsVisible(nextSelection);
>+      aTree.focus();
>+    }
>+    selection.selectEventsSuppressed = false;
>+  },
>+  
>+  sort: function (aTree, aView, aDataSet, aColumn, 
>+                  aLastSortColumn, aLastSortAscending) 
>+  {
>+    var ascending = (aColumn == aLastSortColumn) ? !aLastSortAscending : true;
>+    if (aDataSet.length == 0)
>+      return ascending;
>+
>+    var numericSort = !isNaN(aDataSet[0][aColumn]);
>+    var sortFunction;
>+    if (numericSort) {
>+      sortFunction = function (a, b) { return a[aColumn] - b[aColumn]; }
>+    } else {
>+      sortFunction = function (a, b) { return a[aColumn].toLowerCase().localeCompare(b[aColumn].toLowerCase()); }
>+    }
>+    aDataSet.sort(sortFunction);
>+
>+    if (!ascending)
>+      aDataSet.reverse();
>+    
>+    aTree.view.selection.select(0);
>+    aTree.treeBoxObject.invalidate();
>+    aTree.treeBoxObject.ensureRowIsVisible(0);
>+    
>+    return ascending;
>+  }
>+};
>+
Comment 16 Daniel Brooks [:db48x] 2010-11-24 20:51:51 PST
Comment on attachment 488927 [details] [diff] [review]
added non-persistent sorting to pageinfo, took tree utils from preferences

First, all of the trees in the Page Info dialog box should be sortable, not just the one on the media tab.

>diff --git a/browser/base/content/pageinfo/pageInfo.js b/browser/base/content/pageinfo/pageInfo.js
>--- a/browser/base/content/pageinfo/pageInfo.js
>+++ b/browser/base/content/pageinfo/pageInfo.js
>@@ -65,17 +65,25 @@ pageInfoTreeView.prototype = {
>   },
> 
>   getCellText: function(row, column)
>   {
>     // row can be null, but js arrays are 0-indexed.
>     // colidx cannot be null, but can be larger than the number
>     // of columns in the array. In this case it's the fault of
>     // whoever typoed while calling this function.
>-    return this.data[row][column.index] || "";
>+    var text = this.data[row][column.index];
>+    if (column.index == COL_IMAGE_SIZE) {
>+      if (text == -1) {
>+        return gStrings.unknown;
>+      } else {
>+        return gBundle.getFormattedString("mediaFileSize", [text]);
>+      }
>+    }
>+    return text || "";
>   },

I don't really like this. For one thing, this will apply to all the trees in the window if they have enough columns. Create a subclass that overrides this method and use it just for the media tree.

>+var gPageMediaInfo = {
>+  _lastSortColumn : -1,
>+  _lastSortAscending : false,
>+
>+  onPageMediaSort : function (aColumn)
>+  {
>+    var tree = document.getElementById("imagetree");
>+    var column = tree.columns.getNamedColumn(aColumn);
>+
>+    this._lastSortAscending = 
>+      gTreeUtils.sort(
>+        tree,
>+        gImageView,
>+        gImageView.data,
>+        column.index,
>+        this._lastSortColumn,
>+        this._lastSortAscending
>+      );
>+
>+    this._lastSortColumn = column.index;
>+  }
>+};

Store this information on the individual treeview instances, so that we don't have a lot of different global objects floating around.

This is looking pretty good though; thanks for helping :)
Comment 17 Henrik Skupin (:whimboo) 2010-12-16 07:22:19 PST
*** Bug 619616 has been marked as a duplicate of this bug. ***
Comment 18 Felix Fung (:felix) 2011-09-07 00:52:39 PDT
Created attachment 558744 [details] [diff] [review]
Give all tree views in page info sortable columns

New Changes As Per db48x's Requests:
- getCellText is subclassed by gImageView rather than modifying all pageInfoTreeViews
- moved sorting state and onPageMediaSort into pageInfoTreeViews rather than craete new global variables



Summary of Changes

- Moved permissionutils.js which contained a generic tree sort/delete functions to treeUtils.js in global
- Changed treeSort function to do a check to see whether we want a lexicographical or numeric sort
- gImageView was storing the size of the files as text as in "1.23 kB", so that wouldn't sort correctly. Changed it to store as floats and changed getCellValue to produce the correct output
Comment 19 Daniel Brooks [:db48x] 2012-01-22 23:53:55 PST
Comment on attachment 558744 [details] [diff] [review]
Give all tree views in page info sortable columns

Review of attachment 558744 [details] [diff] [review]:
-----------------------------------------------------------------

I have been remiss! Felix, this looks like a great patch. I would like you to make one small change, however.

::: browser/base/content/pageinfo/pageInfo.js
@@ +205,5 @@
>      props.AppendElement(this._ltrAtom);
>  };
>  
> +gImageView.getCellText = function(row, column) {
> +  var text = this.data[row][column.index];

Might as well call this variable size instead of text.

@@ +619,3 @@
>      if (cacheEntryDescriptor) {
>        var pageSize = cacheEntryDescriptor.dataSize;
> +      kbSize = Number(Math.round(pageSize / 1024 * 100) / 100);

It would be better to store the actual size in the tree to sort by, then do the division in gImageView.getCellText above. This puts all of the presentation logic in one place, making it easier to extend that logic in the future.
Comment 20 Felix Fung (:felix) 2012-01-25 01:06:24 PST
Created attachment 591384 [details] [diff] [review]
Give all tree views in page info sortable columns

Additional Changes:
- Changed signature to gTreeUtils.sort to take a comparator
- Renamed variable 'text' to 'value' as per comment above. (Size isn't quite right either since it can be a value from any column)
- Moved KB display logic to getCellText to gather all the presentation logic
Comment 21 Daniel Brooks [:db48x] 2012-01-25 18:48:18 PST
Comment on attachment 591384 [details] [diff] [review]
Give all tree views in page info sortable columns

Review of attachment 591384 [details] [diff] [review]:
-----------------------------------------------------------------

great. r=db48x
Comment 23 Matt Brubeck (:mbrubeck) 2012-01-27 09:11:31 PST
https://hg.mozilla.org/mozilla-central/rev/3ffb30085569
Comment 24 Colby Russell :crussell (no longer Mozilla-ing) 2012-01-31 10:39:14 PST
Comment on attachment 591384 [details] [diff] [review]
>@@ -116,16 +117,35 @@ pageInfoTreeView.prototype = {
>   performActionOnRow: function(action, row)
>   {
>     if (action == "copy") {
>       var data = this.handleCopy(row)
>       this.tree.treeBody.parentNode.setAttribute("copybuffer", data);
>     }
>   },
> 
>+  onPageMediaSort : function(columnname)

A comment:

The result of this copy and paste is probably not what you wanted.  pageInfoTreeView.prototype now has an onPageMediaSort method for sorting the Meta tree, which is on the General page, not the Media page.

For future reference:

>             <treecol id="meta-name"    label="&generalMetaName;"
>-                     persist="width" flex="1"/>
>+                     persist="width" flex="1"
>+                     onclick="gMetaView.onPageMediaSort('meta-name');"/>
>             <splitter class="tree-splitter"/>
>             <treecol id="meta-content" label="&generalMetaContent;"
>-                     persist="width" flex="4"/>
>+                     persist="width" flex="4"
>+                     onclick="gMetaView.onPageMediaSort('meta-content');"/>

>           <treecol sortSeparators="true" primary="true" persist="width" flex="10"
>-                        width="10" id="image-address" label="&mediaAddress;"/>
>+                        width="10" id="image-address" label="&mediaAddress;"
>+                        onclick="gImageView.onPageMediaSort('image-address');"/>
>           <splitter class="tree-splitter"/>
>           <treecol sortSeparators="true" persist="hidden width" flex="2"
>-                        width="2"  id="image-type"    label="&mediaType;"/>
>+                        width="2"  id="image-type"    label="&mediaType;"
>+                        onclick="gImageView.onPageMediaSort('image-type');"/>
>           <splitter class="tree-splitter"/>
>           <treecol sortSeparators="true" hidden="true" persist="hidden width" flex="2"
>-                        width="2"  id="image-size"    label="&mediaSize;"/>
>+                        width="2"  id="image-size"  label="&mediaSize;" value="size"
>+                        onclick="gImageView.onPageMediaSort('image-size');"/>
>           <splitter class="tree-splitter"/>
>           <treecol sortSeparators="true" hidden="true" persist="hidden width" flex="4"
>-                        width="4"  id="image-alt"    label="&mediaAltHeader;"/>
>+                        width="4"  id="image-alt"    label="&mediaAltHeader;"
>+                        onclick="gImageView.onPageMediaSort('image-alt');"/>
>           <splitter class="tree-splitter"/>
>           <treecol sortSeparators="true" hidden="true" persist="hidden width" flex="1"
>-                        width="1"  id="image-count"    label="&mediaCount;"/>
>+                        width="1"  id="image-count"    label="&mediaCount;"
>+                        onclick="gImageView.onPageMediaSort('image-count');"/>

What you would've wanted to do here is just implement pageInfoTreeView.prototype.cycleHeader and override it for gImageView, instead of adding extra click handlers.

>-function pageInfoTreeView(copycol)
>+function pageInfoTreeView(treeid, copycol)
> {
>   // copycol is the index number for the column that we want to add to
>   // the copy-n-paste buffer when the user hits accel-c
>+  this.treeid = treeid;

this.tree.element in the only consumer (onPageMediaSort/cycleHeader) would've given you what you needed.  It also wouldn't have required changing the constructor callsites:

>-var gMetaView = new pageInfoTreeView(COPYCOL_META_CONTENT);
>-var gImageView = new pageInfoTreeView(COPYCOL_IMAGE);
>-
>+var gMetaView = new pageInfoTreeView('metatree', COPYCOL_META_CONTENT);
>+var gImageView = new pageInfoTreeView('imagetree', COPYCOL_IMAGE);
Comment 25 Colby Russell :crussell (no longer Mozilla-ing) 2012-01-31 10:41:00 PST
Don't know why those fields changed...
Comment 26 Felix Fung (:felix) 2012-01-31 11:35:46 PST
>The result of this copy and paste is probably not what you wanted.  >pageInfoTreeView.prototype now has an onPageMediaSort method for sorting the Meta >tree, which is on the General page, not the Media page.

See comment 16, I intended the meta tree to be sortable.

I will look into overriding cycleHeader
Comment 27 Colby Russell :crussell (no longer Mozilla-ing) 2012-01-31 11:45:05 PST
(In reply to Felix Fung (:felix) from comment #26)
> >The result of this copy and paste is probably not what you wanted.  >pageInfoTreeView.prototype now has an onPageMediaSort method for sorting the Meta >tree, which is on the General page, not the Media page.
> 
> See comment 16, I intended the meta tree to be sortable.

Right.  You probably didn't want the function to be called onPageMediaSort, though, since the Meta tree isn't on the Media page. :)

By the way, thanks for the patch.
Comment 28 t8av 2012-02-12 08:27:10 PST
It seems that you guys work hard for this feature. I admit I do not have the ability to understand your code, but still, I would like to ask why is it so complicated to sort items (files sizes in this case)? Will I be able to enjoy this feature during my life time?
Comment 29 Matt Brubeck (:mbrubeck) 2012-02-12 21:57:21 PST
(In reply to t8av from comment #28)
> Will I be able to enjoy this feature during my life time?

This bug is fixed in Firefox 12, which will be released in April.  You can download a pre-release version today from the "Aurora" channel:

http://mozilla.org/firefox/aurora/
Comment 30 Philip Chee 2012-02-17 02:20:31 PST
> I will look into overriding cycleHeader
I used cycleHeader in the SeaMonkey version of this bug in Bug 76170. I also fixed a bug exposed by making the image tree columns sortable. This bug also exists in Firefox.
Comment 31 t8av 2012-08-30 23:59:34 PDT
(In reply to Matt Brubeck (:mbrubeck) from comment #29)
> (In reply to t8av from comment #28)
> > Will I be able to enjoy this feature during my life time?
> 
> This bug is fixed in Firefox 12, which will be released in April.  You can
> download a pre-release version today from the "Aurora" channel:
> 
> http://mozilla.org/firefox/aurora/

Sorry, maybe I am blind, but now it is already Firefox version 15, and I still cannot sort the items by size.
Comment 32 Henrik Skupin (:whimboo) 2012-08-31 04:08:01 PDT
Works fine for me with the tested Firefox 14.0 version. You have to click the column header to sort by type in ascending/descending mode.
Comment 33 t8av 2012-08-31 12:55:21 PDT
Yes, now I can see it. 

The field "Size" should be shown by default - please do not expect the user to find it by himself in the right icon.

Note You need to log in before you can comment on or make changes to this bug.