587970 - Provide ability "Update all now" within 'Available Updates' screen

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Paul [pwd]]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0b4pre) Gecko/20100816 Minefield/4.0b4pre
Build Identifier: Trunk

Currently users are forced to click "update now" on each individual addon and should you select 'Update Addons Now' you produce bug 587967. There should be a method for updating all addons at once from within that screen.

Reproducible: Always

Comment 1

[Paul [pwd]]

This is definitely a bug, being forced to update one at a time cannot be by design and must be an oversight.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #1)
> This is definitely a bug, being forced to update one at a time cannot be by
> design and must be an oversight.

Yea, that's the one thing that I didn't get done in bug 562622 (and then forgot about it).

Boriss: Want to clarify exactly what we want to do here?

And this would need a string change - so if we absolutely need it for 4.0, it needs to be blocking beta5.

Comment 3

[Jennifer Morrow [:Boriss] (UX)]

(In reply to comment #2)
> (In reply to comment #1)
> > This is definitely a bug, being forced to update one at a time cannot be by
> > design and must be an oversight.
> 
> Yea, that's the one thing that I didn't get done in bug 562622 (and then forgot
> about it).
> 
> Boriss: Want to clarify exactly what we want to do here?
> 
> And this would need a string change - so if we absolutely need it for 4.0, it
> needs to be blocking beta5.

Yes, there should be a global option at the top of the screen.  There is an outdated-but-still-correct-in-basic-functionality mockup of this here:

https://wiki.mozilla.org/images/c/cb/Manual_installation_of_addons_with_manual_pref_set23423.png

I'll attach a mockup that's similar but incorporates the UI changes that have happened since then to this bug in a couple hours.  Such a button would still exist.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Was surprised to find 2 hardcoded strings, so fixed that here too (they're update related). This also fixes an issue where the "no updates found" message could show when there were in fact updates shown (I'm sure there's a bug on that, but I can't find it right now).

The patch adds an "Update Selected Addons" button at the top of the Available Updates pane. Each update item has an "Include in Updates" checkbox - which defaults to being checked.

Boriss: For now, I've left the old "Update Now" button on each update item. That button works the same as it does in other views - updates only that individual addon. There's no other simple way to update just one specific addon from the Available Updates pane, so it kinda makes sense to keep it... but it does add more noise to the UI. So I'm not sure if it should be hidden in the Available Updates pane or not.

Comment 6

[Paul [pwd]]

(In reply to comment #5)
> This also fixes an issue where the "no updates found" message
> could show when there were in fact updates shown (I'm sure there's a bug on
> that, but I can't find it right now).

bug 584035

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 472928 [details] [diff] [review]
Patch v1

Pretty much there, couple of things to fix up though.

>diff --git a/toolkit/mozapps/extensions/content/extensions.css b/toolkit/mozapps/extensions/content/extensions.css

>+/* XXXunf move this stuff to themes */

Please

>+#updates-view[updatetype="available"] .addon .update-available {
>+  -moz-box-orient: vertical;
>+}

Doesn't seem right to flip the box like this. I think just putting the checkbox above the message/button would be better for consistency.

>diff --git a/toolkit/mozapps/extensions/content/extensions.js b/toolkit/mozapps/extensions/content/extensions.js

>   _showAvailableUpdates: function(aIsRefresh, aRequest) {
>+    this._updateSelected.disabled = true;

This defaults to disabled but the update checkboxes default to checked. Those two seem to be at odds.

>+  maybeDisableUpdateSelected: function() {
>+    var numSelected = 0;
>+    for (let i = 0; i < this._listBox.childNodes.length; i++) {
>+      let item = this._listBox.childNodes[i];
>+      if (item.includeUpdate)
>+        numSelected++;

No need for this counter, just set disabled = false and return when you find the first one and disabled = true if the loop ever completed.

>diff --git a/toolkit/mozapps/extensions/test/browser/head.js b/toolkit/mozapps/extensions/test/browser/head.js

> function get_addon_element(aManager, aId) {
>   var doc = aManager.document;
>   var view = doc.getElementById("view-port").selectedPanel;
>-  var listid = view.id == "search-view" ? "search-list" : "addon-list";
>+  var listid = "addon-list";
>+  if (view.id == "search-view")
>+    listid = "search-list";
>+  else if (view.id == "updates-view")
>+    listid = "updates-list";

Might work better as a switch. Maybe if it gets more cases though.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2

(In reply to comment #7)
> >+/* XXXunf move this stuff to themes */
> 
> Please

Oops. fixed.

> >+#updates-view[updatetype="available"] .addon .update-available {
> >+  -moz-box-orient: vertical;
> >+}
> 
> Doesn't seem right to flip the box like this. I think just putting the checkbox
> above the message/button would be better for consistency.

Yea, that was a bit of an ugly hack. Fixed.

> >   _showAvailableUpdates: function(aIsRefresh, aRequest) {
> >+    this._updateSelected.disabled = true;
> 
> This defaults to disabled but the update checkboxes default to checked. Those
> two seem to be at odds.

It gets disabled first so it can't get clicked before the async call completes. Then it will get re-enabled by gUpdatesView.maybeDisableUpdateSelected() once an item sends the IncludeUpdateChanged event (which also needs to wait for an async call in the binding constructor). Added a comment. 
As a bonus, this also means that once we eventually hook up remembering the state of the checkbox for each update, only the binding code needs changed.

I filed bug 594619 for remembering the preference for individual updates, although that seems beyond the scope of 4.0, given we don't yet persist *any* data about updates.

Also talked with Boriss, and clarified the wording. Updated the strings for the button accordingly.

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 473345 [details] [diff] [review]
Patch v2

>+  maybeDisableUpdateSelected: function() {
>+    this._updateSelected.disabled = false;
>+    for (let i = 0; i < this._listBox.childNodes.length; i++) {
>+      let item = this._listBox.childNodes[i];
>+      if (item.includeUpdate)
>+        return;
>+    }
>+    this._updateSelected.disabled = true;
>+  },

I think it'd be better to set disabled to false only when we're sure of the state, just before the return. I imagine it doesn't happen now but it's always possible this code could lead to the button flickering whenever you click a checkbox.

>+.addon .update-available {
>+  -moz-box-align: end;
>+}
>+
>+.addon .update-available > hbox {
>+  -moz-box-align: center;
>+}

We normally do this stuff in the XUL, any reason to put it here?

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2.1 - ready for checkin

(In reply to comment #9)
> We normally do this stuff in the XUL, any reason to put it here?

No reason really - moved it to XUL attributes.

Comment 11

[Dietrich Ayala (:dietrich)]

http://hg.mozilla.org/mozilla-central/rev/93785fd6f873

Comment 12

[Aaron Train [:aaronmt]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20100917 Firefox/4.0b7pre