From 4bb2681d5d7474ea32b464c7a10fade20e7ce6f3 Mon Sep 17 00:00:00 2001 From: ohall-m Date: Mon, 26 Feb 2024 17:00:07 -0500 Subject: [PATCH] Bug 1880490 - Moving Translations `supportedLanguages` This patch refactors Translations `supportedLanguages` from `[SessionState.translationsState]` to `[BrowserState.translationEngine]` in AC. * Additionally adds `InitTranslationsBrowserState` to signal populating `[BrowserState.translationEngine]`. * This patch also makes adjustments to migrates Fenix's `TranslationsDialogBinding` and `TranslationsBinding` to also monitor this new store location. --- .../fenix/browser/TranslationsBinding.kt | 75 +++++++++++----- .../translations/TranslationsDialogBinding.kt | 88 ++++++++++++------- .../translations/TranslationsDialogStore.kt | 6 +- .../translations/TranslationsFlowState.kt | 19 ++++ .../fenix/browser/TranslationsBindingTest.kt | 1 - .../telemetry/TelemetryMiddlewareTest.kt | 1 + .../TranslationsDialogBindingTest.kt | 2 - 7 files changed, 130 insertions(+), 62 deletions(-) create mode 100644 app/src/main/java/org/mozilla/fenix/translations/TranslationsFlowState.kt diff --git a/app/src/main/java/org/mozilla/fenix/browser/TranslationsBinding.kt b/app/src/main/java/org/mozilla/fenix/browser/TranslationsBinding.kt index 433657da3d..4a79afdce2 100644 --- a/app/src/main/java/org/mozilla/fenix/browser/TranslationsBinding.kt +++ b/app/src/main/java/org/mozilla/fenix/browser/TranslationsBinding.kt @@ -5,6 +5,7 @@ package org.mozilla.fenix.browser import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.distinctUntilChangedBy import kotlinx.coroutines.flow.mapNotNull import mozilla.components.browser.state.selector.findTab @@ -15,6 +16,7 @@ import mozilla.components.concept.engine.translate.Language import mozilla.components.concept.engine.translate.initialFromLanguage import mozilla.components.concept.engine.translate.initialToLanguage import mozilla.components.lib.state.helpers.AbstractBinding +import org.mozilla.fenix.translations.TranslationsFlowState /** * A binding for observing [TranslationsState] changes @@ -36,37 +38,62 @@ class TranslationsBinding( ) : AbstractBinding(browserStore) { override suspend fun onState(flow: Flow) { - flow.mapNotNull { state -> state.findTab(sessionId) }.distinctUntilChangedBy { - it.translationsState - }.collect { sessionState -> - val translationsState = sessionState.translationsState + // Browser level flows + val browserFlow = flow.mapNotNull { state -> state } + .distinctUntilChangedBy { + it.translationEngine + } - if (translationsState.isTranslated) { - val fromSelected = translationsState.translationEngineState?.initialFromLanguage( - translationsState.supportedLanguages?.fromLanguages, - ) - val toSelected = translationsState.translationEngineState?.initialToLanguage( - translationsState.supportedLanguages?.toLanguages, + // Session level flows + val sessionFlow = flow.mapNotNull { state -> state.findTab(sessionId) } + .distinctUntilChangedBy { + it.translationsState + } + + // Applying the flows together + sessionFlow + .combine(browserFlow) { sessionState, browserState -> + TranslationsFlowState( + sessionState, + browserState, ) + } + .collect { state -> + // Browser Translations State Behavior (Global) + val browserTranslationsState = state.browserState.translationEngine + val translateFromLanguages = + browserTranslationsState.supportedLanguages?.fromLanguages + val translateToLanguages = + browserTranslationsState.supportedLanguages?.toLanguages - if (fromSelected != null && toSelected != null) { + // Session Translations State Behavior (Tab) + val sessionTranslationsState = state.sessionState.translationsState + if (sessionTranslationsState.isTranslated) { + val fromSelected = sessionTranslationsState.translationEngineState?.initialFromLanguage( + translateFromLanguages, + ) + val toSelected = sessionTranslationsState.translationEngineState?.initialToLanguage( + translateToLanguages, + ) + + if (fromSelected != null && toSelected != null) { + onStateUpdated( + true, + true, + fromSelected, + toSelected, + ) + } + } else if (sessionTranslationsState.isExpectedTranslate) { onStateUpdated( true, - true, - fromSelected, - toSelected, + false, + null, + null, ) + } else { + onStateUpdated(false, false, null, null) } - } else if (translationsState.isExpectedTranslate) { - onStateUpdated( - true, - false, - null, - null, - ) - } else { - onStateUpdated(false, false, null, null) } - } } } diff --git a/app/src/main/java/org/mozilla/fenix/translations/TranslationsDialogBinding.kt b/app/src/main/java/org/mozilla/fenix/translations/TranslationsDialogBinding.kt index 3d1e838197..e8f002b574 100644 --- a/app/src/main/java/org/mozilla/fenix/translations/TranslationsDialogBinding.kt +++ b/app/src/main/java/org/mozilla/fenix/translations/TranslationsDialogBinding.kt @@ -5,17 +5,20 @@ package org.mozilla.fenix.translations import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.distinctUntilChangedBy 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.store.BrowserStore import mozilla.components.concept.engine.translate.initialFromLanguage import mozilla.components.concept.engine.translate.initialToLanguage import mozilla.components.lib.state.helpers.AbstractBinding /** - * Helper for observing Translation state from [BrowserStore]. + * Helper for observing Translation state from both [BrowserState.translationEngine] + * and [TabSessionState.translationsState]. */ class TranslationsDialogBinding( browserStore: BrowserStore, @@ -24,17 +27,59 @@ class TranslationsDialogBinding( private val getTranslatedPageTitle: (localizedFrom: String?, localizedTo: String?) -> String, ) : AbstractBinding(browserStore) { + @Suppress("LongMethod") override suspend fun onState(flow: Flow) { - flow.mapNotNull { state -> state.findTab(sessionId) } + // Browser level flows + val browserFlow = flow.mapNotNull { state -> state } + .distinctUntilChangedBy { + it.translationEngine + } + + // Session level flows + val sessionFlow = flow.mapNotNull { state -> state.findTab(sessionId) } .distinctUntilChangedBy { it.translationsState } - .collect { sessionState -> - val translationsState = sessionState.translationsState + // Applying the flows together + sessionFlow + .combine(browserFlow) { sessionState, browserState -> TranslationsFlowState(sessionState, browserState) } + .collect { + state -> + // Browser Translations State Behavior (Global) + val browserTranslationsState = state.browserState.translationEngine + val translateFromLanguages = + browserTranslationsState.supportedLanguages?.fromLanguages + translateFromLanguages?.let { + translationsDialogStore.dispatch( + TranslationsDialogAction.UpdateTranslateFromLanguages( + translateFromLanguages, + ), + ) + } + + val translateToLanguages = + browserTranslationsState.supportedLanguages?.toLanguages + translateToLanguages?.let { + translationsDialogStore.dispatch( + TranslationsDialogAction.UpdateTranslateToLanguages( + translateToLanguages, + ), + ) + } + + // Dispatch engine level errors + if (browserTranslationsState.engineError != null) { + translationsDialogStore.dispatch( + TranslationsDialogAction.UpdateTranslationError(browserTranslationsState.engineError), + ) + } + + // Session Translations State Behavior (Tab) + val sessionTranslationsState = state.sessionState.translationsState val fromSelected = - translationsState.translationEngineState?.initialFromLanguage( - translationsState.supportedLanguages?.fromLanguages, + sessionTranslationsState.translationEngineState?.initialFromLanguage( + translateFromLanguages, ) fromSelected?.let { @@ -46,8 +91,8 @@ class TranslationsDialogBinding( } val toSelected = - translationsState.translationEngineState?.initialToLanguage( - translationsState.supportedLanguages?.toLanguages, + sessionTranslationsState.translationEngineState?.initialToLanguage( + translateToLanguages, ) toSelected?.let { translationsDialogStore.dispatch( @@ -71,35 +116,18 @@ class TranslationsDialogBinding( ) } - val translateFromLanguages = translationsState.supportedLanguages?.fromLanguages - translateFromLanguages?.let { - translationsDialogStore.dispatch( - TranslationsDialogAction.UpdateTranslateFromLanguages( - translateFromLanguages, - ), - ) - } - - val translateToLanguages = translationsState.supportedLanguages?.toLanguages - translateToLanguages?.let { - translationsDialogStore.dispatch( - TranslationsDialogAction.UpdateTranslateToLanguages( - translateToLanguages, - ), - ) - } - - if (translationsState.isTranslateProcessing) { + if (sessionTranslationsState.isTranslateProcessing) { updateStoreIfIsTranslateProcessing() } - if (translationsState.isTranslated && !translationsState.isTranslateProcessing) { + if (sessionTranslationsState.isTranslated && !sessionTranslationsState.isTranslateProcessing) { updateStoreIfTranslated() } - if (translationsState.translationError != null) { + // A session error may override a browser error + if (sessionTranslationsState.translationError != null) { translationsDialogStore.dispatch( - TranslationsDialogAction.UpdateTranslationError(translationsState.translationError), + TranslationsDialogAction.UpdateTranslationError(sessionTranslationsState.translationError), ) } } diff --git a/app/src/main/java/org/mozilla/fenix/translations/TranslationsDialogStore.kt b/app/src/main/java/org/mozilla/fenix/translations/TranslationsDialogStore.kt index 8fb5cd0ce5..f97d1e0e60 100644 --- a/app/src/main/java/org/mozilla/fenix/translations/TranslationsDialogStore.kt +++ b/app/src/main/java/org/mozilla/fenix/translations/TranslationsDialogStore.kt @@ -25,11 +25,7 @@ class TranslationsDialogStore( initialState, TranslationsDialogReducer::reduce, middlewares, -) { - init { - dispatch(TranslationsDialogAction.FetchSupportedLanguages) - } -} +) /** * The current state of the Translations bottom sheet dialog. diff --git a/app/src/main/java/org/mozilla/fenix/translations/TranslationsFlowState.kt b/app/src/main/java/org/mozilla/fenix/translations/TranslationsFlowState.kt new file mode 100644 index 0000000000..3e2610a98d --- /dev/null +++ b/app/src/main/java/org/mozilla/fenix/translations/TranslationsFlowState.kt @@ -0,0 +1,19 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package org.mozilla.fenix.translations + +import mozilla.components.browser.state.state.BrowserState +import mozilla.components.browser.state.state.TabSessionState + +/** + * Convenience method to create a named pair for the translations flow. + * + * @property sessionState The session or tab state. + * @property browserState The browser or global state. + */ +data class TranslationsFlowState( + val sessionState: TabSessionState, + val browserState: BrowserState, +) diff --git a/app/src/test/java/org/mozilla/fenix/browser/TranslationsBindingTest.kt b/app/src/test/java/org/mozilla/fenix/browser/TranslationsBindingTest.kt index b58169f98b..672397ffe6 100644 --- a/app/src/test/java/org/mozilla/fenix/browser/TranslationsBindingTest.kt +++ b/app/src/test/java/org/mozilla/fenix/browser/TranslationsBindingTest.kt @@ -83,7 +83,6 @@ class TranslationsBindingTest { browserStore.dispatch( TranslationsAction.SetSupportedLanguagesAction( - tabId = tab.id, supportedLanguages = supportLanguages, ), ).joinBlocking() diff --git a/app/src/test/java/org/mozilla/fenix/telemetry/TelemetryMiddlewareTest.kt b/app/src/test/java/org/mozilla/fenix/telemetry/TelemetryMiddlewareTest.kt index dfbb1ba682..3e88112607 100644 --- a/app/src/test/java/org/mozilla/fenix/telemetry/TelemetryMiddlewareTest.kt +++ b/app/src/test/java/org/mozilla/fenix/telemetry/TelemetryMiddlewareTest.kt @@ -86,6 +86,7 @@ class TelemetryMiddlewareTest { val engine: Engine = mockk() every { engine.enableExtensionProcessSpawning() } just runs every { engine.disableExtensionProcessSpawning() } just runs + every { engine.getSupportedTranslationLanguages(any(), any()) } just runs every { engine.isTranslationsEngineSupported(any(), any()) } just runs store = BrowserStore( middleware = listOf(telemetryMiddleware) + EngineMiddleware.create(engine), diff --git a/app/src/test/java/org/mozilla/fenix/translations/TranslationsDialogBindingTest.kt b/app/src/test/java/org/mozilla/fenix/translations/TranslationsDialogBindingTest.kt index 4f0bc45904..06653a4a19 100644 --- a/app/src/test/java/org/mozilla/fenix/translations/TranslationsDialogBindingTest.kt +++ b/app/src/test/java/org/mozilla/fenix/translations/TranslationsDialogBindingTest.kt @@ -88,7 +88,6 @@ class TranslationsDialogBindingTest { browserStore.dispatch( TranslationsAction.SetSupportedLanguagesAction( - tabId = tab.id, supportedLanguages = supportLanguages, ), ).joinBlocking() @@ -199,7 +198,6 @@ class TranslationsDialogBindingTest { val supportedLanguages = TranslationSupport(listOf(fromLanguage), listOf(toLanguage)) browserStore.dispatch( TranslationsAction.SetSupportedLanguagesAction( - tabId = tab.id, supportedLanguages = supportedLanguages, ), ).joinBlocking()