diff --git a/mobile/android/android-components/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngine.kt b/mobile/android/android-components/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngine.kt index bf67d0f02c56..98f27dfd94fd 100644 --- a/mobile/android/android-components/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngine.kt +++ b/mobile/android/android-components/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngine.kt @@ -760,8 +760,8 @@ class GeckoEngine( TranslationsController.RuntimeTranslation.listSupportedLanguages().then( { if (it != null) { - var listOfFromLanguages = mutableListOf() - var listOfToLanguages = mutableListOf() + val listOfFromLanguages = mutableListOf() + val listOfToLanguages = mutableListOf() if (it.fromLanguages != null) { for (each in it.fromLanguages!!) { diff --git a/mobile/android/android-components/components/browser/state/src/main/java/mozilla/components/browser/state/engine/middleware/TranslationsMiddleware.kt b/mobile/android/android-components/components/browser/state/src/main/java/mozilla/components/browser/state/engine/middleware/TranslationsMiddleware.kt index 5d9d77051fc1..81e3b8b48b65 100644 --- a/mobile/android/android-components/components/browser/state/src/main/java/mozilla/components/browser/state/engine/middleware/TranslationsMiddleware.kt +++ b/mobile/android/android-components/components/browser/state/src/main/java/mozilla/components/browser/state/engine/middleware/TranslationsMiddleware.kt @@ -245,6 +245,17 @@ class TranslationsMiddleware( supportedLanguages = it, ), ) + + // Ensures error is cleared, if a tab made this request. + if (tabId != null) { + context.store.dispatch( + TranslationsAction.TranslateSuccessAction( + tabId = tabId, + operation = TranslationOperation.FETCH_SUPPORTED_LANGUAGES, + ), + ) + } + logger.info("Success requesting supported languages.") }, onError = { diff --git a/mobile/android/android-components/components/browser/state/src/test/java/mozilla/components/browser/state/engine/middleware/TranslationsMiddlewareTest.kt b/mobile/android/android-components/components/browser/state/src/test/java/mozilla/components/browser/state/engine/middleware/TranslationsMiddlewareTest.kt index c55c4b57186b..cf30f7060e62 100644 --- a/mobile/android/android-components/components/browser/state/src/test/java/mozilla/components/browser/state/engine/middleware/TranslationsMiddlewareTest.kt +++ b/mobile/android/android-components/components/browser/state/src/test/java/mozilla/components/browser/state/engine/middleware/TranslationsMiddlewareTest.kt @@ -139,6 +139,13 @@ class TranslationsMiddlewareTest { ), ) + verify(context.store, atLeastOnce()).dispatch( + TranslationsAction.TranslateSuccessAction( + tabId = tab.id, + operation = TranslationOperation.FETCH_SUPPORTED_LANGUAGES, + ), + ) + waitForIdle() } diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/translations/TranslationsDialogBinding.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/translations/TranslationsDialogBinding.kt index 70fb6d456d49..e2d25f82dcd1 100644 --- a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/translations/TranslationsDialogBinding.kt +++ b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/translations/TranslationsDialogBinding.kt @@ -11,7 +11,10 @@ import kotlinx.coroutines.flow.mapNotNull import mozilla.components.browser.state.selector.findTab import mozilla.components.browser.state.state.BrowserState import mozilla.components.browser.state.state.TabSessionState +import mozilla.components.browser.state.state.TranslationsBrowserState +import mozilla.components.browser.state.state.TranslationsState import mozilla.components.browser.state.store.BrowserStore +import mozilla.components.concept.engine.translate.TranslationError import mozilla.components.concept.engine.translate.initialFromLanguage import mozilla.components.concept.engine.translate.initialToLanguage import mozilla.components.lib.state.helpers.AbstractBinding @@ -71,13 +74,6 @@ class TranslationsDialogBinding( ) } - // Dispatch engine level errors - if (browserTranslationsState.engineError != null) { - translationsDialogStore.dispatch( - TranslationsDialogAction.UpdateTranslationError(browserTranslationsState.engineError), - ) - } - // Session Translations State Behavior (Tab) val sessionTranslationsState = state.sessionState.translationsState @@ -131,34 +127,86 @@ class TranslationsDialogBinding( updateStoreIfTranslated() } - // A session error may override a browser error - if (sessionTranslationsState.translationError != null) { - val documentLangDisplayName = sessionTranslationsState.translationEngineState - ?.detectedLanguages?.documentLangTag?.let { docLangTag -> - val documentLocale = Locale.forLanguageTag(docLangTag) - val userLocale = state.browserState.locale ?: LocaleManager.getSystemDefault() - LocaleUtils.getLocalizedDisplayName( - userLocale = userLocale, - languageLocale = documentLocale, - ) - } - - translationsDialogStore.dispatch( - TranslationsDialogAction.UpdateTranslationError( - translationError = sessionTranslationsState.translationError, - documentLangDisplayName = documentLangDisplayName, - ), - ) - } - sessionTranslationsState.translationDownloadSize?.let { translationsDialogStore.dispatch( TranslationsDialogAction.UpdateDownloadTranslationDownloadSize(it), ) } + + // Error handling requires both knowledge of browser and session state + updateTranslationError( + sessionTranslationsState = sessionTranslationsState, + browserTranslationsState = browserTranslationsState, + browserState = state.browserState, + ) } } + /** + * Helper function for error handling, which requires considering both + * [TranslationsState] (session) and [TranslationsBrowserState] (browser or global) states. + * + * Will dispatch [TranslationsDialogAction] to update store when appropriate. + * + * Certain errors take priority over others, depending on how important they are. + * Error priority: + * 1. An error in [TranslationsBrowserState] of [EngineNotSupportedError] should be the + * highest priority to process. + * 2. An error in [TranslationsState] of [LanguageNotSupportedError] is the second highest + * priority and requires additional information. + * 3. Displayable browser errors only should be the dialog error when the session has a + * non-displayable error. (Usually expect this case where an error recovery action occurred on + * a different tab and failed.) + * 4. Displayable session errors and null errors are the default dialog error. + * + * @param sessionTranslationsState The session state to consider when dispatching errors. + * @param browserTranslationsState The browser state to consider when dispatching errors. + * @param browserState The browser state to consider when fetching information for errors. + + */ + private fun updateTranslationError( + sessionTranslationsState: TranslationsState, + browserTranslationsState: TranslationsBrowserState, + browserState: BrowserState, + ) { + if (browserTranslationsState.engineError is TranslationError.EngineNotSupportedError) { + // [EngineNotSupportedError] is a browser error that overrides all other errors. + translationsDialogStore.dispatch( + TranslationsDialogAction.UpdateTranslationError(browserTranslationsState.engineError), + ) + } else if (sessionTranslationsState.translationError is TranslationError.LanguageNotSupportedError) { + // [LanguageNotSupportedError] is a special case where we need additional information. + val documentLangDisplayName = sessionTranslationsState.translationEngineState + ?.detectedLanguages?.documentLangTag?.let { docLangTag -> + val documentLocale = Locale.forLanguageTag(docLangTag) + val userLocale = browserState.locale ?: LocaleManager.getSystemDefault() + LocaleUtils.getLocalizedDisplayName( + userLocale = userLocale, + languageLocale = documentLocale, + ) + } + + translationsDialogStore.dispatch( + TranslationsDialogAction.UpdateTranslationError( + translationError = sessionTranslationsState.translationError, + documentLangDisplayName = documentLangDisplayName, + ), + ) + } else if (browserTranslationsState.engineError?.displayError == true && + sessionTranslationsState.translationError?.displayError != true + ) { + // Browser errors should only be displayed with the session error is not displayable nor null. + translationsDialogStore.dispatch( + TranslationsDialogAction.UpdateTranslationError(browserTranslationsState.engineError), + ) + } else if (sessionTranslationsState.translationError?.displayError != false) { + // Displayable session errors and null session errors should be passed to the dialog under most cases. + translationsDialogStore.dispatch( + TranslationsDialogAction.UpdateTranslationError(sessionTranslationsState.translationError), + ) + } + } + private fun updateStoreIfIsTranslateProcessing() { translationsDialogStore.dispatch( TranslationsDialogAction.UpdateTranslationInProgress( diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/translations/TranslationsDialogMiddleware.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/translations/TranslationsDialogMiddleware.kt index 6a1e6f359cd1..37576dc092bb 100644 --- a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/translations/TranslationsDialogMiddleware.kt +++ b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/translations/TranslationsDialogMiddleware.kt @@ -26,6 +26,21 @@ class TranslationsDialogMiddleware( action: TranslationsDialogAction, ) { when (action) { + is TranslationsDialogAction.InitTranslationsDialog -> { + // If the languages are missing, we should attempt to fetch the supported languages. + // This will also ensure the missing languages dialog error state, if fetching fails. + if (browserStore.state.translationEngine.supportedLanguages?.fromLanguages == null || + browserStore.state.translationEngine.supportedLanguages?.toLanguages == null + ) { + browserStore.dispatch( + TranslationsAction.OperationRequestedAction( + tabId = sessionId, + operation = TranslationOperation.FETCH_SUPPORTED_LANGUAGES, + ), + ) + } + } + is TranslationsDialogAction.FetchDownloadFileSizeAction -> { browserStore.dispatch( TranslationsAction.FetchTranslationDownloadSizeAction( diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/translations/TranslationsDialogStore.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/translations/TranslationsDialogStore.kt index 5599d60412c3..273f07e97f71 100644 --- a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/translations/TranslationsDialogStore.kt +++ b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/translations/TranslationsDialogStore.kt @@ -276,16 +276,6 @@ internal object TranslationsDialogReducer { ) } - is TranslationsDialogAction.InitTranslationsDialog -> { - state.copy( - positiveButtonType = if (state.initialTo == null || state.initialFrom == null) { - PositiveButtonType.Disabled - } else { - state.positiveButtonType - }, - ) - } - is TranslationsDialogAction.UpdateTranslationError -> { state.copy( error = action.translationError, @@ -323,6 +313,7 @@ internal object TranslationsDialogReducer { ) } + TranslationsDialogAction.InitTranslationsDialog, is TranslationsDialogAction.TranslateAction, is TranslationsDialogAction.UpdatePageSettingsValue, TranslationsDialogAction.TranslateAction, diff --git a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/translations/TranslationsDialogBindingTest.kt b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/translations/TranslationsDialogBindingTest.kt index 5a96482b04eb..6b860a621293 100644 --- a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/translations/TranslationsDialogBindingTest.kt +++ b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/translations/TranslationsDialogBindingTest.kt @@ -24,6 +24,7 @@ import mozilla.components.support.test.rule.runTestOnMain import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith +import org.mockito.Mockito.never import org.mockito.Mockito.spy import org.mockito.Mockito.verify import org.mozilla.fenix.R @@ -305,6 +306,95 @@ class TranslationsDialogBindingTest { ) } + @Test + fun `WHEN a non-displayable error is sent to the browserStore THEN the translation dialog store is not updated`() = + runTestOnMain { + translationsDialogStore = + spy(TranslationsDialogStore(TranslationsDialogState())) + browserStore = BrowserStore( + BrowserState( + tabs = listOf(tab), + selectedTabId = tabId, + ), + ) + + val binding = TranslationsDialogBinding( + browserStore = browserStore, + translationsDialogStore = translationsDialogStore, + sessionId = tabId, + getTranslatedPageTitle = { localizedFrom, localizedTo -> + testContext.getString( + R.string.translations_bottom_sheet_title_translation_completed, + localizedFrom, + localizedTo, + ) + }, + ) + binding.start() + + val fetchError = TranslationError.UnknownEngineSupportError(null) + browserStore.dispatch( + TranslationsAction.EngineExceptionAction( + error = fetchError, + ), + ).joinBlocking() + + verify(translationsDialogStore, never()).dispatch( + TranslationsDialogAction.UpdateTranslationError(fetchError), + ) + } + + @Test + fun `WHEN a browser and session error is sent to the browserStore THEN the session error takes priority and the translation dialog store is updated`() = + runTestOnMain { + translationsDialogStore = + spy(TranslationsDialogStore(TranslationsDialogState())) + browserStore = BrowserStore( + BrowserState( + tabs = listOf(tab), + selectedTabId = tabId, + ), + ) + + val binding = TranslationsDialogBinding( + browserStore = browserStore, + translationsDialogStore = translationsDialogStore, + sessionId = tabId, + getTranslatedPageTitle = { localizedFrom, localizedTo -> + testContext.getString( + R.string.translations_bottom_sheet_title_translation_completed, + localizedFrom, + localizedTo, + ) + }, + ) + binding.start() + + val sessionError = TranslationError.CouldNotLoadLanguagesError(null) + browserStore.dispatch( + TranslationsAction.TranslateExceptionAction( + tabId = tab.id, + operation = TranslationOperation.FETCH_SUPPORTED_LANGUAGES, + translationError = sessionError, + ), + ).joinBlocking() + + verify(translationsDialogStore).dispatch( + TranslationsDialogAction.UpdateTranslationError(sessionError), + ) + + val engineError = TranslationError.UnknownError(IllegalStateException()) + browserStore.dispatch( + TranslationsAction.EngineExceptionAction( + error = engineError, + ), + ).joinBlocking() + + verify(translationsDialogStore, never()).dispatch( + TranslationsDialogAction.UpdateTranslationError(engineError), + ) + } + @Test fun `WHEN set translation download size action sent to the browserStore THEN update translation dialog store based on operation`() = runTestOnMain { diff --git a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/translations/TranslationsDialogReducerTest.kt b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/translations/TranslationsDialogReducerTest.kt index 4abe334230ea..c743b8352b12 100644 --- a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/translations/TranslationsDialogReducerTest.kt +++ b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/translations/TranslationsDialogReducerTest.kt @@ -129,33 +129,6 @@ class TranslationsDialogReducerTest { assertEquals(PositiveButtonType.InProgress, updatedState.positiveButtonType) } - @Test - fun `WHEN the reducer is called for InitTranslationsDialog THEN a new state for PositiveButtonType is returned`() { - val translationsDialogState = TranslationsDialogState() - - val updatedState = TranslationsDialogReducer.reduce( - translationsDialogState, - TranslationsDialogAction.InitTranslationsDialog, - ) - - assertEquals(PositiveButtonType.Disabled, updatedState.positiveButtonType) - - val spanishLanguage = Language("es", "Spanish") - val englishLanguage = Language("en", "English") - val translationsDialogStateTwo = TranslationsDialogState( - initialFrom = spanishLanguage, - initialTo = englishLanguage, - positiveButtonType = PositiveButtonType.Enabled, - ) - - val updatedStateTwo = TranslationsDialogReducer.reduce( - translationsDialogStateTwo, - TranslationsDialogAction.InitTranslationsDialog, - ) - - assertEquals(PositiveButtonType.Enabled, updatedStateTwo.positiveButtonType) - } - @Test fun `WHEN the reducer is called for UpdateTranslationError THEN a new state with translation error is returned`() { val translationsDialogState = TranslationsDialogState()