Closed Bug 897252 Opened 11 years ago Closed 11 years ago

[fig] Selecting "Remove" in history list generates new "Today" headers

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

(Whiteboard: abouthome-hackathon [fixed-fig])

Attachments

(2 files, 1 obsolete file)

Attached image screenshot
See screenshot. This may be fixed by bug 897250, since the "Remove" context menu item is currently broken, but this symptom is different enough to merit a bug.
Assignee: nobody → margaret.leibovic
Whiteboard: abouthome-hackathon
Attached patch debugging WIP (obsolete) — Splinter Review
What's happening here is that we're calling loadHistorySections when the item is deleted, which calls mHistorySections.append() with any sections it finds. It appears this is happening because we're loading a new cursor.

I first experimented with just initializing mHistorySections in loadHistorySections, but that would lead to NPEs because we can end up referencing mHistorySections before calling loadHistorySections. We could add lots of null checks, but that seems annoying.

Next, I experimented with resetting mHistorySections at the top of loadHistorySections, but this feels kind of dirty because it makes the initial initialization pointless (except for avoiding those null checks). 

Since loadHistorySections iterates through the whole cursor, it feels like we don't want to do that more than necessary, so I came up with this solution to just bail if we've already created history sections. The downside of this is that we won't end up removing a section when all the items in it are deleted, but that doesn't seem too terrible. What is the lifecycle for this fragment? If we go with this approach, it means we would only update the sections once per onActivityCreated/onDetach cycle.

Asking for feedback because I'm not sure the best way to approach this issue.
Attachment #781355 - Flags: feedback?(lucasr.at.mozilla)
Comment on attachment 781355 [details] [diff] [review]
debugging WIP

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

(In reply to :Margaret Leibovic from comment #1)
> Next, I experimented with resetting mHistorySections at the top of
> loadHistorySections, but this feels kind of dirty because it makes the
> initial initialization pointless (except for avoiding those null checks). 

Sounds perfectly valid to simply call mHistorySections.clear() at the top of loadHistorySections(). If a new cursor is loaded, we do want to refresh the list of sections entirely. Simply bailing if mHistorySections has already been filled can cause problems because the index of the first item in the next section (in relation to the removed item) needs to be updated accordingly.
Attachment #781355 - Flags: feedback?(lucasr.at.mozilla) → feedback-
Attached patch patchSplinter Review
Sounds good to me.
Attachment #781355 - Attachment is obsolete: true
Attachment #781760 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 781760 [details] [diff] [review]
patch

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

Cool.
Attachment #781760 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/projects/fig/rev/ef686945229b
Whiteboard: abouthome-hackathon → abouthome-hackathon [fixed-fig]
https://hg.mozilla.org/mozilla-central/rev/ef686945229b
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
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: