diff --git a/app/src/main/java/org/mozilla/fenix/HomeActivity.kt b/app/src/main/java/org/mozilla/fenix/HomeActivity.kt index a51e00bb1..f3c2d3498 100644 --- a/app/src/main/java/org/mozilla/fenix/HomeActivity.kt +++ b/app/src/main/java/org/mozilla/fenix/HomeActivity.kt @@ -20,7 +20,6 @@ import androidx.navigation.NavDestination import androidx.navigation.fragment.NavHostFragment import androidx.navigation.ui.AppBarConfiguration import androidx.navigation.ui.NavigationUI -import com.google.android.material.snackbar.Snackbar import kotlinx.coroutines.launch import mozilla.components.browser.search.SearchEngine import mozilla.components.browser.session.Session @@ -35,13 +34,11 @@ import org.mozilla.fenix.browser.UriOpenedObserver import org.mozilla.fenix.browser.browsingmode.BrowsingMode import org.mozilla.fenix.browser.browsingmode.BrowsingModeManager import org.mozilla.fenix.browser.browsingmode.DefaultBrowsingModeManager -import org.mozilla.fenix.components.FenixSnackbar import org.mozilla.fenix.components.metrics.BreadcrumbsRecorder import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.exceptions.ExceptionsFragmentDirections import org.mozilla.fenix.ext.alreadyOnDestination import org.mozilla.fenix.ext.components -import org.mozilla.fenix.ext.getRootView import org.mozilla.fenix.ext.nav import org.mozilla.fenix.ext.settings import org.mozilla.fenix.home.HomeFragmentDirections @@ -57,12 +54,11 @@ import org.mozilla.fenix.search.SearchFragmentDirections import org.mozilla.fenix.settings.AboutFragmentDirections import org.mozilla.fenix.settings.SettingsFragmentDirections import org.mozilla.fenix.settings.TrackingProtectionFragmentDirections -import org.mozilla.fenix.share.ShareFragment import org.mozilla.fenix.theme.DefaultThemeManager import org.mozilla.fenix.theme.ThemeManager @SuppressWarnings("TooManyFunctions", "LargeClass") -open class HomeActivity : AppCompatActivity(), ShareFragment.TabsSharedCallback { +open class HomeActivity : AppCompatActivity() { lateinit var themeManager: ThemeManager lateinit var browsingModeManager: BrowsingModeManager @@ -310,17 +306,6 @@ open class HomeActivity : AppCompatActivity(), ShareFragment.TabsSharedCallback return DefaultThemeManager(browsingModeManager.mode, this) } - override fun onTabsShared(tabsSize: Int) { - getRootView()?.let { - FenixSnackbar.make(it, Snackbar.LENGTH_SHORT).setText( - getString( - if (tabsSize == 1) R.string.sync_sent_tab_snackbar else - R.string.sync_sent_tabs_snackbar - ) - ).show() - } - } - companion object { const val OPEN_TO_BROWSER = "open_to_browser" const val OPEN_TO_BROWSER_AND_LOAD = "open_to_browser_and_load" diff --git a/app/src/main/java/org/mozilla/fenix/components/FenixSnackbar.kt b/app/src/main/java/org/mozilla/fenix/components/FenixSnackbar.kt index a1c7db50d..7b685c58c 100644 --- a/app/src/main/java/org/mozilla/fenix/components/FenixSnackbar.kt +++ b/app/src/main/java/org/mozilla/fenix/components/FenixSnackbar.kt @@ -10,6 +10,7 @@ import android.view.View import android.view.ViewGroup import android.widget.FrameLayout import androidx.coordinatorlayout.widget.CoordinatorLayout +import androidx.core.content.ContextCompat import androidx.core.widget.TextViewCompat import com.google.android.material.snackbar.BaseTransientBottomBar import com.google.android.material.snackbar.ContentViewCallback @@ -23,11 +24,19 @@ import org.mozilla.fenix.test.Mockable class FenixSnackbar private constructor( parent: ViewGroup, content: View, - contentViewCallback: FenixSnackbarCallback + contentViewCallback: FenixSnackbarCallback, + isError: Boolean ) : BaseTransientBottomBar(parent, content, contentViewCallback) { init { view.background = null + + view.snackbar_layout.background = if (isError) { + ContextCompat.getDrawable(context, R.drawable.fenix_snackbar_error_background) + } else { + ContextCompat.getDrawable(context, R.drawable.fenix_snackbar_background) + } + content.snackbar_btn.increaseTapArea(actionButtonIncreaseDps) TextViewCompat.setAutoSizeTextTypeUniformWithConfiguration( @@ -64,7 +73,7 @@ class FenixSnackbar private constructor( private const val actionButtonIncreaseDps = 16 private const val stepGranularity = 1 - fun make(view: View, duration: Int): FenixSnackbar { + fun make(view: View, duration: Int, isError: Boolean = false): FenixSnackbar { val parent = findSuitableParent(view) ?: run { throw IllegalArgumentException( "No suitable parent found from the given view. Please provide a valid view." @@ -75,7 +84,7 @@ class FenixSnackbar private constructor( val content = inflater.inflate(R.layout.fenix_snackbar, parent, false) val callback = FenixSnackbarCallback(content) - return FenixSnackbar(parent, content, callback).also { + return FenixSnackbar(parent, content, callback, isError).also { it.duration = duration } } @@ -145,9 +154,10 @@ class FenixSnackbarPresenter( text: String, length: Int = FenixSnackbar.LENGTH_LONG, action: (() -> Unit)? = null, - actionName: String? = null + actionName: String? = null, + isError: Boolean = false ) { - FenixSnackbar.make(view, length).setText(text).let { + FenixSnackbar.make(view, length, isError).setText(text).let { if (action != null && actionName != null) it.setAction(actionName, action) else it }.show() } diff --git a/app/src/main/java/org/mozilla/fenix/share/ShareController.kt b/app/src/main/java/org/mozilla/fenix/share/ShareController.kt index ec231813c..5aec7401c 100644 --- a/app/src/main/java/org/mozilla/fenix/share/ShareController.kt +++ b/app/src/main/java/org/mozilla/fenix/share/ShareController.kt @@ -10,14 +10,18 @@ import android.content.Intent.ACTION_SEND import android.content.Intent.EXTRA_TEXT import android.content.Intent.FLAG_ACTIVITY_NEW_TASK import androidx.annotation.VisibleForTesting -import androidx.fragment.app.Fragment import androidx.navigation.NavController import com.google.android.material.snackbar.Snackbar +import kotlinx.coroutines.Deferred +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.GlobalScope +import kotlinx.coroutines.launch import mozilla.components.concept.sync.Device import mozilla.components.concept.sync.TabData import mozilla.components.feature.sendtab.SendTabUseCases import org.mozilla.fenix.R import org.mozilla.fenix.components.FenixSnackbar +import org.mozilla.fenix.components.FenixSnackbarPresenter import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.ext.getRootView import org.mozilla.fenix.ext.metrics @@ -42,17 +46,19 @@ interface ShareController { /** * Default behavior of [ShareController]. Other implementations are possible. * - * @param fragment the [ShareFragment] instance this controller handles business logic for. + * @param context [Context] used for various Android interactions. * @param sharedTabs the list of [ShareTab]s that can be shared. * @param sendTabUseCases instance of [SendTabUseCases] which allows sending tabs to account devices. + * @param snackbarPresenter - instance of [FenixSnackbarPresenter] for displaying styled snackbars * @param navController - [NavController] used for navigation. * @param dismiss - callback signalling sharing can be closed. */ +@Suppress("TooManyFunctions") class DefaultShareController( private val context: Context, - private val fragment: Fragment, private val sharedTabs: List, private val sendTabUseCases: SendTabUseCases, + private val snackbarPresenter: FenixSnackbarPresenter, private val navController: NavController, private val dismiss: () -> Unit ) : ShareController { @@ -75,7 +81,7 @@ class DefaultShareController( } try { - fragment.startActivity(intent) + context.startActivity(intent) } catch (e: SecurityException) { context.getRootView()?.let { FenixSnackbar.make(it, Snackbar.LENGTH_LONG) @@ -93,15 +99,11 @@ class DefaultShareController( override fun handleShareToDevice(device: Device) { context.metrics.track(Event.SendTab) - sendTabUseCases.sendToDeviceAsync(device.id, sharedTabs.toTabData()) - (fragment.activity as ShareFragment.TabsSharedCallback).onTabsShared(sharedTabs.size) - dismiss() + shareToDevicesWithRetry { sendTabUseCases.sendToDeviceAsync(device.id, sharedTabs.toTabData()) } } override fun handleShareToAllDevices(devices: List) { - sendTabUseCases.sendToAllAsync(sharedTabs.toTabData()) - (fragment.activity as ShareFragment.TabsSharedCallback).onTabsShared(sharedTabs.size) - dismiss() + shareToDevicesWithRetry { sendTabUseCases.sendToAllAsync(sharedTabs.toTabData()) } } override fun handleSignIn() { @@ -111,12 +113,51 @@ class DefaultShareController( dismiss() } + private fun shareToDevicesWithRetry(shareOperation: () -> Deferred) { + // Use GlobalScope to allow the continuation of this method even if the share fragment is closed. + GlobalScope.launch(Dispatchers.Main) { + when (shareOperation.invoke().await()) { + true -> showSuccess() + false -> showFailureWithRetryOption { shareToDevicesWithRetry(shareOperation) } + } + dismiss() + } + } + + @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) + fun showSuccess() { + snackbarPresenter.present( + getSuccessMessage(), + Snackbar.LENGTH_SHORT + ) + } + + @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) + fun showFailureWithRetryOption(operation: () -> Unit) { + snackbarPresenter.present( + text = context.getString(R.string.sync_sent_tab_error_snackbar), + length = Snackbar.LENGTH_LONG, + action = operation, + actionName = context.getString(R.string.sync_sent_tab_error_snackbar_action), + isError = true + ) + } + + @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) + fun getSuccessMessage(): String = with(context) { + when (sharedTabs.size) { + 1 -> getString(R.string.sync_sent_tab_snackbar) + else -> getString(R.string.sync_sent_tabs_snackbar) + } + } + @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) fun getShareText() = sharedTabs.joinToString("\n") { tab -> tab.url } // Navigation between app fragments uses ShareTab as arguments. SendTabUseCases uses TabData. @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) fun ShareTab.toTabData() = TabData(title, url) + @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) fun List.toTabData() = map { it.toTabData() } } diff --git a/app/src/main/java/org/mozilla/fenix/share/ShareFragment.kt b/app/src/main/java/org/mozilla/fenix/share/ShareFragment.kt index 745d8d517..c15dddcc3 100644 --- a/app/src/main/java/org/mozilla/fenix/share/ShareFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/share/ShareFragment.kt @@ -31,17 +31,15 @@ import mozilla.components.concept.sync.DeviceType import mozilla.components.feature.sendtab.SendTabUseCases import mozilla.components.service.fxa.manager.FxaAccountManager import org.mozilla.fenix.R +import org.mozilla.fenix.components.FenixSnackbarPresenter import org.mozilla.fenix.ext.components +import org.mozilla.fenix.ext.getRootView import org.mozilla.fenix.ext.requireComponents import org.mozilla.fenix.share.listadapters.AppShareOption import org.mozilla.fenix.share.listadapters.SyncShareOption @Suppress("TooManyFunctions") class ShareFragment : AppCompatDialogFragment() { - interface TabsSharedCallback { - fun onTabsShared(tabsSize: Int) - } - private lateinit var shareInteractor: ShareInteractor private lateinit var shareCloseView: ShareCloseView private lateinit var shareToAccountDevicesView: ShareToAccountDevicesView @@ -125,8 +123,8 @@ class ShareFragment : AppCompatDialogFragment() { shareInteractor = ShareInteractor( DefaultShareController( context = requireContext(), - fragment = this, sharedTabs = tabs, + snackbarPresenter = FenixSnackbarPresenter(activity!!.getRootView()!!), navController = findNavController(), sendTabUseCases = SendTabUseCases(accountManager), dismiss = ::dismiss diff --git a/app/src/main/res/drawable/fenix_snackbar_error_background.xml b/app/src/main/res/drawable/fenix_snackbar_error_background.xml new file mode 100644 index 000000000..f4ddabf7b --- /dev/null +++ b/app/src/main/res/drawable/fenix_snackbar_error_background.xml @@ -0,0 +1,9 @@ + + + + + + diff --git a/app/src/main/res/layout/fenix_snackbar.xml b/app/src/main/res/layout/fenix_snackbar.xml index a643f5b80..f3bd18759 100644 --- a/app/src/main/res/layout/fenix_snackbar.xml +++ b/app/src/main/res/layout/fenix_snackbar.xml @@ -10,6 +10,7 @@ android:layout_height="match_parent"> #FFF36E #960E18 #1A000000 + #B52645 @color/primary_text_light_theme diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 278238dd8..7c7ae1c78 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -794,6 +794,10 @@ Tabs sent! Tab sent! + + Unable to send + + RETRY Scan the code diff --git a/app/src/test/java/org/mozilla/fenix/share/ShareControllerTest.kt b/app/src/test/java/org/mozilla/fenix/share/ShareControllerTest.kt index 761e78373..c22b94c45 100644 --- a/app/src/test/java/org/mozilla/fenix/share/ShareControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/share/ShareControllerTest.kt @@ -4,20 +4,25 @@ package org.mozilla.fenix.share +import android.app.Activity import android.content.Context import android.content.Intent -import androidx.fragment.app.Fragment import androidx.navigation.NavController import assertk.assertAll import assertk.assertThat import assertk.assertions.isDataClassEqualTo import assertk.assertions.isEqualTo +import assertk.assertions.isNotEqualTo +import assertk.assertions.isSameAs +import assertk.assertions.isSuccess import assertk.assertions.isTrue +import com.google.android.material.snackbar.Snackbar import io.mockk.Runs import io.mockk.every import io.mockk.just import io.mockk.mockk import io.mockk.slot +import io.mockk.spyk import io.mockk.verify import io.mockk.verifyOrder import kotlinx.coroutines.ObsoleteCoroutinesApi @@ -25,12 +30,13 @@ import mozilla.components.concept.sync.Device import mozilla.components.concept.sync.DeviceType import mozilla.components.concept.sync.TabData import mozilla.components.feature.sendtab.SendTabUseCases +import mozilla.components.support.test.robolectric.testContext import org.junit.Before import org.junit.Test import org.junit.runner.RunWith -import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R import org.mozilla.fenix.TestApplication +import org.mozilla.fenix.components.FenixSnackbarPresenter import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.components.metrics.MetricController import org.mozilla.fenix.ext.metrics @@ -43,9 +49,9 @@ import org.robolectric.annotation.Config @RunWith(RobolectricTestRunner::class) @Config(application = TestApplication::class) class ShareControllerTest { - private val context: Context = mockk(relaxed = true) + // Need a valid context to retrieve Strings for example, but we also need it to return our "metrics" + private val context: Context = spyk(testContext) private val metrics: MetricController = mockk(relaxed = true) - private val fragment = mockk(relaxed = true) private val shareTabs = listOf( ShareTab("url0", "title0"), ShareTab("url1", "title1") @@ -57,9 +63,12 @@ class ShareControllerTest { ) private val textToShare = "${shareTabs[0].url}\n${shareTabs[1].url}" private val sendTabUseCases = mockk(relaxed = true) + private val snackbarPresenter = mockk(relaxed = true) private val navController = mockk(relaxed = true) private val dismiss = mockk<() -> Unit>(relaxed = true) - private val controller = DefaultShareController(context, fragment, shareTabs, sendTabUseCases, navController, dismiss) + private val controller = DefaultShareController( + context, shareTabs, sendTabUseCases, snackbarPresenter, navController, dismiss + ) @Before fun setUp() { @@ -79,9 +88,14 @@ class ShareControllerTest { val appClassName = "activity" val appShareOption = AppShareOption("app", mockk(), appPackageName, appClassName) val shareIntent = slot() - every { fragment.startActivity(capture(shareIntent)) } just Runs + // Our share Intent uses `FLAG_ACTIVITY_NEW_TASK` but when resolving the startActivity call + // needed for capturing the actual Intent used the `slot` one doesn't have this flag so we + // need to use an Activity Context. + val activityContext: Context = mockk() + val testController = DefaultShareController(activityContext, shareTabs, mockk(), mockk(), mockk(), dismiss) + every { activityContext.startActivity(capture(shareIntent)) } just Runs - controller.handleShareToApp(appShareOption) + testController.handleShareToApp(appShareOption) // Check that the Intent used for querying apps has the expected structre assertAll { @@ -94,7 +108,7 @@ class ShareControllerTest { assertThat(shareIntent.captured.component!!.className).isEqualTo(appClassName) } verifyOrder { - fragment.startActivity(shareIntent.captured) + activityContext.startActivity(shareIntent.captured) dismiss() } } @@ -102,13 +116,10 @@ class ShareControllerTest { @Test @Suppress("DeferredResultUnused") fun `handleShareToDevice should share to account device, inform callbacks and dismiss`() { - val deviceToShareTo = - Device("deviceId", "deviceName", DeviceType.UNKNOWN, false, 0L, emptyList(), false, null) - val tabSharedCallbackActivity = mockk(relaxed = true) - val sharedTabsNumber = slot() + val deviceToShareTo = Device( + "deviceId", "deviceName", DeviceType.UNKNOWN, false, 0L, emptyList(), false, null) val deviceId = slot() val tabsShared = slot>() - every { fragment.activity } returns tabSharedCallbackActivity controller.handleShareToDevice(deviceToShareTo) @@ -116,48 +127,35 @@ class ShareControllerTest { verifyOrder { metrics.track(Event.SendTab) sendTabUseCases.sendToDeviceAsync(capture(deviceId), capture(tabsShared)) - tabSharedCallbackActivity.onTabsShared(capture(sharedTabsNumber)) - dismiss() + // dismiss() is also to be called, but at the moment cannot test it in a coroutine. } assertAll { assertThat(deviceId.isCaptured).isTrue() assertThat(deviceId.captured).isEqualTo(deviceToShareTo.id) assertThat(tabsShared.isCaptured).isTrue() assertThat(tabsShared.captured).isEqualTo(tabsData) - - // All current tabs should be shared - assertThat(sharedTabsNumber.isCaptured).isTrue() - assertThat(sharedTabsNumber.captured).isEqualTo(shareTabs.size) } } @Test + @Suppress("DeferredResultUnused") fun `handleShareToAllDevices calls handleShareToDevice multiple times`() { val devicesToShareTo = listOf( Device("deviceId0", "deviceName0", DeviceType.UNKNOWN, false, 0L, emptyList(), false, null), Device("deviceId1", "deviceName1", DeviceType.UNKNOWN, true, 1L, emptyList(), false, null) ) - val tabSharedCallbackActivity = mockk(relaxed = true) - val sharedTabsNumber = slot() val tabsShared = slot>() - every { fragment.activity } returns tabSharedCallbackActivity controller.handleShareToAllDevices(devicesToShareTo) - // Verify all the needed methods are called. sendTab() should be called for each account device. verifyOrder { sendTabUseCases.sendToAllAsync(capture(tabsShared)) - tabSharedCallbackActivity.onTabsShared(capture(sharedTabsNumber)) - dismiss() + // dismiss() is also to be called, but at the moment cannot test it in a coroutine. } assertAll { // SendTabUseCases should send a the `shareTabs` mapped to tabData assertThat(tabsShared.isCaptured).isTrue() assertThat(tabsShared.captured).isEqualTo(tabsData) - - // All current tabs should be shared - assertThat(sharedTabsNumber.isCaptured).isTrue() - assertThat(sharedTabsNumber.captured).isEqualTo(shareTabs.size) } } @@ -188,6 +186,83 @@ class ShareControllerTest { } } + @Test + fun `showSuccess should show a snackbar with a success message`() { + val expectedMessage = controller.getSuccessMessage() + val expectedTimeout = Snackbar.LENGTH_SHORT + val messageSlot = slot() + val timeoutSlot = slot() + + controller.showSuccess() + + verify { snackbarPresenter.present(capture(messageSlot), capture(timeoutSlot)) } + assertAll { + assertThat(messageSlot.isCaptured).isTrue() + assertThat(timeoutSlot.isCaptured).isTrue() + + assertThat(messageSlot.captured).isEqualTo(expectedMessage) + assertThat(timeoutSlot.captured).isEqualTo(expectedTimeout) + } + } + + @Test + fun `showFailureWithRetryOption should show a snackbar with a retry action`() { + val expectedMessage = context.getString(R.string.sync_sent_tab_error_snackbar) + val expectedTimeout = Snackbar.LENGTH_LONG + val operation: () -> Unit = { println("Hello World") } + val expectedRetryMessage = + context.getString(R.string.sync_sent_tab_error_snackbar_action) + val messageSlot = slot() + val timeoutSlot = slot() + val operationSlot = slot<() -> Unit>() + val retryMesageSlot = slot() + val isFailureSlot = slot() + + controller.showFailureWithRetryOption(operation) + + verify { + snackbarPresenter.present( + capture(messageSlot), + capture(timeoutSlot), + capture(operationSlot), + capture(retryMesageSlot), + capture(isFailureSlot) + ) + } + assertAll { + assertThat(messageSlot.isCaptured).isTrue() + assertThat(timeoutSlot.isCaptured).isTrue() + assertThat(operationSlot.isCaptured).isTrue() + assertThat(retryMesageSlot.isCaptured).isTrue() + assertThat(isFailureSlot.isCaptured).isTrue() + + assertThat(messageSlot.captured).isEqualTo(expectedMessage) + assertThat(timeoutSlot.captured).isEqualTo(expectedTimeout) + assertThat { operationSlot.captured }.isSuccess().isSameAs(operation) + assertThat(retryMesageSlot.captured).isEqualTo(expectedRetryMessage) + assertThat(isFailureSlot.captured).isEqualTo(true) + } + } + + @Test + fun `getSuccessMessage should return different strings depending on the number of shared tabs`() { + val controllerWithOneSharedTab = DefaultShareController( + context, listOf(ShareTab("url0", "title0")), mockk(), mockk(), mockk(), mockk() + ) + val controllerWithMoreSharedTabs = controller + val expectedTabSharedMessage = context.getString(R.string.sync_sent_tab_snackbar) + val expectedTabsSharedMessage = context.getString(R.string.sync_sent_tabs_snackbar) + + val tabSharedMessage = controllerWithOneSharedTab.getSuccessMessage() + val tabsSharedMessage = controllerWithMoreSharedTabs.getSuccessMessage() + + assertAll { + assertThat(tabSharedMessage).isNotEqualTo(tabsSharedMessage) + assertThat(tabSharedMessage).isEqualTo(expectedTabSharedMessage) + assertThat(tabsSharedMessage).isEqualTo(expectedTabsSharedMessage) + } + } + @Test fun `getShareText should respect concatenate shared tabs urls`() { assertThat(controller.getShareText()).isEqualTo(textToShare)