Address bookmarks feedback (#2411)

Task/Issue URL: https://app.asana.com/0/72649045549333/1206383230169466/f

**Description**:
This PR addresses feedback around bookmark editing and deleting.

DuckDuckGo/Assets.xcassets/Images/AddBookmark.imageset/Contents.json

@@ -1,7 +1,7 @@
 {
   "images" : [
     {
-      "filename" : "icon-16-bookmark-add.pdf",
+      "filename" : "AddBookmark.svg",
       "idiom" : "universal"
     }
   ],

DuckDuckGo/Assets.xcassets/Images/AddFolder.imageset/Contents.json

@@ -1,7 +1,7 @@
 {
   "images" : [
     {
-      "filename" : "icon-16-folder-add.pdf",
+      "filename" : "AddFolder.svg",
       "idiom" : "universal"
     }
   ],

DuckDuckGo/Assets.xcassets/Images/BookmarksFolder.imageset/Contents.json

@@ -0,0 +1,12 @@
+{
+  "images" : [
+    {
+      "filename" : "BookmarksFolder.svg",
+      "idiom" : "universal"
+    }
+  ],
+  "info" : {
+    "author" : "xcode",
+    "version" : 1
+  }
+}

DuckDuckGo/Assets.xcassets/Images/Chevron-Medium-Right-16.imageset/Contents.json

@@ -0,0 +1,15 @@
+{
+  "images" : [
+    {
+      "filename" : "Chevron-Medium-Right-16.pdf",
+      "idiom" : "universal"
+    }
+  ],
+  "info" : {
+    "author" : "xcode",
+    "version" : 1
+  },
+  "properties" : {
+    "template-rendering-intent" : "template"
+  }
+}

DuckDuckGo/Assets.xcassets/Images/Chevron-Next-16.imageset/Contents.json → DuckDuckGo/Assets.xcassets/Images/Trash.imageset/Contents.json

@@ -1,7 +1,7 @@
 {
   "images" : [
     {
-      "filename" : "Chevron-Next-16.pdf",
+      "filename" : "Trash.svg",
       "idiom" : "universal"
     }
   ],

DuckDuckGo/Bookmarks/Extensions/Bookmarks+Tab.swift

@@ -0,0 +1,41 @@
+//
+//  Bookmarks+Tab.swift
+//
+//  Copyright © 2024 DuckDuckGo. All rights reserved.
+//
+//  Licensed under the Apache License, Version 2.0 (the "License");
+//  you may not use this file except in compliance with the License.
+//  You may obtain a copy of the License at
+//
+//  http://www.apache.org/licenses/LICENSE-2.0
+//
+//  Unless required by applicable law or agreed to in writing, software
+//  distributed under the License is distributed on an "AS IS" BASIS,
+//  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+//  See the License for the specific language governing permissions and
+//  limitations under the License.
+//
+
+import Foundation
+
+extension Tab {
+
+    @MainActor
+    static func withContentOfBookmark(folder: BookmarkFolder, burnerMode: BurnerMode) -> [Tab] {
+        folder.children.compactMap { entity in
+            guard let url = (entity as? Bookmark)?.urlObject else { return nil }
+            return Tab(content: .url(url, source: .bookmark), shouldLoadInBackground: true, burnerMode: burnerMode)
+        }
+    }
+
+}
+
+extension TabCollection {
+
+    @MainActor
+    static func withContentOfBookmark(folder: BookmarkFolder, burnerMode: BurnerMode) -> TabCollection {
+        let tabs = Tab.withContentOfBookmark(folder: folder, burnerMode: burnerMode)
+        return TabCollection(tabs: tabs)
+    }
+
+}

DuckDuckGo/Bookmarks/Model/Bookmark.swift

@@ -19,7 +19,7 @@
 import Cocoa
 import Bookmarks
 
-internal class BaseBookmarkEntity: Identifiable {
+internal class BaseBookmarkEntity: Identifiable, Equatable, Hashable {
 
     static func singleEntity(with uuid: String) -> NSFetchRequest<BookmarkEntity> {
         let request = BookmarkEntity.fetchRequest()
@@ -99,6 +99,23 @@ internal class BaseBookmarkEntity: Identifiable {
         }
     }
 
+    // Subclasses needs to override to check equality on their properties
+    func isEqual(to instance: BaseBookmarkEntity) -> Bool {
+        id == instance.id &&
+        title == instance.title &&
+        isFolder == instance.isFolder
+    }
+
+    static func == (lhs: BaseBookmarkEntity, rhs: BaseBookmarkEntity) -> Bool {
+        return type(of: lhs) == type(of: rhs) && lhs.isEqual(to: rhs)
+    }
+
+    func hash(into hasher: inout Hasher) {
+        hasher.combine(id)
+        hasher.combine(title)
+        hasher.combine(isFolder)
+    }
+
 }
 
 final class BookmarkFolder: BaseBookmarkEntity {
@@ -139,6 +156,38 @@ final class BookmarkFolder: BaseBookmarkEntity {
 
         super.init(id: id, title: title, isFolder: true)
     }
+
+    override func isEqual(to instance: BaseBookmarkEntity) -> Bool {
+        guard let folder = instance as? BookmarkFolder else {
+            return false
+        }
+        return id == folder.id &&
+        title == folder.title &&
+        isFolder == folder.isFolder &&
+        isParentFolderEqual(lhs: parentFolderUUID, rhs: folder.parentFolderUUID) &&
+        children == folder.children
+    }
+
+    override func hash(into hasher: inout Hasher) {
+        hasher.combine(id)
+        hasher.combine(title)
+        hasher.combine(isFolder)
+        hasher.combine(parentFolderUUID)
+        hasher.combine(children)
+    }
+
+    // In some cases a bookmark folder that is child of the root folder has its `parentFolderUUID` set to `bookmarks_root`. In some other cases is nil. Making sure that comparing a `nil` and a `bookmarks_root` does not return false. Probably would be good idea to remove the optionality of `parentFolderUUID` in the future and set it to `bookmarks_root` when needed.
+    private func isParentFolderEqual(lhs: String?, rhs: String?) -> Bool {
+        switch (lhs, rhs) {
+        case (.none, .none):
+            return true
+        case (.some(let lhsValue), .some(let rhsValue)):
+            return lhsValue == rhsValue
+        case (.some(let value), .none), (.none, .some(let value)):
+            return value == "bookmarks_root"
+        }
+    }
+
 }
 
 final class Bookmark: BaseBookmarkEntity {
@@ -196,12 +245,33 @@ final class Bookmark: BaseBookmarkEntity {
                   parentFolderUUID: bookmark.parentFolderUUID)
     }
 
-}
+    convenience init(from bookmark: Bookmark, withNewUrl url: String, title: String, isFavorite: Bool) {
+        self.init(id: bookmark.id,
+                  url: url,
+                  title: title,
+                  isFavorite: isFavorite,
+                  parentFolderUUID: bookmark.parentFolderUUID)
+    }
 
-extension BaseBookmarkEntity: Equatable {
+    override func isEqual(to instance: BaseBookmarkEntity) -> Bool {
+        guard let bookmark = instance as? Bookmark else {
+            return false
+        }
+        return id == bookmark.id &&
+        title == bookmark.title &&
+        isFolder == bookmark.isFolder &&
+        url == bookmark.url &&
+        isFavorite == bookmark.isFavorite &&
+        parentFolderUUID == bookmark.parentFolderUUID
+    }
 
-    static func == (lhs: BaseBookmarkEntity, rhs: BaseBookmarkEntity) -> Bool {
-        return lhs.id == rhs.id
+    override func hash(into hasher: inout Hasher) {
+        hasher.combine(id)
+        hasher.combine(title)
+        hasher.combine(isFolder)
+        hasher.combine(url)
+        hasher.combine(isFavorite)
+        hasher.combine(parentFolderUUID)
     }
 
 }

DuckDuckGo/Bookmarks/Model/BookmarkFolderInfo.swift

@@ -0,0 +1,32 @@
+//
+//  BookmarkFolderInfo.swift
+//
+//  Copyright © 2024 DuckDuckGo. All rights reserved.
+//
+//  Licensed under the Apache License, Version 2.0 (the "License");
+//  you may not use this file except in compliance with the License.
+//  You may obtain a copy of the License at
+//
+//  http://www.apache.org/licenses/LICENSE-2.0
+//
+//  Unless required by applicable law or agreed to in writing, software
+//  distributed under the License is distributed on an "AS IS" BASIS,
+//  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+//  See the License for the specific language governing permissions and
+//  limitations under the License.
+//
+
+import Foundation
+
+protocol BookmarksEntityIdentifiable {
+    var entityId: String { get }
+    var parentId: String? { get }
+}
+
+struct BookmarkEntityInfo: Equatable, BookmarksEntityIdentifiable {
+    let entity: BaseBookmarkEntity
+    let parent: BookmarkFolder?
+
+    var entityId: String { entity.id }
+    var parentId: String? { parent?.id }
+}

DuckDuckGo/Bookmarks/Model/BookmarkList.swift

@@ -125,19 +125,39 @@ struct BookmarkList {
         }
     }
 
+    mutating func update(bookmark: Bookmark, newURL: String, newTitle: String, newIsFavorite: Bool) -> Bookmark? {
+        guard !bookmark.isFolder else { return nil }
+
+        let newBookmark = Bookmark(from: bookmark, withNewUrl: newURL, title: newTitle, isFavorite: newIsFavorite)
+        return updateBookmarkList(newBookmark: newBookmark, oldBookmark: bookmark)
+    }
+
     mutating func updateUrl(of bookmark: Bookmark, to newURL: String) -> Bookmark? {
         guard !bookmark.isFolder else { return nil }
 
         guard itemsDict[newURL] == nil else {
             os_log("BookmarkList: Update failed, new url already in bookmark list")
             return nil
         }
+
+        let newBookmark = Bookmark(from: bookmark, with: newURL)
+        return updateBookmarkList(newBookmark: newBookmark, oldBookmark: bookmark)
+    }
+
+    func bookmarks() -> [IdentifiableBookmark] {
+        return allBookmarkURLsOrdered
+    }
+
+}
+
+private extension BookmarkList {
+
+    mutating private func updateBookmarkList(newBookmark: Bookmark, oldBookmark bookmark: Bookmark) -> Bookmark? {
         guard itemsDict[bookmark.url] != nil, let index = allBookmarkURLsOrdered.firstIndex(of: IdentifiableBookmark(from: bookmark)) else {
             os_log("BookmarkList: Update failed, no such item in bookmark list")
             return nil
         }
 
-        let newBookmark = Bookmark(from: bookmark, with: newURL)
         let newIdentifiableBookmark = IdentifiableBookmark(from: newBookmark)
 
         allBookmarkURLsOrdered.remove(at: index)
@@ -147,13 +167,8 @@ struct BookmarkList {
         let updatedBookmarks = existingBookmarks.filter { $0.id != bookmark.id }
 
         itemsDict[bookmark.url] = updatedBookmarks
-        itemsDict[newURL] = (itemsDict[newURL] ?? []) + [bookmark]
+        itemsDict[newBookmark.url] = (itemsDict[newBookmark.url] ?? []) + [newBookmark]
 
         return newBookmark
     }
-
-    func bookmarks() -> [IdentifiableBookmark] {
-        return allBookmarkURLsOrdered
-    }
-
 }

DuckDuckGo/Bookmarks/Model/BookmarkManager.swift

@@ -35,7 +35,9 @@ protocol BookmarkManager: AnyObject {
     func remove(folder: BookmarkFolder)
     func remove(objectsWithUUIDs uuids: [String])
     func update(bookmark: Bookmark)
+    func update(bookmark: Bookmark, withURL url: URL, title: String, isFavorite: Bool)
     func update(folder: BookmarkFolder)
+    func update(folder: BookmarkFolder, andMoveToParent parent: ParentFolderType)
     @discardableResult func updateUrl(of bookmark: Bookmark, to newUrl: URL) -> Bookmark?
     func add(bookmark: Bookmark, to parent: BookmarkFolder?, completion: @escaping (Error?) -> Void)
     func add(objectsWithUUIDs uuids: [String], to parent: BookmarkFolder?, completion: @escaping (Error?) -> Void)
@@ -211,12 +213,35 @@ final class LocalBookmarkManager: BookmarkManager {
 
     }
 
+    func update(bookmark: Bookmark, withURL url: URL, title: String, isFavorite: Bool) {
+        guard list != nil else { return }
+        guard getBookmark(forUrl: bookmark.url) != nil else {
+            os_log("LocalBookmarkManager: Failed to update bookmark url - not in the list.", type: .error)
+            return
+        }
+
+        guard let newBookmark = list?.update(bookmark: bookmark, newURL: url.absoluteString, newTitle: title, newIsFavorite: isFavorite) else {
+            os_log("LocalBookmarkManager: Failed to update URL of bookmark.", type: .error)
+            return
+        }
+
+        bookmarkStore.update(bookmark: newBookmark)
+        loadBookmarks()
+        requestSync()
+    }
+
     func update(folder: BookmarkFolder) {
         bookmarkStore.update(folder: folder)
         loadBookmarks()
         requestSync()
     }
 
+    func update(folder: BookmarkFolder, andMoveToParent parent: ParentFolderType) {
+        bookmarkStore.update(folder: folder, andMoveToParent: parent)
+        loadBookmarks()
+        requestSync()
+    }
+
     func updateUrl(of bookmark: Bookmark, to newUrl: URL) -> Bookmark? {
         guard list != nil else { return nil }
         guard getBookmark(forUrl: bookmark.url) != nil else {