In the current implementation of MostRecentPage, the empty page is set before the cursor is loaded, so the list is initially empty and the empty page screen flashes. We should call setEmptyScreen after the cursor is loaded. This means any titles on the screen will be flashed if the empty screen needs to be displayed, but this is less bad then flashing the empty screen, probably.

I moved the setEmptyView code into loadCursor, so that the empty view is only displayed when the we've already loaded the cursor. This means that the simpleCursorAdapter class can't be static anymore, so that we can call setEmptyView on mList, which is not static. (Also had to move the enum out.) I originally had the view visibility code in onLoadFinished, but there was flashing of the title. (I'll take the outcome of this bug and incorporate the approach into bug 895867)

Comment on attachment 787213 [details] [diff] [review] Patch: fix flashing of empty history image Review of attachment 787213 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/MostRecentPage.java @@ +45,5 @@ > > // The view shown by the fragment. > private ListView mList; > > + private TextView title; This should be mTitle for consistency. Also, add a short comment describing the intent of the attribute. @@ +322,5 @@ > + if (mList != null && mList.getEmptyView() == null) { > + // Set empty page view. > + final View emptyView = getActivity().findViewById(R.id.home_empty_view); > + ((ImageView) emptyView.findViewById(R.id.home_empty_image)).setImageResource(R.drawable.icon_most_recent_empty); > + ((TextView) emptyView.findViewById(R.id.home_empty_text)).setText(R.string.home_most_recent_empty); Personally, I kinda hate these in-place castings. They make code just a bit shorter but substantially reduce readability imo. I'd rather have readable code instead of short code. So, I'd go with: final ImageView emptyIcon = (ImageView) w.findViewById(R.id.home_empty_image); emptyIcon.setImageResource(R.drawable.icon_most_recent_empty); Same for home_empty_text. @@ +324,5 @@ > + final View emptyView = getActivity().findViewById(R.id.home_empty_view); > + ((ImageView) emptyView.findViewById(R.id.home_empty_image)).setImageResource(R.drawable.icon_most_recent_empty); > + ((TextView) emptyView.findViewById(R.id.home_empty_text)).setText(R.string.home_most_recent_empty); > + mList.setEmptyView(emptyView); > + } This is not the right place to have this UI update code. The adapter should be kept as a static class. Simply add a private method to MostRecentFragment like updateUiFromCursor(Cursor c) (or something like this) and add this code there. Call updateUiFromCursor() just after the swapCursor() call above. @@ +381,5 @@ > loadFavicons(c); > } else { > super.onLoadFinished(loader, c); > } > + Unrelated? ::: mobile/android/base/resources/values/styles.xml @@ +189,5 @@ > <item name="android:focusable">false</item> > <item name="android:gravity">center|left</item> > <item name="android:paddingLeft">10dip</item> > <item name="android:paddingRight">10dip</item> > + <item name="android:visibility">gone</item> Unrelated to this patch, but putting layout bits in styles really make it harder to grasp our layout files because of the extra level of indirection. We are not very consistent about it right now but, as a general guideline, I'd prefer us to keep layout parameters (width, height, visibility, ...) directly in the layout files and only have parameters that are related to the "visual" aspect (padding, margin, gravity, text appearance, ...) of the views in styles. With that said, there are cases where putting layout parameters in style can be useful. For example, when you want to the same layout file to behave slightly different in certain form factors or orientations. But that's the most common case.

Comment on attachment 787693 [details] [diff] [review] Patch: fix flashing of empty history image v2 Review of attachment 787693 [details] [diff] [review]: ----------------------------------------------------------------- Nice. Please file a follow-up to add UI tests for empty views in the new about:home. ::: mobile/android/base/home/MostRecentPage.java @@ +148,5 @@ > } > } > > + private void updateUiFromCursor(Cursor c) { > + if (c == null || c.getCount() == 0) { This would probably easier to follow if you turned this 'if' into an early return. Something like: // Simply show title and bail if (c != null && c.getCount() > 0) { mTitle.setVisibility(View.VISIBLE); return; } if (emptyView == null) { // init mEmptyView } mList.setEmptyView(mEmptyView); @@ +152,5 @@ > + if (c == null || c.getCount() == 0) { > + mTitle.setVisibility(View.GONE); > + if (mEmptyView == null) { > + // Set empty page view. We delay this so that the empty view won't flash. > + mEmptyView = getActivity().findViewById(R.id.home_empty_view); The empty view should probably be a ViewStub in home_list_with_title. This way we avoid inflating it until it's actually shown. In case you haven't heard of ViewStubs yet, have a look at: http://developer.android.com/reference/android/view/ViewStub.html No need to cover this issue in this patch though. Please file a follow-up bug to track this. @@ +154,5 @@ > + if (mEmptyView == null) { > + // Set empty page view. We delay this so that the empty view won't flash. > + mEmptyView = getActivity().findViewById(R.id.home_empty_view); > + > + ImageView emptyIcon = ((ImageView) mEmptyView.findViewById(R.id.home_empty_image)); Make it final. No need for the enclosing parens. @@ +157,5 @@ > + > + ImageView emptyIcon = ((ImageView) mEmptyView.findViewById(R.id.home_empty_image)); > + emptyIcon.setImageResource(R.drawable.icon_most_recent_empty); > + > + TextView emptyText = ((TextView) mEmptyView.findViewById(R.id.home_empty_text)); Make it final. No need for the enclosing parens.

Landed in fig: https://hg.mozilla.org/projects/fig/rev/c0dc627fa67f

Sorry, correct version landed: https://hg.mozilla.org/projects/fig/rev/51eb7425cb01