591893 - Undo or cancel the installation of add-ons in search pane doesn't list them anymore in "Available Add-ons"

Status: RESOLVED WONTFIX

(Toolkit :: Add-ons Manager, defect)

Description

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

If you undo the installation of an add-on in the search pane, the same add-on will not be listed anymore in the search results for "Available Add-ons". You will have to reopen the Add-ons Manager.

Steps:
1. Open the Add-ons Manager
2. Search for "Dom"
3. Install the Dom Inspector
4. Click on "Undo"
5. Search again for "Dom"

After step 5 no results are shown for the Dom Inspector.

Comment 1

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

Missed to mention the build:
Mozilla/5.0 (Windows NT 5.1; rv:2.0b5pre) Gecko/20100829 Firefox/4.0b5pre

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Do you get the same results when searching for something different the second time? ie, search for "dom", install, undo, search for "bob", search for "dom".

Comment 3

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

Yes, that's doing the trick. So it's a problem with the cached search results? Would that be an easy fix? If yes, we should add "[good first bug]" into the whiteboard.

Comment 4

[Dave Townsend [:mossop] (he/him)]

In order to save time the search view checks whether you are just re-doing the same search and if so just reshows the old results. But it doesn't properly take into account that changes can have been made since then. I think rather than trying to get it to handle every case properly it should just always do the search again.

Comment 5

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

I wonder if this is bug 581483 which you have wontfixed, Dave.

Comment 6

[Dave Townsend [:mossop] (he/him)]

(In reply to comment #5)
> I wonder if this is bug 581483 which you have wontfixed, Dave.

Not if I understand the bugs correctly. This is about add-ons that were in search results, then marked to uninstall and then the uninstall was cancelled. Bug 581483 is about add-ons that are marked to be uninstalled.

Comment 7

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

(In reply to comment #6)
> Not if I understand the bugs correctly. This is about add-ons that were in
> search results, then marked to uninstall and then the uninstall was cancelled.
> Bug 581483 is about add-ons that are marked to be uninstalled.

No, this bug is about "undo installation" and not "undo uninstallation". As seen in the comments above we have an inconsistency in the UI so it would still be a valid request compared to bug 581483.

Comment 8

[Ben Parr [:bparr]]

We could move the caching to AddonRepository. So, on a new search request, the AddonRepository would store all results from AMO, and then return a filtered list of addons not currently installed. When redoing a search, the AddonRepository could skip the AMO request, and just refilter the cached results.

The only problem I see with this approach is the question of when the cache would be cleared. Currently, the cache is cleared whenever the Add-ons Manager tab is closed. Would waiting for xpcom-shutdown before clearing the cache be appropriate, since the cache wouldn't be large?

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Yea, that could work nicely.

Would also need to figure out when to really re-fetch results from AMO, in the case where its been quite some time since a search was first cached. A short-ish interval (say, 30-60min?) would work nicely, I think - so we pick up changes. That would tie in with the caching though.

I'm very wary of keeping cached searches around for the application's entire session - given some sessions last days or weeks. It may be ok to initially not worry too much about clearing the cache in this bug (double check that with Dave), as long as its fixed before final. I have some plans to make caching more intelligent in bug 588249, but I haven't done any of the work on expiring entries yet. But I think having an expiring cache for searches is more important than an expiring cache for metadata of installed addons, since the cache for searches has the opportunity to grow indefinitely (at least with the metadata cache, its limited by what addons are actually installed).

Comment 12

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

Why this got blocking-? No description has been given for. :/

It's a fairly annoying situation for add-on installations. Now even more with the cancel button working (bug 581361) in the progress bar of the search pane.

Dave, can we re-evaluate?

Comment 13

[Dave Townsend [:mossop] (he/him)]

This is a temporary annoyance and only happens for people who actively try to install something, then decide they don't want it, then decide they do want it again all without closing the add-ons manager or searching for something else. I would not hold the release for this.

Comment 14

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

Ben is probably not working on it, so resetting to default.

Comment 15

[Nishant Gupta]

Hi, 

I am interested  to work on this bug as I aim to take part in GSoC this year and work on a Add-On related project and this might give me a good overview of the project. However, since i am a newbie to mozilla code repository, some guidance would be appreciated as to where to start for this bug (I already have the code repository downloaded).

Thanks!

Comment 16

[Tiziana Sellitto [:tiziana]]

Nishant, soorry for the delay! Are you still interested on working on this? 
We could select a mentor who can fallow you through this.

Comment 17

[Mihai Morar, (:MihaiMorar)]

(In reply to Henrik Skupin (:whimboo) from comment #0)
> If you undo the installation of an add-on in the search pane, the same
> add-on will not be listed anymore in the search results for "Available
> Add-ons". You will have to reopen the Add-ons Manager.
> 
> Steps:
> 1. Open the Add-ons Manager
> 2. Search for "Dom"
> 3. Install the Dom Inspector
> 4. Click on "Undo"
> 5. Search again for "Dom"
> 
> After step 5 no results are shown for the Dom Inspector.

On latest nightly 25, when you click on "Undo" at step 4, the page with the results from step 2 is displayed, so no search is required because Dom is displayed.

Probably there were several changes in AMO's structure that included search section too.

Comment 18

[Tiziana Sellitto [:tiziana]]

Mihai, I can reproduce the issue reported. After steps 4 I don't see the "Dom inspector"

Comment 19

[scipio134]

Hi,
is nishant still working on this? I'd love to give it a shot if he is not.

Comment 20

[Tony Mechelynck [:tonymec]. (NEEDINFO me if you want my attention)]

AFAICT nishant never even started working on this (see comment #15 and #16). Maybe Tiziana (or someone) can find a mentor for you.

Comment 21

[scipio134]

In that case, Where is the code for the add-ons installer located. This is my first bug and I am still weary on the firefox architecture.

Comment 22

[Tiziana Sellitto [:tiziana]]

(In reply to scipio134 from comment #21)
> In that case, Where is the code for the add-ons installer located. This is
> my first bug and I am still weary on the firefox architecture.

Try asking in #introduction or #developers channels on irc.mozilla.org for any info you need.
You could also start taking a look at https://developer.mozilla.org/en-US/docs/Introduction#Step_3_-_Fix_the_bug

Comment 23

[Tiziana Sellitto [:tiziana]]

Added needinfo for mentorship

Comment 24

[scipio134]

Can I be assigned to this bug?

Comment 25

[scipio134]

Attachment: line 2368,extensions.js

I fixed it! just had to assign a status on the "onInstallCancel" event.Now when canceled "(disabled)" appears next to the name. when you search for it again it shows up in its original state

Comment 26

[scipio134]

Sorry about that, didn't format patch correctly

Comment 27

[scipio134]

Attachment: extensions.js.patch

Comment 28

[Michael Kohler [:mkohler]]

(In reply to scipio134 from comment #27)
> Created attachment 8355814 [details]
> extensions.js.patch

First, thanks for contributing! Are you sure this is the correct version of the patch? I haven't tested it, but it seems that "setAtrribute" should actually read "setAttribute" since setAtrribute is not a function (http://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=setAtrribute&redirect=true).

Don't let me demotivate you, you're on the right path (for example: the patch format looks correct to me). I still let Blair comment on the actual content of the patch, since I don't know the Addons-Manager Toolkit code.

Again, thanks for contributing!

Comment 29

[scipio134]

Attachment: extensions.js.patch

Fixed the typo, still works

Comment 30

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8355832 [details] [diff] [review]
extensions.js.patch

Review of attachment 8355832 [details] [diff] [review]:
-----------------------------------------------------------------

Unfortunately, you're seeing the symptoms of this bug go away because now onInstallCancelled() is throwing an exception before it's able to call removeInstall() - because gSearchView doesn't have a setAttribute() method. And not having removeInstall() be called will result in other cases being broken.

The best fix here is to follow Dave's advice in comment 4. Currently we detect if we're searching for the same thing again, and re-use the old results. We should skip that check, and just always do a fresh search.

Comment 31

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Oh, and: Welcome to your first Mozilla bug! Looks like you've got the process of creating and submitting patches all sorted out already, awesome :)

Comment 32

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Sorry I missed you on IRC earlier. Not sure what you meant about history - could you post a patch? 

What I meant in comment 31 was removing the entire if-statement and its contents here:
http://hg.mozilla.org/mozilla-central/file/1ad9af3a2ab8/toolkit/mozapps/extensions/content/extensions.js#l2126
Does doing that not fix the bug?

Comment 33

[scipio134]

Attachment: extension.js.patch

So I forgot about this, Sorry. I did a patch removing an if statement so it defaults to the fake history every time. This leaves the code intact if anyone figures out a patch fixing the real history

Comment 34

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Blair: We have a patch! Want to review it?

Comment 35

[jrm4cbm]

Comment on attachment 8426798 [details] [diff] [review]
extension.js.patch

How am I suppose to understand this?  I am not a techie? 
[e-mail]

Comment 36

[Justin Dolske [:Dolske]]

Comment on attachment 8426798 [details] [diff] [review]
extension.js.patch

Blair's out, redirecting to Dave.

Comment 37

[Dave Townsend [:mossop] (he/him)]

Comment on attachment 8426798 [details] [diff] [review]
extension.js.patch

Review of attachment 8426798 [details] [diff] [review]:
-----------------------------------------------------------------

Based on the discussion I don't think this patch was ever meant for review, it removes the code that makes the back/forward button work in the add-ons manager.

Comment 38

[Andrew Swan [:aswan]]

This is about the in-browser search results display which was removed in bug 1263313