Last Comment Bug 759397 - Remove per-item download rate clutter
: Remove per-item download rate clutter
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: Downloads Panel (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal with 1 vote (vote)
: Firefox 17
Assigned To: Mike Conley (:mconley) - (needinfo me!)
:
:
Mentors:
: 747455 750298 786097 858999 (view as bug list)
Depends on:
Blocks: 726447 ReleaseDownloadsPane
  Show dependency treegraph
 
Reported: 2012-05-29 10:41 PDT by Gian-Carlo Pascutto [:gcp]
Modified: 2014-03-21 07:14 PDT (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Truncated display (31.00 KB, image/png)
2012-05-29 10:41 PDT, Gian-Carlo Pascutto [:gcp]
no flags Details
The width of the Downloads Panel should be larger and locale-dependent. (4.00 KB, patch)
2012-06-06 13:03 PDT, :Paolo Amadini
gavin.sharp: review+
mconley: checkin+
Details | Diff | Splinter Review
WIP Patch 1 (1.83 KB, patch)
2012-10-17 11:52 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Splinter Review
Windows 7 - Panel original size (48.99 KB, image/png)
2012-10-17 11:53 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Windows 7 - Panel wider size (32.33 KB, image/png)
2012-10-17 11:54 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Windows XP - Panel original size (23.51 KB, image/png)
2012-10-17 12:47 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Windows XP - Panel wider size (13.74 KB, image/png)
2012-10-17 12:48 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Ubuntu - Panel original size (33.15 KB, image/png)
2012-10-17 12:49 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Ubuntu - Panel wider size (42.49 KB, image/png)
2012-10-17 12:50 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
WIP Patch 2 (2.52 KB, patch)
2012-10-17 12:57 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Splinter Review
Patch v2 (19 bytes, patch)
2012-10-18 11:10 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Splinter Review
Patch v3 (1.60 KB, patch)
2012-10-18 11:49 PDT, Mike Conley (:mconley) - (needinfo me!)
mak77: review+
dao+bmo: review-
Details | Diff | Splinter Review
Windows 7 - Panel new size (Patch v3) (60.09 KB, image/png)
2012-10-18 11:50 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Windows XP - Panel new size (Patch v3) (16.62 KB, image/png)
2012-10-18 11:50 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
OSX - Panel new size (Patch v3) (54.17 KB, image/png)
2012-10-18 12:00 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Ubuntu - Panel new size (Patch v3) (53.18 KB, image/png)
2012-10-18 12:08 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Part 1: Remove download rate from download item (but keep it in tooltip) (8.26 KB, patch)
2012-10-31 09:14 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Splinter Review
Part 2: Update the list width, add locale commentary (2.65 KB, patch)
2012-10-31 10:07 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Splinter Review
Thinner Panel on Ubuntu (35.25 KB, image/png)
2012-10-31 10:22 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Move width attribute to details node (3.18 KB, patch)
2012-10-31 12:01 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Splinter Review
Ubuntu - having moved width attribute (36.84 KB, image/png)
2012-10-31 12:08 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
OSX - having moved width attribute (44.41 KB, image/png)
2012-10-31 12:09 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Windows 7 - having moved width attribute (27.79 KB, image/png)
2012-10-31 12:23 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Windows XP - having moved width attribute (20.65 KB, image/png)
2012-10-31 12:25 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Ubuntu - having moved width attribute (53.23 KB, image/png)
2012-10-31 12:26 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
OSX - having moved width attribute (47.41 KB, image/png)
2012-10-31 12:26 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Ubuntu - having moved width attribute - no rate (44.87 KB, image/png)
2012-10-31 12:32 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
OSX - having moved width attribute - no rate (51.07 KB, image/png)
2012-10-31 12:33 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Windows XP - having moved width attribute - no rate (27.66 KB, image/png)
2012-10-31 12:34 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Windows 7 - having moved width attribute - no rate (33.65 KB, image/png)
2012-10-31 12:34 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Patch v1 (11.88 KB, patch)
2012-11-01 09:53 PDT, Mike Conley (:mconley) - (needinfo me!)
mak77: review-
Details | Diff | Splinter Review
Patch v2 (21.10 KB, patch)
2012-11-06 08:31 PST, Mike Conley (:mconley) - (needinfo me!)
mak77: review+
Details | Diff | Splinter Review
Patch v3 (r+'d by mak) (21.09 KB, patch)
2012-11-06 10:13 PST, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Splinter Review

Description Gian-Carlo Pascutto [:gcp] 2012-05-29 10:41:40 PDT
Created attachment 628019 [details]
Truncated display

The download panels often cuts off the last part of its text. See attached screenshot. This is a "standard" Windows 7 install with default font sizes.
Comment 1 Gian-Carlo Pascutto [:gcp] 2012-05-29 10:42:55 PDT
Note that asides from looking quite silly and unprofessional, there doesn't appear to be an easily discoverable way to resize the panel, so the user can't fix this.
Comment 2 :Paolo Amadini 2012-05-29 10:57:01 PDT
Sure, we're aware of the issue, that will be fixed in an imminent visual refresh.

That's already listed in bug 726447 comment 0 but I'll leave this bug open to
make life easier to other people trying to see if the same issue has been
already reported. So, thanks for filing!
Comment 3 :Paolo Amadini 2012-06-06 13:03:02 PDT
Created attachment 630681 [details] [diff] [review]
The width of the Downloads Panel should be larger and locale-dependent.
Comment 4 Guillaume C. [:ge3k0s] 2012-06-14 06:33:19 PDT
When will this patch and the others be pushed to m-c ?
Comment 5 Guillaume C. [:ge3k0s] 2012-07-04 05:30:16 PDT
These new patchs are almost one month old. Why hasn't been any reviews yet ? They should pushed to central.
Comment 7 Ed Morley [:emorley] 2012-07-18 05:56:27 PDT
https://hg.mozilla.org/mozilla-central/rev/e29a85527782
Comment 8 Eugene Savitsky 2012-07-21 07:03:47 PDT
Shouldn't it also be in FF16 (currently Aurora)?
Comment 9 :Paolo Amadini 2012-07-23 01:42:44 PDT
(In reply to Eugene Savitsky from comment #8)
> Shouldn't it also be in FF16 (currently Aurora)?

The panel should be disabled in the next Aurora build because it's not ready yet,
thus we're not planning to uplift the fixes we'll do in the following days.
Comment 10 Gian-Carlo Pascutto [:gcp] 2012-07-23 22:26:42 PDT
This is still broken for downloads that take >1 hour. The problem is that the resizing is now OK for "xxx minutes" and all that follows, but the text "xxx hours, xxx minutes" is longer.
Comment 11 Gian-Carlo Pascutto [:gcp] 2012-07-23 22:51:06 PDT
...and "xxx minutes, xxx seconds" is still just as broken, too.
Comment 12 Marco Bonardo [::mak] 2012-07-24 03:29:24 PDT
in the localization note we should improve the note to tell what the longest text may look like, with an example. IF we can't make it right in English I see an hard time for localizers.
Comment 13 Marco Bonardo [::mak] 2012-09-14 09:58:35 PDT
*** Bug 747455 has been marked as a duplicate of this bug. ***
Comment 14 Virtual_ManPL [:Virtual] - (ni? me) 2012-09-14 11:11:52 PDT
Can't we make this panel resizable? I think it will be the best idea, like it's in old Download panel.
Comment 15 Marco Bonardo [::mak] 2012-10-15 12:04:05 PDT
*** Bug 750298 has been marked as a duplicate of this bug. ***
Comment 16 Mike Conley (:mconley) - (needinfo me!) 2012-10-17 11:52:55 PDT
Created attachment 672431 [details] [diff] [review]
WIP Patch 1

This patch widens the panel on Windows, and shrinks each download item vertically.

Not sure if this is how we want to fly, but I'm going for it until I hear otherwise.

Screenshots coming.
Comment 17 Mike Conley (:mconley) - (needinfo me!) 2012-10-17 11:53:35 PDT
Created attachment 672432 [details]
Windows 7 - Panel original size
Comment 18 Mike Conley (:mconley) - (needinfo me!) 2012-10-17 11:54:00 PDT
Created attachment 672433 [details]
Windows 7 - Panel wider size
Comment 19 Mike Conley (:mconley) - (needinfo me!) 2012-10-17 11:55:11 PDT
Comment on attachment 672433 [details]
Windows 7 - Panel wider size

Hey Stephen,

Just curious to know what you think about these new dimensions.

-Mike
Comment 20 Mike Conley (:mconley) - (needinfo me!) 2012-10-17 12:01:10 PDT
Comment on attachment 630681 [details] [diff] [review]
The width of the Downloads Panel should be larger and locale-dependent.

This patch has already been checked in.
Comment 21 Mike Conley (:mconley) - (needinfo me!) 2012-10-17 12:47:54 PDT
Created attachment 672467 [details]
Windows XP - Panel original size
Comment 22 Mike Conley (:mconley) - (needinfo me!) 2012-10-17 12:48:14 PDT
Created attachment 672468 [details]
Windows XP - Panel wider size
Comment 23 Mike Conley (:mconley) - (needinfo me!) 2012-10-17 12:49:43 PDT
Created attachment 672471 [details]
Ubuntu - Panel original size
Comment 24 Mike Conley (:mconley) - (needinfo me!) 2012-10-17 12:50:24 PDT
Created attachment 672472 [details]
Ubuntu - Panel wider size
Comment 25 Mike Conley (:mconley) - (needinfo me!) 2012-10-17 12:57:42 PDT
Created attachment 672481 [details] [diff] [review]
WIP Patch 2

checkpointing Windows and Ubuntu.
Comment 26 Marco Bonardo [::mak] 2012-10-18 06:03:46 PDT
Could you please address comment 12?
Comment 27 Marco Bonardo [::mak] 2012-10-18 06:05:53 PDT
also, I find the new width a bit excessive, would 75 be enough?
Comment 28 Mike Conley (:mconley) - (needinfo me!) 2012-10-18 11:10:29 PDT
Created attachment 672862 [details] [diff] [review]
Patch v2

Paolo - you're right - 75ch seemed to do the job just fine.  The longest string I could come up with in English was 65 characters long:

23 hours, 59 minutes remaining — 300 of 695 MB (999.4 bytes/sec)

So the added 10ch seemed to be what it took to not auto-truncate.

I've added a localization comment, as you've suggested.
Comment 29 Gian-Carlo Pascutto [:gcp] 2012-10-18 11:20:26 PDT
23 hours, 59 minutes vs
4 minutes, 59 seconds is one character longer

I hope you have headroom for that one more :-) I presume we don't display seconds when minutes > 10.
Comment 30 Mike Conley (:mconley) - (needinfo me!) 2012-10-18 11:36:15 PDT
(In reply to Gian-Carlo Pascutto (:gcp) from comment #29)
> 23 hours, 59 minutes vs
> 4 minutes, 59 seconds is one character longer
> 
> I hope you have headroom for that one more :-) I presume we don't display
> seconds when minutes > 10.

Hm, I can do you one better:

59 minutes, 59 seconds remaining — 1022 of 1023 KB (999.4 bytes/sec)

That's 69 characters, so I've had to bump the width to 77ch. New patch coming.
Comment 31 Mike Conley (:mconley) - (needinfo me!) 2012-10-18 11:49:37 PDT
Created attachment 672887 [details] [diff] [review]
Patch v3
Comment 32 Mike Conley (:mconley) - (needinfo me!) 2012-10-18 11:50:17 PDT
Created attachment 672888 [details]
Windows 7 - Panel new size (Patch v3)
Comment 33 Mike Conley (:mconley) - (needinfo me!) 2012-10-18 11:50:36 PDT
Created attachment 672889 [details]
Windows XP - Panel new size (Patch v3)
Comment 34 Mike Conley (:mconley) - (needinfo me!) 2012-10-18 12:00:47 PDT
Created attachment 672893 [details]
OSX - Panel new size (Patch v3)
Comment 35 Mike Conley (:mconley) - (needinfo me!) 2012-10-18 12:08:17 PDT
Created attachment 672898 [details]
Ubuntu - Panel new size (Patch v3)
Comment 36 Mike Conley (:mconley) - (needinfo me!) 2012-10-18 12:10:19 PDT
It's strange how 77ch seems to be a lot for all OS's, except for Windows 7. That's the squeaky wheel here.

Ubuntu, for example, can hold the long string with the original 65ch without a problem.
Comment 37 Mike Conley (:mconley) - (needinfo me!) 2012-10-18 12:13:21 PDT
Comment on attachment 672887 [details] [diff] [review]
Patch v3

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

Any idea why Windows 7 is strangely "thin" with 77ch?

I wonder if perhaps it'd be better to special-case Windows 7 to make the panel wider, and leave the 65ch where it is for everybody else...
Comment 38 Marco Bonardo [::mak] 2012-10-22 07:04:41 PDT
Hm, so 'ch' is defined as "Equal to the advance measure of the "0" (ZERO, U+0030) glyph found in the font used to render it.", so my guess is that the win font family has small numbers, others have larger.
I don't think we can take a default that is smaller than the actual number of chars we may have there, since the user may change the font globally in the OS and I suppose we'd inherit the system choice.
Comment 39 Marco Bonardo [::mak] 2012-10-22 07:05:56 PDT
The laternative, if we want a smaller panel, could be to split the string to 2 lines... maybe we should ask Stephen if that'd be acceptable from a ux point of view.
Comment 40 Marco Bonardo [::mak] 2012-10-22 07:12:59 PDT
That would clearly mean we should change padding and the scrollbar to avoid having too tall items... I wonder if if we could come with a better scrollbar design, like filling item background (just a crazy idea but I think I saw this on some mobile device recently)
Comment 41 Marco Bonardo [::mak] 2012-10-22 07:13:18 PDT
ehr, in comment 50 I meant progressbar, not scrollbar.
Comment 42 Marco Bonardo [::mak] 2012-10-22 07:13:41 PDT
comment 40 not 50... sigh.
Comment 43 Marco Bonardo [::mak] 2012-10-23 13:16:12 PDT
*** Bug 786097 has been marked as a duplicate of this bug. ***
Comment 44 Marco Bonardo [::mak] 2012-10-26 07:37:28 PDT
Comment on attachment 672887 [details] [diff] [review]
Patch v3

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

code-wise, this is fine. We just need design directions but there's nothing more I have to see in this patch as of now.
Comment 45 Dão Gottwald [:dao] 2012-10-29 03:53:58 PDT
Comment on attachment 672887 [details] [diff] [review]
Patch v3

Most locales likely got this wrong when the string landed originally, so you need to change the entity name (e.g. downloadPanel.width).
Comment 46 Marco Bonardo [::mak] 2012-10-29 06:16:37 PDT
I suspect locales got this right by actually seeing if the string were cut since they are used to do such testing, we didn't. But yeah renaming may help seeing the updated note.
Btw, we are likely to remove some text than increasing the width.
Comment 47 Dão Gottwald [:dao] 2012-10-29 07:11:55 PDT
(In reply to Marco Bonardo [:mak] from comment #46)
> I suspect locales got this right by actually seeing if the string were cut
> since they are used to do such testing, we didn't. But yeah renaming may
> help seeing the updated note.

Most just copied en-US, apparently.
http://mxr.mozilla.org/l10n-central/search?string=downloads.width

> Btw, we are likely to remove some text than increasing the width.

So should we hold off landing this, to avoid requiring futile work from localizers?
Comment 48 Mike Conley (:mconley) - (needinfo me!) 2012-10-29 07:13:08 PDT
(In reply to Dão Gottwald [:dao] from comment #47)
> So should we hold off landing this, to avoid requiring futile work from
> localizers?

Yes, I think that would be prudent.
Comment 49 Marco Bonardo [::mak] 2012-10-29 07:13:39 PDT
(In reply to Dão Gottwald [:dao] from comment #47)
> > Btw, we are likely to remove some text than increasing the width.
> 
> So should we hold off landing this, to avoid requiring futile work from
> localizers?

Yes, this should not land until we get a finalized design from UX, Stephen working on that.
Comment 50 Marco Bonardo [::mak] 2012-10-31 07:25:49 PDT
So, what about we start by removing the download speed from single items, calculate the new max width and set it (revving the entity name).  Then we file a follow-up to figure out where to put a global download speed indicator, in case we still consider it a UX requirement. We seemed to somehow agree about this solution on IRC and it seems like an improvement overall.
Comment 51 Mike Conley (:mconley) - (needinfo me!) 2012-10-31 07:30:10 PDT
Alrighty - can do.
Comment 52 Girish Sharma [:Optimizer] 2012-10-31 07:31:27 PDT
Does this mean that each entry will not have its own download speed indicated along it ?
Comment 53 Marco Bonardo [::mak] 2012-10-31 07:42:29 PDT
(In reply to Girish Sharma [:Optimizer] from comment #52)
> Does this mean that each entry will not have its own download speed
> indicated along it ?

yes. Though they have a progressbar and the remaining time. Imo the only missing information will be an instant indicator of whether downloads are proceeding fast/slow, that is why we likely need at least a global speed indicator.
The panel has never been intended as a micromanagement ui, single items complete info will always be available in the manager.
Comment 54 Mike Conley (:mconley) - (needinfo me!) 2012-10-31 09:14:09 PDT
Created attachment 677035 [details] [diff] [review]
Part 1: Remove download rate from download item (but keep it in tooltip)
Comment 55 Mike Conley (:mconley) - (needinfo me!) 2012-10-31 09:15:21 PDT
Comment on attachment 677035 [details] [diff] [review]
Part 1: Remove download rate from download item (but keep it in tooltip)

So here's the first bit - going to try to recalculate the panel width now.
Comment 56 Mike Conley (:mconley) - (needinfo me!) 2012-10-31 10:07:21 PDT
Created attachment 677050 [details] [diff] [review]
Part 2: Update the list width, add locale commentary
Comment 57 Mike Conley (:mconley) - (needinfo me!) 2012-10-31 10:22:53 PDT
Created attachment 677057 [details]
Thinner Panel on Ubuntu
Comment 58 Marco Bonardo [::mak] 2012-10-31 10:40:28 PDT
(In reply to Mike Conley (:mconley) from comment #57)
> Created attachment 677057 [details]
> Thinner Panel on Ubuntu

Now that I see this screenshot, is the width in ch relative to the filename font size or the details? There is quite a font-size difference here, much more than on other platforms, that may better explain why it looks larger.
Comment 59 Marco Bonardo [::mak] 2012-10-31 10:45:25 PDT
So, what happens if you set the width on the details box, than on the richlistbox?
Comment 60 Mike Conley (:mconley) - (needinfo me!) 2012-10-31 12:01:43 PDT
Created attachment 677104 [details] [diff] [review]
Move width attribute to details node

Checkpointing alternative patch that moves the width attribute to the details node.
Comment 61 Mike Conley (:mconley) - (needinfo me!) 2012-10-31 12:08:51 PDT
Created attachment 677108 [details]
Ubuntu - having moved width attribute
Comment 62 Mike Conley (:mconley) - (needinfo me!) 2012-10-31 12:09:12 PDT
Created attachment 677109 [details]
OSX - having moved width attribute
Comment 63 Mike Conley (:mconley) - (needinfo me!) 2012-10-31 12:09:38 PDT
Comment on attachment 677035 [details] [diff] [review]
Part 1: Remove download rate from download item (but keep it in tooltip)

Withdrawing review request until we settle on how to move forward.
Comment 64 Mike Conley (:mconley) - (needinfo me!) 2012-10-31 12:23:24 PDT
Created attachment 677114 [details]
Windows 7 - having moved width attribute
Comment 65 Mike Conley (:mconley) - (needinfo me!) 2012-10-31 12:25:02 PDT
Created attachment 677116 [details]
Windows XP - having moved width attribute
Comment 66 Mike Conley (:mconley) - (needinfo me!) 2012-10-31 12:26:10 PDT
Created attachment 677118 [details]
Ubuntu - having moved width attribute
Comment 67 Mike Conley (:mconley) - (needinfo me!) 2012-10-31 12:26:33 PDT
Created attachment 677119 [details]
OSX - having moved width attribute
Comment 68 Mike Conley (:mconley) - (needinfo me!) 2012-10-31 12:32:58 PDT
Created attachment 677122 [details]
Ubuntu - having moved width attribute - no rate
Comment 69 Mike Conley (:mconley) - (needinfo me!) 2012-10-31 12:33:23 PDT
Created attachment 677123 [details]
OSX - having moved width attribute - no rate
Comment 70 Mike Conley (:mconley) - (needinfo me!) 2012-10-31 12:34:10 PDT
Created attachment 677124 [details]
Windows XP - having moved width attribute - no rate
Comment 71 Mike Conley (:mconley) - (needinfo me!) 2012-10-31 12:34:38 PDT
Created attachment 677125 [details]
Windows 7 - having moved width attribute - no rate
Comment 72 Marco Bonardo [::mak] 2012-11-01 04:14:28 PDT
The moved attribute + no rate looks really good.
Comment 73 Mike Conley (:mconley) - (needinfo me!) 2012-11-01 09:53:42 PDT
Created attachment 677458 [details] [diff] [review]
Patch v1

We remove the individual download rates (but keep them in the tooltips), and we move the width attribute to the details node, setting it to 49ch for the en-US locale, which seems sufficient for our longest strings.
Comment 74 Roman R. 2012-11-02 08:43:31 PDT
(In reply to Mike Conley (:mconley) from comment #73)
> We remove the individual download rates (but keep them in the tooltips)

That's a horrible decision! If I have several files being downloaded (which I do regularly), I will have to work the mouse a lot to see what I want to know (the download rates), instead of just glancing at the download box. Why do you think that forcing users to do more work to get what they want is better than making the box wider? Do you have a width deficit or what?
Comment 75 Mike Conley (:mconley) - (needinfo me!) 2012-11-02 11:56:15 PDT
(In reply to Roman R. from comment #74)
> (In reply to Mike Conley (:mconley) from comment #73)
> > We remove the individual download rates (but keep them in the tooltips)
> 
> That's a horrible decision! If I have several files being downloaded (which
> I do regularly), I will have to work the mouse a lot to see what I want to
> know (the download rates), instead of just glancing at the download box. Why
> do you think that forcing users to do more work to get what they want is
> better than making the box wider? Do you have a width deficit or what?

I would argue that the time remaining is of more use to the average user than the download rate per second. Removing the rate reduces the noise in the panel, and keeps it minimal - remember, the idea behind the panel is to be a very light and quick visualization of recent downloads.

Information like the rate of download is, IMO, better suited for the Downloads Manager (being implemented in bug 675902).

My recommendation would be to simply use that window to monitor the rate of ongoing downloads if this is of particular interest to you.
Comment 76 Mike Conley (:mconley) - (needinfo me!) 2012-11-02 11:56:51 PDT
s/"download rate per second"/"download rate".  Sorry for the redundancy. :)
Comment 77 Roman R. 2012-11-04 07:32:55 PST
Ah, yes, I forgot about the Downloads Manager. Then this panel will become a pretty little thing that has too little value to allow it to occupy space :)
Comment 78 Marco Bonardo [::mak] 2012-11-05 02:06:31 PST
(In reply to Roman R. from comment #77)
> Ah, yes, I forgot about the Downloads Manager. Then this panel will become a
> pretty little thing that has too little value to allow it to occupy space :)

That's why you'll be able to remove it from the toolbar.  Most users will find it useful, customization is there for a reason.
Comment 79 Marco Bonardo [::mak] 2012-11-05 11:42:02 PST
Comment on attachment 677458 [details] [diff] [review]
Patch v1

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

::: browser/components/downloads/content/downloads.js
@@ +921,5 @@
>                                              DownloadsCommon.strings.statePaused,
>                                              transfer);
>      } else if (this.dataItem.state == nsIDM.DOWNLOAD_DOWNLOADING) {
> +      // We don't want to display the rate of download in the download item
> +      // description, because it can get easily truncated.

This comment reads as "we don't know how to fix this bug so we remove the feature", that is untrue (we may indeed just make enough space). It should rather explain we want to reduce clutter and have a slim panel design, since we already show the remaining time we can avoid rates for each entry.

And a new bug to investigate a global rate indicator should be filed.

@@ +928,5 @@
> +                                              this.dataItem.maxBytes,
> +                                              this.dataItem.speed,
> +                                              this.lastEstimatedSecondsLeft);
> +
> +      // We are, however, OK with displaying the rate in the tooltip.

I'm starting thinking this difference will be confusing to some users, but let's try.

::: browser/locales/en-US/chrome/browser/downloads/downloads.dtd
@@ +15,5 @@
>  <!-- LOCALIZATION NOTE (downloads.width):
> +     Width of details for a Downloads Panel item (which directly influences the
> +     width of the Downloads Panel) expressed using a CSS unit. The longest
> +     labels that should fit in the item width are usually those of in-progress
> +     downloads and those of blocked downloads.

Where did go the nice comment you added previously to guess the maximum size? We still want that.

Also the LOCALIZATION NOTE is pointing to the old wrong entity name

::: toolkit/mozapps/downloads/DownloadUtils.jsm
@@ +437,5 @@
>     */
>    convertByteUnits: function DU_convertByteUnits(aBytes)
>    {
> +    if (!aBytes)
> +      aBytes = -1;

Is this change actually needed?
It is removing what looks like a valid 0 case, changing an interface (this is a pretty much used helper across the codebase) and doesn't look part of the fix for this bug (if it's actually a bug should be filed apart).
Comment 80 Marco Bonardo [::mak] 2012-11-06 07:26:58 PST
I think we discussed enough on irc and comments about this, clearing the request, we can iterate later.
Comment 81 Mike Conley (:mconley) - (needinfo me!) 2012-11-06 07:43:53 PST
(In reply to Marco Bonardo [:mak] from comment #79)
>
> And a new bug to investigate a global rate indicator should be filed.
> 

The *might* land within the territory of bug 808277.


> ::: toolkit/mozapps/downloads/DownloadUtils.jsm
> @@ +437,5 @@
> >     */
> >    convertByteUnits: function DU_convertByteUnits(aBytes)
> >    {
> > +    if (!aBytes)
> > +      aBytes = -1;
> 
> Is this change actually needed?
> It is removing what looks like a valid 0 case, changing an interface (this
> is a pretty much used helper across the codebase) and doesn't look part of
> the fix for this bug (if it's actually a bug should be filed apart).

You're right - I may have been a little careless here. Not sure what I was thinking. Thanks for spotting it!
Comment 82 Mike Conley (:mconley) - (needinfo me!) 2012-11-06 08:31:31 PST
Created attachment 678765 [details] [diff] [review]
Patch v2

(In reply to Marco Bonardo [:mak] from comment #79)
> Comment on attachment 677458 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 677458 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/downloads/content/downloads.js
> @@ +921,5 @@
> >                                              DownloadsCommon.strings.statePaused,
> >                                              transfer);
> >      } else if (this.dataItem.state == nsIDM.DOWNLOAD_DOWNLOADING) {
> > +      // We don't want to display the rate of download in the download item
> > +      // description, because it can get easily truncated.
> 
> This comment reads as "we don't know how to fix this bug so we remove the
> feature", that is untrue (we may indeed just make enough space). It should
> rather explain we want to reduce clutter and have a slim panel design, since
> we already show the remaining time we can avoid rates for each entry.

Fixed.

> 
> I'm starting thinking this difference will be confusing to some users, but
> let's try.

Yeah - switching over is trivial.

> 
> ::: browser/locales/en-US/chrome/browser/downloads/downloads.dtd
> @@ +15,5 @@
> >  <!-- LOCALIZATION NOTE (downloads.width):
> > +     Width of details for a Downloads Panel item (which directly influences the
> > +     width of the Downloads Panel) expressed using a CSS unit. The longest
> > +     labels that should fit in the item width are usually those of in-progress
> > +     downloads and those of blocked downloads.
> 
> Where did go the nice comment you added previously to guess the maximum
> size? We still want that.

Ah, fixed. Also, I've increased the width slightly from 49ch to 51ch. I did a little bit of tuning after I got the 51 character maximum, but I realized that this tuning was based upon a static string. I thought I'd err on the side of caution, and just keep the 51 character width.

> 
> Also the LOCALIZATION NOTE is pointing to the old wrong entity name
> 

Whoops - fixed.


I've also modified the DownloadUtils.jsm tests to test my new function.
Comment 83 Marco Bonardo [::mak] 2012-11-06 10:02:32 PST
Comment on attachment 678765 [details] [diff] [review]
Patch v2

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

::: browser/locales/en-US/chrome/browser/downloads/downloads.dtd
@@ +30,2 @@
>       -->
> +<!ENTITY downloadDetails.width            "51ch">

The string "59 minutes, 59 seconds remaining - 1022 of 1023 KB" is 50 chars, not 51. I may even be fine adding 1 char for safety, but the comment should be correct.

::: toolkit/mozapps/downloads/DownloadUtils.jsm
@@ +107,5 @@
> +    let [transfer, timeLeft, newLast]
> +      = this._deriveTransferRate(aCurrBytes, aMaxBytes, aSpeed, aLastSec);
> +
> +    if (aSpeed == null)
> +      aSpeed = -1;

what about making _deriveTransferRate also return normalizedSpeed as 4th item, so arguments handling is all there?

@@ +134,5 @@
> +   * @return A pair: [download status text, new value of "last seconds"]
> +   */
> +  getDownloadStatusNoRate:
> +  function DU_getDownloadStatusNoRate(aCurrBytes, aMaxBytes, aSpeed,
> +                                      aLastSec) {

this file seems to use bracing on a new line for methods, so please stick to this code style

@@ +160,5 @@
> +   *                    new value of "last seconds"]
> +   */
> +  _deriveTransferRate: function DU__deriveTransferRate(aCurrBytes,
> +                                                       aMaxBytes, aSpeed,
> +                                                       aLastSec) {

ditto on bracing
Comment 84 Mike Conley (:mconley) - (needinfo me!) 2012-11-06 10:13:49 PST
Created attachment 678794 [details] [diff] [review]
Patch v3 (r+'d by mak)

Good catches all around. Thanks Marco!
Comment 85 Mike Conley (:mconley) - (needinfo me!) 2012-11-06 10:22:54 PST
Landed on mozilla-inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/bd648e7c2656
Comment 86 Ryan VanderMeulen [:RyanVM] 2012-11-06 18:21:57 PST
https://hg.mozilla.org/mozilla-central/rev/bd648e7c2656
Comment 87 Simona B [:simonab ] 2012-11-12 09:15:00 PST
Verified as fixed on the latest Nightly - the download rate per second is now removed from each item and is shown only in the tooltip. 

Verified on Windows 7, Ubuntu 12.04 and Mac OS X 10.7:

Mozilla/5.0 (Windows NT 6.1; rv:19.0) Gecko/19.0 Firefox/19.0        
Build ID: 20121111030749
Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/19.0 Firefox/19.0        
Build ID: 20121112030753
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:19.0) Gecko/19.0 Firefox/19.0        
Build ID: 20121112030753
Comment 88 Matthias Versen [:Matti] 2013-04-07 14:14:53 PDT
*** Bug 858999 has been marked as a duplicate of this bug. ***
Comment 89 Roman Tomjak 2013-07-01 17:48:13 PDT
This is a horrible fix. I was looking at the source for a way to implement per item download speeds only to find out that current behaviour was implemented in order to NOT to show per item download speeds. Kind of looses all of the magic for quick-peek on downloads - if I have to seperately check the tooltips :(
Comment 90 Marco Bonardo [::mak] 2013-07-02 08:05:55 PDT
(In reply to Roman Tomjak from comment #89)
> This is a horrible fix. I was looking at the source for a way to implement
> per item download speeds only to find out that current behaviour was
> implemented in order to NOT to show per item download speeds. Kind of looses
> all of the magic for quick-peek on downloads - if I have to seperately check
> the tooltips :(

we will reintroduce the speed once we have a new more compact design for the panel, no worries.
Comment 91 Dão Gottwald [:dao] 2013-07-02 10:20:30 PDT
(In reply to Marco Bonardo [:mak] from comment #90)
> we will reintroduce the speed once we have a new more compact design for the
> panel, no worries.

Is there already a bug tracking that?
Comment 93 YF (Yang) 2014-03-21 07:14:08 PDT
*** Bug 986184 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.