We should create a new dynamic home panel layout to allow add-ons to create a "speed dial" type of panel with tiles similar to our built-in Top Sites panel. Right now we only support "list" and "grid" layout type, so this would be a third new type.

Typically, "speed dial" panels have smaller cells (not really thumbnails) that show a small logo or favicon and a background color. Top Sites style panels have larger cells that show a logo + color or a thumbnail. Rough thoughts: This new panel style could specify the cell size, maybe just small and large. The small size would be 3 x 5 (?) and the large would be 2 x 3 (basically what we do now). The add-on would decide what to display for the image and how to handle the cell taps.

Here's a WIP I've been working on. This adds support for a new "icon" item type that you can specify for a home panel view. These icons support displaying a background color, icon, and title. Here's an example of how this would work in practice: https://github.com/leibovic/speed-dial/blob/master/bootstrap.js This seems to generally work, but I feel like this will need some UX love to figure out how it should behave in edge cases. Some questions I can think of: * Do we require a title or is it optional? * How will this look on different screen sizes? * What size icon do we require? * What happens if an icon is excluded? * What happens if there's an awkward number of items, such that we can't fill an entire last row? mhaigh, I'm asking you for feedback since you expressed interest in helping out with some of this home panel work. No time like the present to jump in! :) Also, there is (luckily) already a testHomeProvider. I can add a testcase to that for my migration logic here (migrations make me nervous).

What can we do to improve this? See also the questions in my last comment.

(In reply to :Margaret Leibovic from comment #2) > Created attachment 8609119 [details] [diff] [review] > WIP - Create "speed dial" dynamic home panel layout > > Here's a WIP I've been working on. This adds support for a new "icon" item > type that you can specify for a home panel view. These icons support > displaying a background color, icon, and title. This is looking great. I feel like this, plus the layout we're using for the Instagram panel, could make for some pretty great layout alternatives. > Here's an example of how this would work in practice: > https://github.com/leibovic/speed-dial/blob/master/bootstrap.js > > This seems to generally work, but I feel like this will need some UX love to > figure out how it should behave in edge cases. Some questions I can think of: > > * Do we require a title or is it optional? At this size, I think an icon and a background is enough. I'd say make the title (text) optional. > * How will this look on different screen sizes? I'd just spec out the padding around each item, along with the number of items per row so it can scale. Rather than the dimensions of each item. > * What size icon do we require? I can draw a spec up for these things. The icons I gave you here were rather arbitrary. > * What happens if an icon is excluded? I think it would look fine, depending on what the background image is. We'd need a minimum of 1 out of the 3 for sure. > * What happens if there's an awkward number of items, such that we can't > fill an entire last row? Right now that seems OK to me. I think we could even set a maximum as well. Do we know what we'd like users to use this for mostly? That would really help me spec out the designs and improve this if we had more "use cases" in mind.

Comment on attachment 8609120 [details] screenshot Nice! can we get the padding of each "dial" to be 6dp? (I gave you the total by accident before :P) Whats the spec on the type we have there?

Comment on attachment 8609119 [details] [diff] [review] WIP - Create "speed dial" dynamic home panel layout Review of attachment 8609119 [details] [diff] [review]: ----------------------------------------------------------------- I think the panel_icon_item.xml file is missing from the patch, as I can't find this locally. Overall it looks good. I don't 100% understand this area of code yet, but your changes seem sane. Keep firing patches my way and I'll get my head around what's going on sooner or later ::: mobile/android/base/db/BrowserContract.java @@ +333,5 @@ > public static final String URL = "url"; > public static final String TITLE = "title"; > public static final String DESCRIPTION = "description"; > public static final String IMAGE_URL = "image_url"; > + public static final String BGCOLOR = "bgcolor"; Perhaps BG_COLOR (and bg_color) to follow the others (DATASET_ID & IMAGE_URL)? ::: mobile/android/base/home/PanelItemView.java @@ +4,5 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > package org.mozilla.gecko.home; > > +import android.graphics.Color; Move this under the moz imports @@ +95,5 @@ > } > > + private static class IconItemView extends PanelItemView { > + private IconItemView(Context context) { > + super(context, R.layout.panel_icon_item, R.attr.panelIconViewStyle); Did you forget to include panel_icon_item.xml ?

Updated to add panel_icon_item.xml!

Attaching mock of "speed dial"

through this together quickly, let me know if it works for you!

Now the panel should look like the spec (see nexus 5/9 screenshots): Background color or background image, optional title with transparent background, tiles are always squares.

Comment on attachment 8617292 [details] [diff] [review] 1157539_speed_dial_panel.patch Review of attachment 8617292 [details] [diff] [review]: ----------------------------------------------------------------- Awesome! This is looking so good! I feel like we should just get ready to land this. We can mark these APIs as "experimental" and continue to iterate on them if necessary, but I think this makes sense. You should also get a real review from someone in addition to me, since I wrote the first version of this patch and am biased :) ::: mobile/android/base/home/PanelItemView.java @@ +61,4 @@ > } > + if (titleDescContainer != null) { > + titleDescContainer.setVisibility(hasTitle || hasDescription ? View.VISIBLE : View.GONE); > + } I added these null checks in my WIP as a quick work-around, but I think we should explicitly document what views we expect to find in here. @@ +86,5 @@ > + ImageLoader.with(getContext()) > + .load(backgroundUrl) > + .fit() > + .into(background); > + } I also wonder how this will work with ImageItemViews, since you could theoretically specify an image_url and a background_url in that case. Maybe instead of adding a new ICON item type, we should be updating the existing IMAGE item type to support showing an icon on top of a background image. I suppose another concern there is how we display the title/description differently. And also that we want these views to be smaller... I was going to suggest that we explore updating our current IMAGE item type, but if we did that, we would still need to specify something in the API to distinguish between the larger images and the smaller images, so this current approach is probably our best bet. ::: mobile/android/base/home/SquaredRelativeLayout.java @@ +25,5 @@ > + @Override > + protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) { > + super.onMeasure(widthMeasureSpec, heightMeasureSpec); > + > + int squareMeasureSpec = MeasureSpec.makeMeasureSpec(getMeasuredWidth(), MeasureSpec.EXACTLY); Should we update SquaredImageView to do this the same way? This file should probably go in the widget package, since it's more generic than something that's just used on about:home. ::: mobile/android/base/resources/layout/panel_icon_item.xml @@ +15,5 @@ > + > + <ImageView > + android:id="@+id/image" > + android:layout_width="50dp" > + android:layout_height="50dp" We should probably put this in dimens.xml. ::: mobile/android/modules/HomeProvider.jsm @@ +232,5 @@ > yield db.execute(SQL.createItemsTable); > break; > + case 3: > + yield db.execute(SQL.addColumnBackgroundColor); > + yield db.execute(SQL.addColumnBackgroundUrl); This is a pretty straightforward migration, but I wonder if there's a way we can add a test for it in here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/robocop/testHomeProvider.js We should also update that test to account for these new columns.

Great work Sebastian! that padding looks a bit big in these images though, is it 6dp around each view/grid? (adding up to 12dp in between and keeping 6dp around the outside) Just making sure, but I think it might just be that im not viewing these on devices Again, this looks amazing! :D

(In reply to Anthony Lam (:antlam) from comment #16) > Great work Sebastian! that padding looks a bit big in these images though, > is it 6dp around each view/grid? (adding up to 12dp in between and keeping > 6dp around the outside) It's 6dp everywhere but not adding up to 12dp right now (Actually it's horizontal spacing = vertical spacing = outter padding = 6dp). I've added a new screenshot where I've set horizontal and vertical spacing to 12dp but this looks odd..?

(In reply to :Sebastian Kaspari from comment #17) > Created attachment 8617487 [details] > 12dp-spacing.png > > (In reply to Anthony Lam (:antlam) from comment #16) > > Great work Sebastian! that padding looks a bit big in these images though, > > is it 6dp around each view/grid? (adding up to 12dp in between and keeping > > 6dp around the outside) > > It's 6dp everywhere but not adding up to 12dp right now (Actually it's > horizontal spacing = vertical spacing = outter padding = 6dp). I've added a > new screenshot where I've set horizontal and vertical spacing to 12dp but > this looks odd..? Ah definitely odd. It sounds like what you're doing is right anyways ;)

I uploaded a new patch addressing the points above. Martyn: I've set you as reviewer because you already reviewed the WIP patch. :) (In reply to :Margaret Leibovic from comment #15) > ::: mobile/android/base/home/SquaredRelativeLayout.java > @@ +25,5 @@ > > + @Override > > + protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) { > > + super.onMeasure(widthMeasureSpec, heightMeasureSpec); > > + > > + int squareMeasureSpec = MeasureSpec.makeMeasureSpec(getMeasuredWidth(), MeasureSpec.EXACTLY); > > Should we update SquaredImageView to do this the same way? SquaredImageView is simpler: It's enough to measure once and set the new size to getMeasuredWidth() x getMeasuredWidth(). SquaredRelativeLayout is doing another super.onMeasure() pass because it is a ViewGroup and its children need to be re-measured too. Maybe it is possible to calculate the width without calling onMeasure() the first time and then passing this calculated width to super.onMeasure(). Perhaps I should create a follow-up bug to analyze this (RelativeLayout.onMeasure() is roughly 300 lines long).

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c753ae0a435

Comment on attachment 8617940 [details] [diff] [review] 1157539_speed_dial_panel_v2.patch Review of attachment 8617940 [details] [diff] [review]: ----------------------------------------------------------------- LGTM ::: mobile/android/base/home/PanelItemView.java @@ +4,5 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > package org.mozilla.gecko.home; > > +import android.graphics.Color; We generally try to follow the import structure of org.mozilla.gecko.* first then everything else below that. If you're using IntelliJ / Android Studio you can set this in 'Preferences > Editor > Code Style > Java', then select the imports tab and add "org.mozilla.gecko" to the 'Import Layout' section, followed by a blank line. ::: mobile/android/base/resources/values/dimens.xml @@ +183,5 @@ > <dimen name="panel_grid_view_column_width">150dp</dimen> > > <!-- PanelItemView dimensions --> > <dimen name="panel_article_item_height">95dp</dimen> > + <dimen name="panel_icon_item_size">50dp</dimen> This may be a question of style consistency within the app, currently I don't think we have a standard way of thinking about this. Personally I'd rather see us use the dimens file for only these reasons: 1, the dimension is used multiple times and each usage is symantically identical - this is to ensure consistency across files & separate parts of the app 2, the dimension has different values across different configs - this is to cater for difference device configs 3, the dimension needs to be referenced in Java code via the R class - this is to leverage the built in metrics system in Android In this case I'd rather put the dimens in the layout file with a comment, this will mean we don't litter the dimens file with values that don't really belong there. margaret: You recommended putting this dimenension in the dimen file - was there a specific reason for this that I have overlooked? ::: mobile/android/modules/HomeProvider.jsm @@ +20,5 @@ > /* > * SCHEMA_VERSION history: > * 1: Create HomeProvider (bug 942288) > * 2: Add filter column to items table (bug 942295/975841) > + * 3: Add background_color and background_url columns Add bug number for reference @@ +367,5 @@ > title: item.title, > description: item.description, > image_url: item.image_url, > + background_color: item.background_color, > + background_url: item.background_url, Out of interest, do we do any validation on the data before it gets inserted in to the DB? Do we do any validation at all?

margaret: question above about dimensions and the dimen file

(In reply to Martyn Haigh (:mhaigh) from comment #21) > If you're using IntelliJ / Android Studio you can set this in 'Preferences > > Editor > Code Style > Java', then select the imports tab and add > "org.mozilla.gecko" to the 'Import Layout' section, followed by a blank line. Thank you, that's helpful. :) > This may be a question of style consistency within the app, currently I > don't think we have a standard way of thinking about this. Personally I'd > rather see us use the dimens file for only these reasons: Good points. I think the general idea here was to just avoid having these "magic" numbers scattered in code (treating layout xml files as "code" that defines the UI). > 2, the dimension has different values across different configs - this is to > cater for difference device configs (2) might actually apply here (even though it does not for the attached patch): Currently we adjust the number of columns based on the device size (~ phone = 3, tablet = 6). This way the tiles have roughly the same physical size and we can display more of them on a tablet. However a speed dial might have a very limited number of items (assumption!) and therefore we might not want to show more on a tablet but instead bigger tiles. > Out of interest, do we do any validation on the data before it gets inserted > in to the DB? Do we do any validation at all? As far as I have seen we do validate that some properties do exist and the combination of properties make sense (e.g. an item has to have an image_url, title or description). I have run into some of these while extending the API for bug 1172136. Nonetheless we do not validate the values (e.g. Is the image_url actually a valid url?).

(In reply to Martyn Haigh (:mhaigh) from comment #22) > margaret: question above about dimensions and the dimen file I think I was just getting ahead of myself and thinking about how we might use different sizes across different configs (as sebastian mentioned above). However, that's probably premature, so putting it in the layout file would be fine.

I updated the patch: I moved the value back from dimes.xml to the layout for now and addressed the other two points. @margaret: Are you okay with me landing this as-is? I'd like to create follow-up bugs for the next iterations if we will need them. For instance I would like to create one for writing a test that runs the migration code. Another try run of the latest version of the patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f9143cc153a

Try run without unrelated WIP patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56ef41e011c8

(In reply to :Sebastian Kaspari from comment #25) > @margaret: Are you okay with me landing this as-is? I'd like to create > follow-up bugs for the next iterations if we will need them. For instance I > would like to create one for writing a test that runs the migration code. It would probably be best to write a test for the migration code before landing the actual migration... although this one is pretty straightforward, so I think some manual testing would suffice. Other than that, I think it's fine to land this patch as-is. We should also update the MDN docs with these new APIs, but we can mark them as "experimental" until they ship.

I did some extensive testing of the migration with a couple of addons installed and I actually found a problem: After updating addon panels are empty and I can see the following error in the log: > 06-16 14:33:00.805 6294-6314/org.mozilla.fennec_sebastian E/GeckoConsole﹕ [JavaScript Error: > "Error refreshing dataset catfacts.dataset@margaretleibovic.com: Error: Error(s) encountered during > statement execution: duplicate column name: background_color" {file: "resource://gre/modules/addons > /XPIProvider.jsm -> jar:file:///data/data/org.mozilla.fennec_sebastian/files/mozilla/kx6vq4o0.default > /extensions/catfacts@margaretleibovic.com.xpi!/bootstrap.js" line: 63}] For some reason the background_color column seems to be duplicated. Looking at the patch I can't find a reason why this is happening. @Margaret: Could it be that getDatabaseConnection() (HomeProvider.jsm) is called multiple times in parallel, leading to upgradeDatabase() being called multiple times as well (race condition)? Right now I can't see an other reason why the column should exist multiple times. I can also see the following error for some addons after the update. But I'm not sure what is triggering it or if it is just a follow-up bug. > 06-16 14:30:01.550 4361-4688/org.mozilla.fennec_sebastian E/GeckoConsole﹕ [JavaScript Error: > "Error saving data to HomeProvider: Error: Must define SQL to execute as a string: undefined" {file: > "resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///data/data/org.mozilla.fennec_sebastian > /files/mozilla/kx6vq4o0.default/extensions/wikipedia.panel@margaretleibovic.com.xpi!/bootstrap.js" > line: 138}] These two errors only show up after database migration. After re-install or clearing app data everything is fine.

After some more debugging I see strange things happening. It seems like upgradeDatabase() is called for every addon installed but in some cases also for addons that are installed after upgrading. Here I had two addons installed (wikipedia + catfacts) and after installing the new app version I installed the speed dial addon: > 06-16 16:36:20.152 3589-3608/org.mozilla.fennec_sebastian I/GeckoConsole﹕ Upgrading from 2 to 3 > 06-16 16:36:20.673 3589-3608/org.mozilla.fennec_sebastian I/GeckoConsole﹕ Upgrading from 2 to 3 > 06-16 16:36:53.187 3589-3608/org.mozilla.fennec_sebastian I/GeckoConsole﹕ Upgrading from 2 to 3 With the following log code: > /** > * Migrates the database schema to a new version. > */ > function upgradeDatabase(db, oldVersion, newVersion) { > return Task.spawn(function upgrade_database_task() { > Services.console.logStringMessage("Upgrading from " + oldVersion + " to " + newVersion); Most of the time the upgrade method is only called after a panel decides to refresh its data or a new addon is installed. Right after upgrading the app the panels are empty but also do not refresh immediately.

Argh. I found the problem. It's in my code. :) First: > + addBackgroundUrl: > + "ALTER TABLE items ADD COLUMN background_url TEXT", And later: > yield db.execute(SQL.addColumnBackgroundUrl); addBackgroundUrl <-> addColumnBackgroundUrl The migration failed silently at this point and therefore the schema version was never updated, triggering more tries later. I'll create a new patch. There's one problem left though: The migration is only called from HomeProvider.jsm : getDatabaseConnection(). This leads to two problems: * After upgrading Fennec the migration is not performed as long as no addon code calls getDatabaseConnection(). And this doesn't happen very often. In my tests I always installed another addon just to trigger this. * The Java side building the UI expects the new columns to be already present in the database. Without the migration being performed this leads to an empty addon panel showing: "No content could be found for this panel." - until the migration is performed. -> We could try to make the Java side more robust against missing (new) columns and/or try to call the migration code when loading HomeProvider.jsm (follow-up bug for this?).

This updated patch fixes the migration code. In addition to that the Java side now gracefully handles the case that the new columns do not exist yet. Now the migration is working and the addon panels will still be able to show old data. We probably still want to open a bug to run the migration earlier in the future.

Comment on attachment 8624341 [details] [diff] [review] 1157539_speed_dial_panel_v5.patch Review of attachment 8624341 [details] [diff] [review]: ----------------------------------------------------------------- Looking good! Ship it.

I added the new item type Type.ICON to MDN and requested review: https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/Home.jsm/panels It seems like we do not have any docs about the items itself (e.g. something where I could add background_url and background_color).

(In reply to :Sebastian Kaspari from comment #37) > I added the new item type Type.ICON to MDN and requested review: > https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/Home.jsm/ > panels > > It seems like we do not have any docs about the items itself (e.g. something > where I could add background_url and background_color). Obviously it is not clear, but we do have documentation about that here: https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/HomeProvider.jsm/HomeStorage

So for the docs, as I understand it: * the Hub APIs let you create a panel to display data in the browser's home page * the panel can contain a number of views * each view can be of one of two types, a list or a grid * a view gets its data for the actual items to display from a dataset ID, which refers to a dataset stored using the HomeStorage API * this dataset is an array of "items", which have a number of possible attributes (url, title, description, ...) This bug adds a new kind of grid view, a "speed dial". In this view each item is presented with a background image, a background color, and optionally a title and an icon. To create one of these speed dials: * specify GRID as the view's type * specify ICON as the view's itemType * make sure the dataset for the view includes background_icon and background_color for each item So strictly from the point of view of the API docs, there are three changes: * add Item type ICON in the panels API * add background_icon and background_color to the Item attributes in the HomeStorage API Is that all more or less correct? It might also be helpful to add another section in https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/Firefox_Hub_Walkthrough, called "View types" or something, that covers the three main view types, with a screenshot for each one, and quick instructions for how to create a view of that type. If you think that would help, I can write that, but it would help a lot if you could point me at screenshots for each type.

(In reply to Will Bamberg [:wbamberg] from comment #39) > So for the docs, as I understand it: > * the Hub APIs let you create a panel to display data in the browser's home > page > * the panel can contain a number of views > * each view can be of one of two types, a list or a grid > * a view gets its data for the actual items to display from a dataset ID, > which refers to a dataset stored using the HomeStorage API > * this dataset is an array of "items", which have a number of possible > attributes (url, title, description, ...) > > This bug adds a new kind of grid view, a "speed dial". In this view each > item is presented with a background image, a background color, and > optionally a title and an icon. > > To create one of these speed dials: > * specify GRID as the view's type > * specify ICON as the view's itemType > * make sure the dataset for the view includes background_icon and > background_color for each item > > So strictly from the point of view of the API docs, there are three changes: > * add Item type ICON in the panels API > * add background_icon and background_color to the Item attributes in the > HomeStorage API > > Is that all more or less correct? Yes, that all sounds correct to me. > It might also be helpful to add another section in > https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/ > Firefox_Hub_Walkthrough, called "View types" or something, that covers the > three main view types, with a screenshot for each one, and quick > instructions for how to create a view of that type. Yes, that's a good idea. > If you think that would help, I can write that, but it would help a lot if > you could point me at screenshots for each type. Sebastian has some demo add-ons, so I'm going to volunteer him to help you out. He also recently landed a patch in bug 1172136 for a header item that can appear at the top of these panels. We could also update our boilerplate add-on to include these new APIs as well: https://github.com/mozilla/firefox-for-android-addons/tree/master/boilerplate/hub-boilerplate

Thanks Margaret. I'll write something up for that and ask you or Sebastian to check. For screenshots, I guess I could use these for the two "speed dial" types: https://bug1157539.bugzilla.mozilla.org/attachment.cgi?id=8617290 https://bug1172136.bugzilla.mozilla.org/attachment.cgi?id=8629912 ...so I'd just need something similar for the list and default grid types.

Yeah, I added item type icon to MDN recently but what I still have on my list and didn't do yet: * Add background_icon and background_color to item documentation on MDN * Add header (as seen in the screenshots) documentation to MDN. That's actually bug 1172136 The add-on code used for these screenshots is on GitHub: https://github.com/pocmo/speed-dial

OK, I've: * added a "View types" section to the walkthrough doc: https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/Firefox_Hub_Walkthrough#View_types, that lists each of the three types, with screenshots, and explains what data you need in the dataset for them. This section also talks about headers. * added `header` to: https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/Home.jsm/panels#View_options * added `background_color` and `background_url` to https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/HomeProvider.jsm/HomeStorage#Item_attributes Please let me know if this is OK and I'll mark this bug (and bug 1172136) as dev-doc-complete. I don't have an Android phone, so can't generate screenshots at all easily. So if you think new screenshots would be worth doing, it would be really helpful if you could generate them for me. FWIW I think these screenshots are OK.

This is looking really great. Thank you! There's just one thing that needs to be changed: The header does scroll. You can think of it as the first row of the grid.

Thanks for taking a look, and spotting my error. It's now fixed. I wasn't sure if it scrolled or not after the discussion in the other bug, so I just guessed :-).

Added to the 41 release notes using: Generic API let's developers create a grid of icons and then optionally allow for a large header image as wording (thanks Mark) with a link pointing to https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/Firefox_Hub_Walkthrough#View_types