Column Sorting in the Download Manager has no effect

VERIFIED FIXED

Status

VERIFIED FIXED
17 years ago
14 years ago

People

(Reporter: ian, Assigned: janv)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt3])

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

17 years ago
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.
nominating --not sure how easy a fix this would be, or how important to get to
(time-wise).
Keywords: nsbeta1
OS: Linux → All
Hardware: PC → All

Comment 2

17 years ago
nsbeta1-/adt3 per Nav triage team.  Don't think we need this for RTM, but will
reconsider if beta feedback warrants.
Keywords: nsbeta1 → nsbeta1-
Whiteboard: [adt3]
Target Milestone: --- → mozilla1.1alpha

Comment 3

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

17 years ago
-> Jan

I think this is pretty important.
Assignee: blaker → varga
Target Milestone: mozilla1.1alpha → ---

Comment 5

17 years ago
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.
Created attachment 84518 [details]
downloadmanager.js (new version)
Created attachment 84519 [details]
downloadmanager.xul (new version)
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

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

Max.

Comment 10

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

17 years ago
> ------- Additional Comments From moz@compsoc.man.ac.uk  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

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

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

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

Comment 15

17 years ago
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.
Keywords: mozilla1.0.1, nsCatFood
(Reporter)

Comment 16

17 years ago
(actually the next version is 1.1.0)

Comment 17

17 years ago
Next revision, then?
(Reporter)

Comment 18

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

17 years ago
Better safe than sorry. I really, really want this fixed soon.
Keywords: mozilla1.1
(Reporter)

Comment 20

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

Comment 22

17 years ago
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.
Status: NEW → ASSIGNED
So why is the download manager building content instead of using an rdfliner?

Comment 24

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

Comment 25

17 years ago
is bug 147842 related?
(Assignee)

Comment 26

17 years ago
nominating for buffy
Keywords: nsbeta1- → nsbeta1

Comment 27

17 years ago
Nav triage team: nsbeta1+/adt3
Keywords: nsbeta1 → nsbeta1+
(Assignee)

Comment 28

17 years ago
Created attachment 96616 [details] [diff] [review]
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.
(Assignee)

Updated

17 years ago
Attachment #84518 - Attachment is obsolete: true
Attachment #84519 - Attachment is obsolete: true
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).
(Assignee)

Comment 30

17 years ago
Neil, XUL sort service is supposed to update sortActive attribute.
(Assignee)

Comment 31

17 years ago
and the XUL sort service hasn't been fixed to work like XUL tree builder
(Assignee)

Comment 32

16 years ago
jag, could you r/sr ?
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 :-)
Attachment #96616 - Flags: needs-work+
(Assignee)

Comment 34

16 years ago
Created attachment 102734 [details] [diff] [review]
another patch

this hack should make it
Attachment #96616 - Attachment is obsolete: true
(Assignee)

Comment 35

16 years ago
Neil, could you review ?
(Assignee)

Comment 36

16 years ago
Created attachment 102785 [details] [diff] [review]
patch incorporating Neil's suggestions
Attachment #102734 - Attachment is obsolete: true
(Assignee)

Comment 37

16 years ago
Created attachment 102789 [details] [diff] [review]
new version

I'm a moron.
I forgot to include nsXULSortService changes
Attachment #102785 - Attachment is obsolete: true
Comment on attachment 102789 [details] [diff] [review]
new version

Couldn't break this one :-)
Attachment #102789 - Flags: review+
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?
QA Contact: sairuh → petersen
(Assignee)

Comment 40

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

Comment 41

16 years ago
Created attachment 103006 [details] [diff] [review]
patch incorporating Jag's comments
Attachment #102789 - Attachment is obsolete: true
Comment on attachment 103006 [details] [diff] [review]
patch incorporating Jag's comments

sr=jag
Attachment #103006 - Flags: superreview+
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.
Attachment #103006 - Flags: review+
Attachment #103006 - Flags: approval+
(Assignee)

Comment 44

16 years ago
checked in, thanks for r/sr/a

Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 45

16 years ago
Verified in the 2003-01-03-08 trunk build under Windows XP.
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.