959777 - Dynamically build views for third-party panel content from PanelConfig

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[:Margaret Leibovic]

Right now ListPanel (soon to be renamed DynamicPanel) always contains a ListView, but it should pull information from mPanelConfig to determine what kinds of views to create. And then it should also use information from mPanelConfig to determine what data to use to populate these views, querying that data from HomeListsProvider.

Comment 1

[Lucas Rocha (:lucasr)]

I'll bootstrap the infrastructure to make this happen in DynamicPanel.

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Move JSON parsing logic into PanelConfig (r=margaret)

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Factor out PanelConfig validation to be used in all constructors (r=margaret)

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: Define LayoutType, ViewType, and ViewConfig (r=margaret)

Comment 5

[Lucas Rocha (:lucasr)]

Comment on attachment 8361830 [details] [diff] [review]
Move JSON parsing logic into PanelConfig (r=margaret)

To enable us to generate a PanelConfig from a PanelInfo.

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8361831 [details] [diff] [review]
Factor out PanelConfig validation to be used in all constructors (r=margaret)

We should be very strict about what we consider a valid PanelConfig instance. This enables us to validate PanelConfig instances in all constructors (json, parcel, and properties)

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 8361832 [details] [diff] [review]
Define LayoutType, ViewType, and ViewConfig (r=margaret)

Necessary types for the DynamicPanel stuff. The LayoutType and list of ViewConfigs are appended to the PanelConfig instance.

Comment 8

[:Margaret Leibovic]

Comment on attachment 8361830 [details] [diff] [review]
Move JSON parsing logic into PanelConfig (r=margaret)

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

Good idea! This is a much better home for this logic.

::: mobile/android/base/home/HomeConfig.java
@@ +108,5 @@
> +            mId = json.getString(JSON_KEY_ID);
> +
> +            mFlags = EnumSet.noneOf(Flags.class);
> +
> +            final boolean isDefault = (json.optInt(JSON_KEY_DEFAULT, -1) == IS_DEFAULT);

I know this is just being moved over from existing code, but why is the default value an int we need to compare, rather than just a boolean? This IS_DEFAULT field confused me a bit when reading this.

Comment 9

[:Margaret Leibovic]

Comment on attachment 8361831 [details] [diff] [review]
Factor out PanelConfig validation to be used in all constructors (r=margaret)

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

We should be sure to write some docs somewhere about what it means for the config to be valid, especially if we enhance this method to do more than just make sure none of the fields are null.

Comment 10

[:Margaret Leibovic]

Comment on attachment 8361832 [details] [diff] [review]
Define LayoutType, ViewType, and ViewConfig (r=margaret)

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

So many data types! But necessary. I like that it's so centralized in HomeConfig, maybe at the top of this file would be a good place for some "here are what all these types are for" docs.

::: mobile/android/base/home/HomeConfig.java
@@ +394,5 @@
> +    }
> +
> +    public static class ViewConfig implements Parcelable {
> +        private final ViewType mType;
> +        private final String mDataSetId;

mDatasetId (lowercase s), since we're treating "dataset" as one word?

Comment 11

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #10)
> Comment on attachment 8361832 [details] [diff] [review]
> Define LayoutType, ViewType, and ViewConfig (r=margaret)
> 
> Review of attachment 8361832 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So many data types! But necessary. I like that it's so centralized in
> HomeConfig, maybe at the top of this file would be a good place for some
> "here are what all these types are for" docs.

Yeah, I'll try to include as much documentation as possible in the following patches.

> ::: mobile/android/base/home/HomeConfig.java
> @@ +394,5 @@
> > +    }
> > +
> > +    public static class ViewConfig implements Parcelable {
> > +        private final ViewType mType;
> > +        private final String mDataSetId;
> 
> mDatasetId (lowercase s), since we're treating "dataset" as one word?

Done.

Comment 12

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #9)
> Comment on attachment 8361831 [details] [diff] [review]
> Factor out PanelConfig validation to be used in all constructors (r=margaret)
> 
> Review of attachment 8361831 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We should be sure to write some docs somewhere about what it means for the
> config to be valid, especially if we enhance this method to do more than
> just make sure none of the fields are null.

The intention here is to centralize all validation in a single spot. The exception messages give a pretty good clue of it's expected from PanelConfig and ViewConfig. At least for now, it would feel a bit redundant to write docs in addition to the exceptions.

Comment 13

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #8)
> Comment on attachment 8361830 [details] [diff] [review]
> Move JSON parsing logic into PanelConfig (r=margaret)
> 
> Review of attachment 8361830 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good idea! This is a much better home for this logic.
> 
> ::: mobile/android/base/home/HomeConfig.java
> @@ +108,5 @@
> > +            mId = json.getString(JSON_KEY_ID);
> > +
> > +            mFlags = EnumSet.noneOf(Flags.class);
> > +
> > +            final boolean isDefault = (json.optInt(JSON_KEY_DEFAULT, -1) == IS_DEFAULT);
> 
> I know this is just being moved over from existing code, but why is the
> default value an int we need to compare, rather than just a boolean? This
> IS_DEFAULT field confused me a bit when reading this.

You're right. I incorrectly assumed the JSON API didn't support booleans. I'll do that in a follow-up.

Comment 14

[Lucas Rocha (:lucasr)]

Attachment: Create generic parent class TwoLineRow for TwoRowPageRow (r=margaret)

Comment 15

[Lucas Rocha (:lucasr)]

Comment on attachment 8362587 [details] [diff] [review]
Create generic parent class TwoLineRow for TwoRowPageRow (r=margaret)

Prep work for the panel list view widget. We'll reuse TwoLinePageRow's layout but with a different way of binding it to the cursor. Not entirely happy about the naming here, especially the icon-related names (icon, primary icon and secondary icon). Maybe the view for panel list views should only care about the main icon? Thoughts?

Comment 16

[Lucas Rocha (:lucasr)]

Attachment: Dynamically build the UI of DynamicPanel from a PanelConfig (r=margaret)

Comment 17

[Lucas Rocha (:lucasr)]

Attachment: Add name property to ViewConfig

Comment 18

[Lucas Rocha (:lucasr)]

Attachment: Allow passing a string as title in IconTabWidget

Comment 19

[Lucas Rocha (:lucasr)]

Attachment: Add support for TabsPanelLayout in DynamicPanel

Comment 20

[Lucas Rocha (:lucasr)]

Comment on attachment 8362668 [details] [diff] [review]
Dynamically build the UI of DynamicPanel from a PanelConfig (r=margaret)

Ok, here's the real deal. Hopefully the extensive in-code docs will be enough to understand how the whole thing works :-)

Comment 21

[Lucas Rocha (:lucasr)]

Comment on attachment 8362671 [details] [diff] [review]
Add support for TabsPanelLayout in DynamicPanel

This is not for review. I quickly hacked together a more complex layout with a responsive tabbed interface (a la History tab) to exercise the internal API a bit more. It turns out it should be fairly simple to do some crazy stuff :-)

Screenshot: https://dl.dropboxusercontent.com/u/1187037/custom-panel.png

This would be defined with something like:

Home.panels.add({
    id: "my-unique-panel-id",
    title: "My Panel",
    layout: Home.panels.Layout.TABS,
    views: [
    { type: Home.panels.View.LIST,
      dataset: "my-unique-dataset-id1",
      name: "Page 1" },
    { type: Home.panels.View.LIST,
      dataset: "my-unique-dataset-id2",
      name: "Page 2" }
]
});

The views are dynamically created and bound to their respective datasets. This is just for reference. I'll need to refine the code and design a bit more with ibarlow before landing this.

Comment 22

[Lucas Rocha (:lucasr)]

Landing the first 3 patches as they affect bug 942875:

https://hg.mozilla.org/integration/fx-team/rev/7876d54b3860
https://hg.mozilla.org/integration/fx-team/rev/b386dc0a18fd
https://hg.mozilla.org/integration/fx-team/rev/85d71edd4fd7

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7876d54b3860
https://hg.mozilla.org/mozilla-central/rev/b386dc0a18fd
https://hg.mozilla.org/mozilla-central/rev/85d71edd4fd7

Comment 24

[:Margaret Leibovic]

Comment on attachment 8362587 [details] [diff] [review]
Create generic parent class TwoLineRow for TwoRowPageRow (r=margaret)

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

::: mobile/android/base/home/TwoLineRow.java
@@ +23,5 @@
> +    private final TextView mPrimaryText;
> +    private int mPrimaryIconId;
> +
> +    private final TextView mSecondaryText;
> +    private int mSecondaryIconId;

Yeah, I agree with what you had to say about the naming for the icons being a bit confusing. Primary/secondary makes sense for the text, since they're grouped together, but the icons are located in different places, and you wouldn't know where to expect them just based on their names.

I don't think we should worry about this for landing this patch, but maybe we can think of ways to improve this as we create more TwoLineRow consumers.

@@ +54,5 @@
> +    protected void setSecondaryText(int stringId) {
> +        mSecondaryText.setText(stringId);
> +    }
> +
> +    protected void setPrimaryIcon(int iconId) {

Have you thought about how add-on panels will load their icons? For now I think it's fine to only set icons from resource ids in TwoLineRow, but we may need to find a way to re-purpose the favicon logic from TwoLinePageRow if we want to allow add-ons to specify URLs for icons.

Comment 25

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #24)
> Comment on attachment 8362587 [details] [diff] [review]
> Create generic parent class TwoLineRow for TwoRowPageRow (r=margaret)
> 
> Review of attachment 8362587 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/TwoLineRow.java
> @@ +23,5 @@
> > +    private final TextView mPrimaryText;
> > +    private int mPrimaryIconId;
> > +
> > +    private final TextView mSecondaryText;
> > +    private int mSecondaryIconId;
> 
> Yeah, I agree with what you had to say about the naming for the icons being
> a bit confusing. Primary/secondary makes sense for the text, since they're
> grouped together, but the icons are located in different places, and you
> wouldn't know where to expect them just based on their names.
> 
> I don't think we should worry about this for landing this patch, but maybe
> we can think of ways to improve this as we create more TwoLineRow consumers.
> 
> @@ +54,5 @@
> > +    protected void setSecondaryText(int stringId) {
> > +        mSecondaryText.setText(stringId);
> > +    }
> > +
> > +    protected void setPrimaryIcon(int iconId) {
> 
> Have you thought about how add-on panels will load their icons? For now I
> think it's fine to only set icons from resource ids in TwoLineRow, but we
> may need to find a way to re-purpose the favicon logic from TwoLinePageRow
> if we want to allow add-ons to specify URLs for icons.

We'll need a more general-purpose async image loading approach for the panel views. For instance, we'll have to support things like gallery views (with thumbnails and all) in the near future. So, the Favicon-specific stuff can stay in TwoLinePageRow and we shall implement the more general solution just for the views used in panel views e.g. the PanelListRow view. One option could be using Smoothie or something similar. Anyway, this is kinda out of scope for this bug. I filed bug 963046 to track this discussion.

Comment 26

[:Margaret Leibovic]

Comment on attachment 8362668 [details] [diff] [review]
Dynamically build the UI of DynamicPanel from a PanelConfig (r=margaret)

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

I still want to look through this more (and hopefully have at least one other person look at this), but here are some comments I have so far.

::: mobile/android/base/home/DynamicPanel.java
@@ +223,5 @@
>              final Uri fakeItemsUri = HomeListItems.CONTENT_FAKE_URI.buildUpon().
>                  appendQueryParameter(BrowserContract.PARAM_PROFILE, "default").build();
>  
>              final String selection = HomeListItems.PROVIDER_ID + " = ?";
> +            final String[] selectionArgs = new String[] { mDatasetId };

Nice, this fit in well. As part of the content provider work I'm going to make sure this column is referred to as dataset_id, not provider_id.

::: mobile/android/base/home/PanelLayout.java
@@ +85,5 @@
> +         */
> +        public void resetDataset(String datasetId);
> +    }
> +
> +    public PanelLayout(Context context, PanelConfig panelConfig, DatasetHandler datasetHandler) {

This panelConfig argument is unused.

@@ +189,5 @@
> +        }
> +    }
> +
> +    private void maybeSetDataset(View view, Cursor cursor) {
> +        if (view instanceof DatasetBacked) {

Would we ever end up in here is this wasn't a DatasetBacked view? Where this method is currently used, this view comes from a ViewEntry, which should always have a valid ViewConfig, and a valid ViewConfig always has a datasetId (phew).

If we do always expect to be calling this on a view that implements DatasetBacked, we could just use a DatasetBacked type as an argument, instead of a View. In fact, it doesn't look like we use the view in ViewEntry for anything that requires it to be a View, so maybe that should just hold a DatasetBacked instead of a View?

I suppose in the future we could have views that aren't backed by datasets, but I want to make sure we're not adding extra complexity for a use case that doesn't exist yet.

::: mobile/android/base/home/PanelListRow.java
@@ +5,5 @@
> +
> +package org.mozilla.gecko.home;
> +
> +import android.util.Log;
> +import org.mozilla.gecko.favicons.Favicons;

Some unused imports in here.

::: mobile/android/base/home/PanelListView.java
@@ +16,5 @@
> +import android.view.LayoutInflater;
> +import android.view.View;
> +import android.view.ViewGroup;
> +
> +public class PanelListView extends HomeListView

Should we extend HomeListView here, or should we just extend a regular ListView? I think the main thing HomeListView does is deal with context menus, and there currently lots of assumptions in there about what kind of data is in the lists. If we do go this route, we should file a follow-up to make sure HomeListView is generic enough to deal with these dynamic lists.

We should also follow bugs about how we want to deal with both regular and long clicks on the items in these lists. So many things to do!

::: mobile/android/base/resources/layout/panel_list_row.xml
@@ +6,5 @@
> +<org.mozilla.gecko.home.PanelListRow xmlns:android="http://schemas.android.com/apk/res/android"
> +                                     android:layout_width="fill_parent"
> +                                     android:layout_height="@dimen/page_row_height"
> +                                     android:paddingLeft="10dip"
> +                                     android:minHeight="@dimen/page_row_height"/>

It could be good to file a follow-up to rename this dimen value to be more generic.

Comment 27

[:Margaret Leibovic]

Comment on attachment 8362668 [details] [diff] [review]
Dynamically build the UI of DynamicPanel from a PanelConfig (r=margaret)

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

After spending some more time looking through this, I feel fine with us landing it, and we can iterate on it in the tree.

While reviewing this, I sketched out a diagram of all the classes/types for reference, and I think it would be nice to make a more polished version of that to share. Maybe we can work on that together next week :)

Comment 28

[:Margaret Leibovic]

Comment on attachment 8362671 [details] [diff] [review]
Add support for TabsPanelLayout in DynamicPanel

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

This is a really cool proof of concept. I'm excited to see where we get with things next week!

Comment 29

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/53f58523637d
https://hg.mozilla.org/integration/fx-team/rev/2f7bda3e92f7

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/53f58523637d
https://hg.mozilla.org/mozilla-central/rev/2f7bda3e92f7