Closed Bug 919227 Opened 11 years ago Closed 11 years ago

Removing all history items should restore empty screen state

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect)

27 Branch
ARM
Android
defect
Not set
normal

Tracking

(firefox26 affected, firefox27 affected, fennec+)

RESOLVED DUPLICATE of bug 920507
Tracking Status
firefox26 --- affected
firefox27 --- affected
fennec + ---

People

(Reporter: aaronmt, Assigned: liuche)

References

Details

(Keywords: regression, reproducible)

Attachments

(2 files, 1 obsolete file)

Attached image Nightly (09/21) - Screenshot (deleted) —
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)
tracking-fennec: --- → ?
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.
tracking-fennec: ? → 26+
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?
Chenxia, could you have a look at this bug?
Assignee: nobody → liuche
tracking-fennec: 26+ → +
Status: NEW → ASSIGNED
Attachment #817428 - Flags: review?(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 ).
Attachment #817428 - Flags: review?(sriram) → review+
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.
Attachment #817428 - Attachment is obsolete: true
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.
Attachment #817439 - Attachment description: Removing all history items should restore empty screen state. → Removing all history items should restore empty screen state v2
Attachment #817439 - Flags: review?(sriram)
Attachment #817439 - Flags: review?(sriram) → review+
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?
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.
(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().
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.
fyi ... the crash fix for bug 920507 I just submitted also solves this bug. ;-)
Resolving this as a dupe because the patches for these two bugs have converged :)
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: