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.

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.

Stephen - do you have a position on this?

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

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.

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.

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.

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

Are we sure we're OK copying all of this from mozapps/downloads/DownloadUtils.jsm?

Note that this patch relies on Mano's patch in bug 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.

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.

Adding a few more hooks into DownloadUtils to allow us to swap some strings in and out. Getting pretty close to something reviewable.

Screenshots coming.

Clearing the needinfo - we have our spec from shorlander.

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.

Now with more context!

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

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

Comment on attachment 689768 [details] [diff] [review] Patch v1 Ok, I think I'm ready for some review.

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.

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

(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

I don't plan on working on it anytime soon - in fact, I'd forgotten it was assigned to me! It's all yours. :)

Thanks Mike, we'll have someone from project team to take it later!

Assign to Sean to help. Some related discussion on implementation order is since bug 1281662 comment 4.

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.

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

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

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.

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.

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

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

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

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 on attachment 8817589 [details] Bug 812894 - Use the short time format for downloading remaining time.; https://reviewboard.mozilla.org/r/97820/#review103806 Thanks!

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

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

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.

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

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.

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.

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!

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.

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?

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

Thanks!