Support Deletion of Suggested Tiles

RESOLVED FIXED in 4.0

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bmunar, Assigned: bmunar, Mentored)

Tracking

unspecified
Other
iOS

Firefox Tracking Flags

(fxios-v3.0 fixed, fxios+)

Details

Attachments

(1 attachment)

PR
48 bytes, text/x-github-pull-request
sleroux
: review+
tecgirl
: ui-review+
sleroux
: feedback+
Details | Review | Splinter Review
Comment hidden (empty)
(Assignee)

Updated

3 years ago
Mentor: sleroux
tracking-fxios: --- → ?
Here's the skinny on TopSitesPanel and adding support for deleting suggested tiles:

1. The 'removal' code should partially work regardless of what kind of tile it is. The only reason we don't show the [x] for suggested tiles is because we explicitly hide them for suggested tiles [1].

2. The challenging part of removing suggested tiles is that in both the deletion logic and data source we factor in the SuggestedSites separately from the normal top sites. For example, when deleting, we add the number of suggested sites to the data source and use that for determining which cell to delete [2]. We also hide this inside the data source [3]. 

3. Another issue is that we don't actually store any state for a suggested tile anywhere. They are all statically provided from SuggestedSites.swift [4]. We'll need to add a way of remembering when a user deletes one.

Some ideas:

* I think the first step to bringing some sanity into TopSitesPanel is to rid the notion of appending SuggestedSites to the data source every time we use it and bring a uniform view of the sites that we display. This way the removal logic can be somewhat agnostic of what kind of site we're deleting. This might look like building a wrapping cursor object that delegates to two child cursors for top sites/suggested sites so from the outside it looks like a single cursor.

* Along with the cursor abstraction, we could also delegate the persistence/deletion to this object as well so when we tell the object to delete a tile, the TopSitesPanel doesn't care how we delete it - just that it gets deleted. Behind the scenes we can implement the SQL deletion for TopSites and something else for suggested sites.

Overall I think we'll need to build up some abstractions on top of what we have now to make it easier to manage deleting both types of cells while keeping our code clean.

[1] https://github.com/mozilla/firefox-ios/blob/master/Client/Frontend/Home/TopSitesPanel.swift#L154
[2] https://github.com/mozilla/firefox-ios/blob/master/Client/Frontend/Home/TopSitesPanel.swift#L197
[3] https://github.com/mozilla/firefox-ios/blob/master/Client/Frontend/Home/TopSitesPanel.swift#L582
[4] https://github.com/mozilla/firefox-ios/blob/master/Storage/SuggestedSites.swift#L29
(Assignee)

Comment 2

3 years ago
Created attachment 8725084 [details] [review]
PR
Attachment #8725084 - Flags: ui-review?(randersen)
Attachment #8725084 - Flags: review?(sleroux)
(Assignee)

Updated

3 years ago
Assignee: nobody → bmunar
Rank: 5
tracking-fxios: ? → +
Comment on attachment 8725084 [details] [review]
PR

Good start - left some comments
Attachment #8725084 - Flags: feedback+
Comment on attachment 8725084 [details] [review]
PR

Awesome stuff - looks good. Thanks for revisions!
Attachment #8725084 - Flags: review?(sleroux) → review+
(Assignee)

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Re-read this:

https://mail.mozilla.org/pipermail/mobile-firefox-dev/2016-March/001791.html
Flags: needinfo?(bmunar)
(Assignee)

Comment 6

3 years ago
oops forgot/didn't see that, thanks!
Flags: needinfo?(bmunar)
(Assignee)

Updated

3 years ago
Target Milestone: --- → 4.0
Whiteboard: [needsuplift]
v3.x 0cfcf23
status-fxios-v3.0: --- → fixed
Whiteboard: [needsuplift]
Attachment #8725084 - Flags: ui-review?(randersen) → ui-review+
You need to log in before you can comment on or make changes to this bug.