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)
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)
53.18 KB,
image/png
|
Details | |
1.06 KB,
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•11 years ago
|
Assignee: nobody → margaret.leibovic
Whiteboard: abouthome-hackathon
Assignee | ||
Comment 1•11 years ago
|
||
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 2•11 years ago
|
||
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-
Assignee | ||
Comment 3•11 years ago
|
||
Sounds good to me.
Attachment #781355 -
Attachment is obsolete: true
Attachment #781760 -
Flags: review?(lucasr.at.mozilla)
Comment 4•11 years ago
|
||
Comment on attachment 781760 [details] [diff] [review] patch Review of attachment 781760 [details] [diff] [review]: ----------------------------------------------------------------- Cool.
Attachment #781760 -
Flags: review?(lucasr.at.mozilla) → review+
Assignee | ||
Comment 5•11 years ago
|
||
https://hg.mozilla.org/projects/fig/rev/ef686945229b
Whiteboard: abouthome-hackathon → abouthome-hackathon [fixed-fig]
Comment 6•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ef686945229b
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Updated•3 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
•