For #11866 - Update back press logic for SearchFragment

app/src/main/java/org/mozilla/fenix/search/SearchController.kt

@@ -7,9 +7,6 @@ package org.mozilla.fenix.search
 
 import android.content.Intent
 import androidx.navigation.NavController
-import kotlinx.coroutines.CoroutineScope
-import kotlinx.coroutines.delay
-import kotlinx.coroutines.launch
 import mozilla.components.browser.search.SearchEngine
 import mozilla.components.browser.session.Session
 import mozilla.components.support.ktx.kotlin.isUrl
@@ -51,7 +48,6 @@ class DefaultSearchController(
     private val activity: HomeActivity,
     private val store: SearchFragmentStore,
     private val navController: NavController,
-    private val viewLifecycleScope: CoroutineScope,
     private val clearToolbarFocus: () -> Unit
 ) : SearchController {
 
@@ -101,13 +97,7 @@ class DefaultSearchController(
     }
 
     override fun handleEditingCancelled() {
-        viewLifecycleScope.launch {
+        clearToolbarFocus()
-            clearToolbarFocus()
-            // Delay a short amount so the keyboard begins animating away. This makes exit animation
-            // much smoother instead of having two separate parts (keyboard hides THEN animation)
-            delay(KEYBOARD_ANIMATION_DELAY)
-            navController.popBackStack()
-        }
     }
 
     override fun handleTextChanged(text: String) {
@@ -199,8 +189,4 @@ class DefaultSearchController(
             handleExistingSessionSelected(session)
         }
     }
-
-    companion object {
-        internal const val KEYBOARD_ANIMATION_DELAY = 5L
-    }
 }

app/src/main/java/org/mozilla/fenix/search/SearchFragment.kt

@@ -24,7 +24,6 @@ import androidx.core.content.ContextCompat
 import androidx.core.view.isVisible
 import androidx.core.widget.NestedScrollView
 import androidx.fragment.app.Fragment
-import androidx.lifecycle.lifecycleScope
 import androidx.navigation.fragment.findNavController
 import androidx.navigation.fragment.navArgs
 import kotlinx.android.synthetic.main.fragment_search.*
@@ -120,7 +119,6 @@ class SearchFragment : Fragment(), UserInteractionHandler {
             activity = activity,
             store = searchStore,
             navController = findNavController(),
-            viewLifecycleScope = viewLifecycleOwner.lifecycleScope,
             clearToolbarFocus = ::clearToolbarFocus
         )
 
@@ -182,6 +180,7 @@ class SearchFragment : Fragment(), UserInteractionHandler {
     }
 
     private fun clearToolbarFocus() {
+        toolbarView.view.hideKeyboard()
         toolbarView.view.clearFocus()
     }
 
@@ -347,14 +346,14 @@ class SearchFragment : Fragment(), UserInteractionHandler {
     }
 
     override fun onBackPressed(): Boolean {
-        // Note: Actual navigation happens in `handleEditingCancelled` in SearchController
         return when {
             qrFeature.onBackPressed() -> {
                 toolbarView.view.edit.focus()
                 view?.search_scan_button?.isChecked = false
                 toolbarView.view.requestFocus()
+                true
             }
-            else -> true
+            else -> false
         }
     }
 
@@ -426,7 +425,6 @@ class SearchFragment : Fragment(), UserInteractionHandler {
     }
 
     companion object {
-        private const val SHARED_TRANSITION_MS = 250L
         private const val REQUEST_CODE_CAMERA_PERMISSIONS = 1
     }
 }

app/src/main/java/org/mozilla/fenix/search/toolbar/ToolbarView.kt

@@ -59,7 +59,6 @@ class ToolbarView(
 ) {
 
     private var isInitialized = false
-    private var hasBeenCanceled = false
 
     init {
         view.apply {
@@ -96,19 +95,18 @@ class ToolbarView(
             )
 
             edit.setUrlBackground(
-                AppCompatResources.getDrawable(context, R.drawable.search_url_background))
+                AppCompatResources.getDrawable(context, R.drawable.search_url_background)
+            )
 
             private = isPrivate
 
             setOnEditListener(object : mozilla.components.concept.toolbar.Toolbar.OnEditListener {
                 override fun onCancelEditing(): Boolean {
-                    // For some reason, this can be triggered twice on one back press. This only leads to
+                    interactor.onEditingCanceled()
-                    // navigateUp, so let's make sure we only call it once
-                    if (!hasBeenCanceled) interactor.onEditingCanceled()
-                    hasBeenCanceled = true
                     // We need to return false to not show display mode
                     return false
                 }
+
                 override fun onTextChanged(text: String) {
                     url = text
                     this@ToolbarView.interactor.onTextChanged(text)
@@ -144,13 +142,15 @@ class ToolbarView(
             isInitialized = true
         }
 
-        val iconSize = context.resources.getDimensionPixelSize(R.dimen.preference_icon_drawable_size)
+        val iconSize =
+            context.resources.getDimensionPixelSize(R.dimen.preference_icon_drawable_size)
 
         val scaledIcon = Bitmap.createScaledBitmap(
             searchState.searchEngineSource.searchEngine.icon,
             iconSize,
             iconSize,
-            true)
+            true
+        )
 
         val icon = BitmapDrawable(context.resources, scaledIcon)
 

app/src/test/java/org/mozilla/fenix/search/DefaultSearchControllerTest.kt

@@ -4,7 +4,6 @@
 
 package org.mozilla.fenix.search
 
-import androidx.lifecycle.LifecycleCoroutineScope
 import androidx.navigation.NavController
 import androidx.navigation.NavDirections
 import io.mockk.every
@@ -32,7 +31,6 @@ import org.mozilla.fenix.ext.navigateSafe
 import org.mozilla.fenix.ext.searchEngineManager
 import org.mozilla.fenix.ext.settings
 import org.mozilla.fenix.helpers.FenixRobolectricTestRunner
-import org.mozilla.fenix.search.DefaultSearchController.Companion.KEYBOARD_ANIMATION_DELAY
 import org.mozilla.fenix.settings.SupportUtils
 import org.mozilla.fenix.utils.Settings
 import org.mozilla.fenix.whatsnew.clear
@@ -49,7 +47,6 @@ class DefaultSearchControllerTest {
     private val searchEngine: SearchEngine = mockk(relaxed = true)
     private val metrics: MetricController = mockk(relaxed = true)
     private val sessionManager: SessionManager = mockk(relaxed = true)
-    private val lifecycleScope: LifecycleCoroutineScope = mockk(relaxed = true)
     private val clearToolbarFocus: (() -> Unit) = mockk(relaxed = true)
 
     private lateinit var controller: DefaultSearchController
@@ -67,7 +64,6 @@ class DefaultSearchControllerTest {
             activity = activity,
             store = store,
             navController = navController,
-            viewLifecycleScope = lifecycleScope,
             clearToolbarFocus = clearToolbarFocus
         )
 
@@ -123,17 +119,13 @@ class DefaultSearchControllerTest {
             activity = activity,
             store = store,
             navController = navController,
-            viewLifecycleScope = this,
             clearToolbarFocus = clearToolbarFocus
         )
 
         controller.handleEditingCancelled()
 
-        advanceTimeBy(KEYBOARD_ANIMATION_DELAY)
-
         verify {
             clearToolbarFocus()
-            navController.popBackStack()
         }
     }
 

app/src/test/java/org/mozilla/fenix/search/SearchInteractorTest.kt

@@ -4,249 +4,84 @@
 
 package org.mozilla.fenix.search
 
-import android.content.Context
-import androidx.lifecycle.LifecycleCoroutineScope
-import androidx.navigation.NavController
-import androidx.navigation.NavDirections
-import io.mockk.Runs
-import io.mockk.every
-import io.mockk.just
 import io.mockk.mockk
 import io.mockk.verify
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.test.runBlockingTest
 import mozilla.components.browser.search.SearchEngine
-import mozilla.components.browser.search.SearchEngineManager
 import mozilla.components.browser.session.Session
+import org.junit.Before
 import org.junit.Test
-import org.mozilla.fenix.BrowserDirection
-import org.mozilla.fenix.FenixApplication
-import org.mozilla.fenix.HomeActivity
-import org.mozilla.fenix.R
-import org.mozilla.fenix.components.metrics.Event
-import org.mozilla.fenix.components.searchengine.CustomSearchEngineStore.PREF_FILE_SEARCH_ENGINES
-import org.mozilla.fenix.ext.metrics
-import org.mozilla.fenix.ext.navigateSafe
-import org.mozilla.fenix.ext.searchEngineManager
 
 @ExperimentalCoroutinesApi
 class SearchInteractorTest {
 
-    private val lifecycleScope: LifecycleCoroutineScope = mockk(relaxed = true)
+    lateinit var searchController: DefaultSearchController
-    private val clearToolbarFocus = { }
+    lateinit var interactor: SearchInteractor
+
+    @Before
+    fun setup() {
+        searchController = mockk(relaxed = true)
+        interactor = SearchInteractor(
+            searchController
+        )
+    }
 
     @Test
     fun onUrlCommitted() {
-        val context: HomeActivity = mockk(relaxed = true)
-        val store: SearchFragmentStore = mockk()
-        val state: SearchFragmentState = mockk()
-        val searchEngineManager: SearchEngineManager = mockk(relaxed = true)
-        val searchEngine = SearchEngineSource.Default(mockk(relaxed = true))
-        val searchAccessPoint: Event.PerformedSearch.SearchAccessPoint = mockk(relaxed = true)
-
-        every { context.metrics } returns mockk(relaxed = true)
-        every { context.searchEngineManager } returns searchEngineManager
-        every { context.openToBrowserAndLoad(any(), any(), any(), any(), any(), any()) } just Runs
-
-        every { store.state } returns state
-        every { state.session } returns null
-        every { state.searchEngineSource } returns searchEngine
-        every { state.searchAccessPoint } returns searchAccessPoint
-
-        every {
-            context.getSharedPreferences(
-                PREF_FILE_SEARCH_ENGINES,
-                Context.MODE_PRIVATE
-            )
-        } returns mockk(relaxed = true)
-
-        val searchController: SearchController = DefaultSearchController(
-            context,
-            store,
-            mockk(),
-            lifecycleScope,
-            clearToolbarFocus
-        )
-        val interactor = SearchInteractor(searchController)
-
         interactor.onUrlCommitted("test")
 
         verify {
-            context.openToBrowserAndLoad(
+            searchController.handleUrlCommitted("test")
-                searchTermOrURL = "test",
-                newTab = true,
-                from = BrowserDirection.FromSearch,
-                engine = searchEngine.searchEngine
-            )
         }
     }
 
     @Test
     fun onEditingCanceled() = runBlockingTest {
-        val navController: NavController = mockk(relaxed = true)
-        val store: SearchFragmentStore = mockk(relaxed = true)
-
-        every { store.state } returns mockk(relaxed = true)
-
-        val searchController: SearchController = DefaultSearchController(
-            mockk(),
-            store,
-            navController,
-            this,
-            clearToolbarFocus
-        )
-        val interactor = SearchInteractor(searchController)
-
         interactor.onEditingCanceled()
-        advanceTimeBy(DefaultSearchController.KEYBOARD_ANIMATION_DELAY)
 
         verify {
-            clearToolbarFocus()
+            searchController.handleEditingCancelled()
-            navController.popBackStack()
         }
     }
 
     @Test
     fun onTextChanged() {
-        val store: SearchFragmentStore = mockk(relaxed = true)
-        val context: HomeActivity = mockk(relaxed = true)
-
-        every { store.state } returns mockk(relaxed = true)
-
-        val searchController: SearchController = DefaultSearchController(
-            context,
-            store,
-            mockk(),
-            lifecycleScope,
-            clearToolbarFocus
-        )
         val interactor = SearchInteractor(searchController)
 
         interactor.onTextChanged("test")
 
-        verify { store.dispatch(SearchFragmentAction.UpdateQuery("test")) }
+        verify { searchController.handleTextChanged("test") }
-        verify { store.dispatch(SearchFragmentAction.AllowSearchSuggestionsInPrivateModePrompt(false)) }
     }
 
     @Test
     fun onUrlTapped() {
-        val context: HomeActivity = mockk()
-        val store: SearchFragmentStore = mockk()
-        val state: SearchFragmentState = mockk()
-        val searchEngine = SearchEngineSource.Default(mockk(relaxed = true))
-
-        every { context.metrics } returns mockk(relaxed = true)
-        every { context.openToBrowserAndLoad(any(), any(), any()) } just Runs
-
-        every { store.state } returns state
-        every { state.session } returns null
-        every { state.searchEngineSource } returns searchEngine
-
-        every {
-            context.getSharedPreferences(
-                PREF_FILE_SEARCH_ENGINES,
-                Context.MODE_PRIVATE
-            )
-        } returns mockk(relaxed = true)
-
-        val searchController: SearchController = DefaultSearchController(
-            context,
-            store,
-            mockk(),
-            lifecycleScope,
-            clearToolbarFocus
-        )
-        val interactor = SearchInteractor(searchController)
-
         interactor.onUrlTapped("test")
 
         verify {
-            context.openToBrowserAndLoad(
+            searchController.handleUrlTapped("test")
-                "test",
-                true,
-                BrowserDirection.FromSearch
-            )
         }
     }
 
     @Test
     fun onSearchTermsTapped() {
-        val context: HomeActivity = mockk(relaxed = true)
-        val store: SearchFragmentStore = mockk()
-        val state: SearchFragmentState = mockk()
-        val searchEngineManager: SearchEngineManager = mockk(relaxed = true)
-        val searchEngine = SearchEngineSource.Default(mockk(relaxed = true))
-        val searchAccessPoint: Event.PerformedSearch.SearchAccessPoint = mockk(relaxed = true)
-
-        every { context.metrics } returns mockk(relaxed = true)
-        every { context.searchEngineManager } returns searchEngineManager
-        every { context.openToBrowserAndLoad(any(), any(), any(), any(), any(), any()) } just Runs
-
-        every { store.state } returns state
-        every { state.session } returns null
-        every { state.searchEngineSource } returns searchEngine
-        every { state.searchAccessPoint } returns searchAccessPoint
-
-        every {
-            context.getSharedPreferences(
-                PREF_FILE_SEARCH_ENGINES,
-                Context.MODE_PRIVATE
-            )
-        } returns mockk(relaxed = true)
-
-        val searchController: SearchController = DefaultSearchController(
-            context,
-            store,
-            mockk(),
-            lifecycleScope,
-            clearToolbarFocus
-        )
-
-        val interactor = SearchInteractor(searchController)
-
         interactor.onSearchTermsTapped("test")
         verify {
-            context.openToBrowserAndLoad(
+            searchController.handleSearchTermsTapped("test")
-                searchTermOrURL = "test",
-                newTab = true,
-                from = BrowserDirection.FromSearch,
-                engine = searchEngine.searchEngine,
-                forceSearch = true
-            )
         }
     }
 
     @Test
     fun onSearchShortcutEngineSelected() {
-        val context: HomeActivity = mockk(relaxed = true)
-
-        every { context.metrics } returns mockk(relaxed = true)
-
-        val store: SearchFragmentStore = mockk(relaxed = true)
-        val state: SearchFragmentState = mockk(relaxed = true)
-
-        every { store.state } returns state
-
-        val searchController: SearchController = DefaultSearchController(
-            context,
-            store,
-            mockk(),
-            lifecycleScope,
-            clearToolbarFocus
-        )
-        val interactor = SearchInteractor(searchController)
         val searchEngine: SearchEngine = mockk(relaxed = true)
 
         interactor.onSearchShortcutEngineSelected(searchEngine)
 
-        verify { store.dispatch(SearchFragmentAction.SearchShortcutEngineSelected(searchEngine)) }
+        verify { searchController.handleSearchShortcutEngineSelected(searchEngine) }
     }
 
     @Test
     fun onSearchShortcutsButtonClicked() {
-        val searchController: SearchController = mockk(relaxed = true)
-        val interactor = SearchInteractor(searchController)
-
         interactor.onSearchShortcutsButtonClicked()
 
         verify { searchController.handleSearchShortcutsButtonClicked() }
@@ -254,58 +89,21 @@ class SearchInteractorTest {
 
     @Test
     fun onClickSearchEngineSettings() {
-        val navController: NavController = mockk()
-        val store: SearchFragmentStore = mockk()
-
-        every { store.state } returns mockk(relaxed = true)
-        every { navController.currentDestination?.id } returns R.id.searchFragment
-
-        val searchController: SearchController = DefaultSearchController(
-            mockk(),
-            store,
-            navController,
-            lifecycleScope,
-            clearToolbarFocus
-        )
-        val interactor = SearchInteractor(searchController)
-
-        every { navController.navigate(any() as NavDirections) } just Runs
-
         interactor.onClickSearchEngineSettings()
 
         verify {
-            navController.navigateSafe(
+            searchController.handleClickSearchEngineSettings()
-                R.id.searchFragment,
-                SearchFragmentDirections.actionGlobalSearchEngineFragment()
-            )
         }
     }
 
     @Test
     fun onExistingSessionSelected() {
-        val navController: NavController = mockk(relaxed = true)
-        val context: HomeActivity = mockk(relaxed = true)
-        val applicationContext: FenixApplication = mockk(relaxed = true)
-        every { context.applicationContext } returns applicationContext
-        val store: SearchFragmentStore = mockk()
-        every { context.openToBrowser(any(), any()) } just Runs
-
-        every { store.state } returns mockk(relaxed = true)
-
-        val searchController: SearchController = DefaultSearchController(
-            context,
-            store,
-            navController,
-            lifecycleScope,
-            clearToolbarFocus
-        )
-        val interactor = SearchInteractor(searchController)
         val session = Session("http://mozilla.org", false)
 
         interactor.onExistingSessionSelected(session)
 
         verify {
-            context.openToBrowser(BrowserDirection.FromSearch)
+            searchController.handleExistingSessionSelected(session)
         }
     }
 }