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)
Tracking
(firefox26 affected, firefox27 affected, fennec+)
RESOLVED
DUPLICATE
of bug 920507
People
(Reporter: aaronmt, Assigned: liuche)
References
Details
(Keywords: regression, reproducible)
Attachments
(2 files, 1 obsolete file)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
sriram
:
review+
|
Details | Diff | Splinter Review |
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)
Reporter | ||
Comment 1•11 years ago
|
||
http://hg.mozilla.org/mozilla-central/pushloghtml?fromdate=2013-09-20&todate=2013-09-21
Bug 918377? Bug 917455?
Keywords: regression
Reporter | ||
Updated•11 years ago
|
tracking-fennec: --- → ?
Assignee | ||
Comment 2•11 years ago
|
||
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.
Updated•11 years ago
|
tracking-fennec: ? → 26+
Comment 3•11 years ago
|
||
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?
Updated•11 years ago
|
tracking-fennec: 26+ → +
Reporter | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #817428 -
Flags: review?(sriram)
Assignee | ||
Updated•11 years ago
|
status-firefox26:
--- → affected
status-firefox27:
--- → affected
tracking-firefox26:
--- → ?
tracking-firefox27:
--- → ?
Comment 6•11 years ago
|
||
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+
Assignee | ||
Comment 7•11 years ago
|
||
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.
Assignee | ||
Comment 8•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #817428 -
Attachment is obsolete: true
Assignee | ||
Comment 9•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #817439 -
Flags: review?(sriram) → review+
Comment 10•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
(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().
Assignee | ||
Comment 13•11 years ago
|
||
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•11 years ago
|
||
fyi ... the crash fix for bug 920507 I just submitted also solves this bug. ;-)
Assignee | ||
Comment 15•11 years ago
|
||
Resolving this as a dupe because the patches for these two bugs have converged :)
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
Updated•11 years ago
|
tracking-firefox26:
? → ---
tracking-firefox27:
? → ---
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•