From 388c144a6274cfab0318dcc8e0691e43137a5292 Mon Sep 17 00:00:00 2001 From: Sebastian Kaspari Date: Thu, 2 Jul 2020 11:43:40 +0200 Subject: [PATCH] SearchFragmentStore: Remove Session reference from state and read values from BrowserStore. --- .../mozilla/fenix/search/SearchController.kt | 10 +++++----- .../org/mozilla/fenix/search/SearchFragment.kt | 18 +++++++++++------- .../fenix/search/SearchFragmentStore.kt | 9 ++++++--- .../fenix/search/awesomebar/AwesomeBarView.kt | 2 +- .../fenix/search/toolbar/ToolbarView.kt | 2 +- .../search/DefaultSearchControllerTest.kt | 16 ++++++---------- .../fenix/search/SearchFragmentStoreTest.kt | 4 +++- 7 files changed, 33 insertions(+), 28 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/search/SearchController.kt b/app/src/main/java/org/mozilla/fenix/search/SearchController.kt index 062c07322..3a0e2172a 100644 --- a/app/src/main/java/org/mozilla/fenix/search/SearchController.kt +++ b/app/src/main/java/org/mozilla/fenix/search/SearchController.kt @@ -69,7 +69,7 @@ class DefaultSearchController( private fun openSearchOrUrl(url: String) { activity.openToBrowserAndLoad( searchTermOrURL = url, - newTab = store.state.session == null, + newTab = store.state.tabId == null, from = BrowserDirection.FromSearch, engine = store.state.searchEngineSource.searchEngine ) @@ -103,8 +103,8 @@ class DefaultSearchController( override fun handleTextChanged(text: String) { val settings = activity.settings() // Display the search shortcuts on each entry of the search fragment (see #5308) - val textMatchesCurrentUrl = store.state.session?.url ?: "" == text - val textMatchesCurrentSearch = store.state.session?.searchTerms ?: "" == text + val textMatchesCurrentUrl = store.state.url == text + val textMatchesCurrentSearch = store.state.searchTerms == text store.dispatch(SearchFragmentAction.UpdateQuery(text)) store.dispatch( @@ -126,7 +126,7 @@ class DefaultSearchController( override fun handleUrlTapped(url: String) { activity.openToBrowserAndLoad( searchTermOrURL = url, - newTab = store.state.session == null, + newTab = store.state.tabId == null, from = BrowserDirection.FromSearch ) @@ -138,7 +138,7 @@ class DefaultSearchController( activity.openToBrowserAndLoad( searchTermOrURL = searchTerms, - newTab = store.state.session == null, + newTab = store.state.tabId == null, from = BrowserDirection.FromSearch, engine = store.state.searchEngineSource.searchEngine, forceSearch = true diff --git a/app/src/main/java/org/mozilla/fenix/search/SearchFragment.kt b/app/src/main/java/org/mozilla/fenix/search/SearchFragment.kt index 4f5ef089e..cbfab61c6 100644 --- a/app/src/main/java/org/mozilla/fenix/search/SearchFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/search/SearchFragment.kt @@ -30,6 +30,7 @@ import kotlinx.android.synthetic.main.fragment_search.* import kotlinx.android.synthetic.main.fragment_search.view.* import kotlinx.android.synthetic.main.search_suggestions_onboarding.view.* import kotlinx.coroutines.ExperimentalCoroutinesApi +import mozilla.components.browser.state.selector.findTab import mozilla.components.browser.toolbar.BrowserToolbar import mozilla.components.concept.storage.HistoryStorage import mozilla.components.feature.qr.QrFeature @@ -86,11 +87,12 @@ class SearchFragment : Fragment(), UserInteractionHandler { ): View? { val activity = activity as HomeActivity val args by navArgs() - val session = args.sessionId - ?.let(requireComponents.core.sessionManager::findSessionById) + + val tabId = args.sessionId + val tab = tabId?.let { requireComponents.core.store.state.findTab(it) } val view = inflater.inflate(R.layout.fragment_search, container, false) - val url = session?.url.orEmpty() + val url = tab?.content?.url.orEmpty() val currentSearchEngine = SearchEngineSource.Default( requireComponents.search.provider.getDefaultEngine(requireContext()) ) @@ -103,6 +105,8 @@ class SearchFragment : Fragment(), UserInteractionHandler { SearchFragmentStore( SearchFragmentState( query = url, + url = url, + searchTerms = tab?.content?.searchTerms.orEmpty(), searchEngineSource = currentSearchEngine, defaultEngineSource = currentSearchEngine, showSearchSuggestions = shouldShowSearchSuggestions(isPrivate), @@ -111,7 +115,7 @@ class SearchFragment : Fragment(), UserInteractionHandler { showClipboardSuggestions = requireContext().settings().shouldShowClipboardSuggestions, showHistorySuggestions = requireContext().settings().shouldShowHistorySuggestions, showBookmarkSuggestions = requireContext().settings().shouldShowBookmarkSuggestions, - session = session, + tabId = tabId, pastedText = args.pastedText, searchAccessPoint = args.searchAccessPoint ) @@ -234,7 +238,7 @@ class SearchFragment : Fragment(), UserInteractionHandler { (activity as HomeActivity) .openToBrowserAndLoad( searchTermOrURL = result, - newTab = searchStore.state.session == null, + newTab = searchStore.state.tabId == null, from = BrowserDirection.FromSearch ) dialog.dismiss() @@ -265,7 +269,7 @@ class SearchFragment : Fragment(), UserInteractionHandler { searchTermOrURL = SupportUtils.getGenericSumoURLForTopic( SupportUtils.SumoTopic.SEARCH_SUGGESTION ), - newTab = searchStore.state.session == null, + newTab = searchStore.state.tabId == null, from = BrowserDirection.FromSearch ) } @@ -298,7 +302,7 @@ class SearchFragment : Fragment(), UserInteractionHandler { (activity as HomeActivity) .openToBrowserAndLoad( searchTermOrURL = requireContext().components.clipboardHandler.url ?: "", - newTab = searchStore.state.session == null, + newTab = searchStore.state.tabId == null, from = BrowserDirection.FromSearch ) } 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 84cbbcad2..e8e270924 100644 --- a/app/src/main/java/org/mozilla/fenix/search/SearchFragmentStore.kt +++ b/app/src/main/java/org/mozilla/fenix/search/SearchFragmentStore.kt @@ -5,7 +5,6 @@ package org.mozilla.fenix.search import mozilla.components.browser.search.SearchEngine -import mozilla.components.browser.session.Session import mozilla.components.lib.state.Action import mozilla.components.lib.state.State import mozilla.components.lib.state.Store @@ -34,6 +33,9 @@ sealed class SearchEngineSource { /** * The state for the Search Screen * @property query The current search query string + * @property url The current URL of the tab (if this fragment is shown for an already existing tab) + * @property searchTerms The search terms used to search previously in this tab (if this fragment is shown + * for an already existing tab) * @property searchEngineSource The current selected search engine with the context of how it was selected * @property defaultEngineSource The current default search engine source * @property showSearchSuggestions Whether or not to show search suggestions from the search engine in the AwesomeBar @@ -42,11 +44,12 @@ sealed class SearchEngineSource { * @property showClipboardSuggestions Whether or not to show clipboard suggestion in the AwesomeBar * @property showHistorySuggestions Whether or not to show history suggestions in the AwesomeBar * @property showBookmarkSuggestions Whether or not to show the bookmark suggestion in the AwesomeBar - * @property session The current session if available * @property pastedText The text pasted from the long press toolbar menu */ data class SearchFragmentState( val query: String, + val url: String, + val searchTerms: String, val searchEngineSource: SearchEngineSource, val defaultEngineSource: SearchEngineSource.Default, val showSearchSuggestions: Boolean, @@ -55,7 +58,7 @@ data class SearchFragmentState( val showClipboardSuggestions: Boolean, val showHistorySuggestions: Boolean, val showBookmarkSuggestions: Boolean, - val session: Session?, + val tabId: String?, val pastedText: String? = null, val searchAccessPoint: Event.PerformedSearch.SearchAccessPoint? ) : State diff --git a/app/src/main/java/org/mozilla/fenix/search/awesomebar/AwesomeBarView.kt b/app/src/main/java/org/mozilla/fenix/search/awesomebar/AwesomeBarView.kt index a0d479343..fa6328fab 100644 --- a/app/src/main/java/org/mozilla/fenix/search/awesomebar/AwesomeBarView.kt +++ b/app/src/main/java/org/mozilla/fenix/search/awesomebar/AwesomeBarView.kt @@ -198,7 +198,7 @@ class AwesomeBarView( updateSuggestionProvidersVisibility(state) // Do not make suggestions based on user's current URL unless it's a search shortcut - if (state.query == state.session?.url && !state.showSearchShortcuts) { + if (state.query == state.url && !state.showSearchShortcuts) { return } diff --git a/app/src/main/java/org/mozilla/fenix/search/toolbar/ToolbarView.kt b/app/src/main/java/org/mozilla/fenix/search/toolbar/ToolbarView.kt index 31369ed68..4eb97f40b 100644 --- a/app/src/main/java/org/mozilla/fenix/search/toolbar/ToolbarView.kt +++ b/app/src/main/java/org/mozilla/fenix/search/toolbar/ToolbarView.kt @@ -125,7 +125,7 @@ class ToolbarView( /* Only set the search terms if pasted text is null so that the search term doesn't overwrite pastedText when view enters `editMode` */ if (searchState.pastedText.isNullOrEmpty()) { - view.setSearchTerms(searchState.session?.searchTerms.orEmpty()) + view.setSearchTerms(searchState.searchTerms) } // We must trigger an onTextChanged so when search terms are set when transitioning to `editMode` diff --git a/app/src/test/java/org/mozilla/fenix/search/DefaultSearchControllerTest.kt b/app/src/test/java/org/mozilla/fenix/search/DefaultSearchControllerTest.kt index 314468d2c..c2fea785d 100644 --- a/app/src/test/java/org/mozilla/fenix/search/DefaultSearchControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/search/DefaultSearchControllerTest.kt @@ -15,7 +15,6 @@ import mozilla.components.browser.search.SearchEngine import mozilla.components.browser.session.Session import mozilla.components.browser.session.SessionManager import mozilla.components.support.test.robolectric.testContext -import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Before import org.junit.Test @@ -43,7 +42,6 @@ class DefaultSearchControllerTest { private val store: SearchFragmentStore = mockk(relaxed = true) private val navController: NavController = mockk(relaxed = true) private val defaultSearchEngine: SearchEngine? = mockk(relaxed = true) - private val session: Session? = mockk(relaxed = true) private val searchEngine: SearchEngine = mockk(relaxed = true) private val metrics: MetricController = mockk(relaxed = true) private val sessionManager: SessionManager = mockk(relaxed = true) @@ -55,7 +53,7 @@ class DefaultSearchControllerTest { @Before fun setUp() { every { activity.searchEngineManager.defaultSearchEngine } returns defaultSearchEngine - every { store.state.session } returns session + every { store.state.tabId } returns "test-tab-id" every { store.state.searchEngineSource.searchEngine } returns searchEngine every { activity.metrics } returns metrics every { activity.components.core.sessionManager } returns sessionManager @@ -79,7 +77,7 @@ class DefaultSearchControllerTest { verify { activity.openToBrowserAndLoad( searchTermOrURL = url, - newTab = session == null, + newTab = false, from = BrowserDirection.FromSearch, engine = searchEngine ) @@ -105,7 +103,7 @@ class DefaultSearchControllerTest { verify { activity.openToBrowserAndLoad( searchTermOrURL = SupportUtils.getMozillaPageUrl(SupportUtils.MozillaPage.MANIFESTO), - newTab = session == null, + newTab = false, from = BrowserDirection.FromSearch, engine = searchEngine ) @@ -163,14 +161,12 @@ class DefaultSearchControllerTest { @Test fun `show search shortcuts when setting enabled AND query equals url`() { val text = "mozilla.org" - every { session?.url } returns "mozilla.org" + every { store.state.url } returns "mozilla.org" testContext.settings().preferences .edit() .putBoolean(testContext.getString(R.string.pref_key_show_search_shortcuts), true) .apply() - assertEquals(text, session?.url) - controller.handleTextChanged(text) verify { store.dispatch(SearchFragmentAction.ShowSearchShortcutEnginePicker(true)) } @@ -226,7 +222,7 @@ class DefaultSearchControllerTest { verify { activity.openToBrowserAndLoad( searchTermOrURL = url, - newTab = session == null, + newTab = false, from = BrowserDirection.FromSearch ) } @@ -242,7 +238,7 @@ class DefaultSearchControllerTest { verify { activity.openToBrowserAndLoad( searchTermOrURL = searchTerms, - newTab = session == null, + newTab = false, from = BrowserDirection.FromSearch, engine = searchEngine, forceSearch = true 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 061805ac2..383b54b66 100644 --- a/app/src/test/java/org/mozilla/fenix/search/SearchFragmentStoreTest.kt +++ b/app/src/test/java/org/mozilla/fenix/search/SearchFragmentStoreTest.kt @@ -48,6 +48,9 @@ class SearchFragmentStoreTest { } private fun emptyDefaultState(): SearchFragmentState = SearchFragmentState( + tabId = null, + url = "", + searchTerms = "", query = "", searchEngineSource = mockk(), defaultEngineSource = mockk(), @@ -57,7 +60,6 @@ class SearchFragmentStoreTest { showClipboardSuggestions = false, showHistorySuggestions = false, showBookmarkSuggestions = false, - session = null, searchAccessPoint = Event.PerformedSearch.SearchAccessPoint.NONE ) }