Closed Bug 591893 Opened 10 years ago Closed 2 years ago

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

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox35 --- affected
firefox37 --- affected
blocking2.0 --- -

People

(Reporter: whimboo, Unassigned, Mentored)

References

Details

(Whiteboard: [good first bug])

Attachments

(4 obsolete files)

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.
Missed to mention the build:
Mozilla/5.0 (Windows NT 5.1; rv:2.0b5pre) Gecko/20100829 Firefox/4.0b5pre
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".
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.
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.
blocking2.0: --- → ?
blocking2.0: ? → betaN+
Whiteboard: [good first bug]
I wonder if this is bug 581483 which you have wontfixed, Dave.
(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.
(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.
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?
Assignee: nobody → bparr
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).
Duplicate of this bug: 616496
blocking2.0: betaN+ → -
Duplicate of this bug: 623626
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?
blocking2.0: - → ?
Summary: Undo installation of add-ons in search pane doesn't list them anymore in "Available Add-ons" → Undo or cancel the installation of add-ons in search pane doesn't list them anymore in "Available Add-ons"
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.
blocking2.0: ? → -
Status: NEW → ASSIGNED
Ben is probably not working on it, so resetting to default.
Assignee: bparr → nobody
Status: ASSIGNED → NEW
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!
Nishant, soorry for the delay! Are you still interested on working on this? 
We could select a mentor who can fallow you through this.
Flags: needinfo?(nishantg2108)
(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.
Mihai, I can reproduce the issue reported. After steps 4 I don't see the "Dom inspector"
Hi,
is nishant still working on this? I'd love to give it a shot if he is not.
AFAICT nishant never even started working on this (see comment #15 and #16). Maybe Tiziana (or someone) can find a mentor for you.
Flags: needinfo?(tiziana.sel)
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.
(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
Flags: needinfo?(tiziana.sel)
Added needinfo for mentorship
Flags: needinfo?(bmcbride)
Can I be assigned to this bug?
Attached file line 2368,extensions.js (obsolete) —
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
Attachment #8355811 - Flags: review?(bmcbride)
Attachment #8355811 - Attachment description: line 2368, extensions.js → extensions.js
Attachment #8355811 - Attachment description: extensions.js → line 2368,extensions.js
Attachment #8355811 - Attachment is obsolete: true
Attachment #8355811 - Attachment is patch: false
Attachment #8355811 - Flags: review?(bmcbride)
Sorry about that, didn't format patch correctly
Attached file extensions.js.patch (obsolete) —
Attachment #8355814 - Flags: review?(bmcbride)
(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!
Attachment #8355814 - Attachment is obsolete: true
Attachment #8355814 - Attachment is patch: false
Attachment #8355814 - Flags: review?(bmcbride)
Attached patch extensions.js.patch (obsolete) — Splinter Review
Fixed the typo, still works
Attachment #8355832 - Flags: review?(bmcbride)
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.
Attachment #8355832 - Flags: review?(bmcbride) → review-
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 :)
Assignee: nobody → scipio134
Status: NEW → ASSIGNED
Flags: needinfo?(nishantg2108)
Flags: needinfo?(bmcbride)
Attachment #8355832 - Attachment is obsolete: true
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?
Attached patch extension.js.patch (obsolete) — Splinter Review
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
Blair: We have a patch! Want to review it?
Flags: needinfo?(bmcbride)
Comment on attachment 8426798 [details] [diff] [review]
extension.js.patch

How am I suppose to understand this?  I am not a techie? 
jm4g4me@yahoo.com
Comment on attachment 8426798 [details] [diff] [review]
extension.js.patch

Blair's out, redirecting to Dave.
Flags: needinfo?(bmcbride)
Attachment #8426798 - Flags: review?(dtownsend)
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.
Attachment #8426798 - Flags: review?(dtownsend)
Assignee: scipio134 → nobody
Status: ASSIGNED → NEW
Attachment #8426798 - Attachment is obsolete: true
Mentor: dtownsend
Mentor: dtownsend → rhelmer
This is about the in-browser search results display which was removed in bug 1263313
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.