Closed
Bug 553494
Opened 15 years ago
Closed 14 years ago
Support undo uninstall for restartless and already disabled add-ons
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
Tracking
()
VERIFIED
FIXED
mozilla2.0b3
| Tracking | Status | |
|---|---|---|
| blocking2.0 | --- | beta3+ |
People
(Reporter: Unfocused, Assigned: mossop)
References
Details
(Whiteboard: [AddonsRewriteTestday][rewrite])
Attachments
(3 files)
|
(deleted),
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
|
(deleted),
patch
|
Unfocused
:
review+
|
Details | Diff | Splinter Review |
|
(deleted),
patch
|
Unfocused
:
review+
|
Details | Diff | Splinter Review |
Its possible to undo a pending uninstall of normal addons, but restart-less addons don't currently get that luxury.
|
||
Updated•15 years ago
|
Flags: in-testsuite?
Flags: in-litmus?
Version: unspecified → Trunk
|
|
||
Comment 1•15 years ago
|
||
We have to morph this bug a bit because even for old fashion extensions the removal cannot be undone. The following error appears in the Error Console:
Error: Add-on is not marked to be uninstalled
Source File: file:///Applications/Minefield.app/Contents/MacOS/modules/XPIProvider.jsm
Line: 4475
Summary: Support undo uninstall for restart-less addons → Support undo uninstall for add-ons
Whiteboard: [rewrite] → [AddonsRewriteTestday][rewrite]
|
|
||
Updated•15 years ago
|
Summary: Support undo uninstall for add-ons → Support undo uninstall for restartless and already disabled add-ons
|
|
||
Updated•15 years ago
|
blocking2.0: --- → ?
|
Assignee | |
Comment 6•15 years ago
|
||
This is not something we're going to get to in the Firefox 4 timeframe I think.
blocking2.0: ? → -
|
|
||
Comment 7•15 years ago
|
||
(In reply to comment #6)
> This is not something we're going to get to in the Firefox 4 timeframe I think.
That said we should remove the link from the ui. Shall I file a separate bug for that?
|
|
Assignee | |
Comment 8•15 years ago
|
||
(In reply to comment #7)
> (In reply to comment #6)
> > This is not something we're going to get to in the Firefox 4 timeframe I think.
>
> That said we should remove the link from the ui. Shall I file a separate bug
> for that?
Yeah, or make it a part of bug 567120
|
||
Comment 11•14 years ago
|
||
Why is Bug 578170 a dupe, that is to do with the undo link not being removed after a successful undo, this bug seems to be to do with failure of undo for certain add-ons.
|
|
Assignee | |
Comment 12•14 years ago
|
||
(In reply to comment #11)
> Why is Bug 578170 a dupe, that is to do with the undo link not being removed
> after a successful undo, this bug seems to be to do with failure of undo for
> certain add-ons.
Interesting, you can actually undo uninstalling personas? I have no idea how that works right now. Guess it isn't a dupe but this will probably impact that heavily
|
|
Assignee | |
Updated•14 years ago
|
blocking2.0: - → beta3+
|
|
Assignee | |
Updated•14 years ago
|
Assignee: nobody → dtownsend
|
|
Assignee | |
Comment 15•14 years ago
|
||
Here is the overview of how we're going to do this. The undo process for restartless add-ons is going to be purely UI based rather than handled in the backend. This means that backend providers can be as simple as possible without needing to support undoing uninstalls if they have no need to.
In the UI clicking remove on an add-on in the list will disable the add-on and show the undo option. Clicking undo will of course revert that.
Switching away from the list showing the undo item will make the uninstall permanent, as would closing the add-ons manager. After that there is no way to get add-ons back other than re-installing them.
If remove is clicked in the detail view the UI will switch back to the list view and show the undo option there.
Blocks: 563072
|
Reporter | |
Comment 16•14 years ago
|
||
Yea, that sounds like a much quicker way of doing this. Would still like a more permanent undo record post-4.0 though (ie, unlimited undo).
|
|
Assignee | |
Comment 17•14 years ago
|
||
As a first step here I want to remove the restriction on disabling themes. Currently the API only allows you to enable themes on the basis that you are always switching between them. That seemed sensible enough but it really just complicates some things and there is no reason not to allow disabling, disabling just switches back to the default theme. The only theme you should not be able to disable is the default.
Attachment #459873 -
Flags: review?(robert.bugzilla)
|
|
Assignee | |
Comment 18•14 years ago
|
||
This patch depends on the patch from bug 573149.
The mock provider is updated to support enabling, disabling and uninstalling add-ons, restartless or otherwise. There is a very long test covering every case I could think of here.
The actual fix is fairly straightforward, When removing restartless items just mark appropriately and then when hiding the list view finalise the uninstall for items that are waiting.
Attachment #459877 -
Flags: review?
|
|
Assignee | |
Updated•14 years ago
|
Attachment #459877 -
Flags: review? → review?(bmcbride)
|
|
Assignee | |
Comment 19•14 years ago
|
||
Oh, there is one issue with this fix that I wanted to defer to a follow-up bug since I don't think it is blocking and it is kind of irritating to fix.
If you have two copies of the add-ons manager open and click remove in one the other will just show the add-on get disabled. When you close the first manager the add-ons will disappear from the second as it gets uninstalled at that point.
|
||
Updated•14 years ago
|
Attachment #459873 -
Flags: review?(robert.bugzilla) → review+
|
|
Reporter | |
Comment 20•14 years ago
|
||
Comment on attachment 459877 [details] [diff] [review]
patch rev 1
Man, that's a lot of test code.
>- loadView: function(aViewId) {
>+ loadView: function(aViewId, aCallback) {
Add aCallback to the global loadView() wrapper function too.
> <method name="uninstall">
> <body><![CDATA[
>- this.mAddon.uninstall();
>- ]]></body>
>+ // If uninstalling does not require a restart then just disable it
>+ // and show the undo UI.
>+ if (!this.opRequiresRestart("uninstall")) {
>+ this.setAttribute("wasDisabled", this.mAddon.userDisabled);
>+ this.setAttribute("restartrequired", false);
>+ this.setAttribute("status", "uninstalled");
>+ this.mAddon.userDisabled = true;
>+ }
>+ else {
>+ this.mAddon.uninstall();
>+ }
>+ ]]></body>
> </method>
Nit: remove the newline before the "else".
(In reply to comment #19)
> Oh, there is one issue with this fix that I wanted to defer to a follow-up bug
> since I don't think it is blocking and it is kind of irritating to fix.
>
> If you have two copies of the add-ons manager open and click remove in one the
> other will just show the add-on get disabled. When you close the first manager
> the add-ons will disappear from the second as it gets uninstalled at that
> point.
This really irks me :\ And really really REALLY makes me want a more API-level solution post-4.0 - whether it be bug 553499, or something else.
Anyway, when you file the followup bug, could you add a reference to it somewhere in the code (like in the uninstall method of the XBL binding).
Attachment #459877 -
Flags: review?(bmcbride) → review+
|
|
Assignee | |
Comment 21•14 years ago
|
||
Landed: http://hg.mozilla.org/mozilla-central/rev/8bd296358c1e
http://hg.mozilla.org/mozilla-central/rev/58175e77c460
Filed the follow-up bug as bug 582002
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus?
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b3
|
|
Assignee | |
Comment 22•14 years ago
|
||
Backed out due to some odd test failures.
Status: RESOLVED → REOPENED
Flags: in-testsuite+ → in-testsuite?
Resolution: FIXED → ---
|
|
Assignee | |
Comment 23•14 years ago
|
||
This is a follow-up fix that turns off remote searching for this test since it causes problems, maybe we should do this for all tests and just turn it on for those that want it.
Attachment #460300 -
Flags: review?(bmcbride)
|
|
Reporter | |
Updated•14 years ago
|
Attachment #460300 -
Flags: review?(bmcbride) → review+
|
|
Reporter | |
Comment 24•14 years ago
|
||
(In reply to comment #23)
> maybe we should do this for all tests and just turn it on for
> those that want it.
That could be a good idea. It's likely to cause all sorts of issues and/or make tests more complicated otherwise. Followup bug fodder!
|
|
Assignee | |
Comment 25•14 years ago
|
||
Relanded with the fix: http://hg.mozilla.org/mozilla-central/rev/59a9d183d1ec
Flags: in-testsuite? → in-testsuite+
|
|
Assignee | |
Comment 26•14 years ago
|
||
(In reply to comment #24)
> (In reply to comment #23)
> > maybe we should do this for all tests and just turn it on for
> > those that want it.
>
> That could be a good idea. It's likely to cause all sorts of issues and/or make
> tests more complicated otherwise. Followup bug fodder!
Filed bug 582058
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
|
||
Comment 28•14 years ago
|
||
Does not work on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b3) Gecko/20100805 Firefox/4.0b3.
It doesnt show a undo message when I remove a disabled add on.
|
|
Assignee | |
Comment 29•14 years ago
|
||
(In reply to comment #28)
> Does not work on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b3)
> Gecko/20100805 Firefox/4.0b3.
>
> It doesnt show a undo message when I remove a disabled add on.
Could you please file a new bug with steps to reproduce.
|
|
Assignee | |
Comment 30•14 years ago
|
||
(In reply to comment #29)
> (In reply to comment #28)
> > Does not work on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b3)
> > Gecko/20100805 Firefox/4.0b3.
> >
> > It doesnt show a undo message when I remove a disabled add on.
>
> Could you please file a new bug with steps to reproduce.
I filed bug 585339
|
|
||
Comment 31•14 years ago
|
||
Dave, do we have a chance to keep those undo messages open until the next restart? Right now you will lose the capability to undo an uninstall after you have switched the panes.
|
|
Assignee | |
Comment 32•14 years ago
|
||
(In reply to comment #31)
> Dave, do we have a chance to keep those undo messages open until the next
> restart? Right now you will lose the capability to undo an uninstall after you
> have switched the panes.
It would require more work than I'm willing to block the release on.
|
|
||
Comment 33•14 years ago
|
||
(In reply to comment #32)
> It would require more work than I'm willing to block the release on.
Filed bug 590508 for future work.
Also an undo is not possible for already disabled addons. I have filed bug 590509.
Otherwise it looks good for Jetpacks. Marking as verified with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b5pre) Gecko/20100825 Minefield/4.0b5pre
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•