Skip to content

Commit

Permalink
Url variants for bookmark icon (#3341)
Browse files Browse the repository at this point in the history
Task/Issue URL:
https://app.asana.com/0/1207340338530322/1205619096931465/f

**Description**:
The address bar bookmark icon now indicates that a URL is bookmarked,
even if its variants such as different HTTP/HTTPS schemes or trailing
slashes are bookmarked.
  • Loading branch information
tomasstrba authored Oct 10, 2024
1 parent 90f521d commit aaea291
Show file tree
Hide file tree
Showing 8 changed files with 290 additions and 6 deletions.
12 changes: 12 additions & 0 deletions DuckDuckGo.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,10 @@
1D43EB38292B636E0065E5D6 /* BWCommand.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1D43EB37292B636E0065E5D6 /* BWCommand.swift */; };
1D43EB3A292B63B00065E5D6 /* BWRequest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1D43EB39292B63B00065E5D6 /* BWRequest.swift */; };
1D43EB3C292B664A0065E5D6 /* BWMessageIdGenerator.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1D43EB3B292B664A0065E5D6 /* BWMessageIdGenerator.swift */; };
1D4B03D62CA4432000224E99 /* BookmarkUrlExtension.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1D4B03D52CA4431C00224E99 /* BookmarkUrlExtension.swift */; };
1D4B03D72CA4432000224E99 /* BookmarkUrlExtension.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1D4B03D52CA4431C00224E99 /* BookmarkUrlExtension.swift */; };
1D4B03D92CA55DDF00224E99 /* BookmarkUrlExtensionTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1D4B03D82CA55DDF00224E99 /* BookmarkUrlExtensionTests.swift */; };
1D4B03DA2CA55DDF00224E99 /* BookmarkUrlExtensionTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1D4B03D82CA55DDF00224E99 /* BookmarkUrlExtensionTests.swift */; };
1D6216B229069BBF00386B2C /* BWKeyStorage.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1D6216B129069BBF00386B2C /* BWKeyStorage.swift */; };
1D638D612C44F2BA00530DD5 /* ApplicationUpdateDetectorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1D638D602C44F2BA00530DD5 /* ApplicationUpdateDetectorTests.swift */; };
1D638D622C44F2BA00530DD5 /* ApplicationUpdateDetectorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1D638D602C44F2BA00530DD5 /* ApplicationUpdateDetectorTests.swift */; };
Expand Down Expand Up @@ -3232,6 +3236,8 @@
1D43EB37292B636E0065E5D6 /* BWCommand.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BWCommand.swift; sourceTree = "<group>"; };
1D43EB39292B63B00065E5D6 /* BWRequest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BWRequest.swift; sourceTree = "<group>"; };
1D43EB3B292B664A0065E5D6 /* BWMessageIdGenerator.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BWMessageIdGenerator.swift; sourceTree = "<group>"; };
1D4B03D52CA4431C00224E99 /* BookmarkUrlExtension.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BookmarkUrlExtension.swift; sourceTree = "<group>"; };
1D4B03D82CA55DDF00224E99 /* BookmarkUrlExtensionTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = BookmarkUrlExtensionTests.swift; sourceTree = "<group>"; };
1D6216B129069BBF00386B2C /* BWKeyStorage.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BWKeyStorage.swift; sourceTree = "<group>"; };
1D638D602C44F2BA00530DD5 /* ApplicationUpdateDetectorTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = ApplicationUpdateDetectorTests.swift; sourceTree = "<group>"; };
1D69C552291302F200B75945 /* BWVault.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BWVault.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -7613,6 +7619,7 @@
4B9292B12667103000AD2C21 /* BookmarkNodeTests.swift */,
4B9292B32667103000AD2C21 /* BookmarkOutlineViewDataSourceTests.swift */,
4B9292B22667103000AD2C21 /* BookmarkSidebarTreeControllerTests.swift */,
1D4B03D82CA55DDF00224E99 /* BookmarkUrlExtensionTests.swift */,
4B9292B82667103000AD2C21 /* BookmarkTests.swift */,
4B9292B92667103100AD2C21 /* PasteboardBookmarkTests.swift */,
4B9292B42667103000AD2C21 /* PasteboardFolderTests.swift */,
Expand Down Expand Up @@ -8135,6 +8142,7 @@
4B92929426670D2A00AD2C21 /* BookmarkSidebarTreeController.swift */,
BB7B5F972C4ED73800BA4AF8 /* BookmarksSearchAndSortMetrics.swift */,
4B92929726670D2A00AD2C21 /* BookmarkTreeController.swift */,
1D4B03D52CA4431C00224E99 /* BookmarkUrlExtension.swift */,
841BE93A2C6F1CB200E9C2B5 /* MenuItemNode.swift */,
4B92929526670D2A00AD2C21 /* PasteboardBookmark.swift */,
4B92929226670D2A00AD2C21 /* PasteboardFolder.swift */,
Expand Down Expand Up @@ -10924,6 +10932,7 @@
9F56CFAA2B82DC4300BB7F11 /* AddEditBookmarkFolderView.swift in Sources */,
567A23D82C8871290010F66C /* OnboardingFireButtonDialogViewModel.swift in Sources */,
37FD78122A29EBD100B36DB1 /* SyncErrorHandler.swift in Sources */,
1D4B03D62CA4432000224E99 /* BookmarkUrlExtension.swift in Sources */,
987799F42999993C005D8EB6 /* LegacyBookmarksStoreMigration.swift in Sources */,
3706FB7A293F65D500E42796 /* FileDownloadManager.swift in Sources */,
3706FB7B293F65D500E42796 /* BookmarkImport.swift in Sources */,
Expand Down Expand Up @@ -11724,6 +11733,7 @@
3706FE72293F661700E42796 /* ClickToLoadTDSTests.swift in Sources */,
3706FE73293F661700E42796 /* PermissionManagerMock.swift in Sources */,
3706FE74293F661700E42796 /* WebsiteDataStoreMock.swift in Sources */,
1D4B03DA2CA55DDF00224E99 /* BookmarkUrlExtensionTests.swift in Sources */,
3706FE75293F661700E42796 /* WebsiteBreakageReportTests.swift in Sources */,
56D145EF29E6DAD900E3488A /* DataImportProviderTests.swift in Sources */,
569277C529DEE09D00B633EF /* ContinueSetUpModelTests.swift in Sources */,
Expand Down Expand Up @@ -12130,6 +12140,7 @@
4B39AAF627D9B2C700A73FD5 /* NSStackViewExtension.swift in Sources */,
B66CA41E2AD910B300447CF0 /* DataImportView.swift in Sources */,
4BE65477271FCD41008D1D63 /* PasswordManagementLoginItemView.swift in Sources */,
1D4B03D72CA4432000224E99 /* BookmarkUrlExtension.swift in Sources */,
85D0327B2B8E3D090041D1FB /* HistoryCoordinatorExtension.swift in Sources */,
AA80EC54256BE3BC007083E7 /* UserText.swift in Sources */,
B61EF3EC266F91E700B4D78F /* WKWebView+Download.swift in Sources */,
Expand Down Expand Up @@ -13251,6 +13262,7 @@
9FAD623D2BD09DE5007F3A65 /* WebsiteInfoTests.swift in Sources */,
9F180D0F2B69C553000D695F /* Tab+WKUIDelegateTests.swift in Sources */,
4BA1A6E6258C270800F6F690 /* EncryptionKeyGeneratorTests.swift in Sources */,
1D4B03D92CA55DDF00224E99 /* BookmarkUrlExtensionTests.swift in Sources */,
B6106BB326A7F4AA0013B453 /* GeolocationServiceMock.swift in Sources */,
4B8AC93D26B49BE600879451 /* FirefoxLoginReaderTests.swift in Sources */,
3714B1E728EDB7FA0056C57A /* DuckPlayerPreferencesTests.swift in Sources */,
Expand Down
27 changes: 27 additions & 0 deletions DuckDuckGo/Bookmarks/Model/BookmarkManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,12 @@ import os.log
protocol BookmarkManager: AnyObject {

func isUrlBookmarked(url: URL) -> Bool
func isAnyUrlVariantBookmarked(url: URL) -> Bool
func isUrlFavorited(url: URL) -> Bool
func allHosts() -> Set<String>
func getBookmark(for url: URL) -> Bookmark?
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?
Expand Down Expand Up @@ -143,6 +145,11 @@ final class LocalBookmarkManager: BookmarkManager {
return list?[url.absoluteString] != nil
}

/// Checks if any variant of the given URL (http/https, trailing slash) is bookmarked.
func isAnyUrlVariantBookmarked(url: URL) -> Bool {
findBookmark(forVariantUrl: url) != nil
}

func isUrlFavorited(url: URL) -> Bool {
return list?[url.absoluteString]?.isFavorite == true
}
Expand All @@ -159,6 +166,24 @@ final class LocalBookmarkManager: BookmarkManager {
return list?[url]
}

/// Returns the bookmark for the given URL or any of its variants (http/https, trailing slash), if it exists.
func getBookmark(forVariantUrl variantURL: URL) -> Bookmark? {
findBookmark(forVariantUrl: variantURL)
}

/// Finds a bookmark by checking all possible URL variants (http/https, trailing slash).
private func findBookmark(forVariantUrl url: URL) -> Bookmark? {
let urlVariants = url.bookmarkButtonUrlVariants()

for variant in urlVariants {
if let bookmark = getBookmark(for: variant) {
return bookmark
}
}

return nil
}

func getBookmarkFolder(withId id: String) -> BookmarkFolder? {
bookmarkStore.bookmarkFolder(withId: id)
}
Expand All @@ -168,6 +193,8 @@ final class LocalBookmarkManager: BookmarkManager {
}

@discardableResult func makeBookmark(for url: URL, title: String, isFavorite: Bool, index: Int? = nil, parent: BookmarkFolder? = nil) -> Bookmark? {
assert(url.absoluteString.lowercased() == url.absoluteString)

guard list != nil else { return nil }

guard !isUrlBookmarked(url: url) else {
Expand Down
64 changes: 64 additions & 0 deletions DuckDuckGo/Bookmarks/Model/BookmarkUrlExtension.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
//
// BookmarkUrlExtension.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.
//

extension URL {

// Generates all possible URL variants for bookmark comparison, including http/https and optional trailing slashes.
// Variants are only created for URLs with http/https schemes, and trailing slashes are added only if there are no query or fragment components.
func bookmarkButtonUrlVariants() -> [URL] {
var baseUrlString = self.absoluteString

guard let scheme = self.scheme.map(NavigationalScheme.init),
scheme.isHypertextScheme else {
return [self]
}

baseUrlString = baseUrlString.replacingOccurrences(of: "\(scheme.separated())", with: "")

// Generate variants without trailing slash
let withoutTrailingSlash = baseUrlString.trimmingCharacters(in: CharacterSet(charactersIn: "/"))

// Only append trailing slash if there are no query or fragment components
let shouldAddTrailingSlash = self.query == nil && self.fragment == nil
let withTrailingSlash = shouldAddTrailingSlash ? withoutTrailingSlash + "/" : withoutTrailingSlash

// Create variants
let httpVariant = NavigationalScheme.http.separated() + withoutTrailingSlash
let httpsVariant = NavigationalScheme.https.separated() + withoutTrailingSlash
let httpVariantWithSlash = NavigationalScheme.http.separated() + withTrailingSlash
let httpsVariantWithSlash = NavigationalScheme.https.separated() + withTrailingSlash
let variants: [URL?] = [
self,
URL(string: httpVariant),
URL(string: httpsVariant),
shouldAddTrailingSlash ? URL(string: httpVariantWithSlash) : nil,
shouldAddTrailingSlash ? URL(string: httpsVariantWithSlash) : nil
]

var seen = Set<String>()
return variants.compactMap { variant in
guard let url = variant else { return nil }
let normalizedUrl = url.absoluteString.lowercased()
if seen.contains(normalizedUrl) {
return nil // Skip if already added
}
seen.insert(normalizedUrl)
return url
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ final class AddEditBookmarkDialogViewModel: BookmarkDialogEditing {
}

func addOrSave(dismiss: () -> Void) {
guard let url = bookmarkURLPath.url else {
guard let url = bookmarkURLPath.lowercased().url else {
assertionFailure("Invalid URL, default action button should be disabled.")
return
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,9 +257,13 @@ final class AddressBarButtonsViewController: NSViewController {
guard let tabViewModel, tabViewModel.canBeBookmarked else { return false }

var isUrlBookmarked = false
if let url = tabViewModel.tab.content.userEditableUrl,
bookmarkManager.isUrlBookmarked(url: url) {
isUrlBookmarked = true
if let url = tabViewModel.tab.content.userEditableUrl {
let urlVariants = url.bookmarkButtonUrlVariants()

// Check if any of the URL variants is bookmarked
isUrlBookmarked = urlVariants.contains { variant in
return bookmarkManager.isUrlBookmarked(url: variant)
}
}

return clearButton.isHidden && !hasEmptyAddressBar && (isMouseOverNavigationBar || popovers.isEditBookmarkPopoverShown || isUrlBookmarked)
Expand Down Expand Up @@ -688,7 +692,7 @@ final class AddressBarButtonsViewController: NSViewController {

private func updateBookmarkButtonImage(isUrlBookmarked: Bool = false) {
if let url = tabViewModel?.tab.content.userEditableUrl,
isUrlBookmarked || bookmarkManager.isUrlBookmarked(url: url)
isUrlBookmarked || bookmarkManager.isAnyUrlVariantBookmarked(url: url)
{
bookmarkButton.image = .bookmarkFilled
bookmarkButton.mouseOverTintColor = NSColor.bookmarkFilledTint
Expand Down Expand Up @@ -896,7 +900,7 @@ final class AddressBarButtonsViewController: NSViewController {
return (nil, false)
}

if let bookmark = bookmarkManager.getBookmark(forUrl: url.absoluteString) {
if let bookmark = bookmarkManager.getBookmark(forVariantUrl: url) {
if setFavorite {
bookmark.isFavorite = true
bookmarkManager.update(bookmark: bookmark)
Expand Down
113 changes: 113 additions & 0 deletions UnitTests/Bookmarks/Model/BookmarkUrlExtensionTests.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
//
// BookmarkUrlExtensionTests.swift
//
// Copyright © 2021 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 Combine
import Foundation

import XCTest
@testable import DuckDuckGo_Privacy_Browser
final class BookmarkUrlExtensionTests: XCTestCase {

func testWhenUrlIsHttpWithTrailingSlash_ThenCorrectVariantsAreGenerated() {
let originalURL = URL(string: "http://example.com/")!

let variants = originalURL.bookmarkButtonUrlVariants()

XCTAssertEqual(variants.count, 4)
XCTAssertEqual(variants[0], originalURL)
XCTAssertTrue(variants.contains(URL(string: "http://example.com")!))
XCTAssertTrue(variants.contains(URL(string: "https://example.com")!))
XCTAssertTrue(variants.contains(URL(string: "https://example.com/")!))
}

func testWhenUrlIsHttpsWithoutTrailingSlash_ThenCorrectVariantsAreGenerated() {
let originalURL = URL(string: "https://example.com")!

let variants = originalURL.bookmarkButtonUrlVariants()

XCTAssertEqual(variants.count, 4)
XCTAssertEqual(variants[0], originalURL)
XCTAssertTrue(variants.contains(URL(string: "http://example.com")!))
XCTAssertTrue(variants.contains(URL(string: "http://example.com/")!))
XCTAssertTrue(variants.contains(URL(string: "https://example.com/")!))
}

func testWhenUrlIsHttpWithoutTrailingSlash_ThenCorrectVariantsAreGenerated() {
let originalURL = URL(string: "http://example.com")!

let variants = originalURL.bookmarkButtonUrlVariants()

XCTAssertEqual(variants.count, 4)
XCTAssertEqual(variants[0], originalURL)
XCTAssertTrue(variants.contains(URL(string: "https://example.com")!))
XCTAssertTrue(variants.contains(URL(string: "http://example.com/")!))
XCTAssertTrue(variants.contains(URL(string: "https://example.com/")!))
}

func testWhenUrlIsHttpsWithTrailingSlash_ThenCorrectVariantsAreGenerated() {
let originalURL = URL(string: "https://example.com/")!

let variants = originalURL.bookmarkButtonUrlVariants()

XCTAssertEqual(variants.count, 4)
XCTAssertEqual(variants[0], originalURL)
XCTAssertTrue(variants.contains(URL(string: "http://example.com")!))
XCTAssertTrue(variants.contains(URL(string: "https://example.com")!))
XCTAssertTrue(variants.contains(URL(string: "http://example.com/")!))
}

func testWhenUrlHasNoScheme_ThenNoVariantsGenerated() {
let originalURL = URL(string: "example.com")!

let variants = originalURL.bookmarkButtonUrlVariants()

XCTAssertEqual(variants.count, 1)
XCTAssertEqual(variants[0], originalURL)
}

func testWhenUrlHasNonHttpOrHttpsScheme_ThenOnlyOriginalUrlIsReturned() {
let originalURL = URL(string: "file:///Users/example/file.txt")!

let variants = originalURL.bookmarkButtonUrlVariants()

XCTAssertEqual(variants.count, 1)
XCTAssertEqual(variants[0], originalURL)
}

func testWhenUrlHasQueryParameters_ThenVariantsAreGeneratedWithoutTrailingSlashes() {
let originalURL = URL(string: "https://example.com/path/to/resource?query=value")!

let variants = originalURL.bookmarkButtonUrlVariants()

XCTAssertEqual(variants.count, 2)
XCTAssertEqual(variants[0], originalURL)
XCTAssertTrue(variants.contains(URL(string: "http://example.com/path/to/resource?query=value")!))
}

func testWhenUrlHasNoQueryParameters_ThenVariantsIncludeTrailingSlashes() {
let originalURL = URL(string: "https://example.com/path/to/resource")!

let variants = originalURL.bookmarkButtonUrlVariants()

XCTAssertEqual(variants.count, 4)
XCTAssertEqual(variants[0], originalURL)
XCTAssertTrue(variants.contains(URL(string: "http://example.com/path/to/resource")!))
XCTAssertTrue(variants.contains(URL(string: "http://example.com/path/to/resource/")!))
XCTAssertTrue(variants.contains(URL(string: "https://example.com/path/to/resource/")!))
}
}
Loading

0 comments on commit aaea291

Please sign in to comment.