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