919227 - Removing all history items should restore empty screen state

Status: RESOLVED DUPLICATE of bug 920507

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: Nightly (09/21) - Screenshot

Visit a site, remove the site from the history pane and we should show the history empty screen state (implemented in bug 895866). Currently, we show the 'Today' label and an empty list view: see screenshot

--
Nightly (09/21)
LG Nexus 4 (Android 4.3)

Comment 1

[Aaron Train [:aaronmt]]

http://hg.mozilla.org/mozilla-central/pushloghtml?fromdate=2013-09-20&todate=2013-09-21

Bug 918377? Bug 917455?

Comment 2

[Chenxia Liu [:liuche]]

This happens because we use Android's setEmptyView for handling empty adapters. The activity doesn't update the screen immediately, but if you re-open the home screen, the empty screen should display again.

I'm okay with the titles being displayed if you've manually removed the items one by one, because immediately swapping to the empty screen could be jarring.

If you are on the History screen, however, and you clear all your history through settings, exiting settings will not update the empty screen until the next time the cursor is finished loading.

Comment 3

[Lucas Rocha (:lucasr)]

Note how the 'Today' header is still there when it shouldn't. Maybe this bug will be fixed once a patch for bug 920507 lands?

Comment 4

[Lucas Rocha (:lucasr)]

Chenxia, could you have a look at this bug?

Comment 5

[Chenxia Liu [:liuche]]

Attachment: Removing all history items should restore empty screen state.

Comment 6

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 817428 [details] [diff] [review]
Removing all history items should restore empty screen state.

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

r+ after removing notifyDataSetChanged(). And please test it once that statement is removed.

::: mobile/android/base/home/MostRecentPage.java
@@ +350,5 @@
>  
>          @Override
>          public void onLoadFinished(Loader<Cursor> loader, Cursor c) {
>              mAdapter.swapCursor(c);
> +            mAdapter.notifyDataSetChanged();

This might not be needed. swapCursor() will call notifyDataSetChanged() internally. (Atleast that's what Android code says, though it is under many if blocks :P ).

Comment 7

[Chenxia Liu [:liuche]]

notifyDataSetChanged is actually needed, because this is probably the case where the new cursor is being set to null, so notifyDataSetInvalidated() is called instead of notifyDataSetChanged(), and this causes a crash because the ListView that we're called setEmptyView on doesn't get notified.

I could add notifyDataSetChanged to updateUiFromCursor, if the cursor is null if you'd prefer.

Comment 8

[Chenxia Liu [:liuche]]

Attachment: Removing all history items should restore empty screen state v2

Comment 9

[Chenxia Liu [:liuche]]

Comment on attachment 817439 [details] [diff] [review]
Removing all history items should restore empty screen state v2

Moved the notifyDataSetChanged to updateUiFromCursor, and added a comment.

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 817439 [details] [diff] [review]
Removing all history items should restore empty screen state v2

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

::: mobile/android/base/home/MostRecentPage.java
@@ +165,5 @@
>              final TextView emptyText = (TextView) mEmptyView.findViewById(R.id.home_empty_text);
>              emptyText.setText(R.string.home_most_recent_empty);
>  
> +            // In the case where the cursor has been set to null, the adapter may not have notified its observers, so notify them now.
> +            mAdapter.notifyDataSetChanged();

Drive-by: this call should probably live in adapter.swapCursor(), no?

Comment 11

[Sriram Ramasubramanian [:sriram]]

This actually lives in swapCursor(). But it's inside "not null" block.
Probably this statement can reside inside a null check in while swapping cursor.

Comment 12

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #11)
> This actually lives in swapCursor(). But it's inside "not null" block.
> Probably this statement can reside inside a null check in while swapping
> cursor.

Yeah, I meant MostRecentAdapter.swapCursor().

Comment 13

[Chenxia Liu [:liuche]]

I could add this into our implementation of swapCursor, if cursor == null - but this would basically override Android's implementation, which calls notifyDataSetChanged when the cursor is not null and notifyDataSetInvalidated when it is null.

Since we're setting the empty view lazily, we care about notifying the ListView in the single case where the cursor is null and we haven't already set the empty view (apparently notifyDataSetInvalidated is not sufficient), so we should be able to use the Android implementation in all other cases.

Comment 14

[Mark Capella [:capella]]

fyi ... the crash fix for bug 920507 I just submitted also solves this bug. ;-)

Comment 15

[Chenxia Liu [:liuche]]

Resolving this as a dupe because the patches for these two bugs have converged :)