759397 - Remove per-item download rate clutter

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Gian-Carlo Pascutto [:gcp]]

Attachment: 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]]

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]

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]

Attachment: The width of the Downloads Panel should be larger and locale-dependent.

Comment 4

[u428464]

When will this patch and the others be pushed to m-c ?

Comment 5

[u428464]

These new patchs are almost one month old. Why hasn't been any reviews yet ? They should pushed to central.

Comment 6

[:Paolo Amadini]

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

Comment 7

[Ed Morley [:emorley]]

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

Comment 8

[Eugene Savitsky]

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

Comment 9

[:Paolo Amadini]

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

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]]

...and "xxx minutes, xxx seconds" is still just as broken, too.

Comment 12

[Marco Bonardo [:mak]]

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 14

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

Can't we make this panel resizable? I think it will be the best idea, like it's in old Download panel.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Attachment: 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) (:⚙️)]

Attachment: Windows 7 - Panel original size

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Attachment: Windows 7 - Panel wider size

Comment 19

[Mike Conley (:mconley) (:⚙️)]

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) (:⚙️)]

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) (:⚙️)]

Attachment: Windows XP - Panel original size

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Attachment: Windows XP - Panel wider size

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Attachment: Ubuntu - Panel original size

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Attachment: Ubuntu - Panel wider size

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 2

checkpointing Windows and Ubuntu.

Comment 26

[Marco Bonardo [:mak]]

Could you please address comment 12?

Comment 27

[Marco Bonardo [:mak]]

also, I find the new width a bit excessive, would 75 be enough?

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Attachment: 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]]

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) (:⚙️)]

(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) (:⚙️)]

Attachment: Patch v3

Comment 32

[Mike Conley (:mconley) (:⚙️)]

Attachment: Windows 7 - Panel new size (Patch v3)

Comment 33

[Mike Conley (:mconley) (:⚙️)]

Attachment: Windows XP - Panel new size (Patch v3)

Comment 34

[Mike Conley (:mconley) (:⚙️)]

Attachment: OSX - Panel new size (Patch v3)

Comment 35

[Mike Conley (:mconley) (:⚙️)]

Attachment: Ubuntu - Panel new size (Patch v3)

Comment 36

[Mike Conley (:mconley) (:⚙️)]

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) (:⚙️)]

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]]

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]]

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]]

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]]

ehr, in comment 50 I meant progressbar, not scrollbar.

Comment 42

[Marco Bonardo [:mak]]

comment 40 not 50... sigh.

Comment 44

[Marco Bonardo [:mak]]

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]]

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]]

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]]

(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) (:⚙️)]

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

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

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) (:⚙️)]

Alrighty - can do.

Comment 52

[Girish Sharma [:Optimizer]]

Does this mean that each entry will not have its own download speed indicated along it ?

Comment 53

[Marco Bonardo [:mak]]

(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) (:⚙️)]

Attachment: Part 1: Remove download rate from download item (but keep it in tooltip)

Comment 55

[Mike Conley (:mconley) (:⚙️)]

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) (:⚙️)]

Attachment: Part 2: Update the list width, add locale commentary

Comment 57

[Mike Conley (:mconley) (:⚙️)]

Attachment: Thinner Panel on Ubuntu

Comment 58

[Marco Bonardo [:mak]]

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

So, what happens if you set the width on the details box, than on the richlistbox?

Comment 60

[Mike Conley (:mconley) (:⚙️)]

Attachment: Move width attribute to details node

Checkpointing alternative patch that moves the width attribute to the details node.

Comment 61

[Mike Conley (:mconley) (:⚙️)]

Attachment: Ubuntu - having moved width attribute

Comment 62

[Mike Conley (:mconley) (:⚙️)]

Attachment: OSX - having moved width attribute

Comment 63

[Mike Conley (:mconley) (:⚙️)]

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) (:⚙️)]

Attachment: Windows 7 - having moved width attribute

Comment 65

[Mike Conley (:mconley) (:⚙️)]

Attachment: Windows XP - having moved width attribute

Comment 66

[Mike Conley (:mconley) (:⚙️)]

Attachment: Ubuntu - having moved width attribute

Comment 67

[Mike Conley (:mconley) (:⚙️)]

Attachment: OSX - having moved width attribute

Comment 68

[Mike Conley (:mconley) (:⚙️)]

Attachment: Ubuntu - having moved width attribute - no rate

Comment 69

[Mike Conley (:mconley) (:⚙️)]

Attachment: OSX - having moved width attribute - no rate

Comment 70

[Mike Conley (:mconley) (:⚙️)]

Attachment: Windows XP - having moved width attribute - no rate

Comment 71

[Mike Conley (:mconley) (:⚙️)]

Attachment: Windows 7 - having moved width attribute - no rate

Comment 72

[Marco Bonardo [:mak]]

The moved attribute + no rate looks really good.

Comment 73

[Mike Conley (:mconley) (:⚙️)]

Attachment: 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.]

(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) (:⚙️)]

(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) (:⚙️)]

s/"download rate per second"/"download rate".  Sorry for the redundancy. :)

Comment 77

[Roman R.]

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]]

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

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]]

I think we discussed enough on irc and comments about this, clearing the request, we can iterate later.

Comment 81

[Mike Conley (:mconley) (:⚙️)]

(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) (:⚙️)]

Attachment: 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]]

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) (:⚙️)]

Attachment: Patch v3 (r+'d by mak)

Good catches all around. Thanks Marco!

Comment 85

[Mike Conley (:mconley) (:⚙️)]

Landed on mozilla-inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/bd648e7c2656

Comment 86

[Ryan VanderMeulen [:RyanVM]]

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

Comment 87

[Simona Badau, Desktop QA]

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 89

[Roman Tomjak]

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]]

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

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

[Marco Bonardo [:mak]]

bug 812894
Also see http://blog.bonardo.net/2013/04/30/new-download-experience-post-mortem