From 6b309eda561f4469b27971524327a2327bc725a2 Mon Sep 17 00:00:00 2001 From: Craig Russell Date: Mon, 1 Jun 2020 10:44:02 +0100 Subject: [PATCH 1/3] Ensure overflow menu state in sync when navigating back home --- .../app/browser/BrowserTabViewModelTest.kt | 56 +++++++++++++++++++ .../app/browser/BrowserStateModifier.kt | 5 ++ .../app/browser/BrowserTabViewModel.kt | 7 +++ 3 files changed, 68 insertions(+) create mode 100644 app/src/main/java/com/duckduckgo/app/browser/BrowserStateModifier.kt 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 618f1ba0ade1..a5b08708767f 100644 --- a/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt @@ -1831,6 +1831,62 @@ class BrowserTabViewModelTest { verify(mockPixel).fire(Pixel.PixelName.FIREPROOF_WEBSITE_UNDO) } + @Test + fun whenUserBrowsingPressesBackThenCannotAddBookmark() { + setupNavigation(skipHome = false, isBrowsing = true, canGoBack = false) + assertTrue(testee.onUserPressedBack()) + assertFalse(browserViewState().canAddBookmarks) + } + + @Test + fun whenUserBrowsingPressesBackThenCannotSharePage() { + setupNavigation(skipHome = false, isBrowsing = true, canGoBack = false) + assertTrue(testee.onUserPressedBack()) + assertFalse(browserViewState().canSharePage) + } + + @Test + fun whenUserBrowsingPressesBackThenCannotReportSite() { + setupNavigation(skipHome = false, isBrowsing = true, canGoBack = false) + assertTrue(testee.onUserPressedBack()) + assertFalse(browserViewState().canReportSite) + } + + @Test + fun whenUserBrowsingPressesBackThenCannotAddToHome() { + setupNavigation(skipHome = false, isBrowsing = true, canGoBack = false) + assertTrue(testee.onUserPressedBack()) + assertFalse(browserViewState().addToHomeEnabled) + } + + @Test + fun whenUserBrowsingPressesBackThenCannotWhitelist() { + setupNavigation(skipHome = false, isBrowsing = true, canGoBack = false) + assertTrue(testee.onUserPressedBack()) + assertFalse(browserViewState().canWhitelist) + } + + @Test + fun whenUserBrowsingPressesBackThenCannotNavigateBack() { + setupNavigation(skipHome = false, isBrowsing = true, canGoBack = false) + assertTrue(testee.onUserPressedBack()) + assertFalse(browserViewState().canGoBack) + } + + @Test + fun whenUserBrowsingPressesBackThenCannotFindInPage() { + setupNavigation(skipHome = false, isBrowsing = true, canGoBack = false) + assertTrue(testee.onUserPressedBack()) + assertFalse(findInPageViewState().canFindInPage) + } + + @Test + fun whenUserBrowsingPressesBackThenCanGoForward() { + setupNavigation(skipHome = false, isBrowsing = true, canGoBack = false) + assertTrue(testee.onUserPressedBack()) + assertTrue(browserViewState().canGoForward) + } + private inline fun assertCommandIssued(instanceAssertions: T.() -> Unit = {}) { verify(mockCommandObserver, atLeastOnce()).onChanged(commandCaptor.capture()) val issuedCommand = commandCaptor.allValues.find { it is T } diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserStateModifier.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserStateModifier.kt new file mode 100644 index 000000000000..e21d02d042b7 --- /dev/null +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserStateModifier.kt @@ -0,0 +1,5 @@ +package com.duckduckgo.app.browser + + +interface BrowserStateModifier { +} \ No newline at end of file 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 23a6c14ba38b..c5a13dbb3152 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt @@ -471,8 +471,15 @@ class BrowserTabViewModel( browserShowing = false, canGoBack = false, canFireproofSite = false, + canSharePage = false, + canAddBookmarks = false, + canReportSite = false, + addToHomeEnabled = false, + canWhitelist = false, canGoForward = currentGlobalLayoutState() !is Invalidated ) + + findInPageViewState.value = FindInPageViewState() omnibarViewState.value = currentOmnibarViewState().copy(omnibarText = "", shouldMoveCaretToEnd = false) loadingViewState.value = currentLoadingViewState().copy(isLoading = false) From 18c26caf214adfc0938f0c586c58750cc3f49411 Mon Sep 17 00:00:00 2001 From: Craig Russell Date: Mon, 1 Jun 2020 10:56:33 +0100 Subject: [PATCH 2/3] Fix forward navigation --- .../app/browser/BrowserTabViewModelTest.kt | 48 ++++++++++++++++++ .../app/browser/BrowserStateModifier.kt | 49 ++++++++++++++++++- .../app/browser/BrowserTabViewModel.kt | 15 ++---- 3 files changed, 100 insertions(+), 12 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 a5b08708767f..ad65bd089067 100644 --- a/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt @@ -1887,6 +1887,54 @@ class BrowserTabViewModelTest { assertTrue(browserViewState().canGoForward) } + @Test + fun whenUserBrowsingPressesBackAndForwardThenCanAddBookmark() { + setupNavigation(skipHome = false, isBrowsing = true, canGoBack = false) + testee.onUserPressedBack() + testee.onUserPressedForward() + assertTrue(browserViewState().canAddBookmarks) + } + + @Test + fun whenUserBrowsingPressesBackAndForwardThenCanWhitelist() { + setupNavigation(skipHome = false, isBrowsing = true, canGoBack = false) + testee.onUserPressedBack() + testee.onUserPressedForward() + assertTrue(browserViewState().canWhitelist) + } + + @Test + fun whenUserBrowsingPressesBackAndForwardThenCanShare() { + setupNavigation(skipHome = false, isBrowsing = true, canGoBack = false) + testee.onUserPressedBack() + testee.onUserPressedForward() + assertTrue(browserViewState().canSharePage) + } + + @Test + fun whenUserBrowsingPressesBackAndForwardThenCanReportSite() { + setupNavigation(skipHome = false, isBrowsing = true, canGoBack = false) + testee.onUserPressedBack() + testee.onUserPressedForward() + assertTrue(browserViewState().canReportSite) + } + + @Test + fun whenUserBrowsingPressesBackAndForwardThenCanAddToHome() { + setupNavigation(skipHome = false, isBrowsing = true, canGoBack = false) + testee.onUserPressedBack() + testee.onUserPressedForward() + assertTrue(browserViewState().addToHomeEnabled) + } + + @Test + fun whenUserBrowsingPressesBackAndForwardThenCanFindInPage() { + setupNavigation(skipHome = false, isBrowsing = true, canGoBack = false) + testee.onUserPressedBack() + testee.onUserPressedForward() + assertTrue(findInPageViewState().canFindInPage) + } + private inline fun assertCommandIssued(instanceAssertions: T.() -> Unit = {}) { verify(mockCommandObserver, atLeastOnce()).onChanged(commandCaptor.capture()) val issuedCommand = commandCaptor.allValues.find { it is T } diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserStateModifier.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserStateModifier.kt index e21d02d042b7..8a997c782489 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserStateModifier.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserStateModifier.kt @@ -1,5 +1,50 @@ +/* + * Copyright (c) 2020 DuckDuckGo + * + * 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. + */ + package com.duckduckgo.app.browser +import com.duckduckgo.app.browser.BrowserTabViewModel.BrowserViewState +import io.reactivex.annotations.CheckReturnValue + +class BrowserStateModifier { + + @CheckReturnValue + fun copyForBrowserShowing(original: BrowserViewState): BrowserViewState { + return original.copy( + browserShowing = true, + canWhitelist = true, + canFireproofSite = true, + canReportSite = true, + canSharePage = true, + canAddBookmarks = true, + addToHomeEnabled = true + ) + } -interface BrowserStateModifier { -} \ No newline at end of file + @CheckReturnValue + fun copyForHomeShowing(original: BrowserViewState): BrowserViewState { + return original.copy( + browserShowing = false, + canWhitelist = false, + canFireproofSite = false, + canReportSite = false, + canSharePage = false, + canAddBookmarks = false, + addToHomeEnabled = false, + canGoBack = false + ) + } +} 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 c5a13dbb3152..6cbf4f3af663 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt @@ -249,6 +249,7 @@ class BrowserTabViewModel( private lateinit var tabId: String private var webNavigationState: WebNavigationState? = null private var httpsUpgraded = false + private val browserStateModifier = BrowserStateModifier() private val fireproofWebsitesObserver = Observer> { browserViewState.value = currentBrowserViewState().copy(canFireproofSite = canFireproofWebsite()) } @@ -417,7 +418,8 @@ class BrowserTabViewModel( fun onUserPressedForward() { if (!currentBrowserViewState().browserShowing) { - browserViewState.value = currentBrowserViewState().copy(browserShowing = true) + browserViewState.value = browserStateModifier.copyForBrowserShowing(currentBrowserViewState()) + findInPageViewState.value = currentFindInPageViewState().copy(canFindInPage = true) command.value = Refresh } else { command.value = NavigateForward @@ -467,17 +469,10 @@ class BrowserTabViewModel( site = null onSiteChanged() - browserViewState.value = currentBrowserViewState().copy( - browserShowing = false, - canGoBack = false, - canFireproofSite = false, - canSharePage = false, - canAddBookmarks = false, - canReportSite = false, - addToHomeEnabled = false, - canWhitelist = false, + val browserState = browserStateModifier.copyForHomeShowing(currentBrowserViewState()).copy( canGoForward = currentGlobalLayoutState() !is Invalidated ) + browserViewState.value = browserState findInPageViewState.value = FindInPageViewState() omnibarViewState.value = currentOmnibarViewState().copy(omnibarText = "", shouldMoveCaretToEnd = false) From 06337c0ddec5a3e2695669483591d5de5bd753ec Mon Sep 17 00:00:00 2001 From: Craig Russell Date: Mon, 1 Jun 2020 16:08:25 +0100 Subject: [PATCH 3/3] Set navigation state to null when returning to empty tab state This ensures that when you navigate forwards, the state is correctly identified as having changed from the previous state. Without this, the states are compared and we conclude the state is unchanged, and the `site` is never rebuilt so remains null. --- .../main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt | 1 + 1 file changed, 1 insertion(+) 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 6cbf4f3af663..bb83dee2ffa9 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt @@ -468,6 +468,7 @@ class BrowserTabViewModel( private fun navigateHome() { site = null onSiteChanged() + webNavigationState = null val browserState = browserStateModifier.copyForHomeShowing(currentBrowserViewState()).copy( canGoForward = currentGlobalLayoutState() !is Invalidated