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

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: 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.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+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.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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?

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Er, wow, ignore this.

Comment 4

[Dave Townsend [:mossop]]

But seriously, /is there/ a bug for that?

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

I think this bug is now intended to be for that.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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)?

Comment 8

[Dave Townsend [:mossop]]

Ah yeah, we have to fix one or the other and hiding the pane is probably the simplest

Comment 9

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Saves us some code to do this, added tests to cover it.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 11

[Dave Townsend [:mossop]]

Attachment: 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.

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 14

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/f2927563659b

Comment 15

[Dave Townsend [:mossop]]

Backed out due to test failures

Comment 17

[Dave Townsend [:mossop]]

Attachment: 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.

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 19

[Dave Townsend [:mossop]]

This landed a few days ago: http://hg.mozilla.org/mozilla-central/rev/c44ffda23deb

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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