Improve bookmark deletion patterns #3271
Conversation
| let favoritesFolders = BookmarkUtils.fetchFavoritesFolders(for: favoritesDisplayMode, in: context) | ||
| assert(bookmarkManagedObjects.count == objectMap.count) | ||
| bookmarkManagedObjects.forEach { managedObject in | ||
| managedObject.cancelDeletion() |
There was a problem hiding this comment.
@mallexxx this won't work with Sync.
A bookmark that's marked as pending deletion is synced immediately (sync is triggered after context save and Sync Scheduler doesn't introduce any delay at the moment, apart from debouncing by 1s). Upon receivng Sync response, the bookmark is deleted from the database.
As far as I remember, Sync doesn't allow for un-deleting an object, i.e. if a delete is sent to the backend, further updates will be ignored. Hence we can't just cancel deletion here.
iOS supports undo for individual bookmarks and it's done by re-creating a deleted bookmark (with a new UUID), see here: https://github.com/duckduckgo/iOS/blob/main/DuckDuckGo/BookmarksViewController.swift#L1077-L1114.
BookmarkEntity.cancelDeletion() is only used by Sync Engine and we should add documentation there stating that it should not be used elsewhere.
Essentially the requirement is that undo should re-create a bookmark with a new UUID, which you could perhaps do here as well. Just create new managed objects based on BaseBookmarkEntity objects. The problem is only that you'd have to cache deleted items' indexes in parent's children array somehow, to know where to put re-created bookmarks and folders.
Documentation on undo handling for Syncable entitles: https://app.asana.com/0/1201493110486074/1205142204227585/f
Task/Issue URL: https://app.asana.com/0/1207340338530322/1205306631184883/f
Description:
Steps to test this PR:
Definition of Done:
Internal references:
Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation