From 7100a73f7f3a5ba0f368c5af7c85a5c2004d928d Mon Sep 17 00:00:00 2001 From: Tomas Strba Date: Thu, 14 Apr 2022 16:48:44 +0200 Subject: [PATCH 1/3] Navigation bar buttons and their backforward list counts with error --- DuckDuckGo/Browser Tab/Model/Tab.swift | 11 ++- .../Browser Tab/ViewModel/TabViewModel.swift | 3 +- .../View/NavigationButtonMenuDelegate.swift | 78 +++++++++---------- .../ViewModel/BackForwardListItem.swift | 5 +- .../WKBackForwardListItemViewModel.swift | 14 +++- 5 files changed, 64 insertions(+), 47 deletions(-) diff --git a/DuckDuckGo/Browser Tab/Model/Tab.swift b/DuckDuckGo/Browser Tab/Model/Tab.swift index 4084dbec88..457cb67e3c 100644 --- a/DuckDuckGo/Browser Tab/Model/Tab.swift +++ b/DuckDuckGo/Browser Tab/Model/Tab.swift @@ -284,23 +284,28 @@ final class Tab: NSObject { } func goForward() { - guard self.canGoForward else { return } + guard canGoForward else { return } shouldStoreNextVisit = false webView.goForward() } var canGoBack: Bool { - webView.canGoBack + webView.canGoBack || error != nil } func goBack() { - guard self.canGoBack else { + guard canGoBack else { if canBeClosedWithBack { delegate?.closeTab(self) } return } + guard error == nil else { + webView.reload() + return + } + shouldStoreNextVisit = false webView.goBack() } diff --git a/DuckDuckGo/Browser Tab/ViewModel/TabViewModel.swift b/DuckDuckGo/Browser Tab/ViewModel/TabViewModel.swift index a6001f6776..261efe0564 100644 --- a/DuckDuckGo/Browser Tab/ViewModel/TabViewModel.swift +++ b/DuckDuckGo/Browser Tab/ViewModel/TabViewModel.swift @@ -114,6 +114,7 @@ final class TabViewModel { guard let self = self else { return } self.errorViewState.isVisible = self.tab.error != nil self.errorViewState.message = self.tab.error?.localizedDescription + self.updateCanGoBack() } .store(in: &cancellables) } @@ -135,7 +136,7 @@ final class TabViewModel { } func updateCanGoBack() { - canGoBack = tab.canGoBack || tab.canBeClosedWithBack + canGoBack = tab.canGoBack || tab.canBeClosedWithBack || tab.error != nil } private func updateCanBeBookmarked() { diff --git a/DuckDuckGo/Navigation Bar/View/NavigationButtonMenuDelegate.swift b/DuckDuckGo/Navigation Bar/View/NavigationButtonMenuDelegate.swift index 130cdd4e49..7cff7c7cb8 100644 --- a/DuckDuckGo/Navigation Bar/View/NavigationButtonMenuDelegate.swift +++ b/DuckDuckGo/Navigation Bar/View/NavigationButtonMenuDelegate.swift @@ -40,23 +40,17 @@ final class NavigationButtonMenuDelegate: NSObject { extension NavigationButtonMenuDelegate: NSMenuDelegate { func numberOfItems(in menu: NSMenu) -> Int { - if listItems.count > 1 { - return listItems.count - } else if tabCollectionViewModel.selectedTabViewModel?.tab.canBeClosedWithBack == true { - return 1 - } - return 0 + let listItems = listItems + + // Don't show menu if there is just the current item + if listItems.items.count == 0 || (listItems.items.count == 1 && listItems.currentIndex == 0) { return 0 } + + return listItems.items.count } func menu(_ menu: NSMenu, update item: NSMenuItem, at index: Int, shouldCancel: Bool) -> Bool { - let listItems = self.listItems - let listItem: BackForwardListItem - if listItems.count > 1, - let item = listItems[safe: index] { - listItem = .backForwardListItem(item) - } else if let parentTab = tabCollectionViewModel.selectedTabViewModel?.tab.parentTab { - listItem = .goBackToCloseItem(parentTab: parentTab) - } else { + let (listItems, currentIndex) = self.listItems + guard let listItem = listItems[safe: index] else { os_log("%s: Index out of bounds", type: .error, className) return true } @@ -64,29 +58,27 @@ extension NavigationButtonMenuDelegate: NSMenuDelegate { let listItemViewModel = WKBackForwardListItemViewModel(backForwardListItem: listItem, faviconManagement: FaviconManager.shared, historyCoordinating: HistoryCoordinator.shared, - isCurrentItem: listItems[safe: index] === currentListItem) + isCurrentItem: index == currentIndex) item.title = listItemViewModel.title item.image = listItemViewModel.image item.state = listItemViewModel.state item.target = self - item.action = listItemViewModel.isGoBackToCloseItem ? #selector(goBackAction(_:)) : #selector(menuItemAction(_:)) + item.action = #selector(menuItemAction(_:)) item.tag = index return true } @objc func menuItemAction(_ sender: NSMenuItem) { let index = sender.tag - let listItems = self.listItems - - guard index < listItems.count else { + let (listItems, currentIndex) = self.listItems + guard let listItem = listItems[safe: index] else { os_log("%s: Index out of bounds", type: .error, className) return } - let listItem = listItems[index] - guard listItem !== currentListItem else { + guard currentIndex != index else { // current item selected: do nothing return } @@ -95,37 +87,45 @@ extension NavigationButtonMenuDelegate: NSMenuDelegate { return } - selectedTabViewModel.tab.go(to: listItem) - } - - @objc func goBackAction(_: NSMenuItem) { - tabCollectionViewModel.selectedTabViewModel?.tab.goBack() + switch listItem { + case .backForwardListItem(let wkListItem): + selectedTabViewModel.tab.go(to: wkListItem) + case .goBackToCloseItem(parentTab:): + tabCollectionViewModel.selectedTabViewModel?.tab.goBack() + case .error: + break + } } - private var listItems: [WKBackForwardListItem] { + private var listItems: (items: [BackForwardListItem], currentIndex: Int?) { guard let selectedTabViewModel = tabCollectionViewModel.selectedTabViewModel else { os_log("%s: Selected tab view model is nil", type: .error, className) - return [] + return ([], nil) } let backForwardList = selectedTabViewModel.tab.webView.backForwardList - var list = buttonType == .back ? backForwardList.backList.reversed() : backForwardList.forwardList + let wkList = buttonType == .back ? backForwardList.backList.reversed() : backForwardList.forwardList + var list = wkList.map { BackForwardListItem.backForwardListItem($0) } + var currentIndex: Int? - guard let currentItem = selectedTabViewModel.tab.webView.backForwardList.currentItem else { - return list + // Add closing with back button to the list + if list.count == 0, let parentTab = selectedTabViewModel.tab.parentTab { + list.insert(.goBackToCloseItem(parentTab: parentTab), at: 0) } - list.insert(currentItem, at: 0) - return list - } + // Add current item to the list + if let currentItem = selectedTabViewModel.tab.webView.backForwardList.currentItem { + list.insert(.backForwardListItem(currentItem), at: 0) + currentIndex = 0 + } - private var currentListItem: WKBackForwardListItem? { - guard let selectedTabViewModel = tabCollectionViewModel.selectedTabViewModel else { - os_log("%s: Selected tab view model is nil", type: .error, className) - return nil + // Add error to the list + if selectedTabViewModel.tab.error != nil { + list.insert(.error, at: 0) + currentIndex = 0 } - return selectedTabViewModel.tab.webView.backForwardList.currentItem + return (list, currentIndex) } } diff --git a/DuckDuckGo/Navigation Bar/ViewModel/BackForwardListItem.swift b/DuckDuckGo/Navigation Bar/ViewModel/BackForwardListItem.swift index f38d0db239..af0c0289de 100644 --- a/DuckDuckGo/Navigation Bar/ViewModel/BackForwardListItem.swift +++ b/DuckDuckGo/Navigation Bar/ViewModel/BackForwardListItem.swift @@ -18,9 +18,10 @@ import WebKit -enum BackForwardListItem { +enum BackForwardListItem: Equatable { case backForwardListItem(WKBackForwardListItem) case goBackToCloseItem(parentTab: Tab) + case error var url: URL? { switch self { @@ -28,6 +29,8 @@ enum BackForwardListItem { return item.url case .goBackToCloseItem(parentTab: let tab): return tab.content.url + case .error: + return nil } } diff --git a/DuckDuckGo/Navigation Bar/ViewModel/WKBackForwardListItemViewModel.swift b/DuckDuckGo/Navigation Bar/ViewModel/WKBackForwardListItemViewModel.swift index 9d16849c94..2734d0a90d 100644 --- a/DuckDuckGo/Navigation Bar/ViewModel/WKBackForwardListItemViewModel.swift +++ b/DuckDuckGo/Navigation Bar/ViewModel/WKBackForwardListItemViewModel.swift @@ -60,10 +60,16 @@ final class WKBackForwardListItemViewModel { } else { return UserText.closeAndReturnToParent } + case .error: + return UserText.tabErrorTitle } } var image: NSImage? { + if case .error = backForwardListItem { + return nil + } + if backForwardListItem.url == .homePage { return NSImage(named: "HomeFavicon") } @@ -78,16 +84,18 @@ final class WKBackForwardListItemViewModel { } var state: NSControl.StateValue { - if case .backForwardListItem = backForwardListItem { - return isCurrentItem ? .on : .off + if case .goBackToCloseItem = backForwardListItem { + return .off } - return .off + + return isCurrentItem ? .on : .off } var isGoBackToCloseItem: Bool { if case .goBackToCloseItem = backForwardListItem { return true } + return false } From a758d1162bc341334c3e009bddc149dc7c66b2b2 Mon Sep 17 00:00:00 2001 From: Tomas Strba Date: Sun, 24 Apr 2022 22:02:57 +0200 Subject: [PATCH 2/3] Related issues raised in PR addresed --- DuckDuckGo/Browser Tab/ViewModel/TabViewModel.swift | 7 ++++++- .../ViewModel/WebViewStateObserver.swift | 4 ++-- DuckDuckGo/Common/View/AppKit/ProgressView.swift | 4 +++- .../Navigation Bar/View/AddressBarTextField.swift | 1 - .../View/NavigationButtonMenuDelegate.swift | 13 ++++++++++--- 5 files changed, 21 insertions(+), 8 deletions(-) diff --git a/DuckDuckGo/Browser Tab/ViewModel/TabViewModel.swift b/DuckDuckGo/Browser Tab/ViewModel/TabViewModel.swift index 261efe0564..f68c348670 100644 --- a/DuckDuckGo/Browser Tab/ViewModel/TabViewModel.swift +++ b/DuckDuckGo/Browser Tab/ViewModel/TabViewModel.swift @@ -34,7 +34,7 @@ final class TabViewModel { private var webViewStateObserver: WebViewStateObserver? - @Published var canGoForward: Bool = false + @Published private(set) var canGoForward: Bool = false @Published private(set) var canGoBack: Bool = false @Published private(set) var canReload: Bool = false @Published var canBeBookmarked: Bool = false @@ -115,6 +115,7 @@ final class TabViewModel { self.errorViewState.isVisible = self.tab.error != nil self.errorViewState.message = self.tab.error?.localizedDescription self.updateCanGoBack() + self.updateCanGoForward() } .store(in: &cancellables) } @@ -139,6 +140,10 @@ final class TabViewModel { canGoBack = tab.canGoBack || tab.canBeClosedWithBack || tab.error != nil } + func updateCanGoForward() { + canGoForward = tab.canGoForward && tab.error == nil + } + private func updateCanBeBookmarked() { canBeBookmarked = tab.content.url ?? .blankPage != .blankPage } diff --git a/DuckDuckGo/Browser Tab/ViewModel/WebViewStateObserver.swift b/DuckDuckGo/Browser Tab/ViewModel/WebViewStateObserver.swift index 01601af20e..0213373199 100644 --- a/DuckDuckGo/Browser Tab/ViewModel/WebViewStateObserver.swift +++ b/DuckDuckGo/Browser Tab/ViewModel/WebViewStateObserver.swift @@ -65,7 +65,7 @@ final class WebViewStateObserver: NSObject { } tabViewModel.updateCanGoBack() - tabViewModel.canGoForward = webView.canGoForward + tabViewModel.updateCanGoForward() tabViewModel.isWebViewLoading = webView.isLoading } @@ -98,7 +98,7 @@ final class WebViewStateObserver: NSObject { updateTitle() // The title might not change if webView doesn't think anything is different so update title here as well case #keyPath(WKWebView.canGoBack): tabViewModel.updateCanGoBack() - case #keyPath(WKWebView.canGoForward): tabViewModel.canGoForward = webView.canGoForward + case #keyPath(WKWebView.canGoForward): tabViewModel.updateCanGoForward() case #keyPath(WKWebView.isLoading): tabViewModel.isWebViewLoading = webView.isLoading case #keyPath(WKWebView.title): updateTitle() diff --git a/DuckDuckGo/Common/View/AppKit/ProgressView.swift b/DuckDuckGo/Common/View/AppKit/ProgressView.swift index 467950d836..622c379fc0 100644 --- a/DuckDuckGo/Common/View/AppKit/ProgressView.swift +++ b/DuckDuckGo/Common/View/AppKit/ProgressView.swift @@ -179,7 +179,9 @@ final class ProgressView: NSView, CAAnimationDelegate { if (animationDuration ?? 0) > 0 { CATransaction.commit() } else { - self.progressAnimationDidStop(finished: true) + DispatchQueue.main.async { + self.progressAnimationDidStop(finished: true) + } } } diff --git a/DuckDuckGo/Navigation Bar/View/AddressBarTextField.swift b/DuckDuckGo/Navigation Bar/View/AddressBarTextField.swift index 327c3e2f7e..3dda96174c 100644 --- a/DuckDuckGo/Navigation Bar/View/AddressBarTextField.swift +++ b/DuckDuckGo/Navigation Bar/View/AddressBarTextField.swift @@ -276,7 +276,6 @@ final class AddressBarTextField: NSTextField { Pixel.fire(.refresh(source: .reloadURL)) selectedTabViewModel.reload() } else { - Pixel.fire(.navigation(kind: .init(url: url), source: suggestion != nil ? .suggestion : .addressBar)) selectedTabViewModel.tab.update(url: url) } diff --git a/DuckDuckGo/Navigation Bar/View/NavigationButtonMenuDelegate.swift b/DuckDuckGo/Navigation Bar/View/NavigationButtonMenuDelegate.swift index 7cff7c7cb8..5f6af1f238 100644 --- a/DuckDuckGo/Navigation Bar/View/NavigationButtonMenuDelegate.swift +++ b/DuckDuckGo/Navigation Bar/View/NavigationButtonMenuDelegate.swift @@ -109,7 +109,9 @@ extension NavigationButtonMenuDelegate: NSMenuDelegate { var currentIndex: Int? // Add closing with back button to the list - if list.count == 0, let parentTab = selectedTabViewModel.tab.parentTab { + if list.count == 0, + let parentTab = selectedTabViewModel.tab.parentTab, + buttonType == .back { list.insert(.goBackToCloseItem(parentTab: parentTab), at: 0) } @@ -121,8 +123,13 @@ extension NavigationButtonMenuDelegate: NSMenuDelegate { // Add error to the list if selectedTabViewModel.tab.error != nil { - list.insert(.error, at: 0) - currentIndex = 0 + if buttonType == .back { + list.insert(.error, at: 0) + currentIndex = 0 + } else { + list = [] + currentIndex = nil + } } return (list, currentIndex) From 4ea9711cde0c375ef98de63955c5098664e3e2fe Mon Sep 17 00:00:00 2001 From: Tomas Strba Date: Mon, 2 May 2022 15:39:39 +0200 Subject: [PATCH 3/3] Fix of the error message overflow if the window size is small --- .../View/Base.lproj/BrowserTab.storyboard | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/DuckDuckGo/Browser Tab/View/Base.lproj/BrowserTab.storyboard b/DuckDuckGo/Browser Tab/View/Base.lproj/BrowserTab.storyboard index 3ab6fd3b61..f23dec6352 100644 --- a/DuckDuckGo/Browser Tab/View/Base.lproj/BrowserTab.storyboard +++ b/DuckDuckGo/Browser Tab/View/Base.lproj/BrowserTab.storyboard @@ -12,29 +12,29 @@ - + - +