140467 - menu items "remove from list" "launch file" and "show in explorer" disabled in download manager when download completed / finished

Status: VERIFIED FIXED

(SeaMonkey :: Download & File Handling, defect)

Description

[evilmrhenry]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.0rc1) Gecko/20020417
BuildID:    2002041711

If a download is selected in the download manager at the time it finishes, the
right buttons are not enabled.

Reproducible: Always
Steps to Reproduce:
1. Start downloading a small file.
2. Select the file in the download manager.
3. Wait for it to download.

Actual Results:  After the file completed downloading, the cancel button was
still enabled, as were the other buttons associated with files in the process of
downloading.

Expected Results:  The download manager should act as if it had completed
downloading, enabling the correct buttons.

The work-around is to select another item, and then the first item again.

Comment 1

[John Levon]

*** This bug has been marked as a duplicate of 135982 ***

Comment 2

[evilmrhenry]

I do not think this is a dulplicate. 13598 is in the area of the actual download
window, while this bug is in the download manager.

In the download manager, after a file has been downloaded, it remains listed,
and a different set of buttons become active. (Except under the circumstances
listed in this bug.)

I repeat, this bug does not affect the regular download popup at all, just the
download manager. Please take another look at this bug.

Comment 3

[John Levon]

Yes, sorry about that. I can see this, although just leaving the download
manager window resets the buttons. Also the files list themselves as
Finished 0% of 0% done.

Comment 4

[evilmrhenry]

That's OK.

It looks like Mozilla just needs to hit the function that checks which buttons
should be enabled after finishing a download that is selected in the download
manager. It should also be called when clicking on a button, so it should be
easy to find.

Comment 5

[Brad Town]

I see the same behavior in 2002051704 (Win2K).

Comment 6

[Skewer]

I see this in 2002060616 WXP. Marking NEW.

Comment 7

[Blake Ross]

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

Comment 8

[Malcolm Rowe]

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

Comment 9

[Ian Neal]

Taking bug

Comment 10

[Ian Neal]

Accepting

Comment 11

[Ian Neal]

Attachment: Patch v1.0

This fixes things by patching nsDownloadProgressListener.js - there may be a
tidier way using a patch to nsDownloadManager.cpp

Comment 12

[[e-mail]]

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

Comment 13

[[e-mail]]

Comment on attachment 123624 [details] [diff] [review]
Patch v1.0

I think the right way to do this is to dispatch a select event on the tree (see
for example tebbox.xml and tabbrowser.xml)

Comment 14

[[e-mail]]

Oops, I didn't have my glases on - I meant tabbox.xml 8-)

Comment 15

[Michael Lefevre]

summary from dupe was better...

Comment 16

[Ian Neal]

I did try the equivalent of that.

The problem with dispatching a select event from within
nsDownloadProgresListener.js is that the flag that says whether the file is
still downloading or not (found using
gDownloadManager.getDownload(selectedItem.id)) hasn't been updated by the time
onStateChange is triggered. Thus the buttons are incorrectly updated. 

As far as I can see there is no easy way of changing this flag prior to
onStateChange being triggered thus this 'fudge'.

Comment 17

[Ian Neal]

Attachment: Partial patch v1.1

This patches nsDownloadManager.cpp to move the test for
mCurrDownloads.Exists(&key) out of AssertProgressInfoFor so that call to
mCurrDownloads.Remove(&key) can be issued before entering
AssertProgressInfoFor. This means an onselect event can be dispatched

Comment 18

[Ian Neal]

Attachment: Partial Patch v1.1 2nd attempt

Having completely missed where the listener was being called (I only looked
back one step instead of two), this should be a better patch. The call to
nsIDownloadProgressListener's OnStateChange is now made after the call
DownloadEnded

Comment 19

[Ian Neal]

Attachment: Full Patch v1.1

Changed update to be done by dispatching onselect event and fixed
nsDownloadManager.cpp so that flags are updated before OnStateChange is
triggered in the nsDownloadProgressListener.js

Comment 20

[Ian Neal]

Attachment: Patch v1.1a

cvs diff rather than local diff of files

Comment 21

[[e-mail]]

Comment on attachment 123784 [details] [diff] [review]
Patch v1.1a

I don't like the eleA parameter to btnUpdate - in fact I don't see the point of
doing all those checks either...

Comment 22

[Ian Neal]

Attachment: Patch v1.1c

Checks taken out and firing of trigger moved into OnStateChange from the
function now checks aren't done.

Comment 23

[[e-mail]]

Comment on attachment 123789 [details] [diff] [review]
Patch v1.1c

>@@ -315,5 +321,3 @@
> 
>   return result;
> }
>-
>-
Except for this change, r=me

Comment 24

[Ian Neal]

Attachment: Patch v1.1d

Same as v1.1c except not removing the surplus line feeds

Comment 25

[jag (Peter Annema)]

Comment on attachment 123792 [details] [diff] [review]
Patch v1.1d

sr=jag

Make sure this doesn't break code that might be expecting the old/bad
behaviour.

Comment 26

[Ian Neal]

I've tested everything I can on Linux in download manager and it doesn't seem to
break anything. As far as I can see it's only download manager that uses this code.

Comment 27

[Ian Neal]

Seeking driver approval:

Patch has been compiled and tested on Linux with various different combinations
of number of downloads and which downloads and completed downloads are
highlighted and nothing appears to have broken by moving the call to the 2nd
listener to after the code that updates some variables. Only one of these
variables is used by the 2nd listener and was the one that was set to the
incorrect value for the listener to correctly refresh the menu items.

There is a very low risk that changing the order will cause a regression but
testing hasn't shown it.

Comment 28

[Asa Dotzler [:asa]]

Comment on attachment 123792 [details] [diff] [review]
Patch v1.1d

a=asa (on behalf of drivers) for checkin to 1.4

Comment 29

[(not reading, please use [e-mail] instead)]

ok, asa beat me to the punch.

a=asa,sspitzer

Comment 30

[[e-mail]]

Fix checked in.

Comment 31

[Matthias Versen [:Matti]]

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

Comment 32

[Ian Neal]

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

Comment 33

[Takanori MATSUURA]

Is this really fixed?

Even now I cannot remove items from the list in download manager.
I checked 2003052[5678]08 trunk source on my Linux box.

Comment 34

[Ian Neal]

That would probably be bug 201697 which was checked in today

Comment 35

[Chris Petersen]

Marking verified since I can't reproduce on the latest trunk.

Comment 36

[Max Nokhrin]

I've reproduced this a couple of times with build 

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4) Gecko/20030529

Max

Comment 37

[Tom Cumming]

Hate to mention this, but I'm still getting problems under 1.5rc1. I'm not sure
whether this is exactly the same bug or something slightly different.

If I download one file, then this item will not remove from the download manager
("remove" is greyed out and "cancel" is enabled but does nothing.) If I download
more files, they will remove OK, but the top one on the list is stuck and I
cannot remove it at all.

Comment 38

[[e-mail]]

Does Ctrl+Clicking the item help?

Comment 39

[Tom Cumming]

No, Ctrl+clicking does not make any difference. Incidentally, in trying that out
I've now got two files on the list stuck that I cannot remove.

Comment 40

[Tom Cumming]

Hmm, downloaded another file today and the two items that were "stuck" yesterday
removed perfectly OK today. Now I have no idea what is going on, except that
there's still something not quite right.