1000616 - PanelViewItemHandler.mItemOpenListener is null when panel list view replaces empty view

Status: RESOLVED FIXED

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

Description

[:Margaret Leibovic]

STR:
1) Install this add-on: http://people.mozilla.org/~mleibovic/worldcupfeed.xpi
2) Choose a feed
3) Try tapping on an item in the list

Some debugging led me to find that mItemOpenListener is null here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/PanelViewItemHandler.java#60

We should be setting this item open listener in createPanelView here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/PanelLayout.java#370

This looks like some bug related to doing things on the wrong views.

Comment 1

[:Margaret Leibovic]

What's happening is that the panel view isn't actually being lazily created, and our listener is being set to null when the view is detached from the window:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/PanelListView.java#49

An easy fix would be to just get rid of this call to set the listener to null. The listener itself is just a new object we create on the fly in PanelLayout, not a reference to an object that hangs around forever, so I don't think we would have memory problems if we don't free the reference to the listener.

However, we should also figure out why this view isn't being lazily created. In order for this to happen, this cursor must somehow have items:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/PanelLayout.java#283

Lucas, do you know how that could happen?

Comment 2

[:Margaret Leibovic]

Attachment: Don't clear OnItemOpenListener in PanelViews

This is a quick fix to address the first issue I mentioned in my last comment.

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 8411458 [details] [diff] [review]
Don't clear OnItemOpenListener in PanelViews

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

Hmmm, IIRC, we added this code because of some crash caused by an item click being handled after the fragment holding the views had been destroyed (or something along these lines). I'd prefer to simply set the item handler again in onAttachedToWindow().

Comment 4

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #1)
> However, we should also figure out why this view isn't being lazily created.
> In order for this to happen, this cursor must somehow have items:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/
> PanelLayout.java#283

We unconditionally create the panel view When the dynamic panel is first loaded, see:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/FramePanelLayout.java#32

We can avoid creating the panel view when the panel layout is first created but you'll have to change the "there's always an active view" invariant in PanelLayout:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/PanelLayout.java#277

Changing this might have other side effects. I wouldn't do this change for Fx31. Maybe do it in a follow-up to land in Fx32? Up to you.

Comment 5

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #4)
> (In reply to :Margaret Leibovic from comment #1)
> > However, we should also figure out why this view isn't being lazily created.
> > In order for this to happen, this cursor must somehow have items:
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/
> > PanelLayout.java#283
> 
> We unconditionally create the panel view When the dynamic panel is first
> loaded, see:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/
> FramePanelLayout.java#32

Ah, okay, I forgot about that call and was only looking at the one in PanelLayout.

> We can avoid creating the panel view when the panel layout is first created
> but you'll have to change the "there's always an active view" invariant in
> PanelLayout:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/
> PanelLayout.java#277
> 
> Changing this might have other side effects. I wouldn't do this change for
> Fx31. Maybe do it in a follow-up to land in Fx32? Up to you.

I don't think this is a high priority, so a follow-up to land in 32 sounds like a good idea.

Comment 6

[:Margaret Leibovic]

Attachment: (Part 1) Make PanelListView and PanelGridView variable naming and implementation consistent

Some cleanup to start. This actually makes me wonder if PanelView should be an abstract class rather than an interface, but we can think more about that later :)

Comment 7

[:Margaret Leibovic]

Attachment: (Part 2) Reset OnItemOpenListener when PanelViews are reattached

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 8412026 [details] [diff] [review]
(Part 1) Make PanelListView and PanelGridView variable naming and implementation consistent

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

(In reply to :Margaret Leibovic from comment #6)
> Created attachment 8412026 [details] [diff] [review]
> (Part 1) Make PanelListView and PanelGridView variable naming and
> implementation consistent
> 
> Some cleanup to start. This actually makes me wonder if PanelView should be
> an abstract class rather than an interface, but we can think more about that
> later :)

For the record, the reason for not using an abstract class: we'd have to force all panel views to be of the same View type, which is not what we have/need here. We can definitely reduce the code duplication encapsulating the common code into a separate components that implements the common code between list and grid views i.e. Reuse code with composition, not inheritance. PanelListView and PanelGridView would just use something like a PanelAbsListViewImpl.

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 8412029 [details] [diff] [review]
(Part 2) Reset OnItemOpenListener when PanelViews are reattached

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

Nice.

Comment 10

[:Margaret Leibovic]

Landed a folded patch:
https://hg.mozilla.org/integration/fx-team/rev/548827cce8d7

Comment 11

[:Margaret Leibovic]

Comment on attachment 8412029 [details] [diff] [review]
(Part 2) Reset OnItemOpenListener when PanelViews are reattached

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new hub APIs to ship in Fx30
User impact if declined: clicks on panel items won't work sometimes
Testing completed (on m-c, etc.): just landed on fx-team, tested locally
Risk to taking this patch (and alternatives if risky): low-risk, only affects add-ons
String or IDL/UUID changes made by this patch: none

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/548827cce8d7

Comment 13

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e67471d96cb6

Comment 14

[:Margaret Leibovic]

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

Filter on epic-hub-bugs.