430486 - Clear List button doesn't disable when it should.

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[Gabriele Best [:Gabri]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; it; rv:1.9pre) Gecko/2008041907 Minefield/3.0pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; it; rv:1.9pre) Gecko/2008042303 Minefield/3.0pre

After the restore of the 'Clear List' button on the download manager with the last patch of the bug 400495 , it hasn't an icon anymore, it is on the left side of the searchbar (instead of the right one) and it is always activated.

Reproducible: Always



Expected Results:  
1. It needs an icon
2. It have to be on the right side of the searchbar
3. It have not to be always activated

To see the correct behavior install on Firefox 3 Beta 5 this addon https://addons.mozilla.org/it/firefox/addon/6945

Comment 1

[Damian Shaw [Quan]]

I don't agree it should be on the right side. But the other 2 points seem valid.

Comment 2

[Gabriele Best [:Gabri]]

Attachment: Patch for the third point (the button is always activated)

Comment 3

[Mike Connor [:mconnor]]

Comment on attachment 317303 [details] [diff] [review]
Patch for the third point (the button is always activated)

hmm, I think there might e a better way to do this.  Ed?

Comment 4

[Mike Connor [:mconnor]]

I don't agree it needs an icon, or that it needs to be on the right side.  Disabling when appropriate is correct though, morphing for that.

Comment 5

[Gabriele Best [:Gabri]]

Attachment: Patch v2

This works perfectly on my computer (Win XP)

Comment 6

[Ed Lee :Mardak]

Comment on attachment 317350 [details] [diff] [review]
Patch v2

Way too many calls than necessary. It will also enable clear list when doing a search that matches nothing. We can be conservative and show clear list only when there are items and make sure not to enable it when there's nothing.

I'll have a patch with test.

Comment 7

[Ed Lee :Mardak]

Attachment: v3

Update the clear list button:
1) Building the list
2) Adding a single item to an already open list
3) Removing a single item from an already open list

Delay the notification because otherwise when we have 0 items, the notification gets sent before we disable the clear list button... so the ui isn't technically "done".

Reenable the test on windows because it was probably because of "file://dummy/file". Update the test to check for disabled states. Also update the test to use the ui-done notification (this was and old test before we added the notification to simplify testing).

Fix up styling on linux and os x.

Comment 8

[Gabriele Best [:Gabri]]

You have done a lot of not useful changes that don't work properly IMHO.
I've taken the code of the patch v2 from firefox 2 and it works without any problems.

However, it should update the clear list button when (firefox 2 and patch v2 did them):
1) Start the download manager
2) A download is completed 
3) A download is canceled
4) A download is resumed
5) A download is removed (might be the last one)
6) A download is retried after it was canceled
7) Click on 'Clear list' button

With patch v3 I've seen also that clear list button is always activated if there is only a download in progress on the download manager and nothing else.

Comment 9

[Ed Lee :Mardak]

Yes. I know that it'll show the clear list button as clickable when there are active downloads. It'll be clickable whenever there's at least 1 item shown.

If the user sees 1 done and 1 active download vs just 1 active download...

Vs searching for terms and finding nothing..

Comment 10

[Gabriele Best [:Gabri]]

Ok, but why?

I think it's better to implement it only when there are unactive downloads IMHO (like firefox 2).

And why don't you replace   
button.disabled = !gDownloadsView.itemCount; 
with   
button.disabled = !gDownloadManager.canCleanUp; ?

It sounds better :P

Comment 11

[Gabriele Best [:Gabri]]

Ah, can you add a margin-top of 3px/4px for the button under win xp/vista?
Thanks.

Comment 12

[Ed Lee :Mardak]

Attachment: v3.1

Also fix bug 430647.

Comment 13

[Ed Lee :Mardak]

Attachment: v3.2

Make the clear list clickable only if the backend says we can clean up as well as if we have items in the list.

No downloads: not clickable
Only non-active: clickable
Non and active: clickable
Only active: not clickable (fixed with v3.1)

Search + no downloads: not clickable
Search + only non-active: clickable
Search + non + active: clickable
Search + only active: clickable**

This will incorrectly show the clear list as clickable if we have an active download plus a search that matches no inactive downloads. But I've already spent too much time on this, so I don't really care right now.

Comment 14

[Ed Lee :Mardak]

Er. the only active -> not clickable is fixed with 3.2 not 3.1.

Comment 15

[Shawn Wilsher :sdwilsh]

Comment on attachment 317591 [details] [diff] [review]
v3.2

r=sdwilsh for code and test changes.  I don't know anything about the css stuff, so I'll leave that for mconnor.

Comment 16

[Ed Lee :Mardak]

Attachment: screenshot of v3.2

The button is 1 pixel bigger than default (but will allow larger fonts to let it expand) and more space above it.

Comment 17

[Mike Connor [:mconnor]]

Comment on attachment 317591 [details] [diff] [review]
v3.2

looks good, ship it.

Comment 18

[Ed Lee :Mardak]

http://hg.mozilla.org/cvs-trunk-mirror/index.cgi/rev/e8f7ad982adf
http://hg.mozilla.org/mozilla-central/index.cgi/rev/e8f7ad982adf

Checking in toolkit/mozapps/downloads/content/downloads.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v  <--  downloads.js
new revision: 1.150; previous revision: 1.149
done
Checking in toolkit/mozapps/downloads/tests/browser/Makefile.in;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/Makefile.in,v  <--  Makefile.in
new revision: 1.26; previous revision: 1.25
done
Checking in toolkit/mozapps/downloads/tests/browser/browser_cleanup_search.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_cleanup_search.js,v  <--  browser_cleanup_search.js
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/themes/gnomestripe/mozapps/downloads/downloads.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/mozapps/downloads/downloads.css,v  <--  downloads.css
new revision: 1.13; previous revision: 1.12
done
Checking in toolkit/themes/pinstripe/mozapps/downloads/downloads.css;
/cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/downloads/downloads.css,v  <--  downloads.css
new revision: 1.31; previous revision: 1.30
done
Checking in toolkit/themes/winstripe/mozapps/downloads/downloads.css;
/cvsroot/mozilla/toolkit/themes/winstripe/mozapps/downloads/downloads.css,v  <--  downloads.css
new revision: 1.33; previous revision: 1.32
done

Comment 19

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9pre) Gecko/2008042704 Minefield/3.0pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008042704 Minefield/3.0pre

-and-

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008042705 Minefield/3.0pre

Filed bug 431105 to address the last paragraph in comment 13.

in-litmus+:

https://litmus.mozilla.org/show_test.cgi?id=5285