All users were logged out of Bugzilla on October 13th, 2018

Implement new Download Manager status indicator for browser

RESOLVED DUPLICATE of bug 726444

Status

()

RESOLVED DUPLICATE of bug 726444
7 years ago
6 years ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 538839 [details] [diff] [review]
Work in progress

The last part of the new Download Manager user interface is the status indicator
that replaces the former "Downloads" toolbar button.

The current incarnation presents the following features:
 * Counter of the number of active downloads.
 * Circular download progress indicator, if total size is known.
 * Continuously rotating indicator, if total size is unknown.
 * Indication if all downloads are paused (yellow progress indicator).
 * Indication of the presence of completed downloads (stable green).
 * Indication of the presence of failed downloads (stable red).
 * Attention indication for completed downloads (blinking green).
 * Attention indication for failed downloads (blinking red).
(Assignee)

Comment 1

7 years ago
Created attachment 538844 [details]
One completed download, and another one paused

While waiting for the tryserver builds, here is a screenshot showing the
indicator and the downloads panel when there is one completed download and
one download paused at about 65%, with the default theme on Windows XP.

Note that the default position of the indicator is now the tab bar, in part
because download items are a logical extension of the set of open, background
tasks represented by tabs, and in part because this way the indicator remains
visible when browsing pages that hide the navigation bar.
(Assignee)

Comment 2

7 years ago
Created attachment 538846 [details]
Full screen, one failed download, and another one in progress

Another screenshot, this time showing the indicator in full screen, tabs-on-top
mode with the default theme on Windows XP.
(Assignee)

Comment 3

7 years ago
Created attachment 550151 [details] [diff] [review]
Interaction nearly final, no visual design

This is the patch corresponding to the work done in bug 564934.
Attachment #538839 - Attachment is obsolete: true
Attachment #550151 - Flags: review?(mak77)
Comment on attachment 550151 [details] [diff] [review]
Interaction nearly final, no visual design

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

Ok, I finally did a first pass, I know you are near having a new version, so you may include these comments where still applicable. The separate objects approach seems fine, it just takes a while to get the full picture.

::: browser/components/downloads/content/downloadsOverlay.xul
@@ +85,5 @@
>           command="downloadsCmd_pauseResume"/>
>    </keyset>
>  
>    <popupset>
> +    <toolbaritem id="downloads-indicator"

why is this in the popupset? sounds like a weird position for a button element

::: browser/components/downloads/content/indicator.js
@@ +93,5 @@
> +  /**
> +   * This function is called asynchronously just after window initialization.
> +   */
> +  initializeIndicator: function DIC_initializeIndicator() {
> +    this.customizeDone();

For clarity I'd prefer if you create an internal _initialize() method and invoke it from both here and customizeDone

@@ +102,5 @@
> +   */
> +  _customizing: false,
> +
> +  /**
> +   * This function is called when toolbar customization starts.

a brief description of what is expected to do is welcome, like why it is expected to collapse indicator and that it will show the placeholder if not in palette and such. Briefly what is expected to happen when customization starts.

@@ +122,5 @@
> +  customizeDone: function DIC_customizeDone() {
> +    this._customizing = false;
> +    this.updatePosition();
> +
> +    // Initialize the view unless the indicator was removed from the toolbars.

and hide the placeholder unless it has been moved to the palette

@@ +134,5 @@
> +  },
> +
> +  /**
> +   * Determines the position where the indicator should appear, and moves the
> +   * element to the new position.

Is unclear what "the element" is, maybe should be "its associated element" or something like that

@@ +144,5 @@
> +    let placeholder = this._placeholder;
> +
> +    // Firstly, determine if we should always hide the indicator.
> +    if ((!placeholder || DownloadsIndicatorView.canHide) &&
> +        !this._anchorNeeded) {

the comment doesn't help me figure out the cases this happens, and the code is not self-documenting, you may improve the comment or split the condition using a anchorHidden = !placeholder || DownloadsIndicatorView.canHide; if (anchorHidden && !this._anchorNeeded)

@@ +150,5 @@
> +      return null;
> +    }
> +
> +    // Show the indicator if it's located in a visible position.
> +    indicator.collapsed = false;

I don't think that comment is useful

@@ +155,5 @@
> +
> +    // The visibility of the navigation bar is determined by the style sheets.
> +    let navBarStyle = window.getComputedStyle(this._navBar, "");
> +    let navBarCollapsed = (navBarStyle.visibility != "visible" ||
> +                           navBarStyle.display == "none");

May we use isElementVisible from utilityOverlay on this specific bar?

@@ +165,5 @@
> +      if (placeholder.parentNode == this._navBar) {
> +         if (!navBarCollapsed) {
> +           return indicator;
> +         }
> +      } else if (!placeholder.parentNode.collapsed) {

same here, if we may use isElementVisible

@@ +186,5 @@
> +      }
> +      return indicator;
> +    }
> +
> +    // Show the indicator temporarily in the navigation bar, if visible.

I feel a bit overwhelmed by all these special cases, I think that if the button has been removed by the user and tabs toolbar is hidden, we should just do nothing, the user clearly wants to reduce his toolbars content, we should repsect that (and simplify our life).

@@ +199,5 @@
> +
> +  /**
> +   * Indicates whether we must show the anchor for the panel if possible.
> +   */
> +  _anchorNeeded: false,

the naming confuses me, since it's unclear what "needed" means. Also the comment says "we must show" "if possible", where the latter contradicts the former.
At that point I'd prefer a less imperative _tryToAnchorPanel or _anchorRequested. Or, since if there is an anchor available we always want to anchor to it, we may just check the anchor getter to see if one is available?

@@ +301,5 @@
> +  },
> +  get canHide() {
> +    return this._canHide;
> +  },
> +  _canHide: true,

I find this canHide member confusing, it looks like a bool explaining a property of the indicator, but it's inited to null and set to (this._itemCount == 0).
This is more like a hiddenOnEmptyResultset property (inited to false too), maybe the name sucks a bit, bue feels like an improvement.
Also, per convention the setters should always return the final value (fix other setters too, please)

@@ +304,5 @@
> +  },
> +  _canHide: true,
> +
> +  /**
> +   * Text displayed in the indicator.

s/Text/Status text/

@@ +311,5 @@
> +    if (this._counter !== aValue) {
> +      this._counter = aValue;
> +      this._indicatorImage.setAttribute("hidden", aValue ? "true" : "false");
> +      this._indicatorCounter.setAttribute("hidden", !aValue ? "true" : "false");
> +      this._indicatorCounter.setAttribute("value", aValue);

are you forced to use the attributes here rather than the properties?
btw, using removeAttribute in some case may be more efficient than setAttribute to false

@@ +375,5 @@
> +  },
> +
> +  onClick: function DI_onClick(aEvent)
> +  {
> +    aEvent.stopPropagation();

could you just preventDefault here? what are you trying to prevent?

::: browser/components/downloads/src/DownloadsCommon.jsm
@@ +878,5 @@
> +  /**
> +   * Array of view objects that should be notified when the available status
> +   * data changes.
> +   */
> +  views: [],

this can be internal

@@ +892,5 @@
> +  addView: function DIV_addView(aView)
> +  {
> +    // Start receiving events when the first of our views is registered.
> +    if (!this.views.length) {
> +      this._itemCount = 0;

may _itemCoun be != 0 if we are not notified? it is already inited to 0 in its declaration, should rather probably be set to 0 in removeView if we remove the last view?

@@ +1029,5 @@
> +  // propagated to the views.  See _refreshProperties for details.
> +  _canHide: null,
> +  _counter: null,
> +  _percentComplete: null,
> +  _paused: null,

canHide and paused look like boolean, and should be inited to false, counter to 0, percentComplete to -1

@@ +1064,5 @@
> +      return;
> +    }
> +
> +    this._refreshProperties();
> +    for (let [, view] in Iterator(this.views)) {

once this.views becomes this._views, the need to protect it vanishes, then you can avoid the iterator

::: browser/themes/winstripe/browser/downloads/indicator.css
@@ +59,5 @@
> +#downloads-indicator[paused]
> +#downloads-indicator-progress {
> +  background: #FF0
> +              -moz-linear-gradient(rgba(251,252,128,.95), rgba(246,247,128,.47) 49%,
> +                                   rgba(231,232,128,.45) 51%, rgba(225,226,128,.3));

that background hardcoded color sounds strange, what's it for?

@@ +76,5 @@
> +}
> +
> +#downloads-indicator[paused]
> +#downloads-indicator-progress > .progress-remainder {
> +  background-color: #993;

what's this hardcoded color for?
Attachment #550151 - Flags: review?(mak77) → feedback+
(Assignee)

Comment 5

7 years ago
Created attachment 556581 [details] [diff] [review]
Draft of visual design, and another round of code review

This is the patch corresponding to the work done in bug 564934.
Attachment #550151 - Attachment is obsolete: true
(Assignee)

Comment 6

7 years ago
Created attachment 558504 [details] [diff] [review]
First version for user experience feedback

This is the patch corresponding to the work done in bug 564934.
Attachment #556581 - Attachment is obsolete: true

Comment 7

7 years ago
Paolo Amadini,
Can we use similar mechanism for the bug 249338 ? I have added an attachment there https://bugzilla.mozilla.org/show_bug.cgi?id=249338#c46 reflecting the idea. Thanks :)
(Assignee)

Comment 8

7 years ago
(In reply to bogas04 from comment #7)
> Can we use similar mechanism for the bug 249338 ?

I've replied in bug 249338 comment 48 with my current thinking about the upload
progress indication, thanks for asking :-) Feel free to CC me directly on any
bug if you think I can provide useful feedback.
(Assignee)

Comment 9

7 years ago
Created attachment 574269 [details] [diff] [review]
First version for user experience feedback (updated)

This is the patch corresponding to the work done in bug 564934.
Attachment #558504 - Attachment is obsolete: true
(Assignee)

Comment 10

7 years ago
The patch on this bug is now included as part of the patch in bug 726444. Any
pending issue here should have already been filed as one of the bug's follow-ups.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 726444
You need to log in before you can comment on or make changes to this bug.