861613 - request for toggable download animation

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, enhancement)

Description

[Javran Cheng]

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.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Add a pref to toggle the animation

Comment 2

[:Paolo Amadini]

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).

Comment 3

[Marco Bonardo [:mak]]

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.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

(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.

Comment 5

[Marco Bonardo [:mak]]

(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?

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Add a pref to toggle the animation

(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.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

(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 8

[Marco Bonardo [:mak]]

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.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Patch

Comment 10

[:Paolo Amadini]

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!

Comment 11

[Marco Bonardo [:mak]]

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

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

(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.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

(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?

Comment 14

[Marco Bonardo [:mak]]

you can use the checkin-needed keyword for that

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

(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.

Comment 16

[Marco Bonardo [:mak]]

oops!

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e184e6fe6161

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e184e6fe6161

Comment 19

[Jim Jeffery not reading bug-mail 1/2/11]

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

Comment 20

[Marco Bonardo [:mak]]

the pref is 'browser.download.animateNotifications', did you set this properly?

Comment 21

[Jim Jeffery not reading bug-mail 1/2/11]

(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.

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

(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.

Comment 23

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Why this settings is hidden in about:config? Any ideas?

Comment 24

[Marco Bonardo [:mak]]

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.

Comment 25

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

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.

Comment 26

[Xidorn Quan [:xidorn] UTC+10]

(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.

Comment 27

[formerly wakaflocka]

maybe i can add this as a sumo article for turning this off?

Comment 28

[formerly wakaflocka]

that would give it way more attention

Comment 29

[formerly wakaflocka]

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?

Comment 30

[Marco Bonardo [:mak]]

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.

Comment 31

[formerly wakaflocka]

(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

Comment 32

[formerly wakaflocka]

any input.........

Comment 33

[Marco Bonardo [:mak]]

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.

Comment 34

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(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.

Comment 35

[Marco Bonardo [:mak]]

(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.

Comment 36

[formerly wakaflocka]

I dont find it essential to unhide the pref, as long as its there.

Comment 37

[Leho Kraav (:macmaN @lkraav)]

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