591663 - "You don't have any add-ons of this type installed" doesn't disappear as it should

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Tiago Sá]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0b5pre) Gecko/20100828 Firefox/4.0b5pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0b5pre) Gecko/20100828 Firefox/4.0b5pre

I came across this when, for the first time ever (woohoo!) I installed an add-on on Minefield. And the screenshot I'll upload right away explains it pretty well.

Reproducible: Always

Steps to Reproduce:
1. Have no extensions installed
2. Install an extension
Actual Results:  
The "You don't have any add-ons of this type installed" doesn't disappear, and instead groups with the installed extension in an awkward way.

Expected Results:  
"You don't have any add-ons of this type installed" should disappear and give place to just the installed add-on.

Comment 1

[Tiago Sá]

Attachment: Screenshot of the problem

Here's the screenshot.

Comment 2

[Tiago Sá]

Added McBride to the list so this doesn't get lost. Sorry for the bother.

Comment 3

[Tony Mechelynck [:tonymec]]

Yes, it looks awkward, and yet it's true: until you restart, the add-on isn't yet "installed".

Comment 4

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

I think it's a dupe but cannot find the original one. Note that all list views are affected by this issue. See bug 591024. I would say lets track it separately for now.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Yea, that notification should definitely not show.

Comment 6

[Ben Parr [:bparr]]

Going to look into this.

Comment 7

[Ben Parr [:bparr]]

Attachment: Patch and Test

Fixes cases where the hidden state of the empty notice should change but wasn't.

Also fixes problem where installs would show up in a List View that did not match their type (e.g. an extension install showing up when the Language category is showing). This was happening because the Add-ons Manager would add an item onNewInstall, even though it did not know the type of the install.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #7)
> Also fixes problem where installs would show up in a List View that did not
> match their type (e.g. an extension install showing up when the Language
> category is showing). This was happening because the Add-ons Manager would add
> an item onNewInstall, even though it did not know the type of the install.

Think that may fix bug 601459 too.

Comment 9

[Dave Townsend [:mossop]]

(In reply to comment #7)
> Also fixes problem where installs would show up in a List View that did not
> match their type (e.g. an extension install showing up when the Language
> category is showing). This was happening because the Add-ons Manager would add
> an item onNewInstall, even though it did not know the type of the install.

So where does this patch make thenm show up during download when the type is unknown? It sounds like this also fixes bug 571458 (yay!) except that we haven't decided what the right fix for that actually is yet.

Comment 10

[Ben Parr [:bparr]]

The current patch would not insert an installing add-on into the current view until it can match the add-on type with the current list type. So, the add-on would not show up if the type is unknown (e.g. when downloading).

Comment 11

[Dave Townsend [:mossop]]

Will review tomorrow after Boriss confirms the new behavior is good and just.

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 480391 [details] [diff] [review]
Patch and Test

Sorry for the delays. This looks pretty good however it doesn't apply since bug 558134 made a couple of changes in this area. Could you update it and I'll get it reviewed pretty quick. Also adding a test verifying that no download of the wrong type shows up anymore would be useful.

Comment 13

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

Ben, would you have time to update the patch?

Comment 14

[Ben Parr [:bparr]]

I unfortunately won't have time until next next Monday (12/6). I will have a good chunk of time to work on my bugs the week of that Monday though.

Comment 15

[Ben Parr [:bparr]]

Attachment: Patch and Test v2

Update patch so it can actually be applied.

Comment 16

[Dave Townsend [:mossop]]

Comment on attachment 496281 [details] [diff] [review]
Patch and Test v2

Thanks this looks great. I think I need to sit down and think harder about how available installs show up in the API after Firefox 4, it doesn't seem to be quite right yet but this patch seems to be the best way to handle it right now.

I did realise that we've always not sorted newly added items correctly but I filed bug 618396 on that, no need to solve that yet.

Comment 17

[Dave Townsend [:mossop]]

Darn, landed but I forgot to put it in your name, sorry

http://hg.mozilla.org/mozilla-central/rev/245e6ab9e2e3

Comment 18

[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