Closed Bug 1000616 Opened 6 years ago Closed 6 years ago

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

Categories

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

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 31
Tracking Status
firefox30 --- fixed
firefox31 --- fixed
fennec 30+ ---

People

(Reporter: Margaret, Assigned: Margaret)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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.
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?
Flags: needinfo?(lucasr.at.mozilla)
This is a quick fix to address the first issue I mentioned in my last comment.
Attachment #8411458 - Flags: review?(lucasr.at.mozilla)
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().
Attachment #8411458 - Flags: review?(lucasr.at.mozilla) → review-
(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.
Flags: needinfo?(lucasr.at.mozilla)
(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.
tracking-fennec: ? → 30+
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 :)
Attachment #8411458 - Attachment is obsolete: true
Attachment #8412026 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #8412026 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8412029 [details] [diff] [review]
(Part 2) Reset OnItemOpenListener when PanelViews are reattached

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

Nice.
Attachment #8412029 - Flags: review?(lucasr.at.mozilla) → review+
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
Attachment #8412029 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/548827cce8d7
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Attachment #8412029 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Setting P1 hub bugs to block hub v1 EPIC bug (targeting fx30 release).

Filter on epic-hub-bugs.
Blocks: 1014025
You need to log in before you can comment on or make changes to this bug.