In the Downloads Panel, we should make it clear that there might be other downloads that are in progress or just completed in the background, but are not visible in the panel itself. See attachment 677882 [details] for a rough idea.

(In reply to Mike Conley (:mconley)) > Not sure if the global download rate is correct - I'm mainly focusing on the > final item in the list that displays the "invisible" completed and > in-progress downloads. We should probably find a way to integrate the final item and the "Show All Downloads" button in some way, since (in the wireframe) they are in a different place but link to the same window (Stephen to comment on attachment 677882 [details]). This is just to say that I suspect we'll need to use separate XUL elements when implementing the special item, rather than reuse a download item binding.

I've added a new binding for hidden download items, and a DownloadsSum object per window to handle it. There's lots that's incomplete here - for example, I don't think we handle all states properly for hidden downloads. Styling is also lacking, and currently only exists for gnomestripe.

Comment on attachment 678869 [details] [diff] [review] WIP patch 1 Am I heading in the right direction with this?

(I should also point out that I'm not 100% convinced myself that a whole new object, DownloadsSum, is required - but that's where I started with it.)

Comment on attachment 678869 [details] [diff] [review] WIP patch 1 Review of attachment 678869 [details] [diff] [review]: ----------------------------------------------------------------- I'm waiting for a build to test this real-life, in the meanwhile some comment on the code changes ::: browser/components/downloads/content/download.xml @@ +48,5 @@ > oncommand="DownloadsView.onDownloadCommand(event, 'downloadsCmd_show');"/> > </content> > </binding> > + > + <binding id="downloads-sum"> I think this name may be misleading, this is about Other downloads so probably should be named like downloads-other Btw, effectively a binding here sounds excessive since it's used just once, we should just hardcode this special item in the panel contents (downloadsOverlay.xul). I think Paolo suggestion was to not re-use the existing binding but not to make a new one. ::: browser/components/downloads/content/downloads.js @@ +673,5 @@ > + let count = this._dataItems.length; > + let hiddenCount = count - this.kItemCountLimit; > + let hiddenItems = (hiddenCount > 0) ? this._dataItems > + .slice(this.kItemCountLimit) > + : []; the check doesn't look needed, slice would just return an empty array by itself @@ +1391,5 @@ > + > + this._element.setAttribute("status", status); > + this._element.setAttribute("statusTip", status); > + this._element.setAttribute("target", > + DownloadsCommon.strings.hiddenItems(aHiddenItems.length)); There is a fact to take into account here, that is the hidden items string may end up being cut for some localizations, it's possible we may need to add another localizable width to this field and the bigger between this and the detailsWidth will just win. However this may cause width jumping when we switch from hasHidden to !hasHidden... So basically this width would be set on the external object than the text field and be slightly larger... @@ +1394,5 @@ > + this._element.setAttribute("target", > + DownloadsCommon.strings.hiddenItems(aHiddenItems.length)); > + // TODO: Dispatch the ValueChange event for accessibility, if possible. > + } else { > + this._element.collapsed = true; This may be a good first step, if we merge this with Show All Downloads, then here we'd change contents... I suppose we may use a <deck> and flip it between 2 states (has hidden, doesn't have hidden) This indeed would make lot of sense. We could also change the progress bar, make it shorter and add the speed after it? btw, just brainstorming here. ::: browser/locales/en-US/chrome/browser/downloads/downloads.properties @@ +84,5 @@ > fileExecutableSecurityWarning="%S" is an executable file. Executable files may contain viruses or other malicious code that could harm your computer. Use caution when opening this file. Are you sure you want to launch "%S"? > fileExecutableSecurityWarningTitle=Open Executable File? > fileExecutableSecurityWarningDontAsk=Don't ask me this again > + > +hiddenItems=+%1$S other downloads This needs a short localization note explaining the context it's used and what we mean for "hidden" plus what is the argument. As it is "X other dowloads" can be confusing, it's unclear if it means globally, for the session, or just the progressing ones To be honest, I actually dislike both "hidden" and "items" as terms to be used here... surely we should avoid items, there's so much usage across the codebase it may mean anything...

So, after trying it, we clearly need to merge information at the bottom of the panel. But I think this may be the right direction. I know this is a wip but since here, some polish issues: - progress bar should be same size as other downloads - there should be a separator above this last item - I think this should somehow be differentiated from the others, either different separator, or background... and surely an icon. functionally: - I paused all downloads, the text still says "10 minutes remaining" and after resume doesn't seem to update anymore.

(In reply to Marco Bonardo [:mak] from comment #5) > Comment on attachment 678869 [details] [diff] [review] > WIP patch 1 > > Review of attachment 678869 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm waiting for a build to test this real-life, in the meanwhile some > comment on the code changes > > ::: browser/components/downloads/content/download.xml > @@ +48,5 @@ > > oncommand="DownloadsView.onDownloadCommand(event, 'downloadsCmd_show');"/> > > </content> > > </binding> > > + > > + <binding id="downloads-sum"> > > I think this name may be misleading, this is about Other downloads so > probably should be named like downloads-other > > Btw, effectively a binding here sounds excessive since it's used just once, > we should just hardcode this special item in the panel contents > (downloadsOverlay.xul). I think Paolo suggestion was to not re-use the > existing binding but not to make a new one. Ok, fair enough. I'll drop the binding, and use the notion that these are "other downloads" in my naming. > > ::: browser/components/downloads/content/downloads.js > @@ +673,5 @@ > > + let count = this._dataItems.length; > > + let hiddenCount = count - this.kItemCountLimit; > > + let hiddenItems = (hiddenCount > 0) ? this._dataItems > > + .slice(this.kItemCountLimit) > > + : []; > > the check doesn't look needed, slice would just return an empty array by > itself Good eye - thanks. > > @@ +1391,5 @@ > > + > > + this._element.setAttribute("status", status); > > + this._element.setAttribute("statusTip", status); > > + this._element.setAttribute("target", > > + DownloadsCommon.strings.hiddenItems(aHiddenItems.length)); > > There is a fact to take into account here, that is the hidden items string > may end up being cut for some localizations, it's possible we may need to > add another localizable width to this field and the bigger between this and > the detailsWidth will just win. However this may cause width jumping when we > switch from hasHidden to !hasHidden... > So basically this width would be set on the external object than the text > field and be slightly larger... > Good point. So, I think my solution is to add a new width string to the locales for target width. This would be applied to both the target descriptions for individual downloads, and the target descriptions of the "other" downloads. The upshot of this, is that localizers get to set the width of the panel with "longest sentence wins" for each string, and we don't get width switching if the "other" downloads is shown and hidden. > @@ +1394,5 @@ > > + this._element.setAttribute("target", > > + DownloadsCommon.strings.hiddenItems(aHiddenItems.length)); > > + // TODO: Dispatch the ValueChange event for accessibility, if possible. > > + } else { > > + this._element.collapsed = true; > > This may be a good first step, if we merge this with Show All Downloads, > then here we'd change contents... I suppose we may use a <deck> and flip it > between 2 states (has hidden, doesn't have hidden) > This indeed would make lot of sense. > We could also change the progress bar, make it shorter and add the speed > after it? btw, just brainstorming here. Stephen said he'd (hopefully) have some designs sent me way by the end of the day - but yeah, I'm curious what the plans are for the "Show All Downloads" button if we're already showing "other" downloads.

(In reply to Marco Bonardo [:mak] from comment #6) > So, after trying it, we clearly need to merge information at the bottom of > the panel. But I think this may be the right direction. > > I know this is a wip but since here, some polish issues: > - progress bar should be same size as other downloads Yep - this is the case on gnomestripe. I haven't ported the theme-ing over to winstripe or pinstripe yet. > - there should be a separator above this last item > - I think this should somehow be differentiated from the others, either > different separator, or background... and surely an icon. > > functionally: > - I paused all downloads, the text still says "10 minutes remaining" and > after resume doesn't seem to update anymore. Thanks - yeah, I'm studying the way the indicator gets updates about download progress, and will hopefully use a similar model / rules when displaying the progress in the other downloads group.

(In reply to Mike Conley (:mconley) from comment #8) > Thanks - yeah, I'm studying the way the indicator gets updates about > download progress, and will hopefully use a similar model / rules when > displaying the progress in the other downloads group. Some general pointers, while you're looking into this: - Yes, ideally we should be able to reuse DownloadsData code for summarizing the state of multiple downloads, whether all of them or only a portion. - We keep the last summary properties in memory, since we receive a lot of progress notifications, and we don't want to always update the XUL elements if the properties haven't changed, for performance reasons. - Many objects in DownloadsCommon.jsm are now singletons but we're already looking into making them creatable separately, to work on different download sets for Per-Window Private Browsing Mode support. - Progress notifications are very frequent but adding/removing a download is a relatively rare event, we can even re-build the back-end summarization objects from scratch when this happens (if this makes thing simpler). With regard to the current patch, in the front-end code I suggest avoiding variable and function names containing just the word "item", I've always tried to include either "dataItem" or "viewItem" to make it clear which object type we're working on.

Hey Paolo, I just wanted to make sure I was on the right track. This patch is very much a work-in-progress. I'm mostly copying the DownloadsIndicatorData object in DownloadsCommon.jsm with a new DownloadsSummaryData object. There's a lot of common code between the two (especially wrt view registration), but I haven't done any work yet trying to abstract it out / reduce it. Setting DownloadsSummaryData.downloadsIds with an array of download IDs tells the object what downloads we care to receive an aggregated summary about - though, the calculations being performed by _refreshProperties in DownloadsSummaryData are probably in need of fixing up. Mostly copy-paste from DownloadsIndicatorData for now. Basically, I just want to know if this is the right direction - this moves the summarization calculation out of each window into a common space that each window subscribes to. Is this closer?

Comment on attachment 680135 [details] [diff] [review] WIP Patch 2 (In reply to Mike Conley (:mconley) from comment #10) > Setting DownloadsSummaryData.downloadsIds with an array of download IDs > tells the object what downloads we care to receive an aggregated summary > about This is one way to tell the object which downloads to consider. Updating the list on an existing object would need to trigger a summary recalculation, and in general updating the list should happen only once rather than once for each window. > Basically, I just want to know if this is the right direction - this moves > the summarization calculation out of each window into a common space that > each window subscribes to. Looks good, given the known pending developments you mentioned. I should probably take another look at this after you've merged the indicator and panel summary code, some good ideas might arrive while working.

Checkpointing my work so far. I've moved some of the common code between the summary and the indicator data objects, and moved them into DownloadsCommon. DownloadsSummaryData is instantiable, and I still need to abstract a base class out between that and DownloadsIndicatorData. We don't send downloadIds around anymore - finally caught on to Paolo's original suggestion of having a summarizer that knows how many of the top items to exclude, so now each window just subscribes a view to that summary object. Still not styled for Windows or OSX yet, and quite a few problems left with the implementation that I'm still trying to iron out. In particular, the number of reported hidden items gets out of sync when cancelling and then retrying a "hidden" download.

Checkpointing my work again. Fixed the synchronization issue I mentioned earlier. Created a prototype that both DownloadsIndicatorData and instances of DownloadsSummaryData can use, which gets rid of a bit of repeated code between the two. Styling starts tomorrow.

Checkpointing work. - Fixed an issue where restarted downloads weren't being noticed by the summarizer. - Added preliminary styling for gnomestripe (screenshot coming)

I don't have a global download rate yet - still unsure where to put that.

Adds preliminary winstripe styling.

Fixed a few more bugs: - The indicator progress bar wasn't being updated properly after my changes - DownloadsDataPrototypes weren't updating views when they were first added - Got rid of the showMoreDownloads string in downloads.properties, since I believe this supersedes it. Moved showAllDownloads label and access key to downloads.dtd. - Added a big ol' localization note regarding downloadTarget.width. Hopefully I wasn't too verbose. - Added more padding to the bottom of the summary - should have the same inner padding as a downloaditem now. - Progress bar and details in the summary is hidden unless there is at least one download progressing or paused in the hidden group. Some known issues: - The icon to the left of the summary in winstripe is 24x24 when the rest of the icons above it are 32x32. Either we have to find a new icon, or we need to live with the difference. I'm voting for new icon. - When there are many downloads downloading in the summary view, the amount of data displayed in the summary details changes quite rapidly. The more downloads, the more it updates (since it updates with each downloads progress change event). We might want to smooth that a little.

Updated screenshot with WIP Patch 7.

Fixing padding issues in gnomestripe. New Ubuntu screenshots coming.

If it's not clear, these screenshots contain panels with summaries in the following states, respectively: - Has other downloads, none of which are progressing, but at least one is paused - Has other downloads, where at least one is progressing - Has other downloads, where none are progressing or paused

Updating Win XP screenshot.

WIP Patch 9 adds OSX theme-ing.

Ok, I think this thing is finally ready for a real review.

D'oh, wrong file. Here's the right one.

mak: You might notice that I'm not using a deck in this patch (yet). That's because decks can't have hidden or collapsed child nodes, and they retain the size of their largest child node. That means that the "Show All Downloads" button becomes crazy-wide again if we use a deck, since the summary is itself as wide as a download item. Suggestions welcome there. In the meantime, the collapsing / uncollapsing of nodes that we're doing isn't too complicated - it's a one-liner. Anyhow, let me know what you think. I'm eager for feedback. -Mike

Comment on attachment 681822 [details] [diff] [review] Patch v1 Hey Paolo, I'm also interested in your feedback, if you have the cycles! -Mike

Comment on attachment 681822 [details] [diff] [review] Patch v1 Review of attachment 681822 [details] [diff] [review]: ----------------------------------------------------------------- I like the approach in code and I like the result visually. Great job! So this gives me a fantastic occasion for nitpicking :-) You can also wait for Marco's review and address the comments all together. ::: browser/components/downloads/content/downloads.js @@ +542,5 @@ > > + // If we've got some hidden downloads, we should show the summary just > + // below the list. > + this.downloadsHistory.collapsed = DownloadsSummary.visible > + = hiddenCount > 0; nit: I don't particularly like this multiple assignment form... @@ +1416,5 @@ > +/** > + * Manages the summary at the bottom of the downloads panel list if the number > + * of items in the list exceeds the panels limit. > + */ > +const DownloadsSummary = { nit: this is called "Summary" in code and "Other" in the XUL elements. Not a big deal, we can live with it :-) @@ +1424,5 @@ > + * unsubscribes from the DownloadsCommon DownloadsSummaryData singleton. > + */ > + _visible: false, > + set visible(aVisible) > + { nit: place the helper property right below the setter, so that JavaDoc is properly attributed to the setter. @@ +1497,5 @@ > + /** > + * Sets the details for the download summary, such as the time remaining, > + * the amount of bytes transferred, etc. > + * > + * @param aValue nit: trim whitespace at end of line. ::: browser/components/downloads/src/DownloadsCommon.jsm @@ +1105,5 @@ > + refreshView: function DVP_refreshView(aView) > + { > + // Subclasses should override this. > + throw Components.results.NS_ERROR_NOT_IMPLEMENTED; > + }, It seems all subclasses use the same implementation, might as well move it to the base class and override the internal functions. @@ +1433,2 @@ > */ > + _downloadsGenerator: function DID_downloadsGenerator() nit: since this is subclass-specific, it could also be named more specifically, like _activeDownloadsGenerator. @@ +1452,5 @@ > + totalSize: 0, > + totalTransferred: 0, > + rawTimeLeft: -1, > + percentComplete: -1 > + }; I'm concerned this should kept in sync with what summarizeDownloads returns, it might be better if it's obtained by calling summarizeDownloads with an empty array. @@ +1457,5 @@ > > // If no download has been loaded, don't use the methods of the Download > // Manager service, so that it is not initialized unnecessarily. > if (this._itemCount > 0) { > + summary = DownloadsCommon.summarizeDownloads(this._downloadsGenerator()); I think this optimization (don't use the methods of the Download Manager service if item count is zero) can be moved to the generator function itself, so that it would just return an empty array if there are no active downloads. This also plays well with the previous comment. @@ +1681,5 @@ > + rawTimeLeft: -1, > + percentComplete: -1 > + }; > + > + if (this._dataItems.length > 0) { This check can also be removed. ::: browser/locales/en-US/chrome/browser/downloads/downloads.dtd @@ +38,5 @@ > + > + If that number is less than downloadDetails.width, then just set > + downloadTarget.width equal to downloadDetails.width. If it's larger, you > + need to set downloadTarget.width to that number of characters followed by > + a ch. So, we're asking localizers to make computations in any case. Can't we just use one width entity? In fact, the size of the panel shouldn't change based on the state of the summary part. Only if there are no downloads at all, it should be smaller and fit the label (for which we don't need an explicit computation). ::: browser/locales/en-US/chrome/browser/downloads/downloads.properties @@ +75,5 @@ > +# This is displayed in an item at the bottom of the Downloads Panel when > +# there are more downloads than can fit in the list in the panel. Use a > +# semi-colon list of plural forms. > +# See: http://developer.mozilla.org/en/Localization_and_Plurals > +otherDownloads=+%1$S other current download; +%1$S other current downloads The exact text here should be defined by UX. We can also change it later, in that case we'll rename the entity.

Comment on attachment 681822 [details] [diff] [review] Patch v1 I prefer working on the version addressing paolo's comments, so we avoid double comments and misunderstandings

Thanks for the feedback, Paolo! Glad to hear I'm on the right track with it. mak: I'll ask for review once I ensure that everything still looks decent on Windows and OSX. (In reply to Paolo Amadini [:paolo] from comment #30) > Comment on attachment 681822 [details] [diff] [review] > Patch v1 > > Review of attachment 681822 [details] [diff] [review]: > ----------------------------------------------------------------- > > > ::: browser/components/downloads/content/downloads.js > @@ +542,5 @@ > > > > + // If we've got some hidden downloads, we should show the summary just > > + // below the list. > > + this.downloadsHistory.collapsed = DownloadsSummary.visible > > + = hiddenCount > 0; > > nit: I don't particularly like this multiple assignment form... > Okie doke, I broke it up. > @@ +1416,5 @@ > > +/** > > + * Manages the summary at the bottom of the downloads panel list if the number > > + * of items in the list exceeds the panels limit. > > + */ > > +const DownloadsSummary = { > > nit: this is called "Summary" in code and "Other" in the XUL elements. Not a > big deal, we can live with it :-) > It was an easy fix - I went for it. > @@ +1424,5 @@ > > + * unsubscribes from the DownloadsCommon DownloadsSummaryData singleton. > > + */ > > + _visible: false, > > + set visible(aVisible) > > + { > > nit: place the helper property right below the setter, so that JavaDoc is > properly attributed to the setter. > Done. > @@ +1497,5 @@ > > + /** > > + * Sets the details for the download summary, such as the time remaining, > > + * the amount of bytes transferred, etc. > > + * > > + * @param aValue > > nit: trim whitespace at end of line. > Done. > ::: browser/components/downloads/src/DownloadsCommon.jsm > @@ +1105,5 @@ > > + refreshView: function DVP_refreshView(aView) > > + { > > + // Subclasses should override this. > > + throw Components.results.NS_ERROR_NOT_IMPLEMENTED; > > + }, > > It seems all subclasses use the same implementation, might as well move it > to the base class and override the internal functions. > Done. > @@ +1433,2 @@ > > */ > > + _downloadsGenerator: function DID_downloadsGenerator() > > nit: since this is subclass-specific, it could also be named more > specifically, like _activeDownloadsGenerator. > Done, for both subclasses. > @@ +1452,5 @@ > > + totalSize: 0, > > + totalTransferred: 0, > > + rawTimeLeft: -1, > > + percentComplete: -1 > > + }; > > I'm concerned this should kept in sync with what summarizeDownloads returns, > it might be better if it's obtained by calling summarizeDownloads with an > empty array. > Good idea - done. > @@ +1457,5 @@ > > > > // If no download has been loaded, don't use the methods of the Download > > // Manager service, so that it is not initialized unnecessarily. > > if (this._itemCount > 0) { > > + summary = DownloadsCommon.summarizeDownloads(this._downloadsGenerator()); > > I think this optimization (don't use the methods of the Download Manager > service if item count is zero) can be moved to the generator function > itself, so that it would just return an empty array if there are no active > downloads. This also plays well with the previous comment. > Yep, I gotcha. Done. > @@ +1681,5 @@ > > + rawTimeLeft: -1, > > + percentComplete: -1 > > + }; > > + > > + if (this._dataItems.length > 0) { > > This check can also be removed. > Done. > ::: browser/locales/en-US/chrome/browser/downloads/downloads.dtd > @@ +38,5 @@ > > + > > + If that number is less than downloadDetails.width, then just set > > + downloadTarget.width equal to downloadDetails.width. If it's larger, you > > + need to set downloadTarget.width to that number of characters followed by > > + a ch. > > So, we're asking localizers to make computations in any case. Can't we just > use one width entity? > > In fact, the size of the panel shouldn't change based on the state of the > summary part. Only if there are no downloads at all, it should be smaller > and fit the label (for which we don't need an explicit computation). > Ok - I removed the second width, and added a comment to the original one so that they take the otherDownloads string from downloads.properties into account too. > ::: browser/locales/en-US/chrome/browser/downloads/downloads.properties > @@ +75,5 @@ > > +# This is displayed in an item at the bottom of the Downloads Panel when > > +# there are more downloads than can fit in the list in the panel. Use a > > +# semi-colon list of plural forms. > > +# See: http://developer.mozilla.org/en/Localization_and_Plurals > > +otherDownloads=+%1$S other current download; +%1$S other current downloads > > The exact text here should be defined by UX. We can also change it later, in > that case we'll rename the entity. Alright - I've left it for now.

Comment on attachment 682013 [details] [diff] [review] Patch v2 Ok, this still looks right across each OS. Ready for review.

Comment on attachment 682013 [details] [diff] [review] Patch v2 Review of attachment 682013 [details] [diff] [review]: ----------------------------------------------------------------- Some things to fix yet ::: browser/components/downloads/content/downloads.js @@ +1460,5 @@ > + this._detailsNode.collapsed = this._progressNode.collapsed > + = !aShowingProgress; > + this._showingProgress = aShowingProgress; > + }, > + _showingProgress: false, I think would be simpler to directly set/remove an "inprogress" attribute on downloadsSummary, and use display:none in browser/components/downloads/content/downloads.css (or viceversa, depending on the default value), than having all of this setter code @@ +1473,5 @@ > + set percentComplete(aValue) > + { > + if (this._progressNode) { > + this._progressNode.setAttribute("mode", "normal"); > + this._progressNode.setAttribute("value", aValue); couldn't you set mode="normal" directly in the xml? As a possiblity, you could set progressValue and textValue attributes directly on summaryNode, and in the xml have xbl:inherits on the progressbar and details, so basically you may completely remove _progressNode and _detailsNode from this object. @@ +1476,5 @@ > + this._progressNode.setAttribute("mode", "normal"); > + this._progressNode.setAttribute("value", aValue); > + } > + > + return aValue; nit: compact code a bit, remove newlines before the returns when there's a brace before, they don't appear to be needed to help readability, there's already quite a bit of empty space. @@ +1486,5 @@ > + * @param aValue > + * A string representing the description of the summarized > + * downloads. > + */ > + set target(aValue) While "target" makes sense for files, doesn't seem to make any sense in this context, maybe label, summary, description, something else? @@ +1488,5 @@ > + * downloads. > + */ > + set target(aValue) > + { > + if (this._targetNode) { as well as targetNode @@ +1517,5 @@ > + > + /** > + * Element corresponding to the root of the downloads summary. > + */ > + get _rootNode() _rootNode is quite confusing cause too generic, let's make this _summaryNode ::: browser/components/downloads/content/downloadsOverlay.xul @@ +112,5 @@ > + onclick="DownloadsPanel.showDownloadsHistory();"> > + <image class="downloadTypeIcon" /> > + <vbox> > + <description id="downloadsSummaryTarget" > + class="downloadTarget" crop="center" /> I don't think we want crop here ::: browser/components/downloads/src/DownloadsCommon.jsm @@ +203,5 @@ > + return this._summary; > + } > + > + return this._summary = new DownloadsSummaryData(aNumToExclude); > + }, why not making this a lazy getter (delete this.summary, this.summary =) @@ +285,5 @@ > + totalTransferred: totalTransferred, > + slowestSpeed: slowestSpeed, > + rawTimeLeft: rawTimeLeft, > + percentComplete: percentComplete > + }; you may instead initialize an object with default values and set its properties in the loop, then here just return that object, I don't see why we need all of those temp vars. ::: browser/locales/en-US/chrome/browser/downloads/downloads.dtd @@ +24,5 @@ > That's 50 characters, so we set the width at 50ch. > + > + Don't forget that you also have to take the contents of summarized > + downloads into account, too: you must include otherDownloads from > + downloads.properties in the list of strings whose length you consider. This comment may definitely improve, it's sort of unclear to me. it's also slightly wrong, in the sense otherDownloads is printed with a different ch size (it's a "target"), but this width is set on the details field that has a smaller ch. I think that in most cases this width will be enough to accomodate the size of otherDownloads text, though if that doesn't happen just adding more ch here won't prevent the target text to be cut. I can only see 2 solutions: 1. we accept the risk, thinking it's hard that other downloads will be longer than "59 minutes, 59 seconds remaining - 1022 of 1023 KB". If the problem arised we look at it later. 2. we add a further localized width specific to the otherDownloads target field and use it as a min-width. @@ +53,5 @@ > <!ENTITY cmd.removeFromList.accesskey "e"> > <!ENTITY cmd.clearList.label "Clear List"> > <!ENTITY cmd.clearList.accesskey "a"> > > +<!-- LOCALIZATION NOTE (showAllDownloads): The note points to a non existing entity, should be downloadsHistory.label

(In reply to Marco Bonardo [:mak] from comment #34) > Comment on attachment 682013 [details] [diff] [review] > Patch v2 > > Review of attachment 682013 [details] [diff] [review]: > ----------------------------------------------------------------- > > Some things to fix yet > > ::: browser/components/downloads/content/downloads.js > @@ +1460,5 @@ > > + this._detailsNode.collapsed = this._progressNode.collapsed > > + = !aShowingProgress; > > + this._showingProgress = aShowingProgress; > > + }, > > + _showingProgress: false, > > I think would be simpler to directly set/remove an "inprogress" attribute on > downloadsSummary, and use display:none in > browser/components/downloads/content/downloads.css (or viceversa, depending > on the default value), than having all of this setter code Ok, done. > > @@ +1473,5 @@ > > + set percentComplete(aValue) > > + { > > + if (this._progressNode) { > > + this._progressNode.setAttribute("mode", "normal"); > > + this._progressNode.setAttribute("value", aValue); > > couldn't you set mode="normal" directly in the xml? > Yes, done. > As a possiblity, you could set progressValue and textValue attributes > directly on summaryNode, and in the xml have xbl:inherits on the progressbar > and details, so basically you may completely remove _progressNode and > _detailsNode from this object. > As discussed in IRC, this can only be done in XBL bindings. > @@ +1476,5 @@ > > + this._progressNode.setAttribute("mode", "normal"); > > + this._progressNode.setAttribute("value", aValue); > > + } > > + > > + return aValue; > > nit: compact code a bit, remove newlines before the returns when there's a > brace before, they don't appear to be needed to help readability, there's > already quite a bit of empty space. > Done. > @@ +1486,5 @@ > > + * @param aValue > > + * A string representing the description of the summarized > > + * downloads. > > + */ > > + set target(aValue) > > While "target" makes sense for files, doesn't seem to make any sense in this > context, maybe label, summary, description, something else? > I chose "description". XUL and CSS was updated accordingly. > @@ +1488,5 @@ > > + * downloads. > > + */ > > + set target(aValue) > > + { > > + if (this._targetNode) { > > as well as targetNode > Yep, switched. > @@ +1517,5 @@ > > + > > + /** > > + * Element corresponding to the root of the downloads summary. > > + */ > > + get _rootNode() > > _rootNode is quite confusing cause too generic, let's make this _summaryNode > Done. > ::: browser/components/downloads/content/downloadsOverlay.xul > @@ +112,5 @@ > > + onclick="DownloadsPanel.showDownloadsHistory();"> > > + <image class="downloadTypeIcon" /> > > + <vbox> > > + <description id="downloadsSummaryTarget" > > + class="downloadTarget" crop="center" /> > > I don't think we want crop here > Ok, removed. > @@ +285,5 @@ > > + totalTransferred: totalTransferred, > > + slowestSpeed: slowestSpeed, > > + rawTimeLeft: rawTimeLeft, > > + percentComplete: percentComplete > > + }; > > you may instead initialize an object with default values and set its > properties in the loop, then here just return that object, I don't see why > we need all of those temp vars. > Done. > ::: browser/locales/en-US/chrome/browser/downloads/downloads.dtd > @@ +24,5 @@ > > That's 50 characters, so we set the width at 50ch. > > + > > + Don't forget that you also have to take the contents of summarized > > + downloads into account, too: you must include otherDownloads from > > + downloads.properties in the list of strings whose length you consider. > > This comment may definitely improve, it's sort of unclear to me. it's also > slightly wrong, in the sense otherDownloads is printed with a different ch > size (it's a "target"), but this width is set on the details field that has > a smaller ch. > I think that in most cases this width will be enough to accomodate the size > of otherDownloads text, though if that doesn't happen just adding more ch > here won't prevent the target text to be cut. > I can only see 2 solutions: > 1. we accept the risk, thinking it's hard that other downloads will be > longer than "59 minutes, 59 seconds remaining - 1022 of 1023 KB". If the > problem arised we look at it later. > 2. we add a further localized width specific to the otherDownloads target > field and use it as a min-width. I chose the more conservative path, and added a minWidth string to downloads.dtd. This is also shared with the downloadsTarget descriptions in download items so that we don't get varying widths in the event that the min-width is larger than the details width. > > @@ +53,5 @@ > > <!ENTITY cmd.removeFromList.accesskey "e"> > > <!ENTITY cmd.clearList.label "Clear List"> > > <!ENTITY cmd.clearList.accesskey "a"> > > > > +<!-- LOCALIZATION NOTE (showAllDownloads): > > The note points to a non existing entity, should be downloadsHistory.label Good catch, fixed.

(In reply to Marco Bonardo [:mak] from comment #34) > Comment on attachment 682013 [details] [diff] [review] > Patch v2 > > ::: browser/components/downloads/src/DownloadsCommon.jsm > @@ +203,5 @@ > > + return this._summary; > > + } > > + > > + return this._summary = new DownloadsSummaryData(aNumToExclude); > > + }, > > why not making this a lazy getter (delete this.summary, this.summary =) > I can't make it a getter because the views pass in an argument - the number of items to exclude. Or did I misunderstand?

(In reply to Mike Conley (:mconley) from comment #35) > As discussed in IRC, this can only be done in XBL bindings. yep, this was my fault due to rusted memory and the previous binding-based implementation, sorry. > I chose the more conservative path, and added a minWidth string to > downloads.dtd. This is also shared with the downloadsTarget descriptions in > download items so that we don't get varying widths in the event that the > min-width is larger than the details width. makes sense. (In reply to Mike Conley (:mconley) from comment #36) > I can't make it a getter because the views pass in an argument - the number > of items to exclude. Or did I misunderstand? missed that while reading, fine then

Comment on attachment 682508 [details] [diff] [review] Patch v3 Review of attachment 682508 [details] [diff] [review]: ----------------------------------------------------------------- mostly nits, good work ::: browser/components/downloads/content/download.xml @@ +25,5 @@ > <xul:vbox pack="center" > flex="1"> > <xul:description class="downloadTarget" > crop="center" > + style="min-width: &downloadsSummary.minWidth;" this may be worth a <!----> comment to state why we use a downloadsSummary width here ::: browser/components/downloads/content/downloads.js @@ +1419,5 @@ > + */ > +const DownloadsSummary = { > + > + /** > + * Sets the collapsed state of the summary, and automatically subscribes / nit: the / looks strange there, maybe just a plain "or" ::: browser/components/downloads/src/DownloadsCommon.jsm @@ +202,5 @@ > + if (this._summary) { > + return this._summary; > + } > + > + return this._summary = new DownloadsSummaryData(aNumToExclude); nit: may get rid of the newline @@ +235,5 @@ > + numScanning: 0, > + numDownloading: 0, > + totalSize: 0, > + totalTransferred: 0, > + // slowestSpeed will is Infinity so that we can use Math.min to typo: will is @@ +1105,3 @@ > { > // Update immediately even if we are still loading data asynchronously. > + // Subclasses must provide these two functions! Would be better to add these two functions in the prototype and make them throw as well, this comment alone is likely to be lost in the middle of the code. @@ +1158,5 @@ > + * References to existing data should be discarded. > + */ > + onDataInvalidated: function DVP_onDataInvalidated() > + { > + // Subclasses should override this. nit: would be nicer as a @note in the javadoc (not going to repeat) ::: browser/locales/en-US/chrome/browser/downloads/downloads.dtd @@ +33,5 @@ > + A good rule of thumb here is to look at the otherDownloads string > + in downloads.properties, and make a reasonable estimate of its > + maximum length. For English, this seems like a reasonable limit: > + > + +100 other current downloads nit: I'm sure people will think 100 is few, so psychologically I'd bump this to +999 :) ::: browser/themes/winstripe/downloads/downloads.css @@ +29,3 @@ > #downloadsPanel[hasdownloads] > #downloadsHistory { > background-color: hsla(216,45%,88%,.98); > box-shadow: 0px 1px 2px rgb(204,214,234) inset; nit: while here may you please remove the trailing space from here?

Thanks! Fixed all of those nits.

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

This was backed out while investigating a mess of orange that hit inbound all around the same time. It was re-landed once it was exonerated. Sorry for the churn. https://hg.mozilla.org/integration/mozilla-inbound/rev/a3019cbb0945

The idea of showing download rate at the bottom of the panel has been abandoned ?

(In reply to Guillaume C. [:ge3k0s] from comment #43) > The idea of showing download rate at the bottom of the panel has been > abandoned ? Not abandoned, but we're punting that bug to Firefox 20. As mentioned in the feature page (https://wiki.mozilla.org/User:P.A./Panel-based_Download_Manager), the feature as it currently exists will remain enabled in Aurora 19 to get feedback and shake out bugs. It will, however, be disabled in Beta and Firefox 19. Things like the Library-based downloads manager (bug 675902), the expanding download button (bug 801832) and the global rate (bug 812894) bugs will be tackled in the 20 cycle.

Thanks for the quick answer. The feature will be launched in Fx 20, but I think there will maybe be a v2 based on users comments ?

Verified as fixed on the latest Nightly - the progress of the downloads that are not visible in the panel is shown. I verified that the downloads progress is shown in the panel by having downloads in all the states mentioned in Comment 22 on Windows 7, Windows XP, Mac OS X 10.8.2 and Ubuntu 12.10: Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20121204 Firefox/20.0 Build ID: 20121204030754 Mozilla/5.0 (Windows NT 5.1; rv:20.0) Gecko/20121204 Firefox/20.0 Build ID: 20121204030754 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20121204 Firefox/20.0 Build ID: 20121204030754 Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20121204 Firefox/20.0 Build ID: 20121204030754