747903 - Limit the number of items in the Downloads Panel

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Benjamin Smedberg]

When I first saw the new download panel in Nightly I kinda freaked out because I could figure out how to close it (I kept trying to move it using the window borders). Eventually I accidentally got rid of it by clicking in the browser window (which automatically hides it) but I was unnerved for a while.

In the design screenshots at https://wiki.mozilla.org/User:P.A./Panel-based_Download_Manager/Status the download panel visually drops down from the download panel button with an arrow. This makes it clearer how to get rid of the window (by clicking the downloads button again). In the current implementation (at least on Win7) the panel is completely visually divorced from the button and actually overlaps it.

Comment 1

[Benjamin Smedberg]

Attachment: screenshot

Comment 2

[Jim Jeffery not reading bug-mail 1/2/11]

Attachment: New DM Doorhanger Win7

I don't see that, perhaps your Personna is not playing nice.
See attachment

Comment 3

[Siddhartha Dugar [:sdrocking]]

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #2)
> Created attachment 617465 [details]
> New DM Doorhanger Win7
> 
> I don't see that, perhaps your Personna is not playing nice.
> See attachment

The dark icon is visible on the default theme with tabs on bottom.

The height of the list must be restricted somehow so that the arrow connector is always visible. Otherwise the UI gives no clue on how to close the doorhanger.

Comment 4

[Benjamin Smedberg]

ok, this is not caused by personas or themes or addons. It does appear to be caused by the panel being too high. If I delete my downloads then the panel does connect properly. So if there is a bug tracking "3.5 The panel should never extend to the full screen height" from the status page then I guess that this can be duped to that.

Comment 5

[Daniel C]

Bug 746759 is about the overlapping.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

yes, there is a problem when the list is too large the arrow panels lose the anchor.

Comment 7

[omeringen]

Confirmed. You need at least 12 downloads on your list to reproduce this issue.

Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/15.0 Firefox/15.0a1

Comment 8

[:Paolo Amadini]

Attachment: Limit the number of items in the Downloads Panel.

Here we have a fixed, small number of items at any time in the panel. Three
items seems appropriate for most desktop screen sizes: the panel anchors even
if the button is positioned vertically in the center of the screen. Three items
are also enough for handling recent downloads in most cases, considering that
other downloads are one click away.

Varying the number of items with the screen height is probably unnecessary,
and complicated by multi-monitor scenarios.

The visual design is not final, will update when we have more mockups later.

Comment 9

[Axel Hecht [:Pike]]

Comment on attachment 630684 [details] [diff] [review]
Limit the number of items in the Downloads Panel.

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

::: browser/locales/en-US/chrome/browser/downloads/downloads.properties
@@ +73,5 @@
> +showAllDownloadsAccessKey=S
> +showOneMoreDownload=Show 1 More Recent Download
> +showOneMoreDownloadAccessKey=S
> +showManyMoreDownloads=Show %1$S More Recent Downloads
> +showManyMoreDownloadsAccessKey=S

This should use proper plural form handling, and it probably could use some comments on when which string is shown, plus, that they're mutually exclusive. I'm not sure we should support differing accesskeys here, too.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 630684 [details] [diff] [review]
Limit the number of items in the Downloads Panel.

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

Well, first of all, what Axel said, this needs PluralForm.jsm love.
Second, I think we are going too hard here, 3 is a really low number, I'd go for 7, and reduce later where needed. Expecially since this will suck until the Library is ready. Ideally this should be calculated dinamically (available screen / height of one download).

The approach looks fine to me, now to the comments

::: browser/components/downloads/content/downloads.js
@@ +440,5 @@
>  
>    /**
> +   * Ordered array of all the available DownloadsDataItem objects.  This list is
> +   * consulted when items are removed from the panel, to allow invisible items
> +   * to reappear at the bottom.

IT's sort of unclear what "reappear" means here, more specifically when and why.

@@ +568,5 @@
> +    if (aNewest || !hasOverflow) {
> +      // The newly added item is visible in the panel.
> +      this._addViewItem(aDataItem, aNewest);
> +    }
> +    if (aNewest && hasOverflow) {

supposing you added the element in the first if, shouldn't you recalculate hasOverflow here? IT could have been at limit

@@ +576,5 @@
> +
> +    // Don't update the count for every item during batch loads.
> +    if (!this.loading) {
> +      this._itemCountChanged();
> +    }

Otherwise what happens? just perf? please clarify in the comment

@@ +595,5 @@
> +    if (itemIndex < this.kItemCountLimit) {
> +      // The item to remove is visible in the panel.
> +      this._removeViewItem(aDataItem);
> +      if (this._dataItems.length >= this.kItemCountLimit) {
> +        // Resurrect the last item in the panel.

nit: resurrect is a bit exaggerated ("reinsert", "add back" and last should be "next"
"Eventually reinsert the next item into the panel."?

@@ +615,5 @@
>    getViewItem: function DV_getViewItem(aDataItem)
>    {
> +    if (aDataItem.downloadId in this._viewItems) {
> +      return this._viewItems[aDataItem.downloadId];
> +    }

add comment on top, just to briefly explain the code flow (if visible just return it, otherwise...)

@@ +622,5 @@
> +      onStateChange: function () { },
> +      onProgressChange: function () { }
> +    });
> +
> +    return invisibleViewItem;

cause yeah, this is not exactly crystal clear... something like "return a mock item that doesn't react to notifications"?
you can also directly return Object.freeze(...

::: browser/components/downloads/content/downloadsOverlay.xul
@@ -112,1 @@
>                oncommand="DownloadsPanel.showDownloadsHistory();"/>

hm, from the mockup I see there is a static text saying (you have X more recent downloads) and a show all history button, did that change so the button is saying what the static was?

::: browser/components/downloads/src/DownloadsCommon.jsm
@@ +65,5 @@
>    shortTimeLeftHours: true,
>    shortTimeLeftDays: true,
>    statusSeparator: true,
>    statusSeparatorBeforeNumber: true,
> +  showManyMoreDownloads: true,

where is this used?

::: browser/components/downloads/test/browser/browser_basic_functionality.js
@@ +50,5 @@
>        is(dataItem.file, DownloadData[i].target, "Download targets match up");
>        is(dataItem.uri, DownloadData[i].source, "Download sources match up");
>      }
>    } finally {
> +    DownloadsView.kItemCountLimit = originalCountLimit;

Or use a RegisterCleanupFunction up there? Sounds a bit safer vs timeouts

Comment 11

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] (Away 28 Jul - 12 Aug) from comment #10)
> Well, first of all, what Axel said, this needs PluralForm.jsm love.

Ok, I'll special-case this in the string handling module.

> Second, I think we are going too hard here, 3 is a really low number, I'd go
> for 7, and reduce later where needed.

Ok, we can try, though I expect that with more than 3 elements we'll still have
a significant number of cases where the panel deanchors or gets a scrollbar.

I found 3 items comfortable in any case, since with the new design this is just
a status notification and not the management interface.

> Ideally this should be calculated dinamically
> (available screen / height of one download).

The "available screen" part is complicated. It depends on the current button
position (it could be in the center of the screen) and the current monitor's
viewport size (you could have multiple monitors, the available space changes
depending on which monitor displayes the panel).

A height of 3 items was comparable to other doorhangers, thus safer, probably
fitting in the space even in the worst cases.

> ::: browser/components/downloads/content/downloadsOverlay.xul
> @@ -112,1 @@
> >                oncommand="DownloadsPanel.showDownloadsHistory();"/>
> 
> hm, from the mockup I see there is a static text saying (you have X more
> recent downloads) and a show all history button, did that change so the
> button is saying what the static was?

If the mockup is still valid, I think we can implement the button later in a
followup, but will probably need more mockups and styling for all platforms.

Comment 12

[u428464]

Five or six seems to be a good compromise to me, but the best solution would be to change the whole panel behaviour concerning the edge of the screen. Currently the panels are moved to always appear on the screen even if the anchor doesn't indicate the right place. Google Chrome's behaviour seems more logical with the panels cut if off-screen so the arrow always shows the center of the attached icon.

Comment 13

[:Paolo Amadini]

Attachment: Limit the number of items in the Downloads Panel less.

This patch has the limit set to seven items as requested, although I think we
should also consider lowering the limit to five as was suggested, or even less.

Comment 14

[:Paolo Amadini]

Attachment: Limit the number of items in the Downloads Panel.

I talked with Gavin, and decided to go for three items in the panel initially,
as this should cover most use cases and we can play with the number later.

Comment 15

[Asaf Romano (gone)]

Paulo, sorry if I'm raising a point already discussed... Any reason not to compute the number of items we can show without a scrollbar? It should be doable. If we go for three items, many users would have to use the Library UI quite often.

Comment 16

[:Paolo Amadini]

(In reply to Mano from comment #15)
> Paulo, sorry if I'm raising a point already discussed... Any reason not to
> compute the number of items we can show without a scrollbar? It should be
> doable.

It can be done but it's not trivial, see comment 11. We might even need platform
support that we don't have right now. Thus, we can start with a fixed number and
maybe implement the computation in a follow-up.

> If we go for three items, many users would have to use the Library
> UI quite often.

We concluded that one or two concurrent downloads will cover most use cases. In
fact, optimizing for a small number of downloads has been one of our design
assumptions from the beginning:

https://wiki.mozilla.org/User:P.A./Download_user_experience_improvements#Overall_goals

We might be wrong on the actual number, but until we land this patch in Nightly,
we cannot even start to get actual usage feedback about this.

Comment 17

[Asaf Romano (gone)]

Paulo, please see SetPopupPosition and its helpers in in nsMenuPopupFrame. In short, all you need is a JS helper that mimics its available-area detection. Unless I missed something, it does not use any API that's not available to JS (it uses nsIScreenManager and nsIScreen).
If you're looking for an example of using this api in js, see the dragend handler in tabbrowser.xml

I'm afraid that the limitation you're about to introduce here isn't as minor as you suggest. It's minor enough that I wouldn't block the feature for not having it *iff* we know non-trivial platform work is necessary. But let's first double check that's the case in question. It's definitely not minor enough to consider it a possible-future-enhancement, in my opinion.

Comment 18

[Asaf Romano (gone)]

I've been discussing this at length with Paolo. To summarize:
1. We're not sure what is the *desired* behavior. If it's just fixing this bug, then the dynamically-sized list is a better solution. If, as Paolo suggested, the design implies that a very short list is the goal regardless of this bug, then we should go with the fixed-number of items solution. Paolo will correspond with UX about this.
2. It turns out I'm more optimistic than him about how hard would it be to implement the the dynamically-sized list solution. Of course, I'm right and he's wrong :)  Anyway, we'll evaluate this once (1) is sorted out.

Comment 19

[Asaf Romano (gone)]

Technical note to Paulo:

For getting the available height, I think it should be as easy as:

window.screen.availHeight - element.boxObject.screenY

window.screen.availHeight, iirc, only considers the monitor on which the window is placed, so this should Just Work. So, you may not even need to go though the gfx interface.

But it's almost midnight ;)

Comment 20

[:Paolo Amadini]

I moved the technical discussion of how to compute the number of items dynamically
to bug 780837, because it will proceed in parallel, while we try to figure out
what is the next thing we want to make available in Nightly.

Comment 21

[:Paolo Amadini]

Comment on attachment 648351 [details] [diff] [review]
Limit the number of items in the Downloads Panel.

So, the questions for UX here is related to the Downloads Panel's design we
discussed during the most recent design iteration, and whose notes are still
summarized in the wiki:

https://wiki.mozilla.org/User:P.A./Panel-based_Download_Manager#Downloads_panel

The first is about the final design, the second about what we should do next.

1. Do we want the panel to be able to show a potentially large number of items
   (for example, the 15 most recent downloads) if the screen height allows it,
   to facilitate management of concurrent downloads from the panel where
   possible? Or do we want to always show a small number of items (for
   example, 3 downloads) in order to clearly separate download management
   (done in the Library window) from the notification interface? Or do we want
   to have a minimum and a maximum number of items?

2. Even if we want a list with a dynamic number of items, do you see any reason
   not to try a solution with a small, fixed number of items right now, before
   we land the dynamic one later (even some weeks later, to allow testing)?

Technically, we're working on the dynamic solution in bug 780837, we might need
some minor clarifications later.

Comment 22

[Asaf Romano (gone)]

> 2. Even if we want a list with a dynamic number of items, do you see any reason
>    not to try a solution with a small, fixed number of items right now, before
>    we land the dynamic one later (even some weeks later, to allow testing)?

As I told you, that's not a UX question. The more complicated a solution is, it should land _earlier_ in the development cycle. If it turns out to be problematic, we can always take it back later for a more simple and conservative solution.

Comment 23

[Asaf Romano (gone)]

Another question: what's the order of items in the popup? For example, if I started 4 downloads, and the first one completed first, as expected. Would I have to open the Library UI in order to open the completed download?

Comment 24

[:Paolo Amadini]

(In reply to Mano from comment #22)
> > 2. Even if we want a list with a dynamic number of items, do you see any reason
> >    not to try a solution with a small, fixed number of items right now, before
> >    we land the dynamic one later (even some weeks later, to allow testing)?
> 
> As I told you, that's not a UX question. The more complicated a solution is,
> it should land _earlier_ in the development cycle. If it turns out to be
> problematic, we can always take it back later for a more simple and
> conservative solution.

Well, I can agree the dynamic list could land earlier instead of waiting some
weeks. I'm just trying to understand whether the dynamic list (even if it turns
out to require just a few calendar days to be implemented properly) should
prevent all the other changes we already made from landing (exactly for the same
reasons of landing significant changes as early as possible).

Comment 25

[:Paolo Amadini]

(In reply to Mano from comment #23)
> Another question: what's the order of items in the popup? For example, if I
> started 4 downloads, and the first one completed first, as expected. Would I
> have to open the Library UI in order to open the completed download?

With the limit set to three, that's right. With the limit set to 10, you would
have to assume you started 11 concurrent downloads.

The fact is that we don't know how many concurrent downloads are normally
executed by 80% of Firefox users (80/20/2 rule), we're making assumptions here.

Comment 26

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Paolo Amadini [:paolo] from comment #25)
> The fact is that we don't know how many concurrent downloads are normally
> executed by 80% of Firefox users (80/20/2 rule), we're making assumptions
> here.

Add a telemetry probe!

Comment 27

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] from comment #26)
> (In reply to Paolo Amadini [:paolo] from comment #25)
> > The fact is that we don't know how many concurrent downloads are normally
> > executed by 80% of Firefox users (80/20/2 rule), we're making assumptions
> > here.
> 
> Add a telemetry probe!

I has the same thought. But I wouldn't really want to block landing this bug
until the telemetry probe results for release channels come in...

Comment 28

[:Paolo Amadini]

Comment on attachment 648351 [details] [diff] [review]
Limit the number of items in the Downloads Panel.

I just talked with Gavin, and we're considering landing the version with the
limit hardcoded to 3 items, while we figure out how to solve the issues in
bug 780837. Asaf, would you take a look at the code and see if there is
something I should fix before this patch can land?

Comment 29

[Asaf Romano (gone)]

OK...

I'll look into this today/tomorrow, yes.

Comment 30

[Asaf Romano (gone)]

Comment on attachment 648351 [details] [diff] [review]
Limit the number of items in the Downloads Panel.

Generally, _I think_ you could simplify the code much by having just the view items array, with the actual elements created lazily, and then hide/unhide them as necessary. That would also play better with the dynamic approach we want to land later. "Syncing" arrays like this tends to be problematic in the long run, but I'm fine with landing this as-is to avoid more delays.

>   //// Functions handling download items in the list
> 
>   /**
>+   * The list will show at most this number of items at a time.

Please rephrase this as a definition ("The maximum ...")

>+   */
>+  kItemCountLimit: 3,
>+
>+  /**

By the way, I would consider a hidden-preference if we go with this approach after all. No biggie for now.

>    * Indicates whether we are still loading downloads data asynchronously.
>    */
>   loading: false,
> 
>   /**
>-   * Object containing all the available DownloadsViewItem objects, indexed by
>-   * their numeric download identifier.
>+   * Ordered array of all the available DownloadsDataItem objects.  

hrm, "all of the" or "all available" sounds more natural to me.

>  We need to
>+   * keep this array because only a limited number of items are shown at any
>+   * given time,

s/given time/once/

or "any given time" as you stated later. Both work.

Please try to simplify this comment. I think the concept is pretty clear.


>+
>+    let s = DownloadsCommon.strings;
>+    let label = (hiddenCount > 0) ? s.showMoreDownloads(hiddenCount)
>+                                  : s.showAllDownloads;
>+    this.downloadsHistory.setAttribute("label", label);
>+    this.downloadsHistory.setAttribute("accesskey", s.showDownloadsAccessKey);

Always prefer using properties rather than attributes for elements that are already in the document.


>   onDataLoadCompleted: function DV_onDataLoadCompleted()
>   {
>     this.loading = false;
> 
>+    this._itemCountChanged();
>+

The count didn't change, so either comment above this call explaining why this is necessary, or better, rename _itemCountChanged to what it does now.

>+      this._addViewItem(aDataItem, aNewest);
>+    }
>+    if (aNewest && itemsNowOverflow) {
>+      // Remove the last item from the panel to make room for the new one that
>+      // we just added at the top, unless the list still doesn't overflow.

"If the list overflows..."

>+      this._removeViewItem(this._dataItems[this.kItemCountLimit]);
>+    }
>+
>+    // Don't update the count for every item during batch loads, for better
>+    // performance, because the interface won't be visible until load finishes.

"For better performance, ..."

>+    // If the item is visible, just return it, otherwise return a mock object
>+    // that doesn't react to notifications.
>+    if (aDataItem.downloadId in this._viewItems) {
>+      return this._viewItems[aDataItem.downloadId];
>+    } else {

No |else| after return.

>+      return this._invisibleViewItem;

I would just inline this stub object.

Comment 31

[:Paolo Amadini]

Attachment: Final patch

(In reply to Mano from comment #30)
> Generally, _I think_ you could simplify the code much by having just the
> view items array, with the actual elements created lazily, and then
> hide/unhide them as necessary.

This is a good refactoring idea. I kept the the view items object for
performance reasons when receiving progress notifications (they are frequent),
but given the small number of visible items we can now also iterate over an
array to find the element.

> >+      return this._invisibleViewItem;
> 
> I would just inline this stub object.

I preferred to reuse the same object every time, because this function might
be called often. I'm not sure whether the JavaScript engine optimizes this
case, but this is probably more friendly to garbage collection.

Comment 32

[:Paolo Amadini]

https://hg.mozilla.org/integration/mozilla-inbound/rev/52540206fb91

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/52540206fb91

Comment 34

[Simona Badau, Desktop QA]

Verified as fixed on the latest Nightly - the downloads number is now limited to three items. 

Verified on Windows 7, Ubuntu 12.04 and Mac OS X 10.7:

Mozilla/5.0 (Windows NT 6.1; rv:19.0) Gecko/19.0 Firefox/19.0        
Build ID: 20121111030749
Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/19.0 Firefox/19.0        
Build ID: 20121112030753
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:19.0) Gecko/19.0 Firefox/19.0        Build ID: 20121112030753

Comment 35

[:Paolo Amadini]

Comment on attachment 648351 [details] [diff] [review]
Limit the number of items in the Downloads Panel.

Just clearing this ancient ui-review request, that has been addressed already.