Skip to content
This repository has been archived by the owner on Jun 17, 2024. It is now read-only.

Bug 1877278 - AC Translations Check for if the Engine is Supported #5544

Merged
merged 3 commits into from
Feb 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -938,6 +938,16 @@ sealed class TranslationsAction : BrowserAction() {
val translationError: TranslationError,
) : TranslationsAction(), ActionWithTab

/**
* Indicates an app level translations error occurred and to set the [TranslationError] on
* [BrowserState.translationEngine].
*
* @property error The [TranslationError] that occurred.
*/
data class EngineExceptionAction(
val error: TranslationError,
) : TranslationsAction()

/**
* Indicates that the given [operation] data should be fetched for the given [tabId].
*
Expand All @@ -949,6 +959,16 @@ sealed class TranslationsAction : BrowserAction() {
val operation: TranslationOperation,
) : TranslationsAction(), ActionWithTab

/**
* Sets whether the device architecture supports translations or not on
* [BrowserState.translationEngine].
*
* @property isEngineSupported If the engine supports translations on this device.
*/
data class SetEngineSupportedAction(
val isEngineSupported: Boolean,
) : TranslationsAction()

/**
* Sets the languages that are supported by the translations engine.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package mozilla.components.browser.state.engine.middleware
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.launch
import mozilla.components.browser.state.action.BrowserAction
import mozilla.components.browser.state.action.InitAction
import mozilla.components.browser.state.action.TranslationsAction
import mozilla.components.browser.state.selector.findTab
import mozilla.components.browser.state.state.BrowserState
Expand Down Expand Up @@ -40,6 +41,11 @@ class TranslationsMiddleware(
) {
// Pre process actions
when (action) {
is InitAction ->
scope.launch {
requestEngineSupport(context)
}

is TranslationsAction.OperationRequestedAction -> {
when (action.operation) {
TranslationOperation.FETCH_SUPPORTED_LANGUAGES -> {
Expand Down Expand Up @@ -117,6 +123,35 @@ class TranslationsMiddleware(
next(action)
}

/**
* Checks if the translations engine supports the device architecture and updates the state.
*
* @param context Context to use to dispatch to the store.
*/
private fun requestEngineSupport(
context: MiddlewareContext<BrowserState, BrowserAction>,
) {
engine.isTranslationsEngineSupported(
onSuccess = { isEngineSupported ->
context.store.dispatch(
TranslationsAction.SetEngineSupportedAction(
isEngineSupported = isEngineSupported,
),
)
logger.info("Success requesting engine support.")
},

onError = { error ->
context.store.dispatch(
TranslationsAction.EngineExceptionAction(
error = TranslationError.UnknownEngineSupportError(error),
),
)
logger.error("Error requesting engine support: ", error)
},
)
}

/**
* Retrieves the list of supported languages using [scope] and dispatches the result to the
* store via [TranslationsAction.SetSupportedLanguagesAction] or else dispatches the failure
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,17 @@ package mozilla.components.browser.state.reducer
import mozilla.components.browser.state.action.TranslationsAction
import mozilla.components.browser.state.selector.findTab
import mozilla.components.browser.state.state.BrowserState
import mozilla.components.browser.state.state.SessionState
import mozilla.components.browser.state.state.TranslationsState
import mozilla.components.concept.engine.translate.TranslationOperation
import mozilla.components.concept.engine.translate.TranslationPageSettingOperation
import mozilla.components.concept.engine.translate.TranslationPageSettings

internal object TranslationsStateReducer {

/**
* Reducer for [BrowserState.translationEngine] and [SessionState.translationsState]
*/
@Suppress("LongMethod")
fun reduce(state: BrowserState, action: TranslationsAction): BrowserState = when (action) {
is TranslationsAction.TranslateExpectedAction -> {
Expand Down Expand Up @@ -170,6 +174,10 @@ internal object TranslationsStateReducer {
}
}

is TranslationsAction.EngineExceptionAction -> {
state.copy(translationEngine = state.translationEngine.copy(engineError = action.error))
}

is TranslationsAction.SetSupportedLanguagesAction ->
state.copyWithTranslationsState(action.tabId) {
it.copy(
Expand Down Expand Up @@ -278,6 +286,15 @@ internal object TranslationsStateReducer {
}
}
}

is TranslationsAction.SetEngineSupportedAction -> {
state.copy(
translationEngine = state.translationEngine.copy(
isEngineSupported = action.isEngineSupported,
engineError = null,
),
)
}
}

private inline fun BrowserState.copyWithTranslationsState(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import java.util.Locale
* on application startup e.g. as an indicator that tabs have been restored.
* @property locale The current locale of the app. Will be null when following the system default.
* @property awesomeBarState Holds state for interactions with the [AwesomeBar].
* @property translationEngine Holds translation state that applies to the browser.
*/
data class BrowserState(
val tabs: List<TabSessionState> = emptyList(),
Expand All @@ -54,4 +55,5 @@ data class BrowserState(
val showExtensionsProcessDisabledPrompt: Boolean = false,
val extensionsProcessDisabled: Boolean = false,
val awesomeBarState: AwesomeBarState = AwesomeBarState(),
val translationEngine: TranslationsBrowserState = TranslationsBrowserState(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had trouble deciding with the naming here, please LMK if you have ideas!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
val translationEngine: TranslationsBrowserState = TranslationsBrowserState(),
val translationsEngineState: TranslationsEngineState = TranslationsEngineState(),

wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh, should have mentioned, the name of the class that the engine reports the tab state on is TranslationEngineState (singular).

Currently have:

For this one, I really struggled to find a new name:

TranslationsBrowserEngineState? TranslationsAppState? TranslationsEngineStatus? I keep struggling on this one to avoid a naming collision or something that makes sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe TranslationsState should become TabTanslationsState or TranslationsSessionState. Otherwise, TranslationsBrowState is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll leave TranslationsBrowserState for now, thanks for taking a look!

Agreed, I'll add a code health bug to refactor TranslationsState -> TranslationsSessionState. I think that'll help keep things more clear. Filed the session refactor naming to bug 1880352.

) : State
Original file line number Diff line number Diff line change
@@ -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 mozilla.components.browser.state.state

import mozilla.components.concept.engine.translate.TranslationError

/**
* Value type that represents the state of the translations engine within a [BrowserState].
*
* @property isEngineSupported Whether the translations engine supports the device architecture.
* @property engineError Holds the error state of the translations engine.
* See [TranslationsState.translationError] for session level errors.
*/
data class TranslationsBrowserState(
val isEngineSupported: Boolean? = null,
val engineError: TranslationError? = null,
)
Original file line number Diff line number Diff line change
Expand Up @@ -518,4 +518,36 @@ class TranslationsActionTest {
assertTrue(tabState().translationsState.pageSettings?.neverTranslateLanguage!!)
assertTrue(tabState().translationsState.pageSettings?.neverTranslateSite!!)
}

fun `WHEN a SetEngineSupportAction is dispatched THEN the browser store is updated to match`() {
// Initial state
assertNull(store.state.translationEngine.isEngineSupported)

// Dispatch
store.dispatch(
TranslationsAction.SetEngineSupportedAction(
isEngineSupported = true,
),
).joinBlocking()

// Final state
assertTrue(store.state.translationEngine.isEngineSupported!!)
}

@Test
fun `WHEN an EngineExceptionAction is dispatched THEN the browser store is updated to match`() {
// Initial state
assertNull(store.state.translationEngine.engineError)

// Dispatch
val error = TranslationError.UnknownError(Throwable())
store.dispatch(
TranslationsAction.EngineExceptionAction(
error = error,
),
).joinBlocking()

// Final state
assertEquals(store.state.translationEngine.engineError!!, error)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package mozilla.components.browser.state.engine.middleware

import kotlinx.coroutines.test.runTest
import mozilla.components.browser.state.action.BrowserAction
import mozilla.components.browser.state.action.InitAction
import mozilla.components.browser.state.action.TranslationsAction
import mozilla.components.browser.state.selector.findTab
import mozilla.components.browser.state.state.BrowserState
Expand Down Expand Up @@ -36,6 +37,7 @@ import org.junit.Before
import org.junit.Rule
import org.junit.Test
import org.mockito.ArgumentMatchers.anyBoolean
import org.mockito.Mockito.atLeastOnce
import org.mockito.Mockito.spy
import org.mockito.Mockito.verify

Expand Down Expand Up @@ -439,4 +441,51 @@ class TranslationsMiddlewareTest {
),
)
}

@Test
fun `WHEN InitAction is dispatched THEN SetEngineSupportAction is dispatched`() = runTest {
// Send Action
// Note: Will cause a double InitAction
translationsMiddleware.invoke(context, {}, InitAction)
waitForIdle()

// Check expectations
val engineSupportedCallback = argumentCaptor<((Boolean) -> Unit)>()
verify(engine, atLeastOnce()).isTranslationsEngineSupported(
onSuccess = engineSupportedCallback.capture(),
onError = any(),
)
engineSupportedCallback.value.invoke(true)
waitForIdle()

verify(store, atLeastOnce()).dispatch(
TranslationsAction.SetEngineSupportedAction(
isEngineSupported = true,
),
)
waitForIdle()
}

@Test
fun `WHEN InitAction is dispatched AND has an issue THEN TranslateExceptionAction is dispatched`() = runTest() {
// Send Action
// Note: Will cause a double InitAction
translationsMiddleware.invoke(context, {}, InitAction)
waitForIdle()

// Check expectations
val errorCallback = argumentCaptor<((Throwable) -> Unit)>()
verify(engine, atLeastOnce()).isTranslationsEngineSupported(
onSuccess = any(),
onError = errorCallback.capture(),
)
errorCallback.value.invoke(IllegalStateException())
waitForIdle()

verify(store, atLeastOnce()).dispatch(
TranslationsAction.EngineExceptionAction(
error = TranslationError.UnknownEngineSupportError(any()),
),
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,14 @@ sealed class TranslationError(
class EngineNotSupportedError(override val cause: Throwable?) :
TranslationError(errorName = "engine-not-supported", displayError = false, cause = cause)

/**
* Could not determine if the translations engine works on the device architecture.
*
* @param cause The original [Throwable] before it was converted into this error state.
*/
class UnknownEngineSupportError(override val cause: Throwable?) :
TranslationError(errorName = "unknown-engine-support", displayError = false, cause = cause)

/**
* Generic could not compete a translation error.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ class TelemetryMiddlewareTest {
val engine: Engine = mockk()
every { engine.enableExtensionProcessSpawning() } just runs
every { engine.disableExtensionProcessSpawning() } just runs
every { engine.isTranslationsEngineSupported(any(), any()) } just runs
ohall-m marked this conversation as resolved.
Show resolved Hide resolved
store = BrowserStore(
middleware = listOf(telemetryMiddleware) + EngineMiddleware.create(engine),
initialState = BrowserState(),
Expand Down
Loading