958175 - Ensure panels are registered with a unique id

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

We should append the add-on id to whatever id it passes in to register a list/panel.

Comment 1

[:Margaret Leibovic]

So... we can't actually know if a Home.panels.add call came from within an add-on, and it would break modularity patterns to be digging out an add-on id from within Home.jsm.

I propose we just encourage add-ons to make their own unique ids, such as by including a domain they own within the id, and continue to throw an error if an add-on tries to add a panel with an id that's already being used. This will at least ensure a new add-on can't mess up an existing add-on's data, if we're using that id to store the list data.

This isn't a perfect solution, but I think there's enough human vetting going on here (e.g. A.M.O. review), that this solution is good enough.

Comment 2

[:Margaret Leibovic]

And this isn't just about add-ons, this is about any panel that is added, even from within our default set.

Comment 3

[:Margaret Leibovic]

Attachment: Get rid of PanelConfig constructors that don't take an id

This patch explicitly sets an id for default panels as they're created.

One potential issue here is that a new panel registered in JS could duplicate one of these ids, since JS doesn't actually know about these default panels. I chose an id scheme that's similar to how we might choose to store prefs, but then also using the mozilla.org domain, but maybe we should just be using uuids here.

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 8359533 [details] [diff] [review]
Get rid of PanelConfig constructors that don't take an id

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

For add-on based panels, we should probably go with uuids for panel IDs. Also, let's safeguard the UI code against panel ID clashes (between built-in and add-on panels). Maybe change this line here to concatenate PanelConfig.id and PanelConfig.type?

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomeAdapter.java#152

::: mobile/android/base/home/HomeConfigPrefsBackend.java
@@ +57,5 @@
>          final ArrayList<PanelConfig> panelConfigs = new ArrayList<PanelConfig>();
>  
>          panelConfigs.add(new PanelConfig(PanelType.TOP_SITES,
> +                                         mContext.getString(R.string.home_top_sites_title),
> +                                         "org.mozilla.gecko.home.topsites",

These IDs shouldn't be specific to the backend implementation. I'd prefer to have these IDs defined in PanelType as a 'mPanelId' member.

Comment 5

[Lucas Rocha (:lucasr)]

Forgot to say: no need to complicate the IDs for built-in panels as long as we ensure they don't clash with each other nor with add-on panels. We could just double the PanelType string as the ID. The ID for built-in panels is only there for consistency with the add-on panels. We won't be using them in practice i.e. the PanelType string is enough to know what class to use.

But if you feel strongly about them, please define them in PanelType.

Comment 6

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #4)
> Comment on attachment 8359533 [details] [diff] [review]
> Get rid of PanelConfig constructors that don't take an id
> 
> Review of attachment 8359533 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> For add-on based panels, we should probably go with uuids for panel IDs.
> Also, let's safeguard the UI code against panel ID clashes (between built-in
> and add-on panels). Maybe change this line here to concatenate
> PanelConfig.id and PanelConfig.type?
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/
> HomeAdapter.java#152
> 
> ::: mobile/android/base/home/HomeConfigPrefsBackend.java
> @@ +57,5 @@
> >          final ArrayList<PanelConfig> panelConfigs = new ArrayList<PanelConfig>();
> >  
> >          panelConfigs.add(new PanelConfig(PanelType.TOP_SITES,
> > +                                         mContext.getString(R.string.home_top_sites_title),
> > +                                         "org.mozilla.gecko.home.topsites",
> 
> These IDs shouldn't be specific to the backend implementation. I'd prefer to
> have these IDs defined in PanelType as a 'mPanelId' member.

I think that PanelType should be more generic than specific panels. For example, the LIST panel type is currently what we have in place to create add-on based panels, meaning there could be multiple instances of LIST panels.

It sounds like there's some confusion here about the role of PanelType, and how we'll actually translate a newly registered panel into an actual view that's created on about:home. Yesterday, I was thinking about how we should extend the Home.panels API to actually specify what goes inside the panels. My first idea was to have the API look something like this:

Home.panels.add({
  id: "blog.margaretleibovic.com",
  title: "Margaret's Blog",
  type: "list"
});

Then PanelManager would pass that type along and it would end up being created as a PanelType.LIST panel. The issue with this is that there would only be one "widget" within the panel. We've been talking about letting add-ons include multiple types of "widgets" within a panel, but the way our code is currently architected, a panel view gets created from a class corresponding to its PanelType. 

I feel like this comment has started to extend beyond the scope of this bug, but we should talk about what these ids mean, and how we actually intend to use them, as well as how we intend to use PanelType in the future.

If we want panels to contain arbitrary widgets, we could make some generic PanelType that uses some generic fragment class to instantiate the panel, but then that panel will need to somehow know about what kinds of sub-fragments it should be loading to actually make whatever widgets will go inside it.

I'm temped to limit the scope of all this work and argue that we should just have one data set per panel, displayed in some "type" of view. As an example of a multi-widget panel, we sometimes talk about our top sites panel, but even though that has two different kinds of displays, it still pulls from one data set. The display for that panel could just be some kind of "list with thumbnail previews" type.

Anyway, I definitely scope-creeped this comment :) We can discuss this in the stand-up tomorrow.

Comment 7

[:Margaret Leibovic]

Following a discussion on IRC, and thinking about this a bit more, I think we should just use uuids for the panel ids, and encourage add-ons to do the same.

Comment 8

[:Margaret Leibovic]

Attachment: Use uuids for built-in panel ids

I filed bug 959772 about renaming PanelType.LIST to make things clearing, and also bug 959777 about adding support for actually creating that dynamic content.

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 8359974 [details] [diff] [review]
Use uuids for built-in panel ids

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

uuid it is then :-)

::: mobile/android/base/home/HomeConfigPrefsBackend.java
@@ +57,5 @@
>          final ArrayList<PanelConfig> panelConfigs = new ArrayList<PanelConfig>();
>  
>          panelConfigs.add(new PanelConfig(PanelType.TOP_SITES,
> +                                         mContext.getString(R.string.home_top_sites_title),
> +                                         "4becc86b-41eb-429a-a042-88fe8b5a094e",

Turn these hardcoded ids into constants for better visibility?

Comment 10

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #9)

> ::: mobile/android/base/home/HomeConfigPrefsBackend.java
> @@ +57,5 @@
> >          final ArrayList<PanelConfig> panelConfigs = new ArrayList<PanelConfig>();
> >  
> >          panelConfigs.add(new PanelConfig(PanelType.TOP_SITES,
> > +                                         mContext.getString(R.string.home_top_sites_title),
> > +                                         "4becc86b-41eb-429a-a042-88fe8b5a094e",
> 
> Turn these hardcoded ids into constants for better visibility?

Good idea, I can do that.

Comment 11

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/f72fac727137

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f72fac727137