Shorten time strings, reduce padding and margin in the panel to reintroduce per item download speed

VERIFIED FIXED in Firefox 53

Status

()

Firefox
Downloads Panel
VERIFIED FIXED
5 years ago
5 months ago

People

(Reporter: mconley, Assigned: seanlee)

Tracking

(Blocks: 4 bugs)

Trunk
Firefox 53
x86
All
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox53 verified)

Details

(Whiteboard: [CHE-MVP])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments, 10 obsolete attachments)

25.54 KB, patch
Details | Diff | Splinter Review
30.28 KB, image/png
Details
25.80 KB, image/png
Details
75.51 KB, image/png
Details
37.91 KB, image/png
Details
58 bytes, text/x-review-board-request
Paolo
: review+
Details | Review
(Reporter)

Description

5 years ago
Bug 759397 removed the per-download rate indicator to reduce the clutter and noise in the downloads panel.

We decided instead to try displaying a global download rate. We need to sort out how this looks.
(Reporter)

Comment 1

5 years ago
Created attachment 683592 [details]
Wireframe

How do we feel about showing a global rate at the top of the list?

Not sure what we'd do if no downloads were in progress. Perhaps just display nothing. Or collapse the row.
I honestly don't see any other positioning possible...

Last night, instead of sleeping, I was thinking to a graphical vertical representation of speed in the right part of the Summary element. Basically the top limit is kept up-to-date to the maximum speed reached in the current session, while an animated bar fills up to the current speed. Something like a network icon with bars, similar to the mobile phones bars to be clear, but auto-adapting to the session.
There are cons though. This would give information on how much we are filling the available bandwidth, though wouldn't give any info on the global bandwidth (e.g. some other software in background may be slowing us down). Plus may go mad if the user doesn't restart the session when changing connection, like when traveling at the airport.
The first cons may be fixed by a tooltip with the effective speed, fwiw. Not sure how I'd fix the latter, unless the OS gives us the top limit info or we invalidate it on idle.
(Reporter)

Comment 4

5 years ago
Stephen - do you have a position on this?
Flags: needinfo?(shorlander)
(Reporter)

Comment 5

5 years ago
Created attachment 684435 [details]
A second wireframe

This second wireframe adds a visualization of the global download rate. It's got a sliding maximum, and invalidates when all downloads go idle.

I'm still not sure if I'm on the right track here...
Comment on attachment 684435 [details]
A second wireframe

I detect a lot of progress bars here :)

Honestly, if we are going to add a graphical indicator, I'd like it t not be a bar, it's too easy to confuse with a global progress bar.  My take on graphical representation (I repeat, if we really want it) is signal-strength bars
(Reporter)

Comment 7

5 years ago
Created attachment 684442 [details]
A third wireframe

Another idea:

This puts a vertical "strength" indicator along the right side of the panel. The current global rate could then be displayed in a tooltip on that indicator.

Comment 8

5 years ago
I think we should wait for Stephen's feedback before proceeding any further. I
suspect that, since the panel is designed only to surface status and provide
quickly actionable items, a graphical indication might be too much, and just be
confusing. In many cases, there isn't a specific action that can be taken based
on global download rate.
Created attachment 684872 [details]
A fourth wireframe

Another idea can be simply by putting global download rate speed in the bottom info, of course if this info (time&size) represent state of all downloaded files, like it's currently in Downloads panel.
(In reply to Virtual_ManPL [:Virtual] from comment #9)
> Another idea can be simply by putting global download rate speed in the
> bottom info, of course if this info (time&size) represent state of all
> downloaded files, like it's currently in Downloads panel.

that's not feasible due to space constraints, the same reason it was removed from the single items.
(Reporter)

Updated

5 years ago
Summary: Show a global download rate → Shorten time remaining strings and replace download rates per item
(Reporter)

Comment 11

5 years ago
So the idea of a global rate has been mulled about, and now tossed.

We're going to switch back to showing individual rates per download item.

However, in order to reduce the possibility of truncated strings in the "details" description of a download item, we're going to shorten some of the components of the strings considerably.

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

Becomes

59 min 59 secs left - 1022/1023 B (999.4 B/s)

We could also try replacing "hr", "min", and "secs" with "h", "m", and "s". I'll start at the long one and see where we get.
I liked the readability of the current text, was very good for non-advanced users :(
(Reporter)

Comment 13

5 years ago
Created attachment 688868 [details] [diff] [review]
WIP Patch 1

Are we sure we're OK copying all of this from mozapps/downloads/DownloadUtils.jsm?
Assignee: nobody → mconley
Attachment #683592 - Attachment is obsolete: true
Attachment #684435 - Attachment is obsolete: true
Attachment #684442 - Attachment is obsolete: true
Attachment #684872 - Attachment is obsolete: true
Attachment #688868 - Flags: feedback?(mak77)
(Reporter)

Comment 14

5 years ago
Note that this patch relies on Mano's patch in bug 675902.
Depends on: 675902
Comment on attachment 688868 [details] [diff] [review]
WIP Patch 1

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

::: browser/components/downloads/src/DownloadsCommon.jsm
@@ +89,5 @@
> +
> +const kToolkitStrMap = {
> +  statusFormat: "statusFormat3",
> +  timePair: "timePair2",
> +  // Update timeSize in convertTimeUnits if changing the length of this array

which array? this is fake Map with 2 properties :)

@@ +92,5 @@
> +  timePair: "timePair2",
> +  // Update timeSize in convertTimeUnits if changing the length of this array
> +}
> +
> +const kCachedLastMaxSize = 10;

needs doc

@@ +157,5 @@
>    /**
>     * Returns the status-text and status-tip to be displayed for the given
>     * data item.
>     */
> +  getStatusTextAndTipForDataItem: function(aDataItem, aMediumLength) {

aUseAbbrFormat or something like that, this is not a length

@@ +174,5 @@
>      } else if (aDataItem.state == nsIDM.DOWNLOAD_DOWNLOADING) {
>        let newEstimatedSecondsLeft;
> +      if (aMediumLength) {
> +        [status, newEstimatedSecondsLeft] =
> +          DownloadsCommon.getMediumDownloadStatus(aDataItem.currBytes,

s/medium/Abbr/ basically everywhere in the patch

@@ +344,5 @@
> +
> +    return [timeLeft, aSeconds];
> +  },
> +
> +  convertTimeUnits: function DC_convertTimeUnits(aSecs)

modify toolkit's DownloadUtils.convertTimeUnits, add an optional argument to it that is a convertTimeUnitsUnits implementation, if it's not passed will use its own convertTimeUnitsUnits.

may we do the same for getTimeLeft?

::: browser/locales/en-US/chrome/browser/downloads/downloads.properties
@@ +56,5 @@
>  
> +mediumTimeLeftSeconds=sec;secs
> +mediumTimeLeftMinutes=min;mins
> +mediumTimeLeftHours=hr;hrs
> +mediumTimeLeftDays=day;days

I'd suggest s/medium/abbr/

@@ +65,5 @@
> +# LOCALIZATION NOTE (timeLeftDouble): %1$S time left; %2$S time left sub units
> +# example: 11 hours, 2 minutes remaining; 1 day, 22 hours remaining
> +timeLeftDouble=%1$S, %2$S left
> +timeFewSeconds=A few seconds left
> +timeUnknown=Unknown time left

As discussed on irc, this change from "remaining" to "left" is just done to short string a bit, but it opens a can of worms for localizers since "left" is even worse than "remaining" plural-wise, and they'll have to figure out the reason we changed from one to the other one, and eventually stick to the existing "remaining" translation. Lots of noise for nothing. Since the problem has already been solved on their side, let's avoid reopening it until we decide to properly support plurals in toolkit.

Please stick with toolkit strings.
Attachment #688868 - Flags: feedback?(mak77)
(Reporter)

Comment 16

5 years ago
Created attachment 689318 [details] [diff] [review]
WIP Patch 2

Oh yes - passing functions to DownloadUtils.jsm is *much* better. Far less repetition and copying. Thanks for that idea!

Getting pretty close - just a little polish here and there.
Attachment #688868 - Attachment is obsolete: true
(Reporter)

Comment 17

5 years ago
Created attachment 689353 [details] [diff] [review]
WIP Patch 3

Adding a few more hooks into DownloadUtils to allow us to swap some strings in and out. Getting pretty close to something reviewable.
Attachment #689318 - Attachment is obsolete: true
(Reporter)

Comment 18

5 years ago
Created attachment 689768 [details] [diff] [review]
Patch v1

Screenshots coming.
Attachment #689353 - Attachment is obsolete: true
(Reporter)

Comment 19

5 years ago
Clearing the needinfo - we have our spec from shorlander.
Flags: needinfo?(shorlander)
(Reporter)

Comment 20

5 years ago
Created attachment 689773 [details]
Patch on OSX
(Reporter)

Comment 21

5 years ago
Created attachment 689777 [details]
Patch on Ubuntu
(Reporter)

Comment 22

5 years ago
Created attachment 689782 [details]
Patch on Windows 7 (through RDP)

Sorry for the low quality screenshot - I'm at home, and I'm remote-ing into my Windows box. That's why some of the panel shading might look off.
(Reporter)

Comment 23

5 years ago
Created attachment 689783 [details]
Patch on Windows 7 (through RDP)

Now with more context!
Attachment #689782 - Attachment is obsolete: true
(In reply to Mike Conley (:mconley) from comment #22)
> Created attachment 689782 [details]
> Patch on Windows 7 (through RDP)
> 
> Sorry for the low quality screenshot - I'm at home, and I'm remote-ing into
> my Windows box. That's why some of the panel shading might look off.

I have been wondering for some days, why doesn't the windows one has the focus ring. I used to get focus ring earlier, but from a couple of days, it is gone :(
(Reporter)

Comment 25

5 years ago
Created attachment 689785 [details]
Patch on Windows XP (through RDP)
(Reporter)

Comment 26

5 years ago
Created attachment 689786 [details]
Patch on OSX
Attachment #689773 - Attachment is obsolete: true
(Reporter)

Comment 27

5 years ago
Created attachment 689788 [details]
Patch on Ubuntu
Attachment #689777 - Attachment is obsolete: true
(Reporter)

Comment 28

5 years ago
(In reply to Girish Sharma [:Optimizer] from comment #24)
> (In reply to Mike Conley (:mconley) from comment #22)
> > Created attachment 689782 [details]
> > Patch on Windows 7 (through RDP)
> > 
> > Sorry for the low quality screenshot - I'm at home, and I'm remote-ing into
> > my Windows box. That's why some of the panel shading might look off.
> 
> I have been wondering for some days, why doesn't the windows one has the
> focus ring. I used to get focus ring earlier, but from a couple of days, it
> is gone :(

Hrm. Good question. I've filed bug 819428 - I'll have a peek on Monday when I'm back near my Windows machine.
(Reporter)

Comment 29

5 years ago
Comment on attachment 689768 [details] [diff] [review]
Patch v1

Ok, I think I'm ready for some review.
Attachment #689768 - Flags: review?(mak77)
Comment on attachment 689768 [details] [diff] [review]
Patch v1

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

::: browser/components/downloads/src/DownloadsCommon.jsm
@@ +306,5 @@
> +   * @param aIndex
> +   *        The index value for the unit. The index is mapped to a value in
> +   *        kAbbrTimeUnits.
> +   */
> +  convertTimeUnitsUnits: function DC_convertTimeUnitsUnits(aTime, aIndex)

I don't understand this naming (yes I know it's used in toolkit, that doesn't make it suck less), convertTimeIndexToUnit maybe?

@@ +324,5 @@
> +   * @param aIndex
> +   *        The index value for the unit. The index is mapped to a value
> +   *        in kToolkitStrMap.units.
> +   */
> +  byteIndexConverter: function DC_byteIndexConverter(aIndex)

and "convertByteIndexToUnit"

::: browser/locales/en-US/chrome/browser/downloads/downloads.dtd
@@ +23,2 @@
>  
> +     That's 55 characters, so we set the width at 55ch.

hum, I count 56, plus B/s is not the longest form, that would be MB/s or KB/s, so that ends up being 57, that is even larger :(

I don't have further ideas, the only compromise would be something like
"59 mins, 59 secs remaining. 1022 / 1023 KB (999.7 KB/s)" that wins back 2ch, probably not even worth the added complication.

Or properly replace "remaining" with a plural form "left"... regarding this, I found that in the past we changed "left" to "remaining", based on UX request: https://bugzilla.mozilla.org/show_bug.cgi?id=394516#c26
Surely if we decide to go back we should not use one in the panel and another one in the manager, but the UX pointers in the above comment state "left" is just wrong.

Or just wontfix this and keep the panel without speed ratio, this was the original ux idea and at this point I'm not sure benefits are worth all of this.
and for sake of completeness, left replaced remaining, around 2003, cause it was shorter... So looks like we keep changing from one to the other every N years.
Unblocking the feature, while we have clear advantages and disadvantages of both approaches, the time this is taking away could be better spent on other blockers.
No longer blocks: 747422

Updated

5 years ago
Attachment #689768 - Flags: review?(mak77)

Updated

4 years ago
Duplicate of this bug: 862108

Updated

4 years ago
Summary: Shorten time remaining strings and replace download rates per item → Shorten time strings, reduce padding and margin in the panel to reintroduce per item download speed

Updated

4 years ago
Blocks: 496758
Duplicate of this bug: 496758

Updated

4 years ago
Duplicate of this bug: 872690

Updated

4 years ago
Duplicate of this bug: 902263

Updated

4 years ago
Duplicate of this bug: 496758

Comment 38

4 years ago
To clarify, we want to redesign the items in the Downloads Panel to show the
download speed also, in an unobtrusive way.

Updated

4 years ago
Blocks: 963745

Updated

3 years ago
Blocks: 950073
Whiteboard: p=0

Updated

3 years ago
Duplicate of this bug: 986184

Updated

3 years ago
Whiteboard: p=0 → p=5

Updated

3 years ago
No longer blocks: 950073
Flags: firefox-backlog+

Updated

3 years ago
Points: --- → 5
Flags: qe-verify?
Whiteboard: p=5

Updated

a year ago
See Also: → bug 1269958

Comment 40

a year ago
(In reply to Bryant Mao [:bryantmao] from bug 1269958 comment #9)
> One more thing which different from current ETA format is that we'll remove
> "minutes" when the remaining time is above 1 hr, as we think under this time
> scale "minutes" becomes less necessary to show.

Well, if I have to leave in one hour and a half, I may be interested in knowing if my download takes only one hour or it takes one hour and 25 minutes...
Hi Mike, 

This is EPM Wesly from Taipei, nice to meet you!

I learned[1] this bug is suitable to be part of our project scope[2], so wondering do you plan to work on it soon? Or do you think we are able to take it, together with our latest UX spec[3] for moving on?

Thanks!

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1269958#c7
[2] https://mana.mozilla.org/wiki/display/PM/Content+Handling+Enhancement
[3] attachment in https://bugzilla.mozilla.org/show_bug.cgi?id=1269956
Flags: needinfo?(mconley)
(Reporter)

Comment 42

a year ago
I don't plan on working on it anytime soon - in fact, I'd forgotten it was assigned to me! It's all yours. :)
Assignee: mconley → nobody
Flags: needinfo?(mconley)
Thanks Mike, we'll have someone from project team to take it later!
Blocks: 126958
Blocks: 1269958
No longer blocks: 126958
Whiteboard: [CHE-MVP]
(Assignee)

Updated

10 months ago
No longer blocks: 1269958
(Assignee)

Updated

10 months ago
Blocks: 1269956

Updated

8 months ago
Duplicate of this bug: 1311636

Comment 45

7 months ago
Assign to Sean to help. Some related discussion on implementation order is since bug 1281662 comment 4.
Assignee: nobody → selee
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 48

7 months ago
mozreview-review
Comment on attachment 8817589 [details]
Bug 812894 - Use the short time format for downloading remaining time.;

https://reviewboard.mozilla.org/r/97820/#review98604

This is a first pass, I'll review the function code more thoroughly in the next version.

::: browser/components/downloads/DownloadsViewUI.jsm:206
(Diff revision 2)
>        [tip, newEstimatedSecondsLeft] = DownloadUtils.getDownloadStatus(
>                                            this.download.currentBytes,
>                                            totalBytes,
>                                            this.download.speed,
>                                            this.lastEstimatedSecondsLeft);
> +      text = tip;

The tip is already set to be equal to the text below, so you can just set the text in the destructuring assignment above.

::: browser/locales/en-US/chrome/browser/downloads/downloads.dtd:26
(Diff revision 2)
>  
> -     59 minutes, 59 seconds remaining - 1022 of 1023 KB
> +     59m 59s left - 1022 of 1023 KB (120.5 KB/sec)
>  
> -     That's 50 characters, so we set the width at 50ch.
> +     That's 45 characters, so we set the width at 45ch.
>       -->
> -<!ENTITY downloadDetails.width            "50ch">
> +<!ENTITY downloadDetails.width            "45ch">

We should keep the current value even if the string for in-progress downloads is shorter, because the strings for blocked downloads and the new labels we are adding for hover cases may be longer.

In fact, the way the rule is explained in the comment is not that useful. We can update the comment for now to explain, but we may actually consider redefining these widths in a separate bug at some point.

::: toolkit/mozapps/downloads/DownloadUtils.jsm:542
(Diff revision 2)
>        scale *= timeSize[unitIndex];
>        unitIndex++;
>      }
>  
>      let value = convertTimeUnitsValue(time);
> -    let units = convertTimeUnitsUnits(value, unitIndex);
> +    let units = gBundle.GetStringFromName(gStr.shortTimeUnits[unitIndex]);

I suspect we still need the PluralForm call here.

Some languages might not have useful short forms, or the short forms might be different for plurals.
Attachment #8817589 - Flags: review?(paolo.mozmail)
Comment hidden (mozreview-request)
(Assignee)

Comment 50

6 months ago
mozreview-review-reply
Comment on attachment 8817589 [details]
Bug 812894 - Use the short time format for downloading remaining time.;

https://reviewboard.mozilla.org/r/97820/#review98604

> We should keep the current value even if the string for in-progress downloads is shorter, because the strings for blocked downloads and the new labels we are adding for hover cases may be longer.
> 
> In fact, the way the rule is explained in the comment is not that useful. We can update the comment for now to explain, but we may actually consider redefining these widths in a separate bug at some point.

I will close the issue once the relative bug is opened.

> I suspect we still need the PluralForm call here.
> 
> Some languages might not have useful short forms, or the short forms might be different for plurals.

I restore the changes, so the PluralForm in downloads.properties looks like this `shortSeconds=s;s`.
Comment hidden (mozreview-request)

Comment 52

6 months ago
As a reference, this bug will change following UX items:
- Shorten time format to “Xm Xs left — X of X MB (x kB/s)”
- Add the download speed
- “Remaining” > “Left”

Comment 53

6 months ago
mozreview-review
Comment on attachment 8817589 [details]
Bug 812894 - Use the short time format for downloading remaining time.;

https://reviewboard.mozilla.org/r/97820/#review102044

::: browser/components/downloads/DownloadsViewUI.jsm
(Diff revision 4)
> -      // By default, extended status information including the individual
> -      // download rate is displayed in the tooltip. The history view overrides
> -      // the getter and displays the datails in the main area instead.
> -      [text] = DownloadUtils.getDownloadStatusNoRate(
> -                                          this.download.currentBytes,
> -                                          totalBytes,
> -                                          this.download.speed,
> -                                          this.lastEstimatedSecondsLeft);

The getDownloadStatusNoRate function has only one other caller, which is for the Downloads Summary.

You should update the other caller too to include the rate, and remove the implementation of getDownloadStatusNoRate and the related strings.

::: browser/locales/en-US/chrome/browser/downloads/downloads.dtd:22
(Diff revision 4)
> -     59 minutes, 59 seconds remaining - 1022 of 1023 KB
> +     59m 59s left - 1022 of 1023 KB (120.5 KB/sec)
>  
> -     That's 50 characters, so we set the width at 50ch.
> +     That's 45 characters, so we set the width at 45ch.
>       -->
> -<!ENTITY downloadDetails.width            "50ch">
> +<!ENTITY downloadDetails.width            "45ch">

There is a previous review comment for this. Did you just forget to update this version of the patch?

::: toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties:5
(Diff revision 4)
>  # LOCALIZATION NOTE (seconds, minutes, hours, days): Semi-colon list of plural
>  # forms. See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
>  seconds=second;seconds
>  minutes=minute;minutes
>  hours=hour;hours
>  days=day;days

You shouldn't leave unused strings behind.

::: toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties:50
(Diff revision 4)
>  dontQuitButtonWin=Don’t Exit
>  dontQuitButtonMac=Don’t Quit
>  dontGoOfflineButton=Stay Online
>  dontLeavePrivateBrowsingButton2=Stay in Private Browsing
>  downloadsCompleteTitle=Downloads Complete
> -downloadsCompleteMsg=All files have finished downloading. 
> +downloadsCompleteMsg=All files have finished downloading.

On another bug we were asked to keep the trailing space here, to avoid confusing localization tools.

::: toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties:89
(Diff revision 4)
>  # LOCALIZATION NOTE (timePair2): %1$S time number; %2$S time unit
>  # example: 1 minute; 11 hours
>  timePair2=%1$S %2$S
> +# LOCALIZATION NOTE (shortTimePair): %1$S time number; %2$S time unit
> +# example: 1m; 11h
> +shortTimePair=%1$S%2$S

You've used a mix of "shortStringName" and "stringName2" conventions, it should be one or the other. I recommend using the "stringName2" convention.

::: toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties:126
(Diff revision 4)
>  stateFailed=Failed
>  stateCanceled=Canceled
>  # LOCALIZATION NOTE (stateBlocked): 'Parental Controls' should be capitalized
>  stateBlocked=Blocked by Parental Controls
>  stateDirty=Blocked: Download may contain a virus or spyware
> -# LOCALIZATION NOTE (stateBlockedPolicy): 'Security Zone Policy' should be capitalized 
> +# LOCALIZATION NOTE (stateBlockedPolicy): 'Security Zone Policy' should be capitalized

This whitespace fix might be harmless, since it's in a localization note, but again we might want to revert it to be sure.

::: toolkit/mozapps/downloads/DownloadUtils.jsm:79
(Diff revision 4)
> -  timePair: "timePair2",
> -  timeLeftSingle: "timeLeftSingle2",
> -  timeLeftDouble: "timeLeftDouble2",
> -  timeFewSeconds: "timeFewSeconds",
> -  timeUnknown: "timeUnknown",
> +  shortTimePair: "shortTimePair",
> +  timeLeftSingle: "timeLeftSingle3",
> +  shortTimeLeftDouble: "shortTimeLeftDouble",
> +  timeFewSeconds: "timeFewSeconds2",
> +  timeUnknown: "timeUnknown2",

To simplify, follow consistently the same approach that was used before in this file, so don't modify the keys of the object, just add one to the number in the string value.

::: toolkit/mozapps/downloads/DownloadUtils.jsm:90
(Diff revision 4)
>    yesterday: "yesterday",
>    doneScheme: "doneScheme2",
>    doneFileScheme: "doneFileScheme",
>    units: ["bytes", "kilobyte", "megabyte", "gigabyte"],
>    // Update timeSize in convertTimeUnits if changing the length of this array
> -  timeUnits: ["seconds", "minutes", "hours", "days"],
> +  shortTimeUnits:["shortSeconds", "shortMinutes", "shortHours", "shortDays"],

Same here, keep "timeUnits" intact in the code. In this case, though, "shortSeconds" might be clearer than "seconds2" as a string identifier for localizers.
Attachment #8817589 - Flags: review?(paolo.mozmail)
(Assignee)

Updated

6 months ago
Blocks: 1328519
Comment hidden (mozreview-request)
(Assignee)

Comment 55

6 months ago
mozreview-review-reply
Comment on attachment 8817589 [details]
Bug 812894 - Use the short time format for downloading remaining time.;

https://reviewboard.mozilla.org/r/97820/#review102044

I go through all issues and resolved them in the latest patch. Please review it again. Thanks.

> Same here, keep "timeUnits" intact in the code. In this case, though, "shortSeconds" might be clearer than "seconds2" as a string identifier for localizers.

I am not sure if I understand this comment correctly. "short" prefix is remained here for L10N team to recognize the purpose of these chagnes.
(Assignee)

Comment 56

6 months ago
Hi Bryant,

In the latest patch, the detail information in Summary section is with download rate now. The calculation is like this:

Total Downloading Rate = Sum the downloaded rate for all items in Summary section.
Total Remaining Bytes = Sum the remaining bytes for all items in Summary section.

Estimation Time = Total Remaining Bytes / Total Downloading Rate

Could you help to confirm these items?
1. Display Total Downloading Rate in Summary section or now.
2. What's Estimation Time formula if we want to show 1.

Thanks.
Flags: needinfo?(bmao)
(In reply to Sean Lee [:seanlee][:weilonge] from comment #56)
> Hi Bryant,
> 
> In the latest patch, the detail information in Summary section is with
> download rate now. The calculation is like this:
> 
> Total Downloading Rate = Sum the downloaded rate for all items in Summary
> section.
> Total Remaining Bytes = Sum the remaining bytes for all items in Summary
> section.
> 
> Estimation Time = Total Remaining Bytes / Total Downloading Rate
> 
> Could you help to confirm these items?
> 1. Display Total Downloading Rate in Summary section or now.

My answer is no, at least for now. 

Originally, there are two reasons we want to introduce download rate to summary section: 1) The information is consistent since we are now planning to add download rate on each downloading item; 2) User might feel faster if they see the download rate (Our assumption). 

However, considering user may want to see the overall download progress and remaining time rather than the download rate which is not even very precise, as well as the average rate might conflict with our current estimation time formula (longest time), I don't think it worth the efforts.
Flags: needinfo?(bmao)

Comment 58

6 months ago
(In reply to Sean Lee [:seanlee][:weilonge] from comment #55)
> I am not sure if I understand this comment correctly. "short" prefix is
> remained here for L10N team to recognize the purpose of these chagnes.

Yes, this is what I meant. Thanks!

Updated

6 months ago
Blocks: 1329167

Comment 59

6 months ago
mozreview-review
Comment on attachment 8817589 [details]
Bug 812894 - Use the short time format for downloading remaining time.;

https://reviewboard.mozilla.org/r/97820/#review103358

I've filed bug 1329167 about the removal of getDownloadStatusNoRate so that it can be addressed separately in the future. For the moment you can revert the related changes in this patch, which is how I interpreted Bryant's comment.

The rest looks good, but please make sure that all the tests pass before asking for the final review.

::: toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties:82
(Diff revision 5)
>  
> -# LOCALIZATION NOTE (timePair2): %1$S time number; %2$S time unit
> -# example: 1 minute; 11 hours
> -timePair2=%1$S %2$S
> -# LOCALIZATION NOTE (timeLeftSingle2): %1$S time left
> -# example: 1 minute remaining; 11 hours remaining
> +# LOCALIZATION NOTE (timePair3): %1$S time number; %2$S time unit
> +# example: 1m; 11h
> +timePair3=%1$S%2$S
> +# LOCALIZATION NOTE (timeLeftSingle3): %1$S time left
> +# example: 1 minute left; 11 hours left

nit: update the example to say "1m left; 11h left".
Attachment #8817589 - Flags: review?(paolo.mozmail)
Comment hidden (mozreview-request)
(Assignee)

Comment 61

6 months ago
Hi Paolo,

This [1] is the treeherder result of the latest patch. I've checked that these failed cases are not relative to the changes of Downloads Panel. All issues you raised are resolved as well.
Please review it again. Thank you.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=d990ae0e3d67292f1e443326bcfccf5f42671b88

Comment 62

6 months ago
mozreview-review
Comment on attachment 8817589 [details]
Bug 812894 - Use the short time format for downloading remaining time.;

https://reviewboard.mozilla.org/r/97820/#review103806

Thanks!
Attachment #8817589 - Flags: review?(paolo.mozmail) → review+
(Assignee)

Updated

6 months ago
Keywords: checkin-needed

Comment 63

6 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/053b67802207
Use the short time format for downloading remaining time.; r=Paolo
Keywords: checkin-needed

Comment 64

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/053b67802207
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
QA Contact: cristian.comorasu
There's a problem with the plural strings (it was already there)
https://hg.mozilla.org/mozilla-central/diff/053b67802207/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties

Each string needs to have its own localization comment for plural forms, one before the group won't work.
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Use_proper_plural_forms

Not sure if you want to address it here, since it's just comments in the .properties file, or in a new bug.

Side question: is this code going to be used in Android too? I wonder if we can't simplify this relying on Intl APIs (ccing Zibi too).
Flags: needinfo?(selee)
not yet. We are planning to get Intl.RelativeTimeFormat ("in 1 minute") and Intl.UnitFormat ("1 minute", "2 minutes"), but we that's probably closer to H2 2017.
(Assignee)

Comment 67

6 months ago
Hey Francesco,

Do you mean this line[2] should be changed to this[2]:

[1] # LOCALIZATION NOTE (seconds, minutes, hours, days): Semi-colon list of plural

[2] # LOCALIZATION NOTE (shortSeconds, shortMinutes, shortHours, shortDays): Semi-colon list of plural
Flags: needinfo?(selee) → needinfo?(francesco.lodolo)
No, it should become

# LOCALIZATION NOTE (shortSeconds): Semi-colon list of plural
# forms. See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
# s is the short form for seconds
shortSeconds=s;s

# LOCALIZATION NOTE (shortMinutes): Semi-colon list of plural
# forms. See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
# m is the short form for minutes
shortMinutes=m;m

etc.
Flags: needinfo?(francesco.lodolo)
(Assignee)

Updated

6 months ago
Blocks: 1330158
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0

Time strings shortened, download speed visible, bug verified fixed.
Status: RESOLVED → VERIFIED
QA Whiteboard: [bugday-2017-01-11]
I reproduced this issue using 53.0a1, build ID:20161218030213, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx53.0a1, build ID: 20170113030227, on Windows 10 x64, Mac OS X 10.12.3 and Ubuntu 14.04 LTS.

Cheers!
status-firefox53: fixed → verified
Flags: qe-verify?

Comment 71

5 months ago
I'm not sure if this belongs here, but download panel tip pointing on its button is not consistent with few other panels (History, Pocket, Text Encoding, Developer Tools and Horlander's mockup). It is however consistent with few other panels (App menu and Bookmarks). See screenshots below:

Ovlerlapping tip of panel:
Downloads panel http://fii.cz/pqbmwcae
App menu http://fii.cz/ykcbss
Bookmarks http://fii.cz/udfahjuv

Not overlapping tip:
History http://fii.cz/hegskd
Pocket http://fii.cz/aerxh
Developer Tools http://fii.cz/nysuyac
Text Encoding http://fii.cz/wraaxsm
Horlander's mockup http://fii.cz/btqnvsd

personally I do not mind which style of these two is supposed to be the right one but you should pick one and stick with it.

Comment 72

5 months ago
Roman, that's good point related to polishing our user interface. The screenshots are useful reference. Can you file a new bug under "Firefox :: Toolbars and Customization" if one isn't on file already, so this doesn't get lost?

Comment 73

5 months ago
Paolo, yes I can.(In reply to :Paolo Amadini from comment #72)
> Roman, that's good point related to polishing our user interface. The
> screenshots are useful reference. Can you file a new bug under "Firefox ::
> Toolbars and Customization" if one isn't on file already, so this doesn't
> get lost?
Yes I can. Done under https://bugzilla.mozilla.org/show_bug.cgi?id=1331927

Comment 74

5 months ago
Thanks!
You need to log in before you can comment on or make changes to this bug.