1000849 - Panel view sometimes does replace empty view after dataset updates

Status: VERIFIED FIXED

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

Description

[Teodora Vermesan (:TeoVermesan)]

Steps to reproduce:
1. Install "Goal.com Feed"
2. From the pop-up "Welcome to your Goal.com panel!" choose which edition to follow
3. Go to Tools -> Add-ons
4. Disable it
5. Re-enable it

Actual result:
- The user is redirected to the "GOAL.COM" panel and the pop-up shows up again. After choosing the feed edition, the panel is blank. 
Only after closing all tabs and going to that panel, it will be populated with feeds.
Expected result:
- The add-on is re-enabled and all functionalities are restored.

Comment 1

[Aaron Train [:aaronmt]]

Step 0 is referring to using: http://people.mozilla.org/~mleibovic/worldcupfeed.xpi (https://github.com/leibovic/world-cup-feed)

I wasn't able to reproduce this, but this would be solved via bug 999483. Is there anything in console after you pick a language and then see the empty pane?

Comment 2

[Aaron Train [:aaronmt]]

I just tried to reinstall the add-on and selected English and got an empty pane too actually.

D/GeckoDynamicPanel(10993): Loading layout
D/GeckoPanelLayout(10993): Creating panel view: list
D/GeckoDynamicPanel(10993): Created layout of type: frame
D/GeckoFramePanelLayout(10993): Loading
D/GeckoFramePanelLayout(10993): Requesting child request: { index: 0, type: DATASET_LOAD, dataset: [e-mail], filter: org.mozilla.gecko.home.PanelLayout$FilterDetail@42d2bd40 }
D/GeckoPanelLayout(10993): Requesting request: { index: 0, type: DATASET_LOAD, dataset: [e-mail], filter: org.mozilla.gecko.home.PanelLayout$FilterDetail@42d2bd40 }
D/GeckoDynamicPanel(10993): Requesting request: { index: 0, type: DATASET_LOAD, dataset: [e-mail], filter: org.mozilla.gecko.home.PanelLayout$FilterDetail@42d2bd40 }
D/GeckoDynamicPanel(10993): Creating loader for request: { index: 0, type: DATASET_LOAD, dataset: [e-mail], filter: org.mozilla.gecko.home.PanelLayout$FilterDetail@42d2bd40 }
D/GeckoHomeProvider(10993): No profile provided, using 'default'
D/GeckoDynamicPanel(10993): Finished loader for request: { index: 0, type: DATASET_LOAD, dataset: [e-mail], filter: org.mozilla.gecko.home.PanelLayout$FilterDetail@42d2bd40 }
D/GeckoPanelLayout(10993): Delivering request: { index: 0, type: DATASET_LOAD, dataset: [e-mail], filter: org.mozilla.gecko.home.PanelLayout$FilterDetail@42d2bd40 }
D/GeckoPanelLayout(10993): Creating empty view: list

Comment 3

[:Margaret Leibovic]

This sounds like a bug related to empty views.

Comment 4

[:Margaret Leibovic]

This is caused by the null cursor issue that I "fixed" in bug 995420. If the cursor is null, we don't register to get notifications, and therefore the view never hears that there's new data to show.

I think the proper fix here is to make sure SQLiteBridgeContentProvider always returns a cursor, or make HomeProvider.query always return an empty cursor, even if the bridge returns null (that might be safer, since other SQLiteBridgeContentProvider consumers might be expecting null).

Comment 5

[:Margaret Leibovic]

Attachment: Make a dummy MatrixCursor to listen for dataset changed notifications

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8412127 [details] [diff] [review]
Make a dummy MatrixCursor to listen for dataset changed notifications

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

::: mobile/android/base/db/HomeProvider.java
@@ +99,5 @@
> +
> +        // SQLiteBridgeContentProvider may return a null Cursor if the database hasn't been created yet.
> +        // However, we need a non-null cursor in order to listen for notifications.
> +        if (c == null) {
> +            c = new MatrixCursor(ITEMS_COLUMNS);

Strictly speaking, the columns in the resulting cursor should be defined by the given projection in the query() call. Even though this code will work for our current usage (i.e. projection is always null, select all columns), I think we can make this code a bit more future-proof.

Here's what I suggest:
1. Define a HomeItems.DEFAULT_PROJECTION in BrowserContract with the contents of your ITEMS_COLUMNS here.
2. Decide which projection to use:
2.1 If projection is null, do new Matrix(HomeItems.DEFAULT_PROJECTION)
2.2 If projection is not null, do new Matrix(projection)

If we want to be really correct here, we should update queryFakeItems() to only return the columns from the given projection. But I'm fine with just always using HomeItems.DEFAULT_PROJECTION in the fake query as it's only used by us for debugging.

Comment 7

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #6)
> Comment on attachment 8412127 [details] [diff] [review]
> Make a dummy MatrixCursor to listen for dataset changed notifications
> 
> Review of attachment 8412127 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/db/HomeProvider.java
> @@ +99,5 @@
> > +
> > +        // SQLiteBridgeContentProvider may return a null Cursor if the database hasn't been created yet.
> > +        // However, we need a non-null cursor in order to listen for notifications.
> > +        if (c == null) {
> > +            c = new MatrixCursor(ITEMS_COLUMNS);
> 
> Strictly speaking, the columns in the resulting cursor should be defined by
> the given projection in the query() call. Even though this code will work
> for our current usage (i.e. projection is always null, select all columns),
> I think we can make this code a bit more future-proof.
> 
> Here's what I suggest:
> 1. Define a HomeItems.DEFAULT_PROJECTION in BrowserContract with the
> contents of your ITEMS_COLUMNS here.
> 2. Decide which projection to use:
> 2.1 If projection is null, do new Matrix(HomeItems.DEFAULT_PROJECTION)
> 2.2 If projection is not null, do new Matrix(projection)
> 
> If we want to be really correct here, we should update queryFakeItems() to
> only return the columns from the given projection. But I'm fine with just
> always using HomeItems.DEFAULT_PROJECTION in the fake query as it's only
> used by us for debugging.

Good call, I'll update to do this. Right now we only ever query HomeProvider with a null projection, so this wasn't an issue, but your suggestion is more future-proof.

Comment 8

[:Margaret Leibovic]

Attachment: (v2) Make a dummy MatrixCursor to listen for dataset changed notifications

Updated, thanks for the suggestion.

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 8412644 [details] [diff] [review]
(v2) Make a dummy MatrixCursor to listen for dataset changed notifications

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

Look good, thanks.

Comment 10

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/528ed2029bc2

Comment 11

[:Margaret Leibovic]

Comment on attachment 8412644 [details] [diff] [review]
(v2) Make a dummy MatrixCursor to listen for dataset changed notifications

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new hub APIs to ship in Fx30
User impact if declined: panel view won't update to show data the first time data is stored
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/528ed2029bc2

Comment 13

[:Margaret Leibovic]

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

Comment 14

[Teodora Vermesan (:TeoVermesan)]

After step 4&5 from my first comment, the add-on is re-enabled and all functionalities are restored, so:
Verified fixed on:
Build: Firefox for Android 30 Beta 4
Device: Alcatel One Touch (Android 4.1.2)

Comment 15

[Teodora Vermesan (:TeoVermesan)]

After step 4&5, the add-on is re-enabled and all functionalities are restored, so:
Verified fixed on:
Build: Firefox for Android 31.0a2 (2014-05-16)
Device: Alcatel One Touch (Android 4.1.2)

Comment 16

[:Margaret Leibovic]

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

Filter on epic-hub-bugs.