Closed Bug 1023544 Opened 6 years ago Closed 6 years ago

No way to remove dynamic panel added by home feeds add-on

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 33
Tracking Status
firefox30 --- unaffected
firefox31 --- verified
firefox32 --- verified
firefox33 --- verified
fennec 31+ ---

People

(Reporter: Margaret, Assigned: Margaret)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

I suspect this is fallout from bug 1009586.
So, what's really happening here is that we removed the ability for the user to uninstall dynamic panels through the home panels settings (the "Remove" menu item), and instead replaced it with the ability to hide them (the "Hide" menu item).

This decision was good for 2 reasons:

1) Most home panel add-ons install/uninstall a panel when the add-on is installed/unintsalled, so it ties the two concpets together.

2) This is consistent with the options we show for built-in panels.

However, I'm running into a problem with my Home Feeds add-on, because it dynamically installs panels. With this add-on, the user has no way to get rid of panels that they added without uninstalling the add-on (although they can still hide them through the settings).

In this case, I think it should be up to the add-on to uninstall the panel, but the question is where should that happen? If we add a hook to let the add-on know when the user hides a dynamic panel, we could ask if the user wants to actually get rid of that panel forever.

We could also leave it up to the add-on to create some UI somewhere (add-on options?) to let the user uninstall panels, but doing it as part of the home panel seems like the natural place to do this.

Luckily this does not affect Fx30 (where the "Remove" menu item still exists), but we should update the add-on to be more useful for Fx31+.
Blocks: lists
Flags: needinfo?(ibarlow)
Summary: Home panels onuninstall handler is never called → No way to remove dynamic panel added by home feeds add-on
I filed bug 1023551 about the enable/disable hooks, since those would just be good anyway.
(In reply to :Margaret Leibovic from comment #1)
> So, what's really happening here is that we removed the ability for the user
> to uninstall dynamic panels through the home panels settings (the "Remove"
> menu item), and instead replaced it with the ability to hide them (the
> "Hide" menu item).

Can we just go back to having a "remove" option for panels? I'm not sure I understand why we changed this. 

> 2) This is consistent with the options we show for built-in panels.

I don't think add-ons and built in panels need to be consistent in this way.
Flags: needinfo?(ibarlow)
(In reply to Ian Barlow (:ibarlow) from comment #3)
> (In reply to :Margaret Leibovic from comment #1)
> > So, what's really happening here is that we removed the ability for the user
> > to uninstall dynamic panels through the home panels settings (the "Remove"
> > menu item), and instead replaced it with the ability to hide them (the
> > "Hide" menu item).
> 
> Can we just go back to having a "remove" option for panels? I'm not sure I
> understand why we changed this. 

So, the "remove" uninstalled the panel, which would remove it from the home page and from the list in settings. However, it would not unregister the panel (something the add-on always does on startup anyway), so it would still be present in the panel picker dialog when you chose the "Add panel" item. Given this, removing was a reversible action, but this changed when we removed the "Add panel" item.

I don't know that I feel comfortable with a "remove" item without the ability to add the panel back, since the user could end up with a bunch of add-ons whose panels have been uninstalled. If they ever want one of those panels back, they would need to uninstall the add-on and reinstall it, which is confusing. Or if they never want it back, they would have these useless installed add-ons potentially impacting performance.

For the home feeds add-on, the option to remove the panel makes sense, since the user dynamically generated it anyway, and they could re-create it through the subscribe menu. However, for other add-ons that auto-install a panel when the add-on is installed, this is confusing.

I feel like this problem is really specific to the home feeds add-on, or any other add-on that creates panels on the fly.
Ian - Flagging you for some guidance on the approach
tracking-fennec: ? → 31+
Flags: needinfo?(ibarlow)
(In reply to Mark Finkle (:mfinkle) from comment #5)
> Ian - Flagging you for some guidance on the approach

We may need to sit down and pow-wow about this. Ping me if you want to discuss!
Ah, I forgot about the distinction between being registered with no panels, and showing a panel by default. And to be honest I can't really see any value in making that distinction obvious to users, at least not in 99% of cases. 

Here's my perfect world scenario, I'd love for you to punch holes in it if you feel I'm missing something here or if this approach is crazy:

I think removing the only panel instance of an add-on should uninstall that add-on. This would have different implications for standard panels vs ones that are dynamically generated. 

For a standard panel, it's relatively straightforward. I install it, the panel shows up. I remove the panel, the panel goes away and the add-on gets uninstalled along with it. To get it back, I have to go install the add-on again. 

For a dynamic panel, we need to watch it more closely. Let's take your Home Feeds panel as an example, where I install the add-on and subscribe to two feeds. Removing the first feed would just get rid of that panel. Removing the *second* one gets rid of that panel, *and* uninstalls the add-on. Perhaps we could consider throwing up some kind of "hang on, do you want to remove this whole add-on, or just this feed?" dialog, because it's conceivable that a user is just doing some cleanup and might still want the ability to subscribe to other feeds later on. But generally, I think we should lean toward "Remove" being a more permanent action. 

What do you think about this, Margaret?
Flags: needinfo?(ibarlow)
(In reply to Ian Barlow (:ibarlow) from comment #7)
> Ah, I forgot about the distinction between being registered with no panels,
> and showing a panel by default. And to be honest I can't really see any
> value in making that distinction obvious to users, at least not in 99% of
> cases. 
> 
> Here's my perfect world scenario, I'd love for you to punch holes in it if
> you feel I'm missing something here or if this approach is crazy:
> 
> I think removing the only panel instance of an add-on should uninstall that
> add-on. This would have different implications for standard panels vs ones
> that are dynamically generated. 

This is something that would need to be left to the add-on (Fennec doesn't know about what add-ons created what panel), but I can update my sample add-ons and boilerplate repo to do this.

> For a standard panel, it's relatively straightforward. I install it, the
> panel shows up. I remove the panel, the panel goes away and the add-on gets
> uninstalled along with it. To get it back, I have to go install the add-on
> again. 
>
> For a dynamic panel, we need to watch it more closely. Let's take your Home
> Feeds panel as an example, where I install the add-on and subscribe to two
> feeds. Removing the first feed would just get rid of that panel. Removing
> the *second* one gets rid of that panel, *and* uninstalls the add-on.
> Perhaps we could consider throwing up some kind of "hang on, do you want to
> remove this whole add-on, or just this feed?" dialog, because it's
> conceivable that a user is just doing some cleanup and might still want the
> ability to subscribe to other feeds later on. But generally, I think we
> should lean toward "Remove" being a more permanent action. 
> 
> What do you think about this, Margaret?

This sounds reasonable to me. We already do provide add-ons with a hook to listen for when a panel is uninstalled (removed), so the Home Feeds add-on can keep track of how many panels are installed or uninstalled. I think that for the Home Feeds add-on, we don't need to ask the user if they want to uninstall the add-on, since the main functionality of that add-on is really just to add a new menu item to add panels to your home page.

So, here's my plan of action:

1) Add back the remove item to the home panel settings. This should be straightforward, and won't involve string changes, since the same logic already exists for the search engine settings.

2) Update my normal panel add-ons (and boilerplate code) to uninstall themselves if the user uninstalls the panel.

3) Don't do anything to the Home Feeds add-on, it actually already listens for when a panel is uninstalled and wipes it from its memory (so that it won't be registered again on startup).
This is just a partial backout of the patch that landed in bug 1009586, restoring the "Remove" option for dynamic panels.
Attachment #8440100 - Flags: review?(liuche)
(In reply to :Margaret Leibovic from comment #8)

> So, here's my plan of action:
> 
> 1) Add back the remove item to the home panel settings. This should be
> straightforward, and won't involve string changes, since the same logic
> already exists for the search engine settings.

In the patch attached to this bug.

> 2) Update my normal panel add-ons (and boilerplate code) to uninstall
> themselves if the user uninstalls the panel.

Updated the boilerplate code: https://github.com/leibovic/hub-boilerplate/commit/acad66644d974469b57a070928ee734e7ceb2a02

> 3) Don't do anything to the Home Feeds add-on, it actually already listens
> for when a panel is uninstalled and wipes it from its memory (so that it
> won't be registered again on startup).

Tested to verify this works.
Comment on attachment 8440100 [details] [diff] [review]
Add back ability to remove dynamic panels in settings

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

This looks fine for just adding the "remove" back in; are you planning on addressing add-ons without any panels, or will that be up to the add-on? I agree that this is a problem specific to add-ons like home-feed, where you have a many-to-one panel:addon mapping.
Attachment #8440100 - Flags: review?(liuche) → review+
(In reply to Chenxia Liu [:liuche] from comment #11)
> Comment on attachment 8440100 [details] [diff] [review]
> Add back ability to remove dynamic panels in settings
> 
> Review of attachment 8440100 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine for just adding the "remove" back in; are you planning on
> addressing add-ons without any panels, or will that be up to the add-on? I
> agree that this is a problem specific to add-ons like home-feed, where you
> have a many-to-one panel:addon mapping.

My current plan is to leave it up to the add-on. In my last comment I linked to a commit in my hub boilerplate code that does this, and it's actually pretty easy for add-ons to do this.

Also, worst case, the user can uninstall and reinstall the add-on to get the panel back, and if they end up with an add-on that doesn't do anything, they can remove it themselves from the add-ons manager. Hopefully we can educate users enough about add-ons that they know this is the place to go manage them.
https://hg.mozilla.org/mozilla-central/rev/2aeaa45be97e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Build: Firefox for Android 33.0a1 (2014-06-16)
Device: Alcatel One Touch
OS: Android 4.1.2
Installing the home feed add-on, going to the quality.mozilla.org page, add a few panel feeds, 
in Settings -> Customize -> Home, the remove option is present for the dynamic panels, while the other ones have the "Hide" option. Choosing the remove options for one panel will remove it from the about:home.
Comment on attachment 8440100 [details] [diff] [review]
Add back ability to remove dynamic panels in settings

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1009586
User impact if declined: no way to remove home panels that are dynamically added by an add-on
Testing completed (on m-c, etc.): landed on m-c 6/15
Risk to taking this patch (and alternatives if risky): low-risk, adds back home settings logic that we're currently shipping in Fx30
String or IDL/UUID changes made by this patch: none
Attachment #8440100 - Flags: approval-mozilla-beta?
Attachment #8440100 - Flags: approval-mozilla-aurora?
Attachment #8440100 - Flags: approval-mozilla-beta?
Attachment #8440100 - Flags: approval-mozilla-beta+
Attachment #8440100 - Flags: approval-mozilla-aurora?
Attachment #8440100 - Flags: approval-mozilla-aurora+
Build: Firefox for Android 32.0a2 (2014-06-18)
Device: LG Nexus 4
OS: Android 4.4.2
Installing the home feed add-on (and instagram) going to the quality.mozilla.org page, add a few panel feeds, in Settings -> Customize -> Home, the remove option is present only for the dynamic panels, while the other ones have the "Hide" option.
Verified as fixed in
Build: Firefox for Android 31.0b4;
Device: Asus Transformer Pad TF300T (Android 4.2.1).

Based on Teodora's comment 15 and comment 18 I will mark this as verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.