Closed Bug 275223 Opened 20 years ago Closed 12 years ago

Page Info columns should be sortable

Categories

(Firefox :: Page Info Window, enhancement)

enhancement
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 12

People

(Reporter: mikronh, Assigned: felix)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

While in Pag Info, in the "Media" tab.
Pushing the "Address" or "Type" headlines does NOT sort by address/type.
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
Summary: No sorting in Page Info → Page Info columns should be sortable
Version: 1.0 Branch → unspecified
I thought this was a bug...Didn't it used to work in past versions?
*** Bug 317675 has been marked as a duplicate of this bug. ***
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.
The same issue for SeaMonkey is covered on bug 76170.
Assignee: bugs → nobody
Version: unspecified → Trunk
Depends on: 482890
See Also: → 76170
> 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.
Blocks: cuts-control
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.
Attachment #488927 - Flags: feedback? → feedback?(johnath)
> -    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?
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 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;
>+  }
>+};
>+
Attachment #488927 - Flags: review?(db48x)
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 :)
Attachment #488927 - Flags: review?(db48x) → review-
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
Attachment #488927 - Attachment is obsolete: true
Attachment #558744 - Flags: review?(db48x)
Attachment #488927 - Flags: feedback?(johnath)
Assignee: nobody → ffung
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.
Attachment #558744 - Flags: review?(db48x) → review-
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
Attachment #558744 - Attachment is obsolete: true
Attachment #591384 - Flags: review?(db48x)
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
Attachment #591384 - Flags: review?(db48x) → review+
https://hg.mozilla.org/mozilla-central/rev/3ffb30085569
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
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);
Target Milestone: Firefox 12 → ---
Version: Trunk → 13 Branch
Don't know why those fields changed...
Target Milestone: --- → Firefox 12
Version: 13 Branch → Trunk
>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
(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.
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?
(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/
Depends on: 727910
> 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.
Blocks: 736572
Blocks: 736574
(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.
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.
Status: RESOLVED → VERIFIED
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: