134824 - Column Sorting in the Download Manager has no effect

Status: VERIFIED FIXED

(SeaMonkey :: Download & File Handling, defect)

Description

[Hixie (not reading bugmail)]

STEPS TO REPRODUCE
   1. Download lots of stuff.
   2. Open the Download Manager.
   3. Click a column.

ACTUAL RESULTS
   Nothing.

EXPECTED RESULTS
   Sort order should change.

Comment 1

[sairuh (rarely reading bugmail)]

nominating --not sure how easy a fix this would be, or how important to get to
(time-wise).

Comment 2

[Peter Trudelle]

nsbeta1-/adt3 per Nav triage team.  Don't think we need this for RTM, but will
reconsider if beta feedback warrants.

Comment 3

[Jason Bassford]

Or at least change the default sort order (until this bug is fixed) so that the
most recent downloaded files are located at the top.  (I'm sure that most people
don't care to see what they downloaded weeks/months ago at the top.)

Comment 4

[Blake Ross]

-> Jan

I think this is pretty important.

Comment 5

[Dimitrios]

I wholeheartedly agree with Jason's comment #3. Do you think it's better to file
a new bug on that? In most cases, changing column sorting isn't that important.
What it's most important, from the usability point of view, is to have recent
downloads on top. Now we always need scrolling or an uncomfortably large DM
window size.

Comment 6

[Becker-Freyseng, Christoph]

Attachment: downloadmanager.js (new version)

Comment 7

[Becker-Freyseng, Christoph]

Attachment: downloadmanager.xul (new version)

Comment 8

[Becker-Freyseng, Christoph]

Here is a patch (sorry for wrong format,see below(BTW) Attachment 84518 [details] ,
Attachment 84519 [details]) that I just made. It's NOT the COMPLETE fix of this bug! See
below! I have TESTED these things ONLY with LINUX.

There's one problem with comment #3 . I think there is nothing like a
default-sort-attribute in xul:tree. At least with Linux changing "sortActive"
and/or "sortDirection" had no effect (except small icon in column-header) (Is
this a XUL-bug?). So new downloads are just added to the list somewhere
(probably at the end).

So what does this patch do?
- makes an "active-sort" using JScript and XPCOM
- sorts the list initially after dlMgr-startup and when you click on the columns

Where are PROBLEMS:
- new downloads are just added to the list somewhere (as before), because I
haven't found a way to make added item get sorted automagically. Possible fix:
trigger sorting in nsDownloadmanager.AddDownload .
- newly added items behave weird when you trigger an "active-sort" by clicking
on a column! They just don't get sorted correctly. After quitting Mozilla and
launching it again those items behave normal. Maybe this is a problem with the
data-source and mCurrDownloads or so. 
I'd appreciate help with this (because I'm new to XPCOM, XUL, RDF ... I know
about the concepts of it, read lots of the docs so don't hesitate telling me
"have a look there" ... I'm really willing to learn more about XPCOM, XUL, RDF ...)

CONCLUSION: Probably if we can fix the weird behavior of new-added-items a
complete fix of this bug is near.

BTW: How is the correct way to send a patch here? I know there is a page about
it but I just could not find it anymore!

Comment 9

[Max Waterman]

Comment # 3 is related to bug 141410. Perhaps one of them should be marked as a
duplicate.

Max.

Comment 10

[John Levon]

Max, this bug is about being able to sort /at all/ in the download
manager. That bug is quite separate (besides being a huge UI problem
IMO)

Comment 11

[Max Waterman]

> ------- Additional Comments From [e-mail]  2002-05-24 12:02 -------
> Max, this bug is about being able to sort /at all/ in the download
> manager. That bug is quite separate (besides being a huge UI problem
> IMO)

John,

I agree that this bug has a wider scope, but I consider the other bug to be what
was suggested in comment # 3 as an acceptable short term workaround.

IMO, the reason this bug is likely because of the specific problem described in
bug 141410. If people could see the current download, then they wouldn't
complain about not being able to sort as much. Making the sort order such that
the most recent download was displayed at the top of the window would mean that
the developers have more time to work on the underlying issue of the sort
columns not working.

I don't care if there are two bugs open on this, so long as there isn't a
duplication of effort, but I see that 141410 is clearly a subset of this bug.

Perhaps I am misunderstanding something, in which case, I'm sure someone will
enlighten me :)

Max.

Comment 12

[Jason Bassford]

Max: Not true.  I'm the person who posted comment 3.  I don't use the download
manager.  I have my prefs.js set to show only a download status window.  In
fact, speaking personally, I don't like/use the download manager at all and,
given my personal choice, it would be removed.

However, assuming that it's going to exist (and I think it should since enough
people do seem to want it), should I ever choose to go to Tools / Download
Manager and see a history of what I've downloaded, I would prefer to see my most
recent downloads at the top.  I didn't post my comment because I was having
problems seeing what I was downloading at any given moment but because I think
that most people would rather see the most recent downloads at the top of the
list, without having to scroll down...  (Otherwise its usefulness as a download
history tool is somewhat suspect.)

Comment 13

[Max Waterman]

OK, I see the difference in the two bugs now.

However, I do think that the person who filed the other bug would be happy if
the most recent download would be at the top of the list. IMO all they are after
is to be able to see what is currently being downloaded.

Whatever...

Max.

Comment 14

[:deanis74]

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

Comment 15

[Skewer]

I'd like to see this done for mozilla 1.0.1 (yes, that's the very next
version)--the current behavior is kludgy and terrible. The traditional way of
displaying a list of stuff is to put the newest on top so you see it first.
Besides that, anytime a sorting function is completely broken like this really
creates a problem. I don't know how they let this slip into Moz1.0.

Comment 16

[Hixie (not reading bugmail)]

(actually the next version is 1.1.0)

Comment 17

[Skewer]

Next revision, then?

Comment 18

[Hixie (not reading bugmail)]

the next release Mozilla is working on is 1.1.0. It is unclear when (or if)
1.0.1 will be released.

Comment 19

[Skewer]

Better safe than sorry. I really, really want this fixed soon.

Comment 20

[Hixie (not reading bugmail)]

Well, unless you fix it yourself, I doubt it will be getting fixed soon. This is
not a high priority bug for the current assignee.

Comment 21

[Becker-Freyseng, Christoph]

I'm short of time right now, because I've some exams in the next 5 weeks. I
already spent some time making a better fix, but there are some problems. 

You can set the attribute of a true to: flags="dont-build-content", than the
tree sorts fine, but the rest is broken :-(
I posted already in the newsgroup but didn't find time to try the suggestions...

I also had a look at bookmarks and mail as they get their items sorted, but this
code was too complicated for only some days time.

Hopefully I'll have more time in some weeks and then I'll fix it!

Comment 22

[Jan Varga [:janv]]

The attached files look good, but I think doSort() should be implemented in the
tree content view directly. I'll try to do it this way. Otherwise we would need
to implement doSort() for every <tree> that uses content view.

Comment 23

[[e-mail]]

So why is the download manager building content instead of using an rdfliner?

Comment 24

[Robert Wall]

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

Comment 25

[R.K.Aa.]

is bug 147842 related?

Comment 26

[Jan Varga [:janv]]

nominating for buffy

Comment 27

[Samir Gehani]

Nav triage team: nsbeta1+/adt3

Comment 28

[Jan Varga [:janv]]

Attachment: new fix

Ok, so I haven't implemented sorting in tree content view yet.
Originally I wanted to do it using XUL sort service. It seems it would be
better
to not use XUL sort service, because it sorts directly content.
Instead we can just sort cached rows in tree content view, like XUL tree
builder does, so content stays as it is unchanged.

Anyway, for now I created this patch, which calls XUL sort service from JS.
While I was there I renamed nsIXULSortService::Sort() to sort(), and converted
its parameters from string type to nsAString type.
I also fixed nsDownloadManager::AddDownload(), it is more reasonable to add new
items to container after asserting properties, otherwise it confuses XUL sort
service when adding a new item to a sorted tree.

Comment 29

[[e-mail]]

Comment on attachment 96616 [details] [diff] [review]
new fix

doSort doesn't a) seem to update sortActive (so that the code knows which
column to sort when the window is next opened, note that you need to set it to
false on the previous column for persistence to work) b) always toggles the
sort rather than only toggling the sort when the column was already active
(although you have fixed the special case of the inital sort).

Comment 30

[Jan Varga [:janv]]

Neil, XUL sort service is supposed to update sortActive attribute.

Comment 31

[Jan Varga [:janv]]

and the XUL sort service hasn't been fixed to work like XUL tree builder

Comment 32

[Jan Varga [:janv]]

jag, could you r/sr ?

Comment 33

[[e-mail]]

Comment on attachment 96616 [details] [diff] [review]
new fix

I've tested this on Linux and it doesn't work; changing the sorted column
doesn't sort. Reversing the order does work :-)

Comment 34

[Jan Varga [:janv]]

Attachment: another patch

this hack should make it

Comment 35

[Jan Varga [:janv]]

Neil, could you review ?

Comment 36

[Jan Varga [:janv]]

Attachment: patch incorporating Neil's suggestions

Comment 37

[Jan Varga [:janv]]

Attachment: new version

I'm a moron.
I forgot to include nsXULSortService changes

Comment 38

[[e-mail]]

Comment on attachment 102789 [details] [diff] [review]
new version

Couldn't break this one :-)

Comment 39

[jag (Peter Annema)]

Comment on attachment 102789 [details] [diff] [review]
new version


> nsresult
>-XULSortServiceImpl::DoSort(nsIDOMNode* node, const nsString& sortResource, const nsString& sortDirection)
>+XULSortServiceImpl::Sort(nsIDOMNode* node, const nsAString& sortResource, const nsAString& sortDirection)

That nsresult needs to be NS_IMETHODIMP


>+function setSortVariables(tree)
>+{
>+  var node;
>+  for (node = document.getElementById("Name"); node; node = node.nextSibling)
>+    if (node.getAttribute("sortActive") == "true")
>+      break;
>+  if (! node) {

Change the |if (! node)| to |if (!node)| to follow prevailing style.

Also put the multi-line body of the for loop in { } to make it harder for
someone to screw up if they wanna add code to the body.


>+  PRInt32 itemIndex;
>+  downloads->IndexOf(downloadRes, &itemIndex);
>+  if (itemIndex == -1) {
>+    rv = downloads->AppendElement(downloadRes);
>+    if (NS_FAILED(rv)) return rv;
>+  }
>+

So instead of adding the element first and then removing it when something goes
wrong you let stuff succeed or fail first, and then append when everything's
gone right. So why not move it below the remote->Flush?

The code used to move existing items to the end of the list, now it leaves them
in place. Is this change intentional?

Comment 40

[Jan Varga [:janv]]

>That nsresult needs to be NS_IMETHODIMP
fixed

>Change the |if (! node)| to |if (!node)| to follow prevailing style.
>Also put the multi-line body of the for loop in { } to make it harder for
>someone to screw up if they wanna add code to the body.
fixed

>So instead of adding the element first and then removing it when something goes
>wrong you let stuff succeed or fail first, and then append when everything's
>gone right. So why not move it below the remote->Flush?
I don't think this is a good idea, element must be appended before calling Flush()
I tried to move it, but it didn't work well.

>The code used to move existing items to the end of the list, now it leaves them
>in place. Is this change intentional?
Hmm, I'm not sure what you mean, but new items are now correctly inserted according
to sort settings.

Comment 41

[Jan Varga [:janv]]

Attachment: patch incorporating Jag's comments

Comment 42

[jag (Peter Annema)]

Comment on attachment 103006 [details] [diff] [review]
patch incorporating Jag's comments

sr=jag

Comment 43

[Asa Dotzler [:asa]]

Comment on attachment 103006 [details] [diff] [review]
patch incorporating Jag's comments

a=asa for checkin to 1.2 (on behalf of drivers) also noting Neil's review.

Comment 44

[Jan Varga [:janv]]

checked in, thanks for r/sr/a

Comment 45

[Chris Petersen]

Verified in the 2003-01-03-08 trunk build under Windows XP.