So far we've built the Home.panels API to allow for adding new panels with a given id and title, but we need to expand on that to allow consumers to specify what will go in those panels. We want to design a system that will one day allow for multiple "widgets" within a panel, each powered by their own "list", but for v1, we should aim to just support one widget per panel. A simple API that only allows one widget per panel could look something like this: Home.panels.add({ id: "my-panel", title: "My Panel", type: "simple-list", list: "my-panel-list" }); The add-on would then sync data to the lists provider with a list id "my-panel-list" (we should implement this part in bug 942288). Then we would use "my-panel-list" to query the HomeListsProvider for data to back a ListView in the panel. A more advanced API that allows multiple widgets could look something like this: Home.panels.add({ id: "my-panel", title: "My Panel", widgets: [ { type: "simple-list", list: "my-panel-list" }, { type: "thumbnail-list", list: "my-other-panel-list" } ] }); A few concerns/questions here: * We should make sure these ids are unique. I just chose simple ids for demonstration purposes, but we may want to encourage add-ons to use uuids. It would be nice if we could generate the ids ourselves, but restartless add-ons are designed to have their install code run on each app run, so we would need to work around that to persist an id between runs. I think we can punt on this for right now, though. * We should probably pull out the widget types into constants, instead of just passing strings. * Do we want to provide a different API to add/remove widgets from a panel? * Do we want to call these things "widgets"? :) * The term "list" might be confusing if these widget views can also be grids of thumbnails. However, I think we will always be using an ordered set of data for any view, since a cursor always returns data in some order, so it's probably fine to keep this term (and with it HomeListsProvider). My goal here is to start a discussion. Feedback welcome!

(In reply to :Margaret Leibovic from comment #0) > So far we've built the Home.panels API to allow for adding new panels with a > given id and title, but we need to expand on that to allow consumers to > specify what will go in those panels. > > We want to design a system that will one day allow for multiple "widgets" > within a panel, each powered by their own "list", but for v1, we should aim > to just support one widget per panel. > > A simple API that only allows one widget per panel could look something like > this: > > Home.panels.add({ > id: "my-panel", > title: "My Panel", > type: "simple-list", > list: "my-panel-list" > }); > > The add-on would then sync data to the lists provider with a list id > "my-panel-list" (we should implement this part in bug 942288). Then we would > use "my-panel-list" to query the HomeListsProvider for data to back a > ListView in the panel. > > A more advanced API that allows multiple widgets could look something like > this: > > Home.panels.add({ > id: "my-panel", > title: "My Panel", > widgets: [ > { type: "simple-list", > list: "my-panel-list" > }, > { type: "thumbnail-list", > list: "my-other-panel-list" > } > ] > }); The problem with providing both the simple option *and* the advanced version is that it creates some confusing inconsistencies in the API i.e. in the simple version the 'list' is associated with the 'panel' itself while in the advanced version it's associated with a specific 'widget'. It would be nice if we could avoid that somehow. So, maybe what we should do is something like this: Have type as 'single-list' (maybe rename it to something more generic like 'frame-layout') but still define a list of 'widgets' for it. It's just that a panel of type 'frame-layout' will only use a single element from the 'widgets' list. Later we can have a, say, 'flex' type that does fancier things with the list of widgets e.g. responsive design stuff. We can provide only a 'list' widget type for now. Later we can add things like 'grid', 'image', etc. > A few concerns/questions here: > * We should make sure these ids are unique. I just chose simple ids for > demonstration purposes, but we may want to encourage add-ons to use uuids. > It would be nice if we could generate the ids ourselves, but restartless > add-ons are designed to have their install code run on each app run, so we > would need to work around that to persist an id between runs. I think we can > punt on this for right now, though. Maybe we could just allow add-on developers to define panel ids however they want and, under the hood, we could simply ensure that all panel ids defined by the add-on are prefixed with the add-on id before sending panelinfos to the java side. This means the add-ons developers would only have to worry about duplicate panel ids in the same add-on. > * We should probably pull out the widget types into constants, instead of > just passing strings. Yep. Same for panel type. > * Do we want to provide a different API to add/remove widgets from a panel? This would probably add a lot of complexity to DynamicPanel. I'd punt this until we have a specific use case in mind. > * Do we want to call these things "widgets"? :) I'm fine with 'widget'. > * The term "list" might be confusing if these widget views can also be grids > of thumbnails. However, I think we will always be using an ordered set of > data for any view, since a cursor always returns data in some order, so it's > probably fine to keep this term (and with it HomeListsProvider). I wonder if 'dataset' wouldn't be a better name?

Given this last comment, and what we talked about this morning, here's an updated example: Home.panels.add({ id: "my-panel", title: "My Panel", layout: "frame", views: [ { type: "list", dataset: "my-panel-list" } ] }); And for v2, we could expand this support different kinds of layouts, like: Home.panels.add({ id: "my-panel", title: "My Panel", layout: "tabs", views: [ { type: "list", dataset: "my-panel-list" }, { type: "list", dataset: "my-other-panel-list" } ] }); What do we think of this? The only thing that seems kind of weird to me is creating a layout that only supports one view, yet having the "views" property still be an array.

Thinking about this more, maybe "type" is a better property name for specifying the layout type. My only concern is that I wouldn't want the panel type to get confused with the view type, but all sorts of things have types in programming, so I don't think this will actually be confusing in practice.

Here's a quick WIP to start sketching out what the API will look like. I wanted to call this Type object PanelType, and then have a corresponding PanelType enum in Java, but that already exists for something else! So maybe we should call this LayoutType or something like that. Thoughts? Also, right now PanelManager doesn't have any consumers, so it's hard to see exactly how this will be used, but these PanelInfos will have to get converted into PanelConfigs to be used in DynamicPanel. I imagine the settings code will do that. I wonder if we should continue to pass along the views as a JSONArray like this, or if we should do some other processing on that in PanelManager. PanelConfigs are stored as JSON in HomeConfigPrefsBackend, so it might make the most sense to just pass it along as a JSONArray until it needs to be used by something.

Comment on attachment 8360869 [details] [diff] [review] WIP Review of attachment 8360869 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to :Margaret Leibovic from comment #4) > Here's a quick WIP to start sketching out what the API will look like. I > wanted to call this Type object PanelType, and then have a corresponding > PanelType enum in Java, but that already exists for something else! So maybe > we should call this LayoutType or something like that. Thoughts? Yeah, I'd go with 'layout' on the JS side, and something like HomeConfig.LayoutType (to be defined in bug 959777) on the Java side. This seems less prone to confusion. 'type' is probably fine in the context of views (HomeConfig.ViewType on the java side). > Also, right now PanelManager doesn't have any consumers, so it's hard to see > exactly how this will be used, but these PanelInfos will have to get > converted into PanelConfigs to be used in DynamicPanel. I imagine the > settings code will do that. I wonder if we should continue to pass along the > views as a JSONArray like this, or if we should do some other processing on > that in PanelManager. PanelConfigs are stored as JSON in > HomeConfigPrefsBackend, so it might make the most sense to just pass it > along as a JSONArray until it needs to be used by something. Yeah, let's assume the conversion from PanelInfo to PanelConfig will be done lazily by the settings UI. So just storing the JSONArray in PanelInfo is fine at this point. ::: mobile/android/base/home/PanelManager.java @@ +112,5 @@ > > private PanelInfo getPanelInfoFromJSON(JSONObject jsonPanelInfo) throws JSONException { > + final String id = jsonPanelInfo.getString("id"); > + final String title = jsonPanelInfo.getString("title"); > + final String type = jsonPanelInfo.getInt("type"); Isn't type an int? ::: mobile/android/modules/Home.jsm @@ +154,5 @@ > let HomePanels = { > + // Holds set of valid panel types. > + Type: { > + FRAME_LAYOUT: 0 > + }, If we rename this to Layout, I'd go with just 'Layout.FRAME' (instead of FRAME_LAYOUT). @@ +155,5 @@ > + // Holds set of valid panel types. > + Type: { > + FRAME_LAYOUT: 0 > + }, > + Maybe we should also define an 'enum' for view types (instead of relying on strings being passed around)? Something like: View: { LIST: 0 }

(In reply to Lucas Rocha (:lucasr) from comment #5) Thanks for the feedback! > (In reply to :Margaret Leibovic from comment #4) > > Here's a quick WIP to start sketching out what the API will look like. I > > wanted to call this Type object PanelType, and then have a corresponding > > PanelType enum in Java, but that already exists for something else! So maybe > > we should call this LayoutType or something like that. Thoughts? > > Yeah, I'd go with 'layout' on the JS side, and something like > HomeConfig.LayoutType (to be defined in bug 959777) on the Java side. This > seems less prone to confusion. 'type' is probably fine in the context of > views (HomeConfig.ViewType on the java side). Cool, this sounds good. > ::: mobile/android/base/home/PanelManager.java > @@ +112,5 @@ > > > > private PanelInfo getPanelInfoFromJSON(JSONObject jsonPanelInfo) throws JSONException { > > + final String id = jsonPanelInfo.getString("id"); > > + final String title = jsonPanelInfo.getString("title"); > > + final String type = jsonPanelInfo.getInt("type"); > > Isn't type an int? Oops, this is probably left from when I was iterating on different versions of this ;) > ::: mobile/android/modules/Home.jsm > @@ +154,5 @@ > > let HomePanels = { > > + // Holds set of valid panel types. > > + Type: { > > + FRAME_LAYOUT: 0 > > + }, > > If we rename this to Layout, I'd go with just 'Layout.FRAME' (instead of > FRAME_LAYOUT). Yeah, that sounds better. > @@ +155,5 @@ > > + // Holds set of valid panel types. > > + Type: { > > + FRAME_LAYOUT: 0 > > + }, > > + > > Maybe we should also define an 'enum' for view types (instead of relying on > strings being passed around)? Something like: > > View: { > LIST: 0 > } Good idea.

This patch addresses feedback comments. I was uncertain about whether or not to create a LayoutType in PanelInfo, as opposed to just passing a string, but I think that a string is more consistent, since we're passing a JSONArray for the views. I also decided it would be nicer to construct the enum type with a string, rather than an int, since I think the code is more readable if we just have corresponding strings in JS and Java. I held off on making a Java type for ViewType, I figure we can create that when we actually have a consumer for it (in fact, maybe we should hold off on landing this LayoutType as well). With this patch, an example of the API in use looks like: Home.panels.add({ id: "my-unique-panel-id", title: "My Panel", layout: Home.panels.Layout.FRAME, views: [{ type: Home.panels.View.LIST, dataset: "my-unique-dataset-id" }] });

Comment on attachment 8361316 [details] [diff] [review] support home panel layout type and views Review of attachment 8361316 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to :Margaret Leibovic from comment #7) > Created attachment 8361316 [details] [diff] [review] > support home panel layout type and views > > This patch addresses feedback comments. > > I was uncertain about whether or not to create a LayoutType in PanelInfo, as > opposed to just passing a string, but I think that a string is more > consistent, since we're passing a JSONArray for the views. Yeah, I think it's fine it keep the data in raw form until we actually need to create a PanelConfig instance out of it. > I also decided it would be nicer to construct the enum type with a > string, rather than an int, since I think the code is more readable > if we just have corresponding strings in JS and Java. Makes sense. > I held off on making a Java type for ViewType, I figure we can create that > when we actually have a consumer for it (in fact, maybe we should hold off > on landing this LayoutType as well). > > With this patch, an example of the API in use looks like: > > Home.panels.add({ > id: "my-unique-panel-id", > title: "My Panel", > layout: Home.panels.Layout.FRAME, > views: [{ > type: Home.panels.View.LIST, > dataset: "my-unique-dataset-id" > }] > }); Looking good :-) ::: mobile/android/base/home/HomeConfig.java @@ +83,5 @@ > > + /** > + * Used to determine what type of layout to display in a dynamic panel. > + */ > + public static enum LayoutType implements Parcelable { I have this exact code in my patch for bug 959777. You can drop this part from your patch as you're not using it anywhere anyway -- and to save me some time handling the merge conflicts too :-) ::: mobile/android/modules/Home.jsm @@ +204,5 @@ > + throw "Home.panels: Invalid view type: panel.id = " + panel.id + ", view.type = " + view.type; > + } > + > + if (!view.dataset) { > + throw "Home.panels: No dataset provided for view: panel.id = " + panel.id + ", view.type = " + view.type; In the far far future, we might have view types that don't expect a dataset :-) But let's keep this in for now.

(In reply to Lucas Rocha (:lucasr) from comment #8) > ::: mobile/android/base/home/HomeConfig.java > @@ +83,5 @@ > > > > + /** > > + * Used to determine what type of layout to display in a dynamic panel. > > + */ > > + public static enum LayoutType implements Parcelable { > > I have this exact code in my patch for bug 959777. You can drop this part > from your patch as you're not using it anywhere anyway -- and to save me > some time handling the merge conflicts too :-) Haha, I'm glad our brains are in sync! I'll leave this out when I land this patch. > ::: mobile/android/modules/Home.jsm > @@ +204,5 @@ > > + throw "Home.panels: Invalid view type: panel.id = " + panel.id + ", view.type = " + view.type; > > + } > > + > > + if (!view.dataset) { > > + throw "Home.panels: No dataset provided for view: panel.id = " + panel.id + ", view.type = " + view.type; > > In the far far future, we might have view types that don't expect a dataset > :-) But let's keep this in for now. Yeah, once we support that, we can update the exception :)