Only show "Available Updates" pane when pending updates are available

VERIFIED FIXED in mozilla2.0b8

Status

()

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

People

(Reporter: whimboo, Assigned: mossop)

Tracking

Trunk
mozilla2.0b8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
Created attachment 469581 [details]
screenshot

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b5pre) Gecko/20100826 Minefield/4.0b5pre

When you select the "Available Updates" pane and check for updates, and an update or more are found, the "no updates found" label and the update button don't get removed. There is also a huge empty area above the first found update. Re-selecting the pane fixes the problem. 

Steps:
1. Install an outdated version of an extension, i.e. Dom Inspector 2.0.6
2. Disable automatic updates for this extension
3. Select the Automatic Update pane
4. Search for updates

After step 4 you will see something like in the screenshot.

I wonder if we always want to show the "Available Updates" pane or only when there are really updates in the queue.
(Reporter)

Updated

7 years ago
blocking2.0: --- → ?
(Assignee)

Updated

7 years ago
blocking2.0: ? → final+
(Reporter)

Comment 1

7 years ago
As discussed in yesterdays AMO meeting we only want to show the "Available Updates" pane when pending updates exist. That means the issue reported in comment 0 will be gone too with that change.
Blocks: 562622
Summary: List of available updates is not cleared correctly - shows "no updates found" even with updates listed → Only show "Available Updates" pane when pending updates are available
(In reply to comment #1)
> As discussed in yesterdays AMO meeting we only want to show the "Available
> Updates" pane when pending updates exist. That means the issue reported in
> comment 0 will be gone too with that change.

Er, really? That changed... again? Is there a bug for that?
(In reply to comment #2)
> Er, really? That changed... again? Is there a bug for that?

Er, wow, ignore this.
(Assignee)

Comment 4

7 years ago
But seriously, /is there/ a bug for that?
I think this bug is now intended to be for that.
(Reporter)

Updated

7 years ago
Duplicate of this bug: 593022
(Assignee)

Updated

7 years ago
blocking2.0: final+ → -
(Reporter)

Comment 7

7 years ago
Now that this bug is not blocking anymore, how do we we want to fix the presentation of the list view so we do not show the big empty area (see screenshot)?
(Assignee)

Comment 8

7 years ago
Ah yeah, we have to fix one or the other and hiding the pane is probably the simplest
blocking2.0: - → final+
(Assignee)

Updated

7 years ago
Assignee: nobody → dtownsend
(Assignee)

Comment 9

7 years ago
Created attachment 488275 [details] [diff] [review]
patch rev 1

Saves us some code to do this, added tests to cover it.
Attachment #488275 - Flags: review?(bmcbride)
(Assignee)

Updated

7 years ago
Whiteboard: [has patch][needs review Unfocused]
Comment on attachment 488275 [details] [diff] [review]
patch rev 1

>+
>+    this._categoryItem.disabled = this.._categoryItem.badgeCount == 0;

That .. is throwing with an oh-so-helpful E4X exception.

>-  onPropertyChanged: function(aAddon, aProperties) {
>-    if (aProperties.indexOf("applyBackgroundUpdates") != -1)
>-      this.updateManualUpdatersCount();
>-  }

Sadly, removing this makes the following edge case possible:
1. Set addon to manually update
2. Check for updates, and find an update
3. Available Updates pane shows, with "1" in its badge
4. Disable manual updates for that addon
5. Available Updates pane remains visible, with "1" in its badge
6. Switching to the Available Updates pane will show the empty notice

Are there any other possible ways to get to the Available Updates pane, and have it show the empty notice? If not, then that should be able to be removed.
Attachment #488275 - Flags: review?(bmcbride) → review-
(Assignee)

Comment 11

7 years ago
Created attachment 491324 [details] [diff] [review]
patch rev 2

Updated and de-bitrotted.

(In reply to comment #10)
> >-  onPropertyChanged: function(aAddon, aProperties) {
> >-    if (aProperties.indexOf("applyBackgroundUpdates") != -1)
> >-      this.updateManualUpdatersCount();
> >-  }
> 
> Sadly, removing this makes the following edge case possible:
> 1. Set addon to manually update
> 2. Check for updates, and find an update
> 3. Available Updates pane shows, with "1" in its badge
> 4. Disable manual updates for that addon
> 5. Available Updates pane remains visible, with "1" in its badge
> 6. Switching to the Available Updates pane will show the empty notice

Fixed and tested.

> Are there any other possible ways to get to the Available Updates pane, and
> have it show the empty notice? If not, then that should be able to be removed.

Not the Available updates pane I think, but the recent updates pane can so this needs to stay.
Attachment #488275 - Attachment is obsolete: true
Attachment #491324 - Flags: review?(bmcbride)
(Reporter)

Updated

7 years ago
Blocks: 613801
No longer blocks: 613801
Duplicate of this bug: 613801
Comment on attachment 491324 [details] [diff] [review]
patch rev 2

Looks good.

Another edge-case that is now possible (kinda similar to the previous one):
1. Set addon to manually update
2. Check for updates, and find an update
3. Switch to Available Updates pane
4. Go into addon's detail view, from the Available Updates pane
5. Disable manual updates for that addon
6. Available Updates category disappears
7. Go back in history
8. Available Updates pane is shown, as is its category item. But no badge, and the empty notice shows

However, I'm not sure what the best behaviour is for that case. Maybe that is the best behaviour (it's not entirely incorrect, even if its not ideal). Taking them to the extensions list would be unexpected. Maybe jumping back in history twice could work... except there may not be that many items in the history. And dynamically rewriting the history depending on whether the Available Updates pane is visible sounds like a recipe for trouble.

Either way, I think that's followup fodder - so r=me
Attachment #491324 - Flags: review?(bmcbride) → review+
(Assignee)

Updated

7 years ago
Whiteboard: [has patch][needs review Unfocused] → [has patch]
(Assignee)

Comment 14

7 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/f2927563659b
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: --- → mozilla2.0b8
(Assignee)

Comment 15

7 years ago
Backed out due to test failures
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

7 years ago
Duplicate of this bug: 608453
(Reporter)

Updated

7 years ago
Blocks: 608877
(Assignee)

Comment 17

7 years ago
Created attachment 495997 [details] [diff] [review]
bustage fix

Silly mistake. The mock install objects were acting like they were installing restartless add-ons and so vanishing from the list of active installs immediately. For now since we aren't adding the new add-on to the mock provider this way makes more sense.
(Assignee)

Updated

7 years ago
Whiteboard: [has patch][waiting on try]
(Assignee)

Updated

7 years ago
Whiteboard: [has patch][waiting on try] → [has patch]
(Assignee)

Updated

7 years ago
Attachment #495997 - Flags: review?(bmcbride)
(Assignee)

Updated

7 years ago
Whiteboard: [has patch] → [has patch][needs review Unfocused]
Comment on attachment 495997 [details] [diff] [review]
bustage fix

As discussed on IRC: this will need to eventually depend on AddonManager.OP_NEEDS_RESTART_INSTALL - but that's not currently used by any test, and other things are unimplemented that prevents using that anyway.
Attachment #495997 - Flags: review?(bmcbride) → review+
(Assignee)

Comment 19

7 years ago
This landed a few days ago: http://hg.mozilla.org/mozilla-central/rev/c44ffda23deb
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs review Unfocused]
(Reporter)

Comment 20

7 years ago
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101212 Firefox/4.0b8pre
Status: RESOLVED → VERIFIED

Updated

7 years ago
Depends on: 619682
You need to log in before you can comment on or make changes to this bug.