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

VERIFIED FIXED in mozilla2.0b7

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: Paul [pwd], Assigned: Unfocused)

Tracking

Trunk
mozilla2.0b7
Points:
---
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

(Whiteboard: [strings][has patch][AOMTestday])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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
(Reporter)

Updated

7 years ago
Blocks: 562622
Severity: normal → enhancement
(Reporter)

Comment 1

7 years ago
This is definitely a bug, being forced to update one at a time cannot be by design and must be an oversight.
(Reporter)

Updated

7 years ago
Severity: enhancement → normal
(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.
blocking2.0: --- → ?
Keywords: uiwanted
(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.
Duplicate of this bug: 591027
Status: UNCONFIRMED → NEW
blocking2.0: ? → beta6+
Ever confirmed: true
Assignee: nobody → bmcbride
Whiteboard: [strings]
Created attachment 472928 [details] [diff] [review]
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.
Attachment #472928 - Flags: ui-review?(jboriss)
Attachment #472928 - Flags: review?(dtownsend)
Status: NEW → ASSIGNED
(Reporter)

Comment 6

7 years ago
(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 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.
Attachment #472928 - Flags: review?(dtownsend) → review-
Created attachment 473345 [details] [diff] [review]
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.
Attachment #472928 - Attachment is obsolete: true
Attachment #473345 - Flags: review?(dtownsend)
Attachment #472928 - Flags: ui-review?(jboriss)
Whiteboard: [strings] → [strings][has patch][needs review 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?
Attachment #473345 - Flags: review?(dtownsend) → review+
Whiteboard: [strings][has patch][needs review mossop] → [strings][has patch]
Created attachment 473986 [details] [diff] [review]
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.
Attachment #473345 - Attachment is obsolete: true
Flags: in-testsuite+
Flags: in-litmus-
Keywords: uiwanted → checkin-needed
http://hg.mozilla.org/mozilla-central/rev/93785fd6f873
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20100917 Firefox/4.0b7pre
Status: RESOLVED → VERIFIED
Whiteboard: [strings][has patch] → [strings][has patch][AOMTestday]
Target Milestone: --- → mozilla2.0b7
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.