Closed Bug 861613 Opened 7 years ago Closed 6 years ago

request for toggable download animation

Categories

(Firefox :: Downloads Panel, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 24

People

(Reporter: javran.c, Assigned: xidorn)

References

Details

(Keywords: user-doc-needed)

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0
Build ID: 20130409194949

Steps to reproduce:

Download files


Actual results:

Download button shown in the up-right corner shows animation when I add new download task and plays animation when some tasks are done.


Expected results:

I suggest this feature to be togglable, for me it's good to see download status without invoking download manager but somewhat distracting to see download complete animation.
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Component: Untriaged → Downloads Panel
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Version: 20 Branch → Trunk
Attachment #754047 - Flags: review?
Attachment #754047 - Flags: review? → review?(paolo.mozmail)
Comment on attachment 754047 [details] [diff] [review]
Add a pref to toggle the animation

This patch looks good to me! And I think that having an about:config preference
for "browser.download.showNotification" is something we may actually want to
support.

On my second assertion I'd just like a confirmation from Marco, that has a
global view on the Downloads Panel feature and may also want to validate the
preference name (that already looks good to me).
Attachment #754047 - Flags: review?(paolo.mozmail) → review?(mak77)
Comment on attachment 754047 [details] [diff] [review]
Add a pref to toggle the animation

Review of attachment 754047 [details] [diff] [review]:
-----------------------------------------------------------------

Actually, we decided we don't care to give a preference for this, though it won't hurt as an hidden pref so I won't block on that.  But that means we don't want the pref in firefox.js
I also dislike that we have to check the pref every time a new download starts or ends, the easy path here is just to cache the value and require a restart for its use (the hard path would be to refactor DebugPrefObserver to be able to observe multiple prefs and keep the cache up-to-date).
Regarding the pref name, I think since this controls both the starting end the ending notifications, should use plural "notifications" form. But there are also other kind of notifications (for example the glowing arrow or the growl/toaster notification) and this is not a global notifications toggle, so it sounds confusing. Since this is just disabling animations, what about something like browser.download.animate?

This is my take on this, Paolo is still the best person to give final approval on the actual code pieces.
Attachment #754047 - Flags: review?(mak77) → feedback-
(In reply to Marco Bonardo [:mak] from comment #3)
> Comment on attachment 754047 [details] [diff] [review]
> Add a pref to toggle the animation
> 
> Review of attachment 754047 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Actually, we decided we don't care to give a preference for this, though it
> won't hurt as an hidden pref so I won't block on that.  But that means we
> don't want the pref in firefox.js

Do you mean that we should simply remove it from firefox.js, and let users who want to disable the animation add the pref explicitly themselves?

> I also dislike that we have to check the pref every time a new download
> starts or ends, the easy path here is just to cache the value and require a
> restart for its use (the hard path would be to refactor DebugPrefObserver to
> be able to observe multiple prefs and keep the cache up-to-date).

I don't think it is a bad idea to check the pref every time since it is not that frequent. IMHO, requiring restart for this simple pref is counterintuitive.

> Regarding the pref name, I think since this controls both the starting end
> the ending notifications, should use plural "notifications" form. But there
> are also other kind of notifications (for example the glowing arrow or the
> growl/toaster notification) and this is not a global notifications toggle,
> so it sounds confusing. Since this is just disabling animations, what about
> something like browser.download.animate?

You are right that this name could be confusing, but browser.download.animate is also confusing since there is still an animation which represents the progress of downloads. Maybe we could combine the two name, and use something like browser.download.showAnimatedNotifications? (I wonder if this name is too long.)

> This is my take on this, Paolo is still the best person to give final
> approval on the actual code pieces.

Thanks for your reviewing.
(In reply to Xidorn Quan from comment #4)
> Do you mean that we should simply remove it from firefox.js, and let users
> who want to disable the animation add the pref explicitly themselves?

yes

> > I also dislike that we have to check the pref every time a new download
> > starts or ends, the easy path here is just to cache the value and require a
> > restart for its use (the hard path would be to refactor DebugPrefObserver to
> > be able to observe multiple prefs and keep the cache up-to-date).
> 
> I don't think it is a bad idea to check the pref every time since it is not
> that frequent. IMHO, requiring restart for this simple pref is
> counterintuitive.

well, it happene everytime the user starts a download or a download ends. While currently is not a big deal since all of the prefs are in memory I can't tell if this will stay true forever.
Since this is intended as something for advanced users I think it doesn't matter much if it requires a restart. As I said it's also possible to use a pref observer to update the pref cache, just requires some more code changes.

> > Regarding the pref name, I think since this controls both the starting end
> > the ending notifications, should use plural "notifications" form. But there
> > are also other kind of notifications (for example the glowing arrow or the
> > growl/toaster notification) and this is not a global notifications toggle,
> > so it sounds confusing. Since this is just disabling animations, what about
> > something like browser.download.animate?
> 
> You are right that this name could be confusing, but
> browser.download.animate is also confusing since there is still an animation
> which represents the progress of downloads. Maybe we could combine the two
> name, and use something like browser.download.showAnimatedNotifications? (I
> wonder if this name is too long.)

.animateNotifications?
(In reply to Marco Bonardo [:mak] from comment #5)
> (In reply to Xidorn Quan from comment #4)
> > I don't think it is a bad idea to check the pref every time since it is not
> > that frequent. IMHO, requiring restart for this simple pref is
> > counterintuitive.
> 
> well, it happene everytime the user starts a download or a download ends.
> While currently is not a big deal since all of the prefs are in memory I
> can't tell if this will stay true forever.

I found that useToolkitUI is got more frequently than the pref I added. Should it be added to the pref cache as well?

> > You are right that this name could be confusing, but
> > browser.download.animate is also confusing since there is still an animation
> > which represents the progress of downloads. Maybe we could combine the two
> > name, and use something like browser.download.showAnimatedNotifications? (I
> > wonder if this name is too long.)
> 
> .animateNotifications?

Sounds not bad.
Attachment #754047 - Attachment is obsolete: true
Attachment #756571 - Flags: review?(mak77)
(In reply to Marco Bonardo [:mak] from comment #5)
> well, it happene everytime the user starts a download or a download ends.
> While currently is not a big deal since all of the prefs are in memory I
> can't tell if this will stay true forever.

Though I have submitted a new patch which follows your suggestion, I insist that we need not worry about where the prefs are exactly, and just treat them as in memory. Since the prefs are now in memory, I believe that most place of the current code use the prefs directly like what I did in my first patch (one example is useToolkitUI), and changing all of these code could be a large job. If all frequently-used prefs need such a observer to cache, there would be incredible code duplication.

Consequently, I think that, even if prefs will not always be in memory in the future, the pref service will itself provide the cache in memory instead of requiring observer for individual prefs, because the latter is not an efficient method overall.

(In reply to Marco Bonardo [:mak] from comment #3)
> Actually, we decided we don't care to give a preference for this, though it
> won't hurt as an hidden pref so I won't block on that.  But that means we
> don't want the pref in firefox.js

IMHO, prefs like browser.download.debug is what should be hidden instead of the pref I added, since even advanced users do not want to change them, only developers need. In addition, users who want to modify prefs in about:config are in fact advanced, so this pref should not be hidden from it.
Comment on attachment 756571 [details] [diff] [review]
Add a pref to toggle the animation

Review of attachment 756571 [details] [diff] [review]:
-----------------------------------------------------------------

Your previous approach may be fine, whatever I may say there's likely some valid reason to do the opposite, cause this it not a critical choice, so personal taste has a part.
I asked to do this cause this observing code may be useful for future additions and there was already an incomplete mock of it.
The alternative was either to ignore that, mostly to avoid losing a contributor (annoying him with requests he can't handle), or try to ask for that improvement. That often depends on how the contributor approaches the bug, to me you looked quite skilled, so I dared to ask.

Regarding the question if the pref should be invisible or not, we had at least a couple discussions in the team about these animations, and the result both times ended up being that we think they are an important part of the panel interaction, so it is not intended (so far) to give an easy toggle for everyone to change that interaction. The most feedback I saw about them being annoying went from advanced users, that unlikely have issues with an hidden pref. We could change our mind if in future we notice an increase of such requests.

::: browser/components/downloads/src/DownloadsCommon.jsm
@@ +98,4 @@
>    QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver,
>                                           Ci.nsISupportsWeakReference]),
> +  observe: function PO_observe(aSubject, aTopic, aData) {
> +    var pref = this.prefs[aData];

please use "let"

@@ +110,5 @@
> +  register: function PO_register(prefs) {
> +    this.prefs = prefs;
> +    kPrefBranch.addObserver("", this, true);
> +    for (let key in prefs) {
> +      let name = key;

having both name and pref.name is a bit confusing

@@ +128,5 @@
> +    getter: "getBoolPref"
> +  },
> +  animateNotifications: {
> +    name: "animateNotifications",
> +    getter: "getBoolPref"

I think would be cleaner as a simple pref -> default object like:

{
  debugEnabled: false,
  animateNotifications: true
}

typeof value would tell you which getter to use and you may return the default instead of requiring more complex type checks to avoid undefined.
Attachment #756571 - Flags: review?(mak77) → feedback+
Attached patch PatchSplinter Review
Attachment #756571 - Attachment is obsolete: true
Attachment #758973 - Flags: review?(paolo.mozmail)
Attachment #758973 - Flags: feedback?(mak77)
Comment on attachment 758973 [details] [diff] [review]
Patch

At this point, I think Marco may follow you better towards the end of the
review process. Thanks for asking anyways!
Attachment #758973 - Flags: review?(paolo.mozmail)
Attachment #758973 - Flags: review?(mak77)
Attachment #758973 - Flags: feedback?(mak77)
Comment on attachment 758973 [details] [diff] [review]
Patch

Review of attachment 758973 [details] [diff] [review]:
-----------------------------------------------------------------

I must admit I like how you code, I hope to see you fixing other bugs around :)

::: browser/components/downloads/src/DownloadsCommon.jsm
@@ +114,5 @@
> +  register: function PO_register(prefs) {
> +    this.prefs = prefs;
> +    kPrefBranch.addObserver("", this, true);
> +    for (let key in prefs) {
> +      let name = key;

nit: you may just do for (let name in prefs) and avoid the renaming of key to name
Attachment #758973 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [:mak] from comment #11)
> Comment on attachment 758973 [details] [diff] [review]
> Patch
> 
> Review of attachment 758973 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I must admit I like how you code, I hope to see you fixing other bugs around
> :)

I have been in working for some other bugs in fact ;)

> ::: browser/components/downloads/src/DownloadsCommon.jsm
> @@ +114,5 @@
> > +  register: function PO_register(prefs) {
> > +    this.prefs = prefs;
> > +    kPrefBranch.addObserver("", this, true);
> > +    for (let key in prefs) {
> > +      let name = key;
> 
> nit: you may just do for (let name in prefs) and avoid the renaming of key
> to name

The renaming is intended, or it will go wrong. The scope of let in for statement is not the block following it.
(In reply to Marco Bonardo [:mak] from comment #11)
> Comment on attachment 758973 [details] [diff] [review]
> Patch
> 
> Review of attachment 758973 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I must admit I like how you code, I hope to see you fixing other bugs around
> :)

Could you please commit this patch?
you can use the checkin-needed keyword for that
Keywords: checkin-needed
(In reply to Marco Bonardo [:mak] from comment #14)
> you can use the checkin-needed keyword for that

Thx for adding that. Since this bug have not been formally assigned to me, I cannot change that. And I thought that you should have the permission to commit it directly.
oops!
Assignee: nobody → quanxunzhen
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/e184e6fe6161
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Just tested using latest hourly build with this patch.
Added hidden pref: animateNotifications and set to 'false' and restarted browser

After downloading a file, the 'Green Animated Arrow' still appears when download is complete.  Am I missing the point of this patch ?

Win7 x64 m-c hourly build cset: 
https://hg.mozilla.org/mozilla-central/rev/9115d8b717e1
the pref is 'browser.download.animateNotifications', did you set this properly?
(In reply to Marco Bonardo [:mak] from comment #20)
> the pref is 'browser.download.animateNotifications', did you set this
> properly?

No, I just added the animateNotifications part - mis-read the patch when looking for the pref.

Set it per your post, and all is well... patch works fine.  Sorry for the noise.
(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #21)
> (In reply to Marco Bonardo [:mak] from comment #20)
> > the pref is 'browser.download.animateNotifications', did you set this
> > properly?
> 
> No, I just added the animateNotifications part - mis-read the patch when
> looking for the pref.
> 
> Set it per your post, and all is well... patch works fine.  Sorry for the
> noise.

We may need to add some doc for this perf.
We consider animations an important part of the downloads interaction, this is only intended for advanced users so far, we may change our mind if enough requests are reported in feedback.
But if you won't know about this bug, you won't know about this hidden feature. Even http://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=browser.download.animateNotifications&redirect=true shows noting. So I think 99,9% of advanced users still don't know about it.
Using about:config is already advanced thing, so I think we shouldn't hide any preferences there as it's only pain in the ass to use search service for searching how to change some hidden preference and how it's named.
(In reply to Virtual_ManPL [:Virtual] from comment #25)
> But if you won't know about this bug, you won't know about this hidden
> feature. Even
> http://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=browser.
> download.animateNotifications&redirect=true shows noting. So I think 99,9%
> of advanced users still don't know about it.
> Using about:config is already advanced thing, so I think we shouldn't hide
> any preferences there as it's only pain in the ass to use search service for
> searching how to change some hidden preference and how it's named.

I agree with you. Macro, please reconsider it.
maybe i can add this as a sumo article for turning this off?
that would give it way more attention
sorry for the triple posting but this would be good to be in the options panel since i see many people on the forums on it complaning about it, should i write this as another bug?
it's very unlikely we may consider adding it to the options panel.
Though if you have many sources with complains, and you can point us to them, we could evaluate unhiding the pref in about:config.
(In reply to Marco Bonardo [:mak] from comment #30)
> it's very unlikely we may consider adding it to the options panel.
> Though if you have many sources with complains, and you can point us to
> them, we could evaluate unhiding the pref in about:config.

okay some links, all since firefox 20 release (when new panel came)
* https://support.mozilla.org/en-US/questions/957595
* https://support.mozilla.org/en-US/questions/955403
* https://support.mozilla.org/en-US/questions/957613
* https://support.mozilla.org/en-US/questions/958509
any input.........
Please file a separate bug the unhide the pref, if you want to attach a patch, it should be quite simple to do.
While I don't think it's an important fix (I could barely find any feedback in input.mozilla.org), the cost is very low, it's more expensive to keep discussing about it than to just do that.
(In reply to Marco Bonardo [:mak] from comment #33)
> Please file a separate bug the unhide the pref [..]
Reported it here bug #897930

But don't you think we need maybe some new tab in options, where performance-hog options will be located?
For example I'm talking about these:
browser.download.animateNotifications=false
browser.tabs.animate=false
general.smoothScroll=false
They're the great help on my old dad PC to browse web with Firefox using many tabs. Without setting this options Firefox works slow due to hogging animations and my dad used Opera for web browsing.
(In reply to Virtual_ManPL [:Virtual] from comment #34)
> (In reply to Marco Bonardo [:mak] from comment #33)
> > Please file a separate bug the unhide the pref [..]
> Reported it here bug #897930
> 
> But don't you think we need maybe some new tab in options, where
> performance-hog options will be located?

That's out of scope for this. Generally we try to avoid exposing options for every single small details, otherwise you'd get tens of option panes and that would confuse most users.
Your idea to tweak preferences for a slow pc is interesting but doesn't work well as a core feature, we should just continue working on making Firefox faster as a whole. On the other side it would be easy to make a restart-less add-on for that, historically there have been various add-ons promising a faster firefox experience.
Blocks: 897930
No longer depends on: 897930
I dont find it essential to unhide the pref, as long as its there.
I think I may be getting half-consistent crashes over several FF releases now, related to this downloading animation https://crash-stats.mozilla.com/report/index/1fa18d10-3aef-4530-8b1e-445c02130904 being latest. This toggle is def. a welcome testing tool
You need to log in before you can comment on or make changes to this bug.