Add canGoBack() and canGoForward() into view state so it is accounted for (#302)

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.

Author: CDRussell

app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt

@@ -237,42 +237,42 @@ 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)
     }
 
     @Test
     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)
     }
 

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) {

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

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()
     }
 

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

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)