Improve bookmark deletion patterns (#3271)

Task/Issue URL: https://app.asana.com/0/1207340338530322/1205306631184883/f **Description**: - Always display Delete button in Bookmark Manager (disable when no items highlighted) - Support item removal on Del press - Show ••• menu in Bookmarks Sidebar; For bookmark folders in Bookmark Manager **Steps to test this PR**: 1. Validate Delete button is always shown; disabled when no items selected; Removes all selected items 2. Validate Forward Delete key deletes selected bookmarks 3. Validate the ••• menu is displayed and works for the Sidebar folders as well as the Details view in Bookmarks Manager  **Definition of Done**: * [ ] Does this PR satisfy our [Definition of Done](https://app.asana.com/0/1202500774821704/1207634633537039/f)? --- ###### Internal references: [Pull Request Review Checklist](https://app.asana.com/0/1202500774821704/1203764234894239/f) [Software Engineering Expectations](https://app.asana.com/0/59792373528535/199064865822552) [Technical Design Template](https://app.asana.com/0/59792373528535/184709971311943) [Pull Request Documentation](https://app.asana.com/0/1202500774821704/1204012835277482/f) --------- Co-authored-by: Dominik Kapusta <[email protected]> Co-authored-by: Juan Manuel Pereira <[email protected]>
duckduckgo · Oct 19, 2024 · 181d83d · 181d83d
1 parent 8acf064
commit 181d83d
Show file tree

Hide file tree

Showing 30 changed files with 1,671 additions and 796 deletions.
diff --git a/.swiftlint.yml b/.swiftlint.yml
@@ -67,6 +67,8 @@ file_header:
 excluded:
   - DuckDuckGo/Common/Localizables/UserText.swift
   - LocalPackages/*/Package.swift
+  - scripts
+  - DerivedData
   - release
   - vendor
   - DerivedData

diff --git a/DuckDuckGo/Bookmarks/Model/Bookmark.swift b/DuckDuckGo/Bookmarks/Model/Bookmark.swift
@@ -262,6 +262,28 @@ final class Bookmark: BaseBookmarkEntity {
     }
 
 }
+extension BaseBookmarkEntity: CustomDebugStringConvertible {
+    var debugDescription: String {
+        switch self {
+        case let folder as BookmarkFolder:
+            folder.folderDebugDescription
+        case let bookmark as Bookmark:
+            bookmark.bookmarkDebugDescription
+        default:
+            fatalError("Unexpected entity type: \(self)")
+        }
+    }
+}
+extension BookmarkFolder {
+    fileprivate var folderDebugDescription: String {
+        "<BookmarkFolder \(id) \"\(title)\" parent: \(parentFolderUUID ?? "root") children: [\(children.map(\.debugDescription))]>"
+    }
+}
+extension Bookmark {
+    fileprivate var bookmarkDebugDescription: String {
+        "<Bookmark\(isFavorite ? "⭐️" : "") \(id) title: \"\(title)\" url: \"\(url)\" parent: \(parentFolderUUID ?? "root")>"
+    }
+}
 
 extension Array where Element == BaseBookmarkEntity {
     func sorted(by sortMode: BookmarksSortMode) -> [BaseBookmarkEntity] {

diff --git a/DuckDuckGo/Bookmarks/Model/BookmarkManager.swift b/DuckDuckGo/Bookmarks/Model/BookmarkManager.swift
@@ -32,13 +32,13 @@ protocol BookmarkManager: AnyObject {
     func getBookmark(forUrl url: String) -> Bookmark?
     func getBookmark(forVariantUrl variantURL: URL) -> Bookmark?
     func getBookmarkFolder(withId id: String) -> BookmarkFolder?
-    @discardableResult func makeBookmark(for url: URL, title: String, isFavorite: Bool) -> Bookmark?
     @discardableResult func makeBookmark(for url: URL, title: String, isFavorite: Bool, index: Int?, parent: BookmarkFolder?) -> Bookmark?
     func makeBookmarks(for websitesInfo: [WebsiteInfo], inNewFolderNamed folderName: String, withinParentFolder parent: ParentFolderType)
-    func makeFolder(for title: String, parent: BookmarkFolder?, completion: @escaping (BookmarkFolder) -> Void)
-    func remove(bookmark: Bookmark)
-    func remove(folder: BookmarkFolder)
-    func remove(objectsWithUUIDs uuids: [String])
+    func makeFolder(named title: String, parent: BookmarkFolder?, completion: @escaping (Result<BookmarkFolder, Error>) -> Void)
+    func remove(bookmark: Bookmark, undoManager: UndoManager?)
+    func remove(folder: BookmarkFolder, undoManager: UndoManager?)
+    func remove(objectsWithUUIDs uuids: [String], undoManager: UndoManager?)
+    func restore(_ entities: [RestorableBookmarkEntity], undoManager: UndoManager)
     func update(bookmark: Bookmark)
     func update(bookmark: Bookmark, withURL url: URL, title: String, isFavorite: Bool)
     func update(folder: BookmarkFolder)
@@ -68,9 +68,18 @@ protocol BookmarkManager: AnyObject {
     var sortMode: BookmarksSortMode { get set }
 
     func requestSync()
-
 }
-
+extension BookmarkManager {
+    @discardableResult func makeBookmark(for url: URL, title: String, isFavorite: Bool) -> Bookmark? {
+        makeBookmark(for: url, title: title, isFavorite: isFavorite, index: nil, parent: nil)
+    }
+    @discardableResult func makeBookmark(for url: URL, title: String, isFavorite: Bool, index: Int?, parent: BookmarkFolder?) -> Bookmark? {
+        makeBookmark(for: url, title: title, isFavorite: isFavorite, index: index, parent: parent)
+    }
+    func move(objectUUIDs: [String], toIndex index: Int?, withinParentFolder parent: ParentFolderType) {
+        move(objectUUIDs: objectUUIDs, toIndex: index, withinParentFolder: parent) { _ in }
+    }
+}
 final class LocalBookmarkManager: BookmarkManager {
     static let shared = LocalBookmarkManager()
 
@@ -206,8 +215,8 @@ final class LocalBookmarkManager: BookmarkManager {
         let bookmark = Bookmark(id: id, url: url.absoluteString, title: title, isFavorite: isFavorite, parentFolderUUID: parent?.id)
 
         list?.insert(bookmark)
-        bookmarkStore.save(bookmark: bookmark, parent: parent, index: index) { [weak self] success, _  in
-            guard success else {
+        bookmarkStore.save(bookmark: bookmark, index: index) { [weak self] error  in
+            if error != nil {
                 self?.list?.remove(bookmark)
                 return
             }
@@ -225,16 +234,18 @@ final class LocalBookmarkManager: BookmarkManager {
         requestSync()
     }
 
-    func remove(bookmark: Bookmark) {
+    @MainActor
+    func remove(bookmark: Bookmark, undoManager: UndoManager?) {
         guard list != nil else { return }
         guard let latestBookmark = getBookmark(forUrl: bookmark.url) else {
             Logger.bookmarks.error("LocalBookmarkManager: Attempt to remove already removed bookmark")
             return
         }
 
+        undoManager?.registerUndoDeleteEntities([bookmark], bookmarkManager: self)
         list?.remove(latestBookmark)
-        bookmarkStore.remove(objectsWithUUIDs: [bookmark.id]) { [weak self] success, _ in
-            if !success {
+        bookmarkStore.remove(objectsWithUUIDs: [bookmark.id]) { [weak self] error in
+            if error != nil {
                 self?.list?.insert(bookmark)
             }
 
@@ -243,18 +254,58 @@ final class LocalBookmarkManager: BookmarkManager {
         }
     }
 
-    func remove(folder: BookmarkFolder) {
-        bookmarkStore.remove(objectsWithUUIDs: [folder.id]) { [weak self] _, _ in
+    @MainActor
+    func remove(folder: BookmarkFolder, undoManager: UndoManager?) {
+        undoManager?.registerUndoDeleteEntities([folder], bookmarkManager: self)
+        bookmarkStore.remove(objectsWithUUIDs: [folder.id]) { [weak self] _ in
+            self?.loadBookmarks()
+            self?.requestSync()
+        }
+    }
+
+    @MainActor
+    func remove(objectsWithUUIDs uuids: [String], undoManager: UndoManager?) {
+        if let undoManager, let entities = bookmarkStore.bookmarkEntities(withIds: uuids) {
+            undoManager.registerUndoDeleteEntities(entities, bookmarkManager: self)
+        }
+        bookmarkStore.remove(objectsWithUUIDs: uuids) { [weak self] _ in
             self?.loadBookmarks()
             self?.requestSync()
         }
     }
 
-    func remove(objectsWithUUIDs uuids: [String]) {
-        bookmarkStore.remove(objectsWithUUIDs: uuids) { [weak self] _, _ in
+    @MainActor
+    func restore(_ restorableEntities: [RestorableBookmarkEntity], undoManager: UndoManager) {
+        let entitiesAtIndices = restorableEntities.map { entity -> (entity: BaseBookmarkEntity, index: Int?, indexInFavoritesArray: Int?) in
+            switch entity {
+            case let .bookmark(url: url, title: title, isFavorite: isFavorite, parent: parent, index: index, indexInFavoritesArray: indexInFavoritesArray):
+                let bookmark = Bookmark(id: UUID().uuidString, url: url, title: title, isFavorite: isFavorite, parentFolderUUID: parent?.actualId)
+                list?.insert(bookmark)
+                return (bookmark, index, indexInFavoritesArray)
+
+            case let .folder(title: title, parent: parent, index: index, originalId: originalId):
+                return (BookmarkFolder(id: originalId.newId(), title: title, parentFolderUUID: parent?.actualId, children: []), index, nil)
+            }
+        }
+        bookmarkStore.save(entitiesAtIndices: entitiesAtIndices) { [weak self] _ in
             self?.loadBookmarks()
             self?.requestSync()
         }
+
+        var subfolderIds = Set<String>()
+        let topLevelUuids = entitiesAtIndices.reduce(into: [String]()) { (uuids, item) in
+            if item.entity.isFolder {
+                subfolderIds.insert(item.entity.id)
+            }
+            if let parentId = item.entity.parentFolderUUID, subfolderIds.contains(parentId) {
+                // don‘t include a nested item id as its parent will be removed with all its descendants
+                return
+            }
+            uuids.append(item.entity.id)
+        }
+        undoManager.registerUndo(withTarget: self) { @MainActor this in
+            this.remove(objectsWithUUIDs: topLevelUuids, undoManager: undoManager)
+        }
     }
 
     func update(bookmark: Bookmark) {
@@ -321,17 +372,17 @@ final class LocalBookmarkManager: BookmarkManager {
 
     // MARK: - Folders
 
-    func makeFolder(for title: String, parent: BookmarkFolder?, completion: @escaping (BookmarkFolder) -> Void) {
-
+    func makeFolder(named title: String, parent: BookmarkFolder?, completion: @escaping (Result<BookmarkFolder, Error>) -> Void) {
         let folder = BookmarkFolder(id: UUID().uuidString, title: title, parentFolderUUID: parent?.id, children: [])
 
-        bookmarkStore.save(folder: folder, parent: parent) { [weak self] success, _  in
-            guard success else {
+        bookmarkStore.save(folder: folder) { [weak self] error  in
+            if let error {
+                completion(.failure(error))
                 return
             }
             self?.loadBookmarks()
             self?.requestSync()
-            completion(folder)
+            completion(.success(folder))
         }
     }
 
@@ -370,7 +421,6 @@ final class LocalBookmarkManager: BookmarkManager {
                 self?.requestSync()
             }
             completion(error)
-
         }
     }
 
@@ -464,6 +514,174 @@ final class LocalBookmarkManager: BookmarkManager {
 
         return result
     }
+
+}
+// MARK: - UndoManager
+@MainActor
+final class KnownBookmarkFolderIdList {
+    struct WeakRef {
+        weak var value: KnownBookmarkFolderIdList?
+    }
+    static var restorableFolders = [String: WeakRef]()
+
+    var knownIds: Set<String>
+    var actualId: String?
+
+    private init(uuid: String) {
+        self.knownIds = [uuid]
+        self.actualId = uuid
+    }
+
+    static func list(withId uuid: String) -> KnownBookmarkFolderIdList {
+        if let list = restorableFolders[uuid]?.value {
+            return list
+        }
+        let list = KnownBookmarkFolderIdList(uuid: uuid)
+        restorableFolders[uuid] = WeakRef(value: list)
+        return list
+    }
+
+    func resolve(withId actualId: String) {
+        self.knownIds.insert(actualId)
+        self.actualId = actualId
+    }
+
+    /// update known folder (ex) id list with actual id so the restored bookmarks referencing an old id are restored to this folder
+    func newId() -> String {
+        let newId = UUID().uuidString
+        self.resolve(withId: newId)
+        return newId
+    }
+
+    deinit {
+        MainActor.assumeIsolated {
+            for id in knownIds {
+                Self.restorableFolders[id] = nil
+            }
+        }
+    }
+}
+enum RestorableBookmarkEntity {
+    case bookmark(url: String, title: String, isFavorite: Bool, parent: KnownBookmarkFolderIdList?, index: Int?, indexInFavoritesArray: Int?)
+    case folder(title: String, parent: KnownBookmarkFolderIdList?, index: Int?, originalId: KnownBookmarkFolderIdList)
+
+    @MainActor
+    init(bookmarkEntity: BaseBookmarkEntity, index: Int?, indexInFavoritesArray: Int?) {
+        switch bookmarkEntity {
+        case let bookmark as Bookmark:
+            self = .bookmark(url: bookmark.url, title: bookmark.title, isFavorite: bookmark.isFavorite, parent: bookmark.parentFolderUUID.map(KnownBookmarkFolderIdList.list(withId:)), index: index, indexInFavoritesArray: indexInFavoritesArray)
+        case let folder as BookmarkFolder:
+            self = .folder(title: folder.title, parent: folder.parentFolderUUID.map(KnownBookmarkFolderIdList.list(withId:)), index: index, originalId: .list(withId: folder.id))
+        default:
+            fatalError("Unexpected entity type \(bookmarkEntity)")
+        }
+    }
+
+    var parent: KnownBookmarkFolderIdList? {
+        switch self {
+        case .bookmark(_, _, _, parent: let parent, index: _, indexInFavoritesArray: _),
+             .folder(_, parent: let parent, index: _, originalId: _):
+            return parent
+        }
+    }
+    var index: Int? {
+        switch self {
+        case .bookmark(_, _, _, _, index: let index, indexInFavoritesArray: _),
+             .folder(_, _, index: let index, originalId: _):
+            return index
+        }
+    }
+    var title: String {
+        switch self {
+        case .bookmark(_, title: let title, _, _, _, _),
+             .folder(title: let title, _, _, originalId: _):
+            return title
+        }
+    }
+}
+extension [RestorableBookmarkEntity] {
+    @MainActor
+    init(entities: [BaseBookmarkEntity], bookmarkManager: some BookmarkManager) {
+        assert(Set(entities.map(\.parentFolderUUID)).count == 1, "Removing multiple items at different levels has not been implemented/tested!")
+        assert(Set(entities.map(\.id)).count == entities.count, "Some entities are repeated in the passed array")
+
+        var folderCache = [String: BookmarkFolder]()
+        var removedFolderStack = [(folder: BookmarkFolder, index: Int?)]()
+        self = entities.map { entity in
+
+            let parent: BookmarkFolder? = {
+                guard let parentId = entity.parentFolderUUID else {
+                    return nil
+                }
+                if let cachedFolder = folderCache[parentId] {
+                    return cachedFolder
+                }
+                guard let folder = bookmarkManager.getBookmarkFolder(withId: parentId) else {
+                    return nil
+                }
+                folderCache[folder.id] = folder
+                return folder
+            }()
+
+            let siblings = parent?.children ?? bookmarkManager.list?.topLevelEntities
+            let index = siblings?.firstIndex(where: { $0.id == entity.id }) ?? -1
+
+            if let folder = entity as? BookmarkFolder {
+                removedFolderStack.append((folder, index))
+            }
+
+            if let bookmark = entity as? Bookmark, bookmark.isFavorite {
+                let indexInFavoritesArray = bookmarkManager.list?.favoriteBookmarks.firstIndex(of: bookmark)
+                return RestorableBookmarkEntity(bookmarkEntity: bookmark, index: index, indexInFavoritesArray: indexInFavoritesArray)
+            }
+
+            return RestorableBookmarkEntity(bookmarkEntity: entity, index: index, indexInFavoritesArray: nil)
+        }.sorted {
+            $0.index ?? Int.max < $1.index ?? Int.max
+        }
+        removedFolderStack.sort {
+            $0.index ?? Int.max > $1.index ?? Int.max
+        }
+
+        while let (folder, _) = removedFolderStack.popLast() {
+            for entity in folder.children {
+                // children items of a removed folder are inserted in the original order so we don‘t need to track their indices
+                if let bookmark = entity as? Bookmark, bookmark.isFavorite {
+                    let indexInFavoritesArray = bookmarkManager.list?.favoriteBookmarks.firstIndex(of: bookmark)
+                    self.append(RestorableBookmarkEntity(bookmarkEntity: entity, index: nil, indexInFavoritesArray: indexInFavoritesArray))
+                } else {
+                    self.append(RestorableBookmarkEntity(bookmarkEntity: entity, index: nil, indexInFavoritesArray: nil))
+                }
+
+                if let folder = entity as? BookmarkFolder {
+                    removedFolderStack.append((folder, nil))
+                }
+            }
+        }
+    }
+}
+
+private extension UndoManager {
+
+    @MainActor
+    func registerUndoDeleteEntities(_ entities: [BaseBookmarkEntity], bookmarkManager: some BookmarkManager) {
+        let restorableEntities = [RestorableBookmarkEntity].init(entities: entities, bookmarkManager: bookmarkManager)
+        registerUndo(withTarget: bookmarkManager) { bookmarkManager in
+            bookmarkManager.restore(restorableEntities, undoManager: self)
+        }
+        if !isUndoing {
+            let actionName = if entities.count == 1 {
+                if entities[0].isFolder {
+                    UserText.deleteFolder
+                } else {
+                    UserText.deleteBookmark
+                }
+            } else {
+                UserText.mainMenuEditDelete
+            }
+            setActionName(actionName)
+        }
+    }
 }
 
 private extension String {
@@ -487,7 +705,6 @@ private extension String {
     private func removeAccents() -> String {
         // Normalize the string to NFD (Normalization Form Decomposition)
         let normalizedString = self as NSString
-        let range = NSRange(location: 0, length: normalizedString.length)
 
         // Apply the transform to remove diacritics
         let transformedString = normalizedString.applyingTransform(.toLatin, reverse: false) ?? ""