76170 - Add column sort for page info

Status: RESOLVED FIXED

(SeaMonkey :: Page Info, enhancement)

Description

[Eric Vaandering (no email)]

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)]

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]]

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]]

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)]

*** Bug 101904 has been marked as a duplicate of this bug. ***

Comment 5

[Daniel Brooks [:db48x]]

I'll be back from vacation soon.

Comment 6

[Peter Lairo]

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]]

0.9.9 it is

Comment 8

[sairuh (rarely reading bugmail)]

mass moving open bugs pertaining to page info to [e-mail] as qa contact.

to find all bugspam pertaining to this, set your search string to
"BigBlueDestinyIsHere".

Comment 9

[Daniel Brooks [:db48x]]

*** Bug 123950 has been marked as a duplicate of this bug. ***

Comment 10

[Adam Hauner]

Daniel, could you set new target milestone? M099 is past... 

Comment 11

[Daniel Brooks [:db48x]]

yea, let me retriage a bunch of stuff.

Comment 12

[Daniel Brooks [:db48x]]

I should be able to get to all of these by the time 1.1 is released.

Comment 13

[Jeremy M. Dolan]

This still being considered for 1.1b? Just had a big use for sorting links on my
page by URL. :)

Comment 14

[HJ]

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]

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]

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]

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]

I think bug 275223 and 228110 are duplicates of this bug

Comment 20

[Lewis Rosenthal]

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]

Attachment: 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

[[e-mail]]

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]

> 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]

Attachment: 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]

> Fixed.

Yeah!! Thank you.

Comment 26

[Philip Chee]

>> Fixed.
> Yeah!! Thank you.
Not yet. Still needs to be reviewed before I can check this in.

Comment 27

[[e-mail]]

(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

[[e-mail]]

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]

Attachment: 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]

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]

Was this for Seamonkey only? What about Firefox?

Comment 32

[Philip Chee]

> 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]

Thanks, I missed that:) Seems that bug is fixed in FF12.

Comment 34

[Philip Chee]

Attachment: 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

[[e-mail]]

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]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/1fd048add6cb