Splitting this off from bug 1157539, since that already has a large enough scope.

Margaret, I was wondering how an addon is going to define this header image. Should the header image be a new view type that can be combined with other view types? Or should there be a specific data item in the storage that defines what banner image to show?

I was thinking that this header image could just be specified as an option on the view, in addition to these existing options: https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/Home.jsm/panels#View_options I was thinking we could specify an image url, similarly to how we do for backImageUrl, although I now realize that this case will be a bit more complicated, since we'd also want to specify a URL that will open when you click the image. So maybe the API would need to look something like this: { title: "My panel", views: [{ dataset: DATASET_ID, type: Home.panels.View.GRID, header: { imageUrl: ..., url: ... } }] } My main hesitation with making this a new view type that can be combined with other views is that we'll open up a lot of new questions about how different views can be combined with each other. There may also be some technical difficulties, like how to make the whole panel scroll together. I suppose if we were to do this multiple view approach, the API could look something like this: { title: "My panel", views: [{ dataset: HEADER_DATASET_ID, type: Home.panels.View.IMAGE }, { dataset: DATASET_ID, type: Home.panels.View.GRID }] } We could decide to only support very specific combinations of views, but in that case, it seems like it would be clearer to just have the more specific API like above. However, I now realize that this second option would make it easier to dynamically update the header image, since we would only need to update the storage, as opposed to the entire panel. But maybe this header image isn't something that would need to be updated often. For now, I would start with the first approach, since it seems like it would be easier to implement, and then we can explore a more generic solution if we find we need it. We could also mark this new API as "experimental" to give ourselves the option to drop support for it if we find we need to change our approach.

Screenshot of the current state.

This is the WIP version of the patch to add support for headers (See attached screenshot). After some tinkering I ended up implementing option 2. My addon code currently looks like this: > function optionsCallback() { > return { > title: "Speed dial", > layout: "linear", > views: [{ > dataset: BANNER_DATASET_ID, > type: Home.panels.View.BANNER > },{ > dataset: DATASET_ID, > type: Home.panels.View.GRID, > itemType: Home.panels.Item.ICON > }], > default: true, > position: 3 > }; > } I introduced a new layout "linear" that is basically a LinearLayout with orientation vertical (because the default "frame" only supports one view). Right now I named the new view type BANNER because it tries to fill the whole width of the panel and then adjusts the height to keep the aspect ratio of the image. This is something you do not want to happen with every image so I thought BANNER is more descriptive than IMAGE in this case. Sometimes I ran into an issue where only one of the two views received a dataset resulting in one of the views not displaying any data. Because of this and the possibility of combining views in a unintended or broken way, I currently tend to go back to implementing option 1 after all. I am now familiar enough with the code that this seems to be a pretty straight-forward thing to implement. Currently this header is static, meaning that it does not scroll when you scroll the grid. Unfortunately GridViews do not support header/footer like ListViews do. We could try to manually "move" the header away when the GridView is scrolled (synchronized scrolling) but this would only work for one case: 1st view = banner + 2nd view = grid. This new "linear" layout offers many more combinations where this approach would not work. So I would only like to go this route if we implement option 1 (because then we are in control of the layouting). Overall it would be nice to have RecyclerView (-> bug 1171288) for this. Then we could just switch the LayoutManager depending on whether we want to display just a grid or a grid with a header (again -> option 1). And then "everything just scrolls"(tm).

This patch implements option 1 (only static, non-scrollable header). It's much nicer and shouldn't have so many side effects. The matching addon code looks like this: > function optionsCallback() { > return { > title: "Speed dial", > views: [{ > dataset: DATASET_ID, > type: Home.panels.View.GRID, > itemType: Home.panels.Item.ICON, > header: { > image_url: "http://fennec.androidzeitgeist.com/speeddial/header.png", > url: "http://www.mozilla.org" > } > }], > default: true, > position: 3 > }; > }

This should be much easier with RecyclerView now (Using the "option 1" approach). Eventually we could replace all our grid and list implementations for home panels with just one RecyclerView implementation that just uses different LayoutManager implementations. I'll file a follow-up bug for this depending on how things go here. ;)

Bug 1172136 - Create API for add-ons to add header image to grid panels. r?mhaigh

Sample using the new API: https://github.com/pocmo/speed-dial/commit/8487e2b2376fac4ae9416f6a25551a0288d03ea9

Comment on attachment 8629911 [details] MozReview Request: Bug 1172136 - Create API for add-ons to add header image to grid panels. r?mhaigh https://reviewboard.mozilla.org/r/12653/#review11279 Good overall but I have a critisism about the existing architecture of HomeConfig.java which stems from my dislike of using null objects. I'd prefer if all config objects implemented a common interface and either there was also a EmptyConfig object which we could instantiate if the config in question wasn't used, or the config objects could handle being empty. Together with this I'd prefer if objects were responsible for themselves, so instead of this codeblock: if (mHeaderConfig != null) { json.put(JSON_KEY_HEADER, mHeaderConfig.toJSON()); } instead we passed the json object in to the mHeaderConfig object and it assign whatever values are needed to the json object. Anyway - just a thought. I know you're just following the existing architecture on this one, so from that point of view I don't have a problem. ::: mobile/android/base/home/PanelRecyclerViewAdapter.java:124 (Diff revision 1) > + if (viewConfig.hasHeaderConfig()) { return viewConfig.hasHeaderConfig() ? 1 : 0;

(In reply to Martyn Haigh (:mhaigh) from comment #12) > Good overall but I have a critisism about the existing architecture of > HomeConfig.java which stems from my dislike of using null objects. I agree. And with > 1600 lines this thing is biiig. It wasn't easy to navigate inside the class while writing the patch. How do we approach these things usually? Do we try to address these problems when (re)writing code in this area or do we go ahead and create a "fix technical debt" bug? I think I haven't seen such bugs yet and typically they are around forever without any progress.