Closed Bug 430486 Opened 12 years ago Closed 12 years ago

Clear List button doesn't disable when it should.

Categories

(Toolkit :: Downloads API, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9

People

(Reporter: Gabri, Assigned: Mardak)

References

Details

(Whiteboard: [fixes bug 430597])

Attachments

(2 files, 4 obsolete files)

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
I don't agree it should be on the right side. But the other 2 points seem valid.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: unspecified → Trunk
Attachment #317303 - Flags: review?(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?
Attachment #317303 - Flags: review?(mconnor) → review?(edilee)
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.
Flags: wanted-firefox3+
Summary: Restore the 'Clear List' button style → Clear List button doesn't disable when it should.
Attachment #317303 - Flags: review?(edilee)
Attached patch Patch v2 (obsolete) — Splinter Review
This works perfectly on my computer (Win XP)
Attachment #317303 - Attachment is obsolete: true
Attachment #317350 - Flags: review?(mconnor)
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.
Attachment #317350 - Flags: review?(mconnor) → review-
Component: Theme → Download Manager
Flags: in-testsuite?
QA Contact: theme → download.manager
Attached patch v3 (obsolete) — Splinter Review
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.
Assignee: nobody → edilee
Attachment #317350 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #317390 - Flags: review?(mconnor)
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.
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..
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
Ah, can you add a margin-top of 3px/4px for the button under win xp/vista?
Thanks.
Blocks: 400495
Attached patch v3.1 (obsolete) — Splinter Review
Also fix bug 430647.
Attachment #317390 - Attachment is obsolete: true
Attachment #317531 - Flags: review?(mconnor)
Attachment #317390 - Flags: review?(mconnor)
Blocks: 430647
Whiteboard: [has patch][need review mconnor][fixes bug 430647]
Attached patch v3.2Splinter Review
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.
Attachment #317531 - Attachment is obsolete: true
Attachment #317591 - Flags: review?(mconnor)
Attachment #317531 - Flags: review?(mconnor)
Er. the only active -> not clickable is fixed with 3.2 not 3.1.
Blocks: 430597
Whiteboard: [has patch][need review mconnor][fixes bug 430647] → [has patch][need review mconnor][fixes bug 430597]
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.
Attachment #317591 - Flags: review+
Attached image 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.
Attachment #317651 - Flags: ui-review?
Comment on attachment 317591 [details] [diff] [review]
v3.2

looks good, ship it.
Attachment #317591 - Flags: ui-review+
Attachment #317591 - Flags: review?(mconnor)
Attachment #317591 - Flags: review+
Attachment #317591 - Flags: approval1.9+
Attachment #317651 - Flags: ui-review?
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [has patch][need review mconnor][fixes bug 430597] → [fixes bug 430597]
Target Milestone: --- → Firefox 3
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
Status: RESOLVED → VERIFIED
Flags: in-litmus+
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.