From d9074aaa2c92f70367aaf8c15a145c8d706e7205 Mon Sep 17 00:00:00 2001 From: Mugurell Date: Tue, 20 Dec 2022 19:07:53 +0200 Subject: [PATCH] [fenix] For https://github.com/mozilla-mobile/fenix/issues/25816: Ensure the "show search suggestions" user option is followed Previously the check for the "Search -> Show search suggestions" user setting was only used in the default SearchFragmentState but not again if users change the current search engine as part of the unified search feature. This comes to ensure that that check is always made when needing to configure new search engine results. --- .../fenix/search/SearchDialogController.kt | 16 +- .../fenix/search/SearchDialogFragment.kt | 18 +- .../fenix/search/SearchFragmentStore.kt | 46 ++++- .../search/SearchDialogControllerTest.kt | 5 +- .../fenix/search/SearchFragmentStoreTest.kt | 192 ++++++++++++------ .../SearchSelectorToolbarActionTest.kt | 5 + 6 files changed, 202 insertions(+), 80 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/search/SearchDialogController.kt b/app/src/main/java/org/mozilla/fenix/search/SearchDialogController.kt index 1164a3d506..e1c7270c20 100644 --- a/app/src/main/java/org/mozilla/fenix/search/SearchDialogController.kt +++ b/app/src/main/java/org/mozilla/fenix/search/SearchDialogController.kt @@ -217,10 +217,22 @@ class SearchDialogController( fragmentStore.dispatch(SearchFragmentAction.SearchTabsEngineSelected(searchEngine)) } searchEngine == store.state.search.selectedOrDefaultSearchEngine -> { - fragmentStore.dispatch(SearchFragmentAction.SearchDefaultEngineSelected(searchEngine, settings)) + fragmentStore.dispatch( + SearchFragmentAction.SearchDefaultEngineSelected( + engine = searchEngine, + browsingMode = activity.browsingModeManager.mode, + settings = settings, + ), + ) } else -> { - fragmentStore.dispatch(SearchFragmentAction.SearchShortcutEngineSelected(searchEngine, settings)) + fragmentStore.dispatch( + SearchFragmentAction.SearchShortcutEngineSelected( + engine = searchEngine, + browsingMode = activity.browsingModeManager.mode, + settings = settings, + ), + ) } } diff --git a/app/src/main/java/org/mozilla/fenix/search/SearchDialogFragment.kt b/app/src/main/java/org/mozilla/fenix/search/SearchDialogFragment.kt index 926b2433eb..eac3ae10a4 100644 --- a/app/src/main/java/org/mozilla/fenix/search/SearchDialogFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/search/SearchDialogFragment.kt @@ -53,7 +53,7 @@ import mozilla.components.browser.toolbar.BrowserToolbar import mozilla.components.concept.engine.EngineSession import mozilla.components.concept.menu.candidate.DrawableMenuIcon import mozilla.components.concept.menu.candidate.TextMenuCandidate -import mozilla.components.concept.storage.HistoryStorage +import mozilla.components.concept.toolbar.AutocompleteProvider import mozilla.components.concept.toolbar.Toolbar import mozilla.components.feature.qr.QrFeature import mozilla.components.feature.toolbar.ToolbarAutocompleteFeature @@ -247,12 +247,16 @@ class SearchDialogFragment : AppCompatDialogFragment(), UserInteractionHandler { engineForSpeculativeConnects, { store.state.searchEngineSource.searchEngine?.type != SearchEngine.Type.APPLICATION }, ).apply { - addDomainProvider( - ShippedDomainsProvider().also { shippedDomainsProvider -> - shippedDomainsProvider.initialize(requireContext()) - }, + updateAutocompleteProviders( + listOfNotNull( + historyStorageProvider(), + // Assume the history provider has priority 0 and set priority 1 for the domains provider + // to ensure the first source checked for autocomplete suggestions is history. + ShippedDomainsProvider(1).also { shippedDomainsProvider -> + shippedDomainsProvider.initialize(requireContext()) + }, + ), ) - historyStorageProvider()?.also(::addHistoryStorageProvider) } } @@ -659,7 +663,7 @@ class SearchDialogFragment : AppCompatDialogFragment(), UserInteractionHandler { dismissAllowingStateLoss() } - private fun historyStorageProvider(): HistoryStorage? { + private fun historyStorageProvider(): AutocompleteProvider? { return if (requireContext().settings().shouldShowHistorySuggestions) { requireComponents.core.historyStorage } else { diff --git a/app/src/main/java/org/mozilla/fenix/search/SearchFragmentStore.kt b/app/src/main/java/org/mozilla/fenix/search/SearchFragmentStore.kt index 32278bdd87..93aff7e7d4 100644 --- a/app/src/main/java/org/mozilla/fenix/search/SearchFragmentStore.kt +++ b/app/src/main/java/org/mozilla/fenix/search/SearchFragmentStore.kt @@ -4,6 +4,7 @@ package org.mozilla.fenix.search +import androidx.annotation.VisibleForTesting import mozilla.components.browser.state.search.SearchEngine import mozilla.components.browser.state.selector.findTab import mozilla.components.browser.state.state.SearchState @@ -136,12 +137,6 @@ fun createInitialSearchFragmentState( val tab = tabId?.let { components.core.store.state.findTab(it) } val url = tab?.content?.url.orEmpty() - val shouldShowSearchSuggestions = when (activity.browsingModeManager.mode) { - BrowsingMode.Normal -> settings.shouldShowSearchSuggestions - BrowsingMode.Private -> - settings.shouldShowSearchSuggestions && settings.shouldShowSearchSuggestionsInPrivate - } - val searchEngineSource = if (searchEngine != null) { SearchEngineSource.Shortcut(searchEngine) } else { @@ -154,7 +149,10 @@ fun createInitialSearchFragmentState( searchTerms = tab?.content?.searchTerms.orEmpty(), searchEngineSource = searchEngineSource, defaultEngine = null, - showSearchSuggestions = shouldShowSearchSuggestions, + showSearchSuggestions = shouldShowSearchSuggestions( + browsingMode = activity.browsingModeManager.mode, + settings = settings, + ), showSearchSuggestionsHint = false, showSearchShortcuts = false, areShortcutsAvailable = false, @@ -184,12 +182,20 @@ sealed class SearchFragmentAction : Action { /** * Action when default search engine is selected. */ - data class SearchDefaultEngineSelected(val engine: SearchEngine, val settings: Settings) : SearchFragmentAction() + data class SearchDefaultEngineSelected( + val engine: SearchEngine, + val browsingMode: BrowsingMode, + val settings: Settings, + ) : SearchFragmentAction() /** * Action when shortcut search engine is selected. */ - data class SearchShortcutEngineSelected(val engine: SearchEngine, val settings: Settings) : SearchFragmentAction() + data class SearchShortcutEngineSelected( + val engine: SearchEngine, + val browsingMode: BrowsingMode, + val settings: Settings, + ) : SearchFragmentAction() /** * Action when history search engine is selected. @@ -242,7 +248,7 @@ private fun searchStateReducer(state: SearchFragmentState, action: SearchFragmen is SearchFragmentAction.SearchDefaultEngineSelected -> state.copy( searchEngineSource = SearchEngineSource.Default(action.engine), - showSearchSuggestions = action.settings.shouldShowSearchSuggestions, + showSearchSuggestions = shouldShowSearchSuggestions(action.browsingMode, action.settings), showSearchShortcuts = action.settings.shouldShowSearchShortcuts, showClipboardSuggestions = action.settings.shouldShowClipboardSuggestions, showSearchTermHistory = action.settings.showUnifiedSearchFeature && @@ -256,7 +262,7 @@ private fun searchStateReducer(state: SearchFragmentState, action: SearchFragmen is SearchFragmentAction.SearchShortcutEngineSelected -> state.copy( searchEngineSource = SearchEngineSource.Shortcut(action.engine), - showSearchSuggestions = true, + showSearchSuggestions = shouldShowSearchSuggestions(action.browsingMode, action.settings), showSearchShortcuts = when (action.settings.showUnifiedSearchFeature) { true -> false false -> action.settings.shouldShowSearchShortcuts @@ -356,3 +362,21 @@ private fun searchStateReducer(state: SearchFragmentState, action: SearchFragmen } } } + +/** + * Check whether search suggestions should be shown in the AwesomeBar. + * + * @param browsingMode Current browsing mode: normal or private. + * @param settings Persistence layer containing user option's for showing search suggestions. + * + * @return `true` if search suggestions should be shown `false` otherwise. + */ +@VisibleForTesting +internal fun shouldShowSearchSuggestions( + browsingMode: BrowsingMode, + settings: Settings, +) = when (browsingMode) { + BrowsingMode.Normal -> settings.shouldShowSearchSuggestions + BrowsingMode.Private -> + settings.shouldShowSearchSuggestions && settings.shouldShowSearchSuggestionsInPrivate +} diff --git a/app/src/test/java/org/mozilla/fenix/search/SearchDialogControllerTest.kt b/app/src/test/java/org/mozilla/fenix/search/SearchDialogControllerTest.kt index e31b2d1d66..1f858255dd 100644 --- a/app/src/test/java/org/mozilla/fenix/search/SearchDialogControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/search/SearchDialogControllerTest.kt @@ -45,6 +45,7 @@ import org.mozilla.fenix.GleanMetrics.SearchShortcuts import org.mozilla.fenix.GleanMetrics.UnifiedSearch import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R +import org.mozilla.fenix.browser.browsingmode.BrowsingMode import org.mozilla.fenix.components.Core import org.mozilla.fenix.components.metrics.MetricsUtils import org.mozilla.fenix.helpers.FenixRobolectricTestRunner @@ -375,6 +376,8 @@ class SearchDialogControllerTest { @Test fun handleSearchShortcutEngineSelected() { val searchEngine: SearchEngine = mockk(relaxed = true) + val browsingMode = BrowsingMode.Private + every { activity.browsingModeManager.mode } returns browsingMode var focusToolbarInvoked = false createController( @@ -384,7 +387,7 @@ class SearchDialogControllerTest { ).handleSearchShortcutEngineSelected(searchEngine) assertTrue(focusToolbarInvoked) - verify { store.dispatch(SearchFragmentAction.SearchShortcutEngineSelected(searchEngine, settings)) } + verify { store.dispatch(SearchFragmentAction.SearchShortcutEngineSelected(searchEngine, browsingMode, settings)) } assertNotNull(SearchShortcuts.selected.testGetValue()) val recordedEvents = SearchShortcuts.selected.testGetValue()!! diff --git a/app/src/test/java/org/mozilla/fenix/search/SearchFragmentStoreTest.kt b/app/src/test/java/org/mozilla/fenix/search/SearchFragmentStoreTest.kt index bd7007ec07..aa145a62ac 100644 --- a/app/src/test/java/org/mozilla/fenix/search/SearchFragmentStoreTest.kt +++ b/app/src/test/java/org/mozilla/fenix/search/SearchFragmentStoreTest.kt @@ -8,6 +8,8 @@ import io.mockk.MockKAnnotations import io.mockk.every import io.mockk.impl.annotations.MockK import io.mockk.mockk +import io.mockk.mockkStatic +import io.mockk.verify import kotlinx.coroutines.test.runTest import mozilla.components.browser.state.search.RegionState import mozilla.components.browser.state.search.SearchEngine @@ -60,50 +62,54 @@ class SearchFragmentStoreTest { every { settings.shouldShowSearchShortcuts } returns true every { settings.showUnifiedSearchFeature } returns true every { settings.shouldShowHistorySuggestions } returns true - - val expected = SearchFragmentState( - query = "", - url = "", - searchTerms = "", - searchEngineSource = SearchEngineSource.None, - defaultEngine = null, - showSearchShortcutsSetting = true, - showSearchSuggestions = false, - showSearchSuggestionsHint = false, - showSearchShortcuts = false, - areShortcutsAvailable = false, - showClipboardSuggestions = false, - showSearchTermHistory = true, - showHistorySuggestionsForCurrentEngine = false, - showAllHistorySuggestions = true, - showBookmarkSuggestions = false, - showSyncedTabsSuggestions = false, - showSessionSuggestions = true, - tabId = null, - pastedText = "pastedText", - searchAccessPoint = MetricsUtils.Source.ACTION, - ) - - assertEquals( - expected, - createInitialSearchFragmentState( - activity, - components, + mockkStatic("org.mozilla.fenix.search.SearchFragmentStoreKt") { + val expected = SearchFragmentState( + query = "", + url = "", + searchTerms = "", + searchEngineSource = SearchEngineSource.None, + defaultEngine = null, + showSearchShortcutsSetting = true, + showSearchSuggestions = false, + showSearchSuggestionsHint = false, + showSearchShortcuts = false, + areShortcutsAvailable = false, + showClipboardSuggestions = false, + showSearchTermHistory = true, + showHistorySuggestionsForCurrentEngine = false, + showAllHistorySuggestions = true, + showBookmarkSuggestions = false, + showSyncedTabsSuggestions = false, + showSessionSuggestions = true, tabId = null, pastedText = "pastedText", searchAccessPoint = MetricsUtils.Source.ACTION, - ), - ) - assertEquals( - expected.copy(tabId = "tabId"), - createInitialSearchFragmentState( - activity, - components, - tabId = "tabId", - pastedText = "pastedText", - searchAccessPoint = MetricsUtils.Source.ACTION, - ), - ) + ) + + assertEquals( + expected, + createInitialSearchFragmentState( + activity, + components, + tabId = null, + pastedText = "pastedText", + searchAccessPoint = MetricsUtils.Source.ACTION, + ), + ) + + assertEquals( + expected.copy(tabId = "tabId"), + createInitialSearchFragmentState( + activity, + components, + tabId = "tabId", + pastedText = "pastedText", + searchAccessPoint = MetricsUtils.Source.ACTION, + ), + ) + + verify(exactly = 2) { shouldShowSearchSuggestions(BrowsingMode.Normal, settings) } + } } @Test @@ -175,20 +181,31 @@ class SearchFragmentStoreTest { every { settings.shouldShowHistorySuggestions } returns true every { settings.shouldShowBookmarkSuggestions } returns false every { settings.shouldShowSyncedTabsSuggestions } returns false - - store.dispatch(SearchFragmentAction.SearchDefaultEngineSelected(searchEngine, settings)).join() - - assertNotSame(initialState, store.state) - assertEquals(SearchEngineSource.Default(searchEngine), store.state.searchEngineSource) - assertTrue(store.state.showSearchSuggestions) - assertFalse(store.state.showSearchShortcuts) - assertTrue(store.state.showClipboardSuggestions) - assertFalse(store.state.showSearchTermHistory) - assertFalse(store.state.showHistorySuggestionsForCurrentEngine) - assertTrue(store.state.showAllHistorySuggestions) - assertFalse(store.state.showBookmarkSuggestions) - assertFalse(store.state.showSyncedTabsSuggestions) - assertTrue(store.state.showSessionSuggestions) + every { settings.shouldShowSearchSuggestions } returns true + every { settings.shouldShowSearchSuggestionsInPrivate } returns true + + mockkStatic("org.mozilla.fenix.search.SearchFragmentStoreKt") { + store.dispatch( + SearchFragmentAction.SearchDefaultEngineSelected( + engine = searchEngine, + browsingMode = BrowsingMode.Private, + settings = settings, + ), + ).join() + + assertNotSame(initialState, store.state) + assertEquals(SearchEngineSource.Default(searchEngine), store.state.searchEngineSource) + assertTrue(store.state.showSearchSuggestions) + assertFalse(store.state.showSearchShortcuts) + assertTrue(store.state.showClipboardSuggestions) + assertFalse(store.state.showSearchTermHistory) + assertFalse(store.state.showHistorySuggestionsForCurrentEngine) + assertTrue(store.state.showAllHistorySuggestions) + assertFalse(store.state.showBookmarkSuggestions) + assertFalse(store.state.showSyncedTabsSuggestions) + assertTrue(store.state.showSessionSuggestions) + verify { shouldShowSearchSuggestions(BrowsingMode.Private, settings) } + } } @Test @@ -202,8 +219,15 @@ class SearchFragmentStoreTest { every { settings.shouldShowHistorySuggestions } returns true every { settings.shouldShowBookmarkSuggestions } returns true every { settings.shouldShowSyncedTabsSuggestions } returns true + every { settings.shouldShowSearchSuggestions } returns true - store.dispatch(SearchFragmentAction.SearchShortcutEngineSelected(searchEngine, settings)).join() + store.dispatch( + SearchFragmentAction.SearchShortcutEngineSelected( + engine = searchEngine, + browsingMode = BrowsingMode.Normal, + settings = settings, + ), + ).join() assertNotSame(initialState, store.state) assertEquals(SearchEngineSource.Shortcut(searchEngine), store.state.searchEngineSource) @@ -231,11 +255,17 @@ class SearchFragmentStoreTest { every { settings.shouldShowBookmarkSuggestions } returns false every { settings.shouldShowSyncedTabsSuggestions } returns false - store.dispatch(SearchFragmentAction.SearchShortcutEngineSelected(searchEngine, settings)).join() + store.dispatch( + SearchFragmentAction.SearchShortcutEngineSelected( + engine = searchEngine, + browsingMode = BrowsingMode.Normal, + settings = settings, + ), + ).join() assertNotSame(initialState, store.state) assertEquals(SearchEngineSource.Shortcut(searchEngine), store.state.searchEngineSource) - assertTrue(store.state.showSearchSuggestions) + assertFalse(store.state.showSearchSuggestions) assertFalse(store.state.showSearchShortcuts) assertFalse(store.state.showClipboardSuggestions) assertTrue(store.state.showSearchTermHistory) @@ -256,8 +286,16 @@ class SearchFragmentStoreTest { every { settings.shouldShowHistorySuggestions } returns true every { settings.shouldShowBookmarkSuggestions } returns false every { settings.shouldShowSyncedTabsSuggestions } returns true + every { settings.shouldShowSearchSuggestions } returns true + every { settings.shouldShowSearchSuggestionsInPrivate } returns true - store.dispatch(SearchFragmentAction.SearchShortcutEngineSelected(searchEngine, settings)).join() + store.dispatch( + SearchFragmentAction.SearchShortcutEngineSelected( + engine = searchEngine, + browsingMode = BrowsingMode.Private, + settings = settings, + ), + ).join() assertNotSame(initialState, store.state) assertEquals(SearchEngineSource.Shortcut(searchEngine), store.state.searchEngineSource) @@ -549,6 +587,42 @@ class SearchFragmentStoreTest { assertFalse(store.state.showSearchShortcuts) } + @Test + fun `GIVEN normal browsing mode and search suggestions enabled WHEN checking if search suggestions should be shown THEN return true`() { + var settings: Settings = mockk { + every { shouldShowSearchSuggestions } returns false + every { shouldShowSearchSuggestionsInPrivate } returns false + } + assertFalse(shouldShowSearchSuggestions(BrowsingMode.Normal, settings)) + + settings = mockk { + every { shouldShowSearchSuggestions } returns true + every { shouldShowSearchSuggestionsInPrivate } returns false + } + assertTrue(shouldShowSearchSuggestions(BrowsingMode.Normal, settings)) + } + + @Test + fun `GIVEN private browsing mode and search suggestions enabled WHEN checking if search suggestions should be shown THEN return true`() { + var settings: Settings = mockk { + every { shouldShowSearchSuggestions } returns false + every { shouldShowSearchSuggestionsInPrivate } returns false + } + assertFalse(shouldShowSearchSuggestions(BrowsingMode.Private, settings)) + + settings = mockk { + every { shouldShowSearchSuggestions } returns false + every { shouldShowSearchSuggestionsInPrivate } returns true + } + assertFalse(shouldShowSearchSuggestions(BrowsingMode.Private, settings)) + + settings = mockk { + every { shouldShowSearchSuggestions } returns true + every { shouldShowSearchSuggestionsInPrivate } returns true + } + assertTrue(shouldShowSearchSuggestions(BrowsingMode.Private, settings)) + } + private fun emptyDefaultState( searchEngineSource: SearchEngineSource = mockk(), defaultEngine: SearchEngine? = mockk(), diff --git a/app/src/test/java/org/mozilla/fenix/search/toolbar/SearchSelectorToolbarActionTest.kt b/app/src/test/java/org/mozilla/fenix/search/toolbar/SearchSelectorToolbarActionTest.kt index f7a7f7feff..fb99cd00ee 100644 --- a/app/src/test/java/org/mozilla/fenix/search/toolbar/SearchSelectorToolbarActionTest.kt +++ b/app/src/test/java/org/mozilla/fenix/search/toolbar/SearchSelectorToolbarActionTest.kt @@ -34,6 +34,7 @@ import org.junit.Test import org.junit.runner.RunWith import org.mozilla.fenix.GleanMetrics.UnifiedSearch import org.mozilla.fenix.R +import org.mozilla.fenix.browser.browsingmode.BrowsingMode import org.mozilla.fenix.components.metrics.MetricsUtils import org.mozilla.fenix.ext.settings import org.mozilla.fenix.helpers.FenixRobolectricTestRunner @@ -111,6 +112,7 @@ class SearchSelectorToolbarActionTest { store.dispatch( SearchDefaultEngineSelected( engine = testSearchEngine, + browsingMode = BrowsingMode.Normal, settings = mockk(relaxed = true), ), ) @@ -141,6 +143,7 @@ class SearchSelectorToolbarActionTest { store.dispatch( SearchDefaultEngineSelected( engine = testSearchEngine, + browsingMode = BrowsingMode.Private, settings = mockk(relaxed = true), ), ) @@ -169,6 +172,7 @@ class SearchSelectorToolbarActionTest { store.dispatch( SearchDefaultEngineSelected( engine = testSearchEngine, + browsingMode = BrowsingMode.Private, settings = mockk(relaxed = true), ), ) @@ -185,6 +189,7 @@ class SearchSelectorToolbarActionTest { store.dispatch( SearchDefaultEngineSelected( engine = testSearchEngine, + browsingMode = BrowsingMode.Normal, settings = mockk(relaxed = true), ), )