Add column sort for page info

RESOLVED FIXED in seamonkey2.10

Status

SeaMonkey
Page Info
--
enhancement
RESOLVED FIXED
17 years ago
6 years ago

People

(Reporter: Eric Vaandering (no email), Assigned: Philip Chee)

Tracking

(Depends on: 1 bug)

Trunk
seamonkey2.10
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.10 fixed)

Details

Attachments

(4 attachments)

(Reporter)

Description

17 years ago
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.
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?
Assignee: blakeross → db48x
Blocks: 52730
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: PC → All
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
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
Blocks: 81328
No longer blocks: 52730
*** Bug 101904 has been marked as a duplicate of this bug. ***
I'll be back from vacation soon.
Target Milestone: --- → mozilla0.9.8

Comment 6

16 years ago
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 ;)
0.9.9 it is
Target Milestone: mozilla0.9.8 → mozilla0.9.9
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".
QA Contact: sairuh → pmac

Updated

16 years ago
Blocks: 82059
No longer blocks: 81328

Updated

16 years ago
Component: XP Apps: GUI Features → Page Info
*** Bug 123950 has been marked as a duplicate of this bug. ***

Comment 10

16 years ago
Daniel, could you set new target milestone? M099 is past... 
yea, let me retriage a bunch of stuff.
I should be able to get to all of these by the time 1.1 is released.
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.9 → mozilla1.1beta

Updated

16 years ago
Priority: -- → P2

Comment 13

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

Comment 14

14 years ago
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?
Severity: normal → enhancement
Keywords: helpwanted
Summary: Need to be able to sort in page info → Add column sort for page info
Target Milestone: mozilla1.1beta → Future

Comment 15

14 years ago
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

13 years ago
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

13 years ago
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.
Product: Browser → Seamonkey

Comment 18

11 years ago
I think bug 275223 and 228110 are duplicates of this bug
Status: ASSIGNED → NEW
Priority: P2 → --
QA Contact: pmac
Target Milestone: Future → ---

Updated

9 years ago
Depends on: 482890
(Assignee)

Updated

8 years ago
Blocks: 228110
(Assignee)

Updated

7 years ago
Duplicate of this bug: 610085

Comment 20

7 years ago
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.
(Assignee)

Comment 21

6 years ago
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.
Assignee: db48x → philip.chee
Status: NEW → ASSIGNED
Attachment #596025 - Flags: feedback?(neil)
(Assignee)

Updated

6 years ago
Keywords: helpwanted
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.
Attachment #596025 - Flags: feedback?(neil) → feedback+
(Assignee)

Comment 23

6 years ago
> 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.
(Assignee)

Comment 24

6 years ago
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.
Attachment #596262 - Flags: review?(neil)

Comment 25

6 years ago
> Fixed.

Yeah!! Thank you.
(Assignee)

Comment 26

6 years ago
>> Fixed.
> Yeah!! Thank you.
Not yet. Still needs to be reviewed before I can check this in.
(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 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.
Attachment #596262 - Flags: review?(neil) → review+
(Assignee)

Comment 29

6 years ago
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, "", "", ""]);
Attachment #596487 - Flags: review+
(Assignee)

Comment 30

6 years ago
Pushed to comm-central.
http://hg.mozilla.org/comm-central/rev/c36b1ed900b9

FIXED! Thank you all in the CC list for your patience!
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
status-seamonkey2.10: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.10

Comment 31

6 years ago
Was this for Seamonkey only? What about Firefox?
(Assignee)

Comment 32

6 years ago
> 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

6 years ago
Thanks, I missed that:) Seems that bug is fixed in FF12.
(Assignee)

Comment 34

6 years ago
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()
Attachment #598181 - Flags: review?(neil)
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?
Attachment #598181 - Flags: review?(neil) → review+
(Assignee)

Comment 36

6 years ago
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/1fd048add6cb
(Assignee)

Updated

6 years ago
Attachment #598181 - Attachment description: Patch v2.0 Supplemental fix to trigger onselect handler. → Patch v2.0 Supplemental fix to trigger onselect handler [check-in comment 36].
You need to log in before you can comment on or make changes to this bug.