Last Comment Bug 76170 - Add column sort for page info
: Add column sort for page info
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Page Info (show other bugs)
: Trunk
: All All
: -- enhancement with 10 votes (vote)
: seamonkey2.10
Assigned To: Philip Chee
:
Mentors:
: 101904 123950 610085 (view as bug list)
Depends on: 482890
Blocks: 82059 228110
  Show dependency treegraph
 
Reported: 2001-04-16 09:05 PDT by Eric Vaandering (no email)
Modified: 2012-02-17 07:39 PST (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
Patch v1.0 WIP (11.02 KB, patch)
2012-02-10 05:45 PST, Philip Chee
neil: feedback+
Details | Diff | Splinter Review
Patch v1.1 fix nits. (10.85 KB, patch)
2012-02-10 20:05 PST, Philip Chee
neil: review+
Details | Diff | Splinter Review
Patch v1.2 as checked in. r=Neil (10.83 KB, patch)
2012-02-12 08:25 PST, Philip Chee
philip.chee: review+
Details | Diff | Splinter Review
Patch v2.0 Supplemental fix to trigger onselect handler [check-in comment 36]. (922 bytes, patch)
2012-02-17 02:39 PST, Philip Chee
neil: review+
Details | Diff | Splinter Review

Description Eric Vaandering (no email) 2001-04-16 09:05:04 PDT
The column headers in the various Page Info tabs should sort the lists by these
columns. In other words, clicking on "Image URL" should sort the list by the
URL.  Same comment for all other columns.
Comment 1 sairuh (rarely reading bugmail) 2001-04-16 13:16:37 PDT
yes, this would be useful if, eg, there are a bunch of images on a given page
and you'd want to sort 'em for easier viewing.

->daniel?
Comment 2 Daniel Brooks [:db48x] 2001-04-17 08:28:44 PDT
yes, I'll take it. probably take rewriting the sort stuff that everyone else
uses, cause the treeutils everyone else uses assumes that there's only one tree
you want to sort in.

db48x
Comment 3 Daniel Brooks [:db48x] 2001-05-16 18:02:18 PDT
it looks like there are two ways to fix this. 1) Rewrite nxXULSortService, and
2) rewrite page info to use outliners instead of trees. Since 2 provies more
benefits than 1, and I'm less likely to screw anything else up by doing 2 than I
would by attempting 1, it looks like 2 is the way to go. Probably more work
though :P
Comment 4 sairuh (rarely reading bugmail) 2001-09-27 14:24:24 PDT
*** Bug 101904 has been marked as a duplicate of this bug. ***
Comment 5 Daniel Brooks [:db48x] 2001-12-24 09:00:08 PST
I'll be back from vacation soon.
Comment 6 Peter Lairo 2002-01-04 02:09:08 PST
From duped bug 101904:
View - Page Info - "Forms & Images" -> Collumns should be Sortable

Potential use: e.g., It might be usefull to quickly find the *largest images* on
a web page ;)
Comment 7 Daniel Brooks [:db48x] 2002-01-17 12:20:20 PST
0.9.9 it is
Comment 8 sairuh (rarely reading bugmail) 2002-01-24 15:39:14 PST
mass moving open bugs pertaining to page info to pmac@netscape.com as qa contact.

to find all bugspam pertaining to this, set your search string to
"BigBlueDestinyIsHere".
Comment 9 Daniel Brooks [:db48x] 2002-02-06 15:22:57 PST
*** Bug 123950 has been marked as a duplicate of this bug. ***
Comment 10 Adam Hauner 2002-04-02 21:12:41 PST
Daniel, could you set new target milestone? M099 is past... 
Comment 11 Daniel Brooks [:db48x] 2002-04-02 23:54:14 PST
yea, let me retriage a bunch of stuff.
Comment 12 Daniel Brooks [:db48x] 2002-04-03 00:07:41 PST
I should be able to get to all of these by the time 1.1 is released.
Comment 13 Jeremy M. Dolan 2002-06-18 01:36:42 PDT
This still being considered for 1.1b? Just had a big use for sorting links on my
page by URL. :)
Comment 14 HJ 2004-02-22 04:48:37 PST
Yeah, 1.1 looks foolish. Lets future this for now and add keyword helpwanted.

Daniel, are you, or someone else, working on this? If not, why is it still
assigned to you?
Comment 15 Rob Hudson 2004-04-08 13:32:09 PDT
Same bug applies to Firefox.  Just attempted to sort in the page info media tab
to sort by type and it doesn't work.  Would be a nice consistent feature.
Comment 16 Atlanx 2004-06-13 03:03:48 PDT
We are now at Mozilla 1.7 (RC3) and sorting on the Page Info tabs isn't
available until now.
Is someone working on this bug?
Comment 17 Justin 2004-11-05 20:49:28 PST
If sorting isn't going to be made avalible, then the columns should atleast be
fixed so that they aren't clickable. At the moment they are clickable--have the
action of a button. This implies that they are supposed to do something. So this
should really be marked as a minor bug rather than an enchantment.
Comment 18 s5t1e3v4e3m11 2007-04-03 22:15:22 PDT
I think bug 275223 and 228110 are duplicates of this bug
Comment 19 Philip Chee 2010-11-05 20:37:32 PDT
*** Bug 610085 has been marked as a duplicate of this bug. ***
Comment 20 Lewis Rosenthal 2011-02-15 15:25:31 PST
LOL...

I just ran into this while checking a web page for a client. There were tons of media files getting pulled, and I wanted to see what was causing the lag. Instinctively, I clicked on the column display button, added "Size" to the list, and clicked on that to sort. Imagine my surprise when nothing happened. :-)

I tend to agree with Justin's comment 17 from 2004: until this is functional, it shouldn't "look" like a bug. That said, it's probably just as much work to make these columns sort as it is to make them fixed and not buttons.
Comment 21 Philip Chee 2012-02-10 05:45:13 PST
Created attachment 596025 [details] [diff] [review]
Patch v1.0 WIP

This is vaguely based on Firefox Bug 275223. In Bug 275223 Comment 24, Colby
Russell suggested using the CycleHeader() instead of adding gumpzillion click
handlers.

> +  cycleHeader: function cycleHeader(col)

I got the following hint from our permissions/treeUtils.js

>   // this is a temporary hack for 1.7, we should implement
>   // display and sort variables here for trees in general

> +const COL_IMAGE_SIZENUM = 7;

> +gImageView.cycleHeader = function(col)
> +{
> +  var index = col.index;
> +  var comparator;
> +  switch (col.index) {
> +    case COL_IMAGE_SIZE:
> +      index = COL_IMAGE_SIZENUM;
> +    case COL_IMAGE_COUNT:
> +      comparator = function numComparator(a, b) { return a[index] - b[index]; };
> +      break;
> +  }
> +
> +  this.doSort(col, comparator);
> +};

>      var sizeText;
> +    var pageSize;
>      if (cacheEntryDescriptor) {
> -      var pageSize = cacheEntryDescriptor.dataSize;
> +      pageSize = cacheEntryDescriptor.dataSize;
>        var kbSize = Math.round(pageSize / 1024 * 100) / 100;
>        sizeText = gBundle.getFormattedString("mediaFileSize", [formatNumber(kbSize)]);
>      }
>      else
>        sizeText = gStrings.unknown;
> -    gImageView.addRow([url, type, sizeText, alt, 1, elem, isBg]);
> +    gImageView.addRow([url, type, sizeText, alt, 1, elem, isBg, pageSize]);

I needed to pad out the gLinkView table otherwise I'd get a[index] is undefined
or b[index] is undefined on the last two columns.

> -    gLinkView.addRow([elem.alt, elem.href, gStrings.linkArea, elem.target]);
> +    gLinkView.addRow([elem.alt, elem.href, gStrings.linkArea, elem.target, ""]);

I'm still getting this error when trying to sort in the first column. I don't
know why though.
Comment 22 neil@parkwaycc.co.uk 2012-02-10 06:28:09 PST
Comment on attachment 596025 [details] [diff] [review]
Patch v1.0 WIP

>+    var index = col.index;
>+    var comparator = function comparator(a, b) {
>+      return a[index].toLowerCase().localeCompare(b[index].toLowerCase());
>+    };
Doesn't doSort do this for you?

>+    var ascending = (col.index == this.sortcol) ? !this.sortdir : true;
Could write this as col.index != this.sortcol || !this.sortdir
Or if that's too confusing:
var descending = this.sortdir && col.index == this.sortcol;
this.sortdir = !descending;
...
if (descending)
  this.data.reverse();
Or if that's too confusing, rename sortdir to sortasc:
// Sort descending if we're sorting the already ascending sorted column
var descending = this.sortasc && col.index == this.sortcol;
this.sortasc = !descending;
Or another option would be to special-case descending sort, as all you have to do is to flip the attribute and reverse the array:
if (this.sortasc && col.index == this.sortcol) {
  this.sortasc = false;
  col.element.setAttribute("sortDirection", "descending");
  this.data.reverse();
} else {
   // sort ascending
}

>+    var treecols = this.tree.element.getElementsByTagName("treecol");
Consider using this.tree.columns instead.

>+    if (!comparator)
>+    {
Nit: { on same line

The rest looks reasonable, but I didn't test it because you didn't give sufficiently useful STR for your error.
Comment 23 Philip Chee 2012-02-10 10:51:47 PST
> The rest looks reasonable, but I didn't test it because you didn't give sufficiently
> useful STR for your error.

Oops. Sorry
1. go to http://www.mozilla.org/
2. Open Page Info, go to Links tab.
3. Try to toggle the Name column.

Error Console Message:
Error: '[JavaScript Error: "b[index] is undefined" {file: "chrome://navigator/content/pageinfo/pageInfo.js" line: 126}]' when calling method: [nsITreeView::cycleHeader] = NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS
Source file: chrome://global/content/bindings/tree.xml
Line: 1364

> pageInfo.js" line: 126
This might be slightly different as I've been tweaking the code after posting the patch.
Comment 24 Philip Chee 2012-02-10 20:05:18 PST
Created attachment 596262 [details] [diff] [review]
Patch v1.1 fix nits.

>>+    var comparator = function comparator(a, b) {
>>+      return a[index].toLowerCase().localeCompare(b[index].toLowerCase());
>>+    };
> Doesn't doSort do this for you?
Fixed.

>>+    var ascending = (col.index == this.sortcol) ? !this.sortdir : true;
> Could write this as col.index != this.sortcol || !this.sortdir
> Or if that's too confusing:
I've decided to use your first suggestion.

>>+    var treecols = this.tree.element.getElementsByTagName("treecol");
> Consider using this.tree.columns instead.
nsITreeColumns doesn't have a iterator so I needed to do:
Array.slice(this.tree.columns, 0)
But otherwise done.

>>+    if (!comparator)
>>+    {
> Nit: { on same line
Fixed.
Comment 25 Atlanx 2012-02-11 01:44:04 PST
> Fixed.

Yeah!! Thank you.
Comment 26 Philip Chee 2012-02-11 07:14:58 PST
>> Fixed.
> Yeah!! Thank you.
Not yet. Still needs to be reviewed before I can check this in.
Comment 27 neil@parkwaycc.co.uk 2012-02-11 09:19:41 PST
(In reply to Philip Chee from comment #23)
> > The rest looks reasonable, but I didn't test it because you didn't give sufficiently
> > useful STR for your error.
> 
> Oops. Sorry
> 1. go to http://www.mozilla.org/
> 2. Open Page Info, go to Links tab.
> 3. Try to toggle the Name column.
> 
> Error Console Message:
> Error: '[JavaScript Error: "b[index] is undefined" {file:
> "chrome://navigator/content/pageinfo/pageInfo.js" line: 126}]' when calling
> method: [nsITreeView::cycleHeader] =
> NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS
> Source file: chrome://global/content/bindings/tree.xml
> Line: 1364
> 
> > pageInfo.js" line: 126
> This might be slightly different as I've been tweaking the code after
> posting the patch.

This is due to a bug in the patch for bug 394170 which tries to use the non-existent elem.language (note that elem.getAttribute("language") is not much better because it can return null).
Comment 28 neil@parkwaycc.co.uk 2012-02-11 09:28:50 PST
Comment on attachment 596262 [details] [diff] [review]
Patch v1.1 fix nits.

>+    var treecols = Array.slice(this.tree.columns, 0);
>+    for (let treecol of treecols)
How about using Array.forEach(this.tree.columns, ...); instead?
If not, you shouldn't need the , 0 parameter to slice().

>+    this.tree.view.selection.clearSelection();
Shouldn't need this if you're going to select a row anyway.

>+    this.tree.view.selection.select(0);
>+    this.tree.invalidate();
>+    this.tree.ensureRowIsVisible(0);
Nit: Invalidate first please.

It occurs to me that you should consider refusing to sort unless you have at least two rows.
Comment 29 Philip Chee 2012-02-12 08:25:44 PST
Created attachment 596487 [details] [diff] [review]
Patch v1.2 as checked in. r=Neil

>>+    var treecols = Array.slice(this.tree.columns, 0);
>>+    for (let treecol of treecols)
> How about using Array.forEach(this.tree.columns, ...); instead?
Fixed.

>>+    this.tree.view.selection.clearSelection();
> Shouldn't need this if you're going to select a row anyway.
Fixed.

>>+    this.tree.view.selection.select(0);
>>+    this.tree.invalidate();
>>+    this.tree.ensureRowIsVisible(0);
> Nit: Invalidate first please.
Fixed.

> It occurs to me that you should consider refusing to sort unless you have at least two rows.
Em, I'll leave that to a possible follow up bug if someone complains.

>>> pageInfo.js" line: 126
>> This might be slightly different as I've been tweaking the code after
>> posting the patch.
> 
> This is due to a bug in the patch for bug 394170 which tries to use the
> non-existent elem.language (note that elem.getAttribute("language") is
> not much better because it can return null).

Since I was already touching this line, I fixed this using:

> -    gLinkView.addRow([elem.type || elem.language, elem.src || gStrings.linkScriptInline, gStrings.linkScript]);
> +    gLinkView.addRow([elem.type || elem.getAttribute("language") || gStrings.notSet,
> +                      elem.src || gStrings.linkScriptInline,
> +                      gStrings.linkScript, "", "", ""]);
Comment 30 Philip Chee 2012-02-12 08:27:59 PST
Pushed to comm-central.
http://hg.mozilla.org/comm-central/rev/c36b1ed900b9

FIXED! Thank you all in the CC list for your patience!
Comment 31 :aceman 2012-02-12 08:37:17 PST
Was this for Seamonkey only? What about Firefox?
Comment 32 Philip Chee 2012-02-12 10:22:02 PST
> Was this for Seamonkey only? What about Firefox?
Here's a hint. In Comment 21 I wrote:
> This is vaguely based on Firefox Bug 275223. In Bug 275223 Comment 24, Colby
> Russell suggested using the CycleHeader() instead of adding gumpzillion click
> handlers.
Comment 33 :aceman 2012-02-12 11:27:10 PST
Thanks, I missed that:) Seems that bug is fixed in FF12.
Comment 34 Philip Chee 2012-02-17 02:39:52 PST
Created attachment 598181 [details] [diff] [review]
Patch v2.0 Supplemental fix to trigger onselect handler [check-in comment 36].

>>+    this.tree.view.selection.clearSelection();
> Shouldn't need this if you're going to select a row anyway.

It turns out this is needed to workaround a selection bug. STR:
1. Go to http://www.mozilla.org/
2. Open the Page Info window. Select the Media Tab.
3. Click several times on the Address column header.
4. Notice that the Preview pane doesn't change although the row selection does.

q.v.
http://mxr.mozilla.org/seamonkey/source/mail/components/preferences/viewpasswords.js#584
> 584         // update selection
> 585         // note: we need to deselect before reselecting in order to trigger ...Selected()
Comment 35 neil@parkwaycc.co.uk 2012-02-17 03:28:36 PST
Comment on attachment 598181 [details] [diff] [review]
Patch v2.0 Supplemental fix to trigger onselect handler [check-in comment 36].

Fair enough, but could you add a comment to that effect too please?
Comment 36 Philip Chee 2012-02-17 07:38:12 PST
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/1fd048add6cb

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