From 194e6ec13dcd808f72e8d4341bf28e353eb29e84 Mon Sep 17 00:00:00 2001 From: Craig Russell Date: Mon, 2 Jul 2018 17:39:39 +0100 Subject: [PATCH] Add canGoBack() and canGoForward() into view state so it is accounted for A regression meant that the view state was checked to see if it was identical to the last seen view state. If it was, rendering would skip. But for these forward/back buttons, they also relied on WebView state; not just the view state. So we were erroneously skipping updates for them because the view state was identical. The fix is to include these in the view state itself, meaning it won't skip the rendering if canGoBack() or canGoForward() has changed. onProgressChanged is the earliest place where the webView starts reporting the correct new states. But I've included the update in loadingFinished() too just in case onProgressChanged could be bypassed somehow. --- .../app/browser/BrowserTabViewModelTest.kt | 18 +++++++++--------- .../app/browser/BrowserChromeClient.kt | 6 ++++-- .../app/browser/BrowserTabFragment.kt | 4 ++-- .../app/browser/BrowserTabViewModel.kt | 10 +++++++--- .../app/browser/BrowserWebViewClient.kt | 9 ++++++--- .../app/browser/WebViewClientListener.kt | 4 ++-- 6 files changed, 30 insertions(+), 21 deletions(-) diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt index 86f2f5b0c028..4d85c6d59e34 100644 --- a/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt @@ -237,28 +237,28 @@ class BrowserTabViewModelTest { @Test fun whenViewModelNotifiedThatWebViewHasFinishedLoadingThenViewStateIsUpdated() { - testee.loadingFinished() + testee.loadingFinished(null, false, false) assertFalse(loadingViewState().isLoading) } @Test fun whenLoadingFinishedWithUrlThenSiteVisitedEntryAddedToLeaderboardDao() { testee.url.value = "http://example.com/abc" - testee.loadingFinished() + testee.loadingFinished(null, false, false) verify(mockNetworkLeaderboardDao).insert(SiteVisitedEntity("example.com")) } @Test fun whenLoadingFinishedWithUrlThenOmnibarTextUpdatedToMatch() { val exampleUrl = "http://example.com/abc" - testee.loadingFinished(exampleUrl) + testee.loadingFinished(exampleUrl, false, false) assertEquals(exampleUrl, omnibarViewState().omnibarText) } @Test fun whenLoadingFinishedWithQueryUrlThenOmnibarTextUpdatedToShowQuery() { val queryUrl = "http://duckduckgo.com?q=test" - testee.loadingFinished(queryUrl) + testee.loadingFinished(queryUrl, false, false) assertEquals("test", omnibarViewState().omnibarText) } @@ -266,13 +266,13 @@ class BrowserTabViewModelTest { fun whenLoadingFinishedWithNoUrlThenOmnibarTextUpdatedToMatch() { val exampleUrl = "http://example.com/abc" testee.urlChanged(exampleUrl) - testee.loadingFinished(null) + testee.loadingFinished(null, false, false) assertEquals(exampleUrl, omnibarViewState().omnibarText) } @Test fun whenLoadingFinishedWithNoUrlThenSiteVisitedEntryNotAddedToLeaderboardDao() { - testee.loadingFinished() + testee.loadingFinished(null, false, false) verify(mockNetworkLeaderboardDao, never()).insert(SiteVisitedEntity("example.com")) } @@ -331,13 +331,13 @@ class BrowserTabViewModelTest { @Test fun whenViewModelGetsProgressUpdateThenViewStateIsUpdated() { - testee.progressChanged(0) + testee.progressChanged(0, false, false) assertEquals(0, loadingViewState().progress) - testee.progressChanged(50) + testee.progressChanged(50, false, false) assertEquals(50, loadingViewState().progress) - testee.progressChanged(100) + testee.progressChanged(100, false, false) assertEquals(100, loadingViewState().progress) } diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserChromeClient.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserChromeClient.kt index 393099ed73a8..cc0664158be0 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserChromeClient.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserChromeClient.kt @@ -50,8 +50,10 @@ class BrowserChromeClient @Inject constructor() : WebChromeClient() { customView = null } - override fun onProgressChanged(view: WebView, newProgress: Int) { - webViewClientListener?.progressChanged(newProgress) + override fun onProgressChanged(webView: WebView, newProgress: Int) { + val canGoBack = webView.canGoBack() + val canGoForward = webView.canGoForward() + webViewClientListener?.progressChanged(newProgress, canGoBack, canGoForward) } override fun onReceivedTitle(view: WebView, title: String) { diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt index 1d2bf409e5af..36d7e5074dbc 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt @@ -817,8 +817,8 @@ class BrowserTabFragment : Fragment(), FindListener { private fun renderPopupMenus(browserShowing: Boolean, viewState: BrowserViewState) { popupMenu.contentView.apply { - backPopupMenuItem.isEnabled = browserShowing && webView?.canGoBack() ?: false - forwardPopupMenuItem.isEnabled = browserShowing && webView?.canGoForward() ?: false + backPopupMenuItem.isEnabled = browserShowing && viewState.canGoBack + forwardPopupMenuItem.isEnabled = browserShowing && viewState.canGoForward refreshPopupMenuItem.isEnabled = browserShowing newTabPopupMenuItem.isEnabled = browserShowing addBookmarksPopupMenuItem?.isEnabled = viewState.canAddBookmarks diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt index 7012fd550e27..8222634d09a9 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt @@ -90,7 +90,9 @@ class BrowserTabViewModel( val showFireButton: Boolean = true, val showMenuButton: Boolean = true, val canSharePage: Boolean = false, - val canAddBookmarks: Boolean = false + val canAddBookmarks: Boolean = false, + val canGoBack: Boolean = false, + val canGoForward: Boolean = false ) data class OmnibarViewState( @@ -244,11 +246,12 @@ class BrowserTabViewModel( autoCompleteViewState.value = AutoCompleteViewState(false) } - override fun progressChanged(newProgress: Int) { + override fun progressChanged(newProgress: Int, canGoBack: Boolean, canGoForward: Boolean) { Timber.v("Loading in progress $newProgress") val progress = currentLoadingViewState() loadingViewState.value = progress.copy(progress = newProgress) + browserViewState.value = currentBrowserViewState().copy(canGoBack = canGoBack, canGoForward = canGoForward) } override fun goFullScreen(view: View) { @@ -271,7 +274,7 @@ class BrowserTabViewModel( onSiteChanged() } - override fun loadingFinished(url: String?) { + override fun loadingFinished(url: String?, canGoBack: Boolean, canGoForward: Boolean) { Timber.v("Loading finished") val currentOmnibarViewState = currentOmnibarViewState() @@ -281,6 +284,7 @@ class BrowserTabViewModel( loadingViewState.value = currentLoadingViewState.copy(isLoading = false) omnibarViewState.value = currentOmnibarViewState.copy(omnibarText = omnibarText) + browserViewState.value = currentBrowserViewState().copy(canGoBack = canGoBack, canGoForward = canGoForward) registerSiteVisit() } diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt index 47a71f11287f..81473f8631cb 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt @@ -75,14 +75,17 @@ class BrowserWebViewClient @Inject constructor( } } - override fun onPageStarted(view: WebView?, url: String?, favicon: Bitmap?) { + override fun onPageStarted(webView: WebView, url: String?, favicon: Bitmap?) { currentUrl = url webViewClientListener?.loadingStarted() webViewClientListener?.urlChanged(url) } - override fun onPageFinished(view: WebView?, url: String?) { - webViewClientListener?.loadingFinished(url) + override fun onPageFinished(webView: WebView, url: String?) { + val canGoBack = webView.canGoBack() + val canGoForward = webView.canGoForward() + + webViewClientListener?.loadingFinished(url, canGoBack, canGoForward) } @WorkerThread diff --git a/app/src/main/java/com/duckduckgo/app/browser/WebViewClientListener.kt b/app/src/main/java/com/duckduckgo/app/browser/WebViewClientListener.kt index 46a46b597070..a776e65ed6ea 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/WebViewClientListener.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/WebViewClientListener.kt @@ -24,8 +24,8 @@ import com.duckduckgo.app.trackerdetection.model.TrackingEvent interface WebViewClientListener { fun loadingStarted() - fun loadingFinished(url: String? = null) - fun progressChanged(newProgress: Int) + fun loadingFinished(url: String? = null, canGoBack: Boolean, canGoForward: Boolean) + fun progressChanged(newProgress: Int, canGoBack: Boolean, canGoForward: Boolean) fun titleReceived(title: String) fun urlChanged(url: String?) fun trackerDetected(event: TrackingEvent)