Closed Bug 759120 Opened 10 years ago Closed 10 years ago

History list doesn't update after you remove the last history item using context menu

Categories

(Firefox for Android Graveyard :: General, defect)

15 Branch
ARM
Android
defect
Not set
normal

Tracking

(firefox15 verified, firefox16 verified)

VERIFIED FIXED
Firefox 16
Tracking Status
firefox15 --- verified
firefox16 --- verified

People

(Reporter: paul.feher, Assigned: Margaret)

References

Details

Attachments

(1 file)

Nightly Fennec 15.0a1 (2012-05-27)
Device: HTC Desire Z 
OS: Android 2.3.3

Steps to reproduce:
1. Make sure you have entrees in the History section.
2. Tap the URL bar to access the History tab.
3. Long tap on one entree for the context menu to appear.
4. Repeat step 4 and remove all entrees using the remove button.

Expected:
A toast notification appears whit the message "Page Removed" and all entrees from the History section are removed. 

Actual:
If there is only one item in the history section the toast notification appears but the item is still present. Only after going back and accessing another time the awesome-menu the History tab is updated and the entree is removed.
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/LocalBrowserDB.java#263

I wonder if BrowserDB.removeHistoryEntry is receiving a null ID or something.

Looks like a Margaret bug :)
tracking-fennec: --- → ?
Oh, this looks like the operation is not executed right away because when I exit out of the AwesomeScreen and return back to the History tab, the item is removed.
The problem here is that the ContentObserver makes a new HistoryQueryTask to update the list, and that just bails if there are no items in history:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AwesomeBarTabs.java#540

Ideally, we should fix the "FIXME" in there and display some message when there is no history to show, but as a solution to this specific problem, we should empty the list.
Assignee: nobody → margaret.leibovic
Blocks: 671131
Summary: Can't remove the last item from the history section using context menu → History list doesn't update after you remove the last history item using context menu
Attached patch patchSplinter Review
Since we're not actually handling the empty history case, I think we should just go ahead and set up the history list adapter with empty lists.

Also, I'm not sure if there's a way to clear out the contents of the list view other than just changing the adapter.
Attachment #630229 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 630229 [details] [diff] [review]
patch

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

::: mobile/android/base/AwesomeBarTabs.java
@@ -550,5 @@
>              return HistorySection.OLDER;
>          }
>  
>          protected void onPostExecute(Pair<GroupList,List<ChildrenList>> result) {
> -            // FIXME: display some sort of message when there's no history

Please file a bug to track this FIXME.
Attachment #630229 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Lucas Rocha (:lucasr) from comment #5)

> ::: mobile/android/base/AwesomeBarTabs.java
> @@ -550,5 @@
> >              return HistorySection.OLDER;
> >          }
> >  
> >          protected void onPostExecute(Pair<GroupList,List<ChildrenList>> result) {
> > -            // FIXME: display some sort of message when there's no history
> 
> Please file a bug to track this FIXME.

I'm not sure that we even need to show a message, but I filed bug 762086 to track the issue and get some UX input.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f0ed50cedde
Target Milestone: --- → Firefox 16
Comment on attachment 630229 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): this problem has always existed, but bug 671131 exposed it, so this patch just needs to be on aurora, not beta
User impact if declined: removing the last history item in the history tab will look like it didn't work because the list won't update to be empty
Testing completed (on m-c, etc.): just landed on inbound
Risk to taking this patch (and alternatives if risky): low-risk, just removes special-case early return for an empty history
String or UUID changes made by this patch: n/a
Attachment #630229 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/7f0ed50cedde
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Looks good on inbound, verified fixed on 16.0a1
Attachment #630229 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
tracking-fennec: ? → ---
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.