Update home config when new panels are added/removed

RESOLVED FIXED in Firefox 30

Status

()

Firefox for Android
Awesomescreen
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Margaret, Assigned: lucasr)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 30
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: shovel-ready)

(Reporter)

Description

5 years ago
Bug 862805 added a JS API to let add-ons register new lists to show in about:home, but right now those changes only take effect on restart.

That patch included sending a "HomeLists:Added" message whenever a new list is added, so we can use that to notify the appropriate parties that things should get updated.

That patch did not include anything notification for removing a list, so we'll need to add something to the JS side as well (perhaps another message) to deal with that.
(Reporter)

Comment 1

5 years ago
Idea: instead of passing a gecko message, is there a way to listen for changes to shared preferences? One downside to this is that we would need to re-read the preferences and re-load all the lists, but it would be nice to avoid listening for messages from gecko.
(Assignee)

Updated

4 years ago
Assignee: nobody → lucasr.at.mozilla
(Reporter)

Comment 2

4 years ago
The world has changed since I filed this bug. Once bug 958192 lands, we will keep track of the current set of panels in PanelManager.

To be clear, in this bug I'm talking about what happens when Home.panels.add/remove are called, e.g. when an add-on in installed/uninstalled.

I don't know that we actually need to worry about the case where new panels are added, since the plan is to request to set of available panels whenever the user visits a settings page to edit their panels. 

However, when an add-on is uninstalled, we'll need to propagate that change up to the home config and refresh the UI.
Depends on: 958192
Summary: Update home config when new lists are added/removed → Update home config when new panels are added/removed
(Reporter)

Comment 3

4 years ago
(In reply to :Margaret Leibovic from comment #2)
> The world has changed since I filed this bug. Once bug 958192 lands, we will
> keep track of the current set of panels in PanelManager.

Actually, this isn't totally correct. We'll use PanelManager to interface with Home.jsm, where we'll actually hold a set of available panels in memory.
(Assignee)

Comment 4

4 years ago
For the sake of simplicity, we could do all these updates in the HomeConfig invalidation routine (bug 949174). The following events might trigger an invalidation:

- Locale changes
- An add-on is updated to a new version
- An add-on is uninstalled

The invalidation routine would use the PanelManager to:
- Check if the current panels in HomeConfig are still available
- Re-fetch any layout changes for each panel in HomeConfig
- Re-fetch all strings for each the panel i.e. only the panel title for now

In terms of code, it will probably look a bit like this:

List<String> ids = new ArrayList<String>();

// Populate the ids list from the current list of PanelConfigs in HomeConfig
for (int i = 0; i < panelConfigs.size(); i++) {
    ids.add(panelConfigs.get(I).getId());
}

public void requestPanelsById(ids, new RequestCallback() {
   @Override
   public void onComplete(List<PanelInfo> panelInfos) {
       // Recreate list of PanelConfigs based on the returned
       // list of PanelInfos. The uninstalled panels will be implicitly
       // discarded here. The strings in the PanelInfo instances here
       // should be matching the current locale in Gecko. 
       List<PanelConfig> newPanelConfigs = new ArrayList<PanelConfig>();
       for (int i = 0; i < panelInfos.size(); i++) {
           PanelConfig panelConfig = convertInfoToConfig(panelInfos.get(i));
           newPanelConfigs.add(panelConfig);
       }

       // Trigger HomeConfig.save() off main thread passing newPanelConfigs().
   }
});

Where requestPanelsById() is just a kind of generalization of your requestAvailablePanels() method in bug 958192. The auto-add behaviour will have to go through a different path.
(Reporter)

Comment 5

4 years ago
(In reply to Lucas Rocha (:lucasr) from comment #4)

> - An add-on is updated to a new version
> - An add-on is uninstalled

To be clear, we won't actually get events to know the add-on is updated/uninstalled, rather the add-on might add a new panel when it's updated, or it will remove the panel as part of its uninstall handler. Also, we should only need to be updating the HomeConfig when the panels themselves change, not the data inside them (so an add-on syncing new data into its list won't be part of the add-on updating, but rather just something that happens periodically, and querying the content provider will take care of updating that on the UI side). I think we both already know this, but I prefer to err on the side of over-communicating here :)

> The invalidation routine would use the PanelManager to:
> - Check if the current panels in HomeConfig are still available
> - Re-fetch any layout changes for each panel in HomeConfig
> - Re-fetch all strings for each the panel i.e. only the panel title for now
> 
> In terms of code, it will probably look a bit like this:
> 
> List<String> ids = new ArrayList<String>();
> 
> // Populate the ids list from the current list of PanelConfigs in HomeConfig
> for (int i = 0; i < panelConfigs.size(); i++) {
>     ids.add(panelConfigs.get(I).getId());
> }
> 
> public void requestPanelsById(ids, new RequestCallback() {
>    @Override
>    public void onComplete(List<PanelInfo> panelInfos) {
>        // Recreate list of PanelConfigs based on the returned
>        // list of PanelInfos. The uninstalled panels will be implicitly
>        // discarded here. The strings in the PanelInfo instances here
>        // should be matching the current locale in Gecko. 
>        List<PanelConfig> newPanelConfigs = new ArrayList<PanelConfig>();
>        for (int i = 0; i < panelInfos.size(); i++) {
>            PanelConfig panelConfig = convertInfoToConfig(panelInfos.get(i));
>            newPanelConfigs.add(panelConfig);
>        }
> 
>        // Trigger HomeConfig.save() off main thread passing
> newPanelConfigs().
>    }
> });
> 
> Where requestPanelsById() is just a kind of generalization of your
> requestAvailablePanels() method in bug 958192. The auto-add behaviour will
> have to go through a different path.

This sounds good. So, given this, we won't need the "HomePanels:Remove" message, but rather we will trigger something to invalidate the HomeConfig when the set of available panels changes?
(Assignee)

Comment 6

4 years ago
(In reply to :Margaret Leibovic from comment #5)
 > This sounds good. So, given this, we won't need the "HomePanels:Remove"
> message, but rather we will trigger something to invalidate the HomeConfig
> when the set of available panels changes?

Pretty much. But we'll have to be a bit smart when handling add-on updates. IIURC, add-ons will cleanup their current panels and then re-add them when a new version is installed?

We'll have to:
1. Avoid triggering a HomeConfig invalidation if the panels are being removed as part of an add-on update (otherwise they'll be unintentionally dropped from HomeConfig).
2. Avoid triggering two HomeConfig invalidations when add-ons are updates. One for when the panels are removed and another for when the panels are re-added.
(Reporter)

Comment 7

4 years ago
(In reply to Lucas Rocha (:lucasr) from comment #6)
> (In reply to :Margaret Leibovic from comment #5)
>  > This sounds good. So, given this, we won't need the "HomePanels:Remove"
> > message, but rather we will trigger something to invalidate the HomeConfig
> > when the set of available panels changes?
> 
> Pretty much. But we'll have to be a bit smart when handling add-on updates.
> IIURC, add-ons will cleanup their current panels and then re-add them when a
> new version is installed?

I'm not sure exactly how updates work, but I imagine when restartless add-ons are updated, this is basically what happens. And remember, the way restartless add-ons work is they're basically "installed" on every startup, which makes things like persistent state between app runs (like a persistent panel in the UI) tricky :/
(Assignee)

Comment 8

4 years ago
(In reply to :Margaret Leibovic from comment #7)
> (In reply to Lucas Rocha (:lucasr) from comment #6)
> > (In reply to :Margaret Leibovic from comment #5)
> >  > This sounds good. So, given this, we won't need the "HomePanels:Remove"
> > > message, but rather we will trigger something to invalidate the HomeConfig
> > > when the set of available panels changes?
> > 
> > Pretty much. But we'll have to be a bit smart when handling add-on updates.
> > IIURC, add-ons will cleanup their current panels and then re-add them when a
> > new version is installed?
> 
> I'm not sure exactly how updates work, but I imagine when restartless
> add-ons are updated, this is basically what happens. And remember, the way
> restartless add-ons work is they're basically "installed" on every startup,
> which makes things like persistent state between app runs (like a persistent
> panel in the UI) tricky :/

We should use the 'reason' argument that bootstrapped add-ons take to give enough context to figure out what to do with each add()/remove() call. I filed bug 968188 to track this.
You need to log in before you can comment on or make changes to this bug.