1270162 - Empty view flashes when navigating to history panel

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[:Margaret Leibovic]

I'm seeing this on the most recently Nightly on my Nexus 5. I suspect the query is taking long enough that we show the empty view before displaying the history data.

Is there a way to avoid showing anything until the query finishes? This sounds similar to issues ahunt was investigating with the top sites panel.

Comment 1

[Chenxia Liu [:liuche]]

This logic might get a little hairy because we're managing two empty states here. What's probably happening is that the synced devices query is finishing first, and so we check the state and see that the history adapter is empty and set the history empty state visible. We could try to be clever about not "unhiding" an empty state based on a) the query that finished loading and b) the state the panel is in (history or sync). Though in reality, this is only a problem for history (and not sync, because sync is in a smartfolder and navigating to the the smartfolder is probably enough time to finish the query).

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java#275
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java#360

Comment 2

[tuhina chatterjee]

I would like to take this issue and work on it.

Comment 3

[tuhina chatterjee]

I have a few doubts.

1- how does the synced devices query work?
2- what is the workflow of RemoteTabsCursorLoader class ?
3- what is smart folder ? (It is mentioned that sync doesnot face this issue since it is in smartfolder)

P.S : Its my second bug.Please excuse me if the questions are too silly.

Comment 4

[Chenxia Liu [:liuche]]

To clarify for this bug, the relevant code is in CombinedHistoryPanel, which is loading the content for the History panel of the home pager.

When there is no history content, we show an "empty view" placeholder image (you can see this if you clear your history).

The problem that's happening in this bug is that we are showing the empty view image very briefly before we've loaded the history contents from the History cursor.

The main relevant pieces of code are as follows:

a) The code where we start loading the history cursor (this is async):
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java?q=file%3Acombinedhistorypanel&redirect_type=single#298

b) This is where we update the empty view on the callback from the cursor loader - of note, this is called when either the History or the Sync cursor load is finished:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java?q=file%3Acombinedhistorypanel&redirect_type=single#337
There are other places where we call updateEmptyView() as well.

c) The code to update the empty view:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java?q=file%3Acombinedhistorypanel&redirect_type=single#462

So to fix this bug, we should only update the empty view (in c) of the panel level if it matches the cursor that was returned (this could be PARENT/history or CHILD_SYNC/clients or CHILD_RECENT_TABS/recent tabs).

We can start off just fixing this for the PARENT/history case.

Comment 5

[Chenxia Liu [:liuche]]

> 1- how does the synced devices query work?
We make a query to the database - the specifics of the query aren't important.
> 2- what is the workflow of RemoteTabsCursorLoader class ?
This is a Cursor that loads content from a database - it's just a standard Android Cursor.
> 3- what is smart folder ? (It is mentioned that sync doesnot face this issue
> since it is in smartfolder)
The smart folder is the group (Recently Closed or Synced Devices) that is in the History panel (when you have those items).

I used the previous comment to explain this bug. I only answered your questions briefly because I don't think the details are necessary for fixing this bug.

Comment 6

[tuhina chatterjee]

Attachment: Fix#1.patch

Force completing the mDB.getRecentHistory query before views are generated. Though this leads to a slight delay in generating the view.

Comment 7

[Chenxia Liu [:liuche]]

Comment on attachment 8784709 [details] [diff] [review]
Fix#1.patch

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

This won't compile - get() is not a method of Cursor. Please make sure your patches build and verify that they're doing the correct thing before submitting them for review.

https://developer.android.com/reference/android/database/Cursor.html

Comment 8

[Jan Henning [:JanH]]

(In reply to tuhina chatterjee from comment #6)
> Force completing the mDB.getRecentHistory query before views are generated.
> Though this leads to a slight delay in generating the view.

Hmm, do I understand this correctly that - if it would work at all, see liuche's comment - this would delay the display of the whole history panel until the query completes?

If so, I don't think that this would be a good idea  - on older or otherwise underpowered phones, completing the history query can take a non-trivial amount of time, especially if Firefox is just starting up or the device is otherwise busy (or both). Therefore, I feel that delaying the display of the whole history panel (if I understand the proposed behaviour correctly) would be unacceptable, because it also prevents the user from simply accessing the recent tabs or synced devices folders, whose queries might resolver faster than query for the recent history.

Comment 9

[Chenxia Liu [:liuche]]

Agreed, please see the end of comment #4 for the suggested way to fix this bug.

Comment 10

[tuhina chatterjee]

Attachment: fix.patch

Comment 11

[Jan Henning [:JanH]]

Comment on attachment 8787346 [details] [diff] [review]
fix.patch

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

Sorry, but that's not the solution, either. The problem is not calling setVisibility when the panelLevel is not PARENT, but rather that showEmptyHistoryView can end up "true" because we're querying the number of items in the history adapter even before the database query has finished and the cursor is fully loaded.

Comment 12

[tuhina chatterjee]

The issue of loading the EmptyView of history panel arises because  the interface PanelStateUpdateHandler calls updateEmptyView[1] before the mHistoryAdapter is initialised. This causes [2] mHistoryAdapter.getCount() to return NUM_SMART_FOLDERS [3]. Is there a way to bypass the call to updateEmptyView() in PanelStateUpdateHandler[1] ? Any suggestions?

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java#357
[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java?q=file%3Acombinedhistorypanel&redirect_type=single#468
[3] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java?q=file%3Acombinedhistorypanel&redirect_type=single#72

Comment 13

[Jan Henning [:JanH]]

We've already got the PanelLevel enum, which is also corresponding to the number of empty views - each panel level (History, Sync Clients, Recent Tabs) has got its own dedicated empty view.
So one possible approach might be to have the callers of updateEmptyView() pass in their corresponding panel level as a parameter, so updateEmptyView() can identify the caller and then only update the empty view if the "panel level" of the caller matches the actual current panel level.

Comment 14

[tuhina chatterjee]

Attachment: change.patch

Comment 15

[Jan Henning [:JanH]]

Comment on attachment 8792323 [details] [diff] [review]
change.patch

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

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java
@@ +371,5 @@
>          if (mPanelStateUpdateHandler == null) {
>              mPanelStateUpdateHandler = new PanelStateUpdateHandler() {
>                  @Override
>                  public void onPanelStateUpdated() {
> +                    updateEmptyView(mPanelLevel);

Almost there, but if you look closely at the home panels, you'll notice that you can still see the history empty view flash by.
This is because here you're still calling updateEmptyView() with simply the current panel level, so we might decide to show the empty history view even though the history cursor might not yet have finished loading.

So instead, have the caller(s) of onPanelStateUpdated() provide the appropriate level and then just pass that through to updateEmptyView() here.

Comment 16

[tuhina chatterjee]

Attachment: change_new.patch

Comment 17

[Jan Henning [:JanH]]

Comment on attachment 8793002 [details] [diff] [review]
change_new.patch

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

Seems to work nicely now, thanks for persevering.
I'll leave the full review to liuche, though.

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java
@@ +477,5 @@
>              }
>          }
>      }
>  
> +    private void updateEmptyView(PanelLevel level) {

Maybe call the variable here (and possibly elsewhere) something like senderLevel to make the intention clearer?

@@ +482,4 @@
>          boolean showEmptyHistoryView = false;
>          boolean showEmptyClientsView = false;
>          boolean showEmptyRecentTabsView = false;
> +        if(mPanelLevel == level){

Or alternatively a small comment here?

Comment 18

[Jan Henning [:JanH]]

Comment on attachment 8793002 [details] [diff] [review]
change_new.patch

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

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java
@@ +500,5 @@
>  
>          final boolean showEmptyView = showEmptyClientsView || showEmptyHistoryView || showEmptyRecentTabsView;
>          mRecyclerView.setOverScrollMode(showEmptyView ? View.OVER_SCROLL_NEVER : View.OVER_SCROLL_IF_CONTENT_SCROLLS);
>  
>          mClientsEmptyView.setVisibility(showEmptyClientsView ? View.VISIBLE : View.GONE);

One afterthought: While your original patch from comment 10 wasn't enough to solve this bug, it was not a bad idea nevertheless.
I.e. each of these setVisibility() calls needs to be moved into the appropriate case of the switch statement above.

Otherwise the empty view on the history panel will e.g. be erroneously hidden again if you press the "Clear browsing history" button.
With the current patch, when history is cleared, first the history cursor updates, so we start start showing the empty view, then the RecentTabsAdapter clears its tabs as well and updates the panel state - because in the latter case the sender level won't match mPanelLevel, we skip the switch statement, but we still call setVisibility(), which will hide the history empty view again.

Comment 19

[tuhina chatterjee]

Attachment: fix.patch

Comment 20

[Chenxia Liu [:liuche]]

Comment on attachment 8794616 [details] [diff] [review]
fix.patch

I think JanH is actually a great person for reviewing this because he's been working with this code. Since you've been doing all the work for the reviewing so far, I trust you to have the final say in this review, JanH!

Comment 21

[Jan Henning [:JanH]]

Comment on attachment 8794616 [details] [diff] [review]
fix.patch

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

Well then, looks good.

One small tip for the future: Don't just copy the bug title for the commit message, instead make it a brief summary of what your patch actually does.
Also, if you know who's going to review your patch (admittedly it was a bit unclear this time :-) ), you can already include the r=reviewername line in the commit message when uploading a patch to save the sherrifs a little bit of work.

Comment 22

[Jan Henning [:JanH]]

I think test coverage of the home panels is rather light, but in any case just to be on the safe side:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0b87d774d7d

Comment 23

[Jan Henning [:JanH]]

Looks okay to me.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/e8b68216e610
Empty view flashes when navigating to history panel. r=JanH

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e8b68216e610