965622 - Handle the case where there are no items for a given dataset id

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

Description

[:Margaret Leibovic]

When we load dynamic panels, we do queries to fetch the items associated with the dataset for each view.

However, it can be the case that there are no items for the specified dataset (e.g. the add-on didn't store any data yet).

We should do something to handle this. For the simplest case, we can just show some sort of generic "no data" message, but in the future, it would be nice to allow the add-on to specify what to say. This would also be a good place to put some UI for launching into some authentication UI (e.g. "Sign into your ___ account to see stuff!").

Comment 1

[:Margaret Leibovic]

Ian, can you help me come up with some default text and icon to show when a dynamic panel view doesn't have any content? I want to add an API to let an add-on override that text/icon, but I think it would be good to have a default.

Comment 2

[:Margaret Leibovic]

Attachment: WIP

This is a pretty rough WIP built on top of lucasr's panel-hack patch, but it lets us start to explore what this API should look like.

We should probably make a proper type for the empty view options, rather than passing around and storing a JSONObject. We also need to move this empty view logic out to somewhere that can be shared between all the different DatasetBacked dynamic panel views (e.g. grid views).

Comment 3

[:Margaret Leibovic]

ibarlow, gentle poke. Do you have thoughts about what kind of UI we should let add-ons show when there's no data for a view? I'm thinking we'll build an API that lets them specify an image and some text.

Note: this is a separate issue from the auth UI. Let's cover that in bug 942281. I think that if an add-on requires auth, we'll check that before we check if there's empty data or not, so a user should always be authenticated by the time they end up seeing one of these empty views.

Comment 4

[Lucas Rocha (:lucasr)]

IMO, we should use the same UI template than the built-in panels (big icon + text blob). In terms of code, we can simply use home_empty_panel.xml in panel views.

The text and image would be defined by the add-on via view options. Something like:

{ type: Home.panels.View.LIST,
  dataset: "mydataset",
  emptyState: { image: "SOME URL",
                text: "SOME TEXT" }
}

Or a slight variation of this:

{ type: Home.panels.View.LIST,
  dataset: "mydataset",
  emptyImage: "SOME URL",
  emptyText: "SOME TEXT"
}

Comment 5

[Ian Barlow (:ibarlow)]

Sure -- here's what I was thinking for the empty panel states. I've included both the "unauthenticated" and "authenticated but empty" versions here. 

They are based on our existing empty pages. My thinking is that for add-ons that want it, they can add a little tip like what we do in our Reading list panel. 

http://cl.ly/image/282D0R39321u

Comment 6

[:Margaret Leibovic]

Attachment: (WIP) Create empty views for dynamic panel views

This patch tweaks PanelListView and PanelGridView to make them in charge of their own empty views. Looking at how similar the logic is for these two, I wonder if instead I should just move this out to somewhere in PanelLayout.

The JS API looks like this:

  function optionsCallback() {
    return {
      title: "List Demo",
      layout: Home.panels.Layout.FRAME,
      views: [{
        type: Home.panels.View.LIST,
        dataset: DATASET_ID,
        emptyState: {
          text: "Larry says there's no data here",
          imageUrl: "drawable://larry"
        }
      }]
    };
  }

  Home.panels.register(PANEL_ID, optionsCallback);

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 8382424 [details] [diff] [review]
(WIP) Create empty views for dynamic panel views

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

As I suggested before, we should experiment with having PanelLayout manage empty views for panel views. So, instead of having an onDatasetEmpty() call when the cursor is empty, you'd actually replace the view with an EmptyStateLayout using the same LayoutParams from the original view. There are at least two nice benefits with this approach: first, you'd avoid adding an extra container to the view tree (win); second, we wouldn't need to implement empty state for each panel view (win). The main drawback is the extra complexity in PanelLayout to manage panel and empty views on the fly. I think we should at least try this approach and see how it would look as it seems to bring some nice benefits.

::: mobile/android/base/home/HomeConfig.java
@@ +775,5 @@
> +        }
> +
> +        public EmptyViewConfig(String text, String imageUrl) {
> +            mText = text;
> +            mImageUrl = imageUrl;

EmptyViewConfig instances should be validated just like PanelConfig and ViewConfig. If the add-on dev doesn't provide any emptyState, we'll simply fallback to default text and image. However, if the dev provides an empty state, it has have at least a text or an image.

::: mobile/android/base/home/PanelGridView.java
@@ +42,5 @@
>  
>      public PanelGridView(Context context, ViewConfig viewConfig) {
> +        super(context);
> +
> +        LayoutInflater.from(context).inflate(R.layout.home_empty_view_stub, this);

This is not necessary. You don't need to stub something you'll only inflate on demand anyway. Just inflate home_empty_view when the empty view is needed.

@@ +77,5 @@
> +        mEmptyView = emptyViewStub.inflate();
> +
> +        final EmptyViewConfig emptyViewConfig = mViewConfig.getEmptyViewConfig();
> +
> +        final String text = (emptyViewConfig == null) ? null : emptyViewConfig.getText();

Factor out this code into a separate PanelEmptyLayout (similar to what you did with PanelAuthLayout).

@@ +87,5 @@
> +        }
> +
> +        final String imageUrl = (emptyViewConfig == null) ? null : emptyViewConfig.getImageUrl();
> +        final ImageView emptyImage = (ImageView) findViewById(R.id.home_empty_image);
> +        BitmapUtils.getDrawable(getContext(), imageUrl, new BitmapUtils.BitmapLoader() {

Use Picasso.

::: mobile/android/base/home/PanelLayout.java
@@ +73,5 @@
>       * backed by datasets.
>       */
>      public interface DatasetBacked {
>          public void setDataset(Cursor cursor);
> +        public void onDatasetEmpty();

I'd prefer something like showEmptyState().

::: mobile/android/base/home/PanelListView.java
@@ +43,5 @@
>  
>      public PanelListView(Context context, ViewConfig viewConfig) {
>          super(context);
>  
> +        LayoutInflater.from(context).inflate(R.layout.home_empty_view_stub, this);

Same here. See comment on PanelGridView.

@@ +83,5 @@
> +        }
> +
> +        final String imageUrl = (emptyViewConfig == null) ? null : emptyViewConfig.getImageUrl();
> +        final ImageView emptyImage = (ImageView) findViewById(R.id.home_empty_image);
> +        BitmapUtils.getDrawable(getContext(), imageUrl, new BitmapUtils.BitmapLoader() {

Use Picasso instead.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +321,5 @@
>  <!-- Localization note (home_reading_list_hint_accessible): This string is used
>       as alternate text for accessibility. It is not visible in the UI. -->
>  <!ENTITY home_reading_list_hint_accessible "TIP: Save articles to your reading list by long pressing the reader mode button when it appears in the title bar.">
>  
> +<!ENTITY home_default_empty "Oops, there\'s no data to show here.">

Double-check this string with ibarlow.

Comment 8

[Chenxia Liu [:liuche]]

Comment on attachment 8382424 [details] [diff] [review]
(WIP) Create empty views for dynamic panel views

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

Good start - as for your question about where to put the empty view code, maybe lift things out into an abstract PanelView?

::: mobile/android/base/home/HomeConfig.java
@@ +755,5 @@
> +        private final String mText;
> +        private final String mImageUrl;
> +
> +        private static final String JSON_KEY_TEXT = "text";
> +        private static final String JSON_KEY_IMAGE_URL = "imageUrl";

Are we supporting add-ons including images, well as providing a url? (Is there a bug about image/icon fetching/storage for home panels?)

::: mobile/android/base/home/PanelListView.java
@@ +43,5 @@
>  
>      public PanelListView(Context context, ViewConfig viewConfig) {
>          super(context);
>  
> +        LayoutInflater.from(context).inflate(R.layout.home_empty_view_stub, this);

I'm a little confused - why is this a ViewStub? Those should be used for lazy inflation, but inflating it in the constructor just creates it every time, right? Could you just use home_empty_panel here?

Comment 9

[:Margaret Leibovic]

Attachment: (Part 1) Add EmptyViewConfig to HomeConfig

Splitting up this patch and spreading out the review love :)

Comment 10

[:Margaret Leibovic]

Attachment: (Part 2 - WIP) Manage empty views in PanelLayout

Here's a functional proof of concept, what do you two think?

A few things we need to think about:

* Right now this works because FramePanelLayout is the only type of panel layout we support, but if we want to support a different type of layout with multiple views, we'll have to be smarter about where we put this view.

* This patch only shows the empty view, we'll need some logic to hide it once the view has data.

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 8385773 [details] [diff] [review]
(Part 2 - WIP) Manage empty views in PanelLayout

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

Yeah, that's roughly what I had in mind. A few extra notes:

- The PanelLayout implementation (FramePanelLayout only for now) is probably where the logic for *placing* the empty view should be. So, I guess PanelLayout could have an abstract method like setPanelViewAsEmpty(View, EmptyView). You'll also need a method that does the opposite. 
- We should avoid adding emptyview-specific logic (text, image, etc) directly in PanelLayout. So, turn home_empty_panel.xml into a custom view with some APIs to update it (setMessage, setImageResource, setImageFromUrl). Port current users of home_empty_panel.xml to use it. This could be called EmptyPanelView.
- Use EmptyPanelView in PanelLayout. 
- Maybe the emptyView instance should be stored in the associated ViewDetail instance? As opposed to the extra map you introduced in this patch.

::: mobile/android/base/home/PanelLayout.java
@@ +292,5 @@
> +            final String imageUrl = (emptyViewConfig == null) ? null : emptyViewConfig.getImageUrl();
> +            final ImageView imageView = (ImageView) findViewById(R.id.home_empty_image);
> +
> +            if (TextUtils.isEmpty(imageUrl)) {
> +                imageView.setImageResource(R.drawable.icon_home_empty_firefox);

Just use Picasso's placeholder support instead.

Comment 12

[:Margaret Leibovic]

I just realized we should try to land something here before the merge next week if we want to land strings for default text for the empty view.

ibarlow, can you help me come up with a string for this text? This is the case where there's no data in the dataset, but the user may or may not be authenticated.

Comment 13

[Chenxia Liu [:liuche]]

Comment on attachment 8385767 [details] [diff] [review]
(Part 1) Add EmptyViewConfig to HomeConfig

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

This looks good! At some point, we should actually use this part of HomeConfig for our built-in panels, so file a follow-up for that.

::: mobile/android/base/home/HomeConfig.java
@@ +754,5 @@
> +            mText = in.readString();
> +            mImageUrl = in.readString();
> +        }
> +
> +        public EmptyViewConfig(EmptyViewConfig emptyViewConfig) {

I guess this is technically unnecessary because none of the fields of EmptyViewConfig are references or mutable - this might protect us from forgetting if we add anything that is a reference though.

@@ +768,5 @@
> +        public String getText() {
> +            return mText;
> +        }
> +
> +        public String getImageUrl() {

I'm curious, what should we do about local resources, e.g. for built-in panels? Maybe url isn't the right name for this variable, but it's ok for this, I think.

Comment 14

[Ian Barlow (:ibarlow)]

Hey Margaret, how about this: http://cl.ly/image/1E2i280Y2q27

String: "No content could be found for this panel."

Comment 15

[:Margaret Leibovic]

(In reply to Chenxia Liu [:liuche] from comment #13)

> @@ +768,5 @@
> > +        public String getText() {
> > +            return mText;
> > +        }
> > +
> > +        public String getImageUrl() {
> 
> I'm curious, what should we do about local resources, e.g. for built-in
> panels? Maybe url isn't the right name for this variable, but it's ok for
> this, I think.

Hm, we probably should think about this before we land it, since it's part of a public API. Perhaps instead of "imageUrl" we could just use "icon", and this should be a path to an image, whether that's a URL or a local resource.

Comment 16

[:Margaret Leibovic]

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

> - We should avoid adding emptyview-specific logic (text, image, etc)
> directly in PanelLayout. So, turn home_empty_panel.xml into a custom view
> with some APIs to update it (setMessage, setImageResource, setImageFromUrl).
> Port current users of home_empty_panel.xml to use it. This could be called
> EmptyPanelView.

I started working on this, but it feels out of scope for this bug, since this will actually change logic in all the built-in panels. I'm a bit concerned about finishing this in time to land something before the merge, so perhaps we should move this bit into a follow-up, so that we can land the basic functionality with a string first.

Comment 17

[:Margaret Leibovic]

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

> - Maybe the emptyView instance should be stored in the associated ViewDetail
> instance? As opposed to the extra map you introduced in this patch.

How do I do this? I'm not familiar with this concept, and Google is failing to help me.

Comment 18

[:Margaret Leibovic]

Attachment: (Part 2) Manage empty views in PanelLayout

I know I'm being lazy here and this doesn't address all your feedback, but I would prefer to land something we know works now, then do the refactor work in a follow-up bug.

Also, as a follow-up we'll need to add some listener to hide the empty view if data becomes available, but that would actually depend on us fixing bug 972098.

Comment 19

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #17)
> (In reply to Lucas Rocha (:lucasr) from comment #11)
> 
> > - Maybe the emptyView instance should be stored in the associated ViewDetail
> > instance? As opposed to the extra map you introduced in this patch.
> 
> How do I do this? I'm not familiar with this concept, and Google is failing
> to help me.

Sorry, I actually meant ViewState, the class that holds the state of active panel views in PanelLayout.

Comment 20

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #18)
> Also, as a follow-up we'll need to add some listener to hide the empty view
> if data becomes available, but that would actually depend on us fixing bug
> 972098.

I've just posted patches for bug 972098. Independently of the approach we take there, a change in the dataset from JS should trigger the same code path for dataset loading (loader restart -> layout.deliverdataset). So, you'll just need to hide or show the empty state when the dataset cursor is delivered.

Comment 21

[Lucas Rocha (:lucasr)]

Comment on attachment 8390199 [details] [diff] [review]
(Part 2) Manage empty views in PanelLayout

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

This is a good direction but it's too fragile in terms of layout assumptions. What we want here is to replace the panel view with the empty and vice-versa depending the contents of the delivered Cursor. Here's what I suggest:

Get rid of the showEmptyView()/hideEmptyView() abstract methods. I think there's a reliable way of replacing a view in the layout with needing any custom code for each PanelLayout subclass. Something like:

// inflate empty view
View emptyView = ...

ViewGroup parent = (ViewGroup) panelView.getParent();
parent.addView(emptyView, parent.indexOfChild(panelView), panelView.getLayoutParams());
parent.removeView(panelView);

And vice-versa, to replace the empty view with the panel view. Put this code into a replacePanelView(View current, View new). I haven't tested this code but it should work.

More suggestions below below.

::: mobile/android/base/home/PanelLayout.java
@@ +266,5 @@
>              dsb.setDataset(cursor);
> +
> +            if (cursor == null || cursor.getCount() == 0) {
> +                showEmptyView(view, getEmptyView(view));
> +            }

You have to hide the empty view if the dataset is not empty (so that we properly update the UI if the dataset gets some content later). This should be something along the lines of:

if (cursor == null || cursor.getCount() == 0) {
    replacePanelView(view, getEmptyView());
} else {
    replacePanelView(mEmptyViewMap.get(view), view);
}

You'll handle the case where the panel view instance gets garbage collected before the dataset gets its content - in which case you'll have to recreate it using createPanelView().

@@ +278,5 @@
> +        if (mEmptyViewMap.containsKey(view)) {
> +            return mEmptyViewMap.get(view);
> +        }
> +
> +        final View emptyView = LayoutInflater.from(getContext()).inflate(R.layout.home_empty_panel, this);

You should not assume you can add the view directly to the layout. Also, you should ensure that the empty takes the exact same spot in the UI than the matching panel view.

@@ +294,5 @@
> +        final ImageView imageView = (ImageView) findViewById(R.id.home_empty_image);
> +        Picasso.with(getContext())
> +               .load(imageUrl)
> +               .placeholder(R.drawable.icon_home_empty_firefox)
> +               .into(imageView);

I still think this should be factored out into a separate custom view (EmptyPanelView or something). Please file a follow-up.

Comment 22

[:Margaret Leibovic]

Attachment: (Part 0) String for dynanmic panel empty view text

I guess I just need a rubber stamp here. You can also review my localization note :)

Comment 23

[:Margaret Leibovic]

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

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0e2a2db3af5c

Comment 25

[:Margaret Leibovic]

Attachment: (Part 2 v2) Manage empty views in PanelLayout

I filed bug 985134 about refactoring the empty view layout logic.

Comment 26

[:Margaret Leibovic]

Attachment: (Part 1) Add EmptyViewConfig to HomeConfig

Rebased.

Comment 27

[:Margaret Leibovic]

Attachment: (Part 2 v2) Manage empty views in PanelLayout

Rebased on top of latest m-c.

Comment 28

[Lucas Rocha (:lucasr)]

Comment on attachment 8393223 [details] [diff] [review]
(Part 2 v2) Manage empty views in PanelLayout

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

This patch has fundamental problem that it assumes the View key (in mViewStateMap) will be around indefinitely while showing empty state. However, mViewStateMap is a WeakHashMap, which means once you replace the view with the empty state, there will be no references to the original View instance anymore and it will eventually get garbage collected (at which point you will have no way to the replace the empty state with the view anymore).

We'll need to refactor PanelLayout to use a view id or index as the key in mViewStateMap instead of the View instance itself so that we can keep a soft reference to the empty view and panel view in the associated ViewState instance. This way we can actually re-create the views if they get gargage collected. I'm planning to implement these PanelLayout changes as part of bug 976064.

::: mobile/android/base/home/PanelLayout.java
@@ +409,5 @@
>          private final ViewConfig mViewConfig;
>          private LinkedList<FilterDetail> mFilterStack;
>  
> +        // The emtpy view associated with this view.
> +        private View mEmptyView;

Use a Soft

Comment 29

[:Margaret Leibovic]

Attachment: (Part 1) Add EmptyViewConfig to HomeConfig

Unbitrotting (again).

Comment 30

[:Margaret Leibovic]

Attachment: (Part 1.5) Update maybeSetDataset to take a ViewConfig instead of a View

A bit of refactoring to make my updated Part 2 patch easier to write.

Comment 31

[Lucas Rocha (:lucasr)]

Comment on attachment 8404763 [details] [diff] [review]
(Part 1.5) Update maybeSetDataset to take a ViewConfig instead of a View

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

Nice. You probably meant ViewState instead of ViewConfig though :-)

Comment 32

[:Margaret Leibovic]

Attachment: (Part 2 v3) Manage empty views in PanelLayout

This new version keeps a soft reference to the empty view in the ViewState.

I found an issue that on rotation we call maybeSetDataset for a view that's no longer attached to the window, so I added a null check to replacePanelView to address this. However, I want to double check with you that this situation is expected.

Using the remote console, I tested saving/deleting datasets while a test panel was visible, and I saw the empty view hide/show as expected.

Comment 33

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #31)
> Comment on attachment 8404763 [details] [diff] [review]
> (Part 1.5) Update maybeSetDataset to take a ViewConfig instead of a View
> 
> Review of attachment 8404763 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice. You probably meant ViewState instead of ViewConfig though :-)

Doh, yeah. /me updates commit message.

Comment 34

[Lucas Rocha (:lucasr)]

Comment on attachment 8404767 [details] [diff] [review]
(Part 2 v3) Manage empty views in PanelLayout

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

Looks like the right direction but still needs work.

::: mobile/android/base/home/PanelLayout.java
@@ +378,1 @@
>          final View view = viewState.getView();

The empty view handling is a bit more complex than it looks. The original panel view might get garbage collected while you're showing the empty view. The reason why I changed mViewStates to not depend on the View instance was to allow us to handle this case properly.

Whenever a dataset gets delivered, there's always an active view (either the panel view or the empty view). By active, I mean the view has a parent in the view hierarchy. The soft reference to the active view (in ViewState) will never be garbage collected because the view tree has references to it. But that's not the case for the inactive view.

So, whenever we switch from one state to another (empty to not empty, and vice-versa), we have to ensure that the new active view (which was previously inactive) is still around, and recreate it if it's not.

This means that the null check you're doing here might cause the empty view to stick around, for example.

When a dataset gets delivered to the layout, you'll have to:
1. Figure out which view currently active, panel or empty (you can just check getParent() != null or something). Maybe add a getActiveView() method to ViewState to make it easier to follow.
2. Figure out which view will be active, panel or empty.
3. Ensure the newly active view is around, create it if not.
4. If the panel becomes inactive (you're about to show empty state), make sure you call setDataset(null) on it so that it doesn't hold any dangling Cursor references when if/when it gets garbage collected.
5. If the new active view is different than the current active view, call replacePanelView().

I made createPanelView() idempotent with this case in mind. You should probably do the same for createEmptyView(). This way you ensure the view is around before replacing views.

The code will look roughly like this (assuming both createPanelView() and createEmptyView() are idempotent):

final View activeView = viewState.getActiveView();

final View newView;
if (cursor == null || cursor.getCount() == 0) {
    newView = createEmptyView(viewConfig);     
    activeView.setDataset(null);
} else {
    newView = createPanelView(viewConfig);
    newView.setDataset(cursor);
}

if (newView != activeView) {
    replacePanelView(activeView, newView);
}

Comment 35

[:Margaret Leibovic]

Comment on attachment 8404763 [details] [diff] [review]
(Part 1.5) Update maybeSetDataset to take a ViewConfig instead of a View

I determined that I don't actually need this patch anymore.

Comment 36

[:Margaret Leibovic]

Attachment: (Part 2 v4) Manage empty views in PanelLayout

Updated to address latest comments.

Comment 37

[:Margaret Leibovic]

Attachment: (Part 2 v4) Manage empty views in PanelLayout

Oops, forgot to call viewState.setEmptyView(). Fixed now!

Comment 38

[Lucas Rocha (:lucasr)]

Comment on attachment 8406172 [details] [diff] [review]
(Part 2 v4) Manage empty views in PanelLayout

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

Looks good with the suggested changes.

::: mobile/android/base/home/PanelLayout.java
@@ +273,5 @@
>          }
>  
> +        final View activeView = viewState.getActiveView();
> +        if (activeView == null) {
> +            return;

We should probably throw an IllegalStateException here instead of silently bailing. Having no active view is definitely a bug.

@@ +276,5 @@
> +        if (activeView == null) {
> +            return;
> +        }
> +
> +        final ViewConfig viewConfig = viewState.getViewConfig();

nit: add empty line here.

@@ +278,5 @@
> +        }
> +
> +        final ViewConfig viewConfig = viewState.getViewConfig();
> +        final View newView;
> +

nit: remove empty line here.

@@ +405,5 @@
>              dsb.setDataset(cursor);
>          }
>      }
>  
> +    // TODO: Refactor this into a custom view (bug 985134)

You've already added this comment below. Remove this one.

@@ +412,5 @@
> +
> +        ViewState viewState = mViewStates.get(viewConfig.getIndex());
> +        if (viewState == null) {
> +            viewState = new ViewState(viewConfig);
> +            mViewStates.put(viewConfig.getIndex(), viewState);

createEmptyView shouldn't really trigger a ViewState state to be registered as it's only supposed to created for panel views that are already active. Simply throw an IllegalStateException if viewState is null here.

@@ +434,5 @@
> +            final String imageUrl = (emptyViewConfig == null) ? null : emptyViewConfig.getImageUrl();
> +            final ImageView imageView = (ImageView) view.findViewById(R.id.home_empty_image);
> +            Picasso.with(getContext())
> +                   .load(imageUrl)
> +                   .placeholder(R.drawable.icon_home_empty_firefox)

I think we want to use error() instead of placeholder here i.e. don't show anything until the image is actually loaded and fallback to the default image if Picasso fails to load the image for some reason.

@@ +500,5 @@
> +        public View getActiveView() {
> +            final View view = getView();
> +            if (view != null && view.getParent() != null) {
> +                return view;
> +            }

nit: add empty line here.

@@ +504,5 @@
> +            }
> +            final View emptyView = getEmptyView();
> +            if (emptyView != null && emptyView.getParent() != null) {
> +                return emptyView;
> +            }

nit: add empty line here.

Comment 39

[:Margaret Leibovic]

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

Comment 40

[:Margaret Leibovic]

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

> @@ +434,5 @@
> > +            final String imageUrl = (emptyViewConfig == null) ? null : emptyViewConfig.getImageUrl();
> > +            final ImageView imageView = (ImageView) view.findViewById(R.id.home_empty_image);
> > +            Picasso.with(getContext())
> > +                   .load(imageUrl)
> > +                   .placeholder(R.drawable.icon_home_empty_firefox)
> 
> I think we want to use error() instead of placeholder here i.e. don't show
> anything until the image is actually loaded and fallback to the default
> image if Picasso fails to load the image for some reason.

Sounds good. As a follow-up, I think we should set a height/width on the image so that the text below it doesn't change position when the image loads. It would also be good to have a recommended image size for add-on developers to use (similar to what I mentioned in bug 988930).

Comment 41

[:Margaret Leibovic]

Documented here: https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/Home.jsm/panels

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aa6dbd116736
https://hg.mozilla.org/mozilla-central/rev/b17e8a8c381f

Comment 43

[:Margaret Leibovic]

Attachment: patch for aurora

Note to release managers: I'm requesting uplift for a series of Firefox Hub bugs. The main fixes we need are in bugs at the end of the series, but trying to rebase those patches proved difficult and risky, so I think we should just uplift the dependecies.

Note to sheriffs: I have a local patch series that I can land on aurora when these bugs all get approval.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): dependency for initial Firefox Hub release (promoted feed add-ons in fx30)
User impact if declined: panels won't update when data changes
Testing completed (on m-c, etc.): baked on m-c
Risk to taking this patch (and alternatives if risky): only affects dynamic panels, need an add-on to trigger this
String or IDL/UUID changes made by this patch: none

Comment 44

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9dbdb733f3ad

Comment 45

[:Margaret Leibovic]

Setting P1 hub bugs to block hub v1 EPIC bug (targeting fx30 release).

Filter on epic-hub-bugs.