The default bug view has changed. See this FAQ.

Remove per-item download rate clutter

VERIFIED FIXED in Firefox 17

Status

()

Firefox
Downloads Panel
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: gcp, Assigned: mconley)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 17
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments, 22 obsolete attachments)

31.00 KB, image/png
Details
4.00 KB, patch
Gavin
: review+
mconley
: checkin+
Details | Diff | Splinter Review
48.99 KB, image/png
Details
23.51 KB, image/png
Details
33.15 KB, image/png
Details
35.25 KB, image/png
Details
44.87 KB, image/png
Details
51.07 KB, image/png
Details
27.66 KB, image/png
Details
33.65 KB, image/png
Details
21.09 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
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

5 years ago
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!
Blocks: 726447

Comment 3

5 years ago
Created attachment 630681 [details] [diff] [review]
The width of the Downloads Panel should be larger and locale-dependent.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #630681 - Flags: review?(mak77)
When will this patch and the others be pushed to m-c ?

Updated

5 years ago
Blocks: 767321
These new patchs are almost one month old. Why hasn't been any reviews yet ? They should pushed to central.

Updated

5 years ago
No longer blocks: 767321
Attachment #630681 - Flags: review?(mak77) → review+

Comment 6

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e29a85527782
Target Milestone: --- → Firefox 17
https://hg.mozilla.org/mozilla-central/rev/e29a85527782
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 8

5 years ago
Shouldn't it also be in FF16 (currently Aurora)?

Comment 9

5 years ago
(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.
(Reporter)

Comment 10

5 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 11

5 years ago
...and "xxx minutes, xxx seconds" is still just as broken, too.
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.

Updated

5 years ago
Duplicate of this bug: 747455
Can't we make this panel resizable? I think it will be the best idea, like it's in old Download panel.

Updated

5 years ago
Duplicate of this bug: 750298

Updated

5 years ago
Blocks: 747422
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.
Assignee: paolo.mozmail → mconley
Created attachment 672432 [details]
Windows 7 - Panel original size
Created attachment 672433 [details]
Windows 7 - Panel wider size
Comment on attachment 672433 [details]
Windows 7 - Panel wider size

Hey Stephen,

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

-Mike
Attachment #672433 - Flags: feedback?(shorlander)
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.
Attachment #630681 - Flags: checkin+
Created attachment 672467 [details]
Windows XP - Panel original size
Created attachment 672468 [details]
Windows XP - Panel wider size
Created attachment 672471 [details]
Ubuntu - Panel original size
Created attachment 672472 [details]
Ubuntu - Panel wider size
Created attachment 672481 [details] [diff] [review]
WIP Patch 2

checkpointing Windows and Ubuntu.
Attachment #672431 - Attachment is obsolete: true
Could you please address comment 12?
also, I find the new width a bit excessive, would 75 be enough?
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.
Attachment #672433 - Attachment is obsolete: true
Attachment #672468 - Attachment is obsolete: true
Attachment #672472 - Attachment is obsolete: true
Attachment #672481 - Attachment is obsolete: true
Attachment #672433 - Flags: feedback?(shorlander)
(Reporter)

Comment 29

5 years ago
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.
(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.
Created attachment 672887 [details] [diff] [review]
Patch v3
Attachment #672862 - Attachment is obsolete: true
Created attachment 672888 [details]
Windows 7 - Panel new size (Patch v3)
Created attachment 672889 [details]
Windows XP - Panel new size (Patch v3)
Created attachment 672893 [details]
OSX - Panel new size (Patch v3)
Created attachment 672898 [details]
Ubuntu - Panel new size (Patch v3)
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 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...
Attachment #672887 - Flags: feedback?(mak77)
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.
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.

Updated

5 years ago
Flags: needinfo?(shorlander)
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)
ehr, in comment 50 I meant progressbar, not scrollbar.
comment 40 not 50... sigh.

Updated

5 years ago
Duplicate of this bug: 786097
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.
Attachment #672887 - Flags: feedback?(mak77) → review+
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).
Attachment #672887 - Flags: review-
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.
(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?
(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.
Attachment #672887 - Attachment is obsolete: true
(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.
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.
Alrighty - can do.
Does this mean that each entry will not have its own download speed indicated along it ?
(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.
Created attachment 677035 [details] [diff] [review]
Part 1: Remove download rate from download item (but keep it in tooltip)
Attachment #672888 - Attachment is obsolete: true
Attachment #672889 - Attachment is obsolete: true
Attachment #672893 - Attachment is obsolete: true
Attachment #672898 - Attachment is obsolete: true
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.
Attachment #677035 - Flags: review?(mak77)
Created attachment 677050 [details] [diff] [review]
Part 2: Update the list width, add locale commentary
Attachment #677035 - Attachment description: Remove download rate from download item (but keep it in tooltip) → Part 1: Remove download rate from download item (but keep it in tooltip)
Created attachment 677057 [details]
Thinner Panel on Ubuntu
(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.
So, what happens if you set the width on the details box, than on the richlistbox?
Created attachment 677104 [details] [diff] [review]
Move width attribute to details node

Checkpointing alternative patch that moves the width attribute to the details node.
Created attachment 677108 [details]
Ubuntu - having moved width attribute
Created attachment 677109 [details]
OSX - having moved width attribute
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.
Attachment #677035 - Flags: review?(mak77)
Created attachment 677114 [details]
Windows 7 - having moved width attribute
Created attachment 677116 [details]
Windows XP - having moved width attribute
Created attachment 677118 [details]
Ubuntu - having moved width attribute
Attachment #677108 - Attachment is obsolete: true
Created attachment 677119 [details]
OSX - having moved width attribute
Attachment #677109 - Attachment is obsolete: true
Created attachment 677122 [details]
Ubuntu - having moved width attribute - no rate
Created attachment 677123 [details]
OSX - having moved width attribute - no rate
Created attachment 677124 [details]
Windows XP - having moved width attribute - no rate
Created attachment 677125 [details]
Windows 7 - having moved width attribute - no rate
The moved attribute + no rate looks really good.
Attachment #677114 - Attachment is obsolete: true
Attachment #677116 - Attachment is obsolete: true
Attachment #677118 - Attachment is obsolete: true
Attachment #677119 - Attachment is obsolete: true
Attachment #677050 - Attachment is obsolete: true
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.
Attachment #677035 - Attachment is obsolete: true
Attachment #677104 - Attachment is obsolete: true
Attachment #677458 - Flags: review?(mak77)

Comment 74

4 years ago
(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?
(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.
s/"download rate per second"/"download rate".  Sorry for the redundancy. :)

Comment 77

4 years ago
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 :)
(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 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).
Attachment #677458 - Flags: review?(mak77) → review-
I think we discussed enough on irc and comments about this, clearing the request, we can iterate later.
Flags: needinfo?(shorlander)
(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!
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.
Attachment #677458 - Attachment is obsolete: true
Attachment #678765 - Flags: review?(mak77)
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
Attachment #678765 - Flags: review?(mak77) → review+
Created attachment 678794 [details] [diff] [review]
Patch v3 (r+'d by mak)

Good catches all around. Thanks Marco!
Attachment #678765 - Attachment is obsolete: true
Summary: New Download panel cuts off download speed text → Remove per-item download rate clutter
Landed on mozilla-inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/bd648e7c2656
https://hg.mozilla.org/mozilla-central/rev/bd648e7c2656
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago4 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
Duplicate of this bug: 858999

Comment 89

4 years ago
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 :(
(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.
(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?
bug 812894
Also see http://blog.bonardo.net/2013/04/30/new-download-experience-post-mortem

Updated

3 years ago
Duplicate of this bug: 986184
You need to log in before you can comment on or make changes to this bug.