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

ohall-m · 2024-02-09T18:33:38Z

This patch supports the workflow for checking if the device architecture supports translations.

This patch adds:

New TranslationsBrowserState to hold global translations engine state
New SetEngineSupportedAction to set the isEngineSupported value on TranslationsBrowserState
New EngineExceptionAction to set errors on TranslationsBrowserState

Pull Request checklist

Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry or does not need one
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

Breaking Changes: If this is a breaking Android Components change, please push a draft PR on Reference Browser to address the breaking issues.

To download an APK when reviewing a PR (after all CI tasks finished running):

Click on Checks at the top of the PR page.
Click on the firefoxci-taskcluster group on the left to expand all tasks.
Click on the build-apk-{fenix,focus,klar}-debug task you're interested in.
Click on View task in Taskcluster in the new DETAILS section.
The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

https://bugzilla.mozilla.org/show_bug.cgi?id=1877278

ohall-m · 2024-02-09T20:54:16Z

...e/src/main/java/mozilla/components/browser/state/engine/middleware/TranslationsMiddleware.kt

@@ -39,8 +41,52 @@ class TranslationsMiddleware(
    ) {
        // Pre process actions
        when (action) {
+            // Initially adding the tab
+            is TabListAction.AddTabAction -> {


This bug is checking and adding to the store for if the engine is supported. (We won't show entryways into the feature if the engine isn't supported.)

Between AddTabAction (new tab), RestoreAction (reopening after a shutdown), and SelectTabAction (reopening after a shutdown and changing tabs), all the possibilities seem to be covered to ensure isEngineSupported is set on each tab.

We had a conversation about this style flow over here. Please let me know if you have suggestions or ideas!

1873672 will probably use these same actions to initialize the rest of the translations data needs. I'd like to get a stronger sense of how well it works for this case alone first, however we go about it.

Hey, @MatthewTighe and @gabrielluong, what do you think about this? Is there anything else I should consider or do you think it is good to start testing this option?

Based on the amount of research you did, I feel like this is good enough. I don't really have enough context on these actions to provide a better suggestion.

My understanding of what we are trying to achieve here is to make sure all the tabs have this engine supported property set. The only alternative I could see is calling this on-demand for the individual tab that is displayed instead of doing this for all the tabs. If I recall correctly, this alternative was what you first tried to do initially. Did we decide against this?

Do you have a sense of whether or not this operation is expensive? For instance, what happens if an user tries to add/restore 50, 100 or 1000 tabs? This might be worth testing with the debug tools.

Do we need to check if the engine is supported per tab?

I was re-reading the description for one of the properties below:

@Property isEngineSupported If the engine supports translations on this device.

and was wondering if we could just set this only once since this seem to indicate whether or not the engine supports translations on the device, which seems like a global state and not based on per tab, but on the user's device. Does the translation engine settings need to fetched per tab or do we only need to know if the engine is ready?

+1 to this question about whether the state is global. though it is useful on the one hand to have all the translation state encapsulated in TranslationState, any global state could definitely move to the top-level browser state or even a BrowserState.translationState. If you went the latter route, the current TranslationState may make more sense as SessionTranslationState

Thanks for taking a look!

Do we need to check if the engine is supported per tab?

Not necessarily, this won't change once found.

Does the translation engine settings need to fetched per tab or do we only need to know if the engine is ready?

This isEngineSupported could be a global setting. 100% it is based on the users device and my understanding is it won't change, so once we know, we know. (Only exception might be something like a possible firmware update, but I think that would be an extreme example and generally not easily changed - more details.)

The engine automatically becoming ready is fully managed by Gecko AFAIK currently. They are also managing the new single process 1815339.

LMK how we normally handle something like this and I can refactor for sure!

If you went the latter route, the current TranslationState may make more sense as SessionTranslationState

That could be interesting because supported languages are also not directly tied to the current tab. I'd say download state would probably make sense there too.

(I think there could also be an argument made for the never translate site and languages global settings to be on the global state, but they have a lot of coupling with the page settings, so they probably shouldn't be on a global state and stay tied to one for tabs/sessions.)

Do you have an example of something similar to this I could pattern off of?

Just looking through BrowserState, there is global extension state and then tab-specific extension state in TabSessionState. I'm not really familiar with extensions, but I would be happy to chat through any implementation details if you'd like

Awesome, thanks! I'll try following it and see where I end up. (Interestingly, I had to follow a lot of extensions patterns on the GeckoView side as well, so that makes sense too!)

Thanks for taking a look! I followed how WebExtensions managed this as a general pattern to follow and it seems like a better pattern overall and reduces a lot of the weird tab actions. I'm still going to have to figure this out for initializing tabs, but I think the Init action is perfect for this case and have some new ideas to look into for the tab cases.

fenix/app/src/test/java/org/mozilla/fenix/telemetry/TelemetryMiddlewareTest.kt

...ser/state/src/main/java/mozilla/components/browser/state/reducer/TranslationsStateReducer.kt

...ponents/browser/state/src/main/java/mozilla/components/browser/state/action/BrowserAction.kt

...e/src/main/java/mozilla/components/browser/state/engine/middleware/TranslationsMiddleware.kt

gabrielluong · 2024-02-12T18:57:28Z

...e/src/main/java/mozilla/components/browser/state/engine/middleware/TranslationsMiddleware.kt

@@ -39,8 +41,52 @@ class TranslationsMiddleware(
    ) {
        // Pre process actions
        when (action) {
+            // Initially adding the tab
+            is TabListAction.AddTabAction -> {


Based on the amount of research you did, I feel like this is good enough. I don't really have enough context on these actions to provide a better suggestion.

My understanding of what we are trying to achieve here is to make sure all the tabs have this engine supported property set. The only alternative I could see is calling this on-demand for the individual tab that is displayed instead of doing this for all the tabs. If I recall correctly, this alternative was what you first tried to do initially. Did we decide against this?

Do you have a sense of whether or not this operation is expensive? For instance, what happens if an user tries to add/restore 50, 100 or 1000 tabs? This might be worth testing with the debug tools.

MatthewTighe · 2024-02-12T19:22:55Z

...e/src/main/java/mozilla/components/browser/state/engine/middleware/TranslationsMiddleware.kt

@@ -39,8 +41,52 @@ class TranslationsMiddleware(
    ) {
        // Pre process actions
        when (action) {
+            // Initially adding the tab
+            is TabListAction.AddTabAction -> {


+1 to this question about whether the state is global. though it is useful on the one hand to have all the translation state encapsulated in TranslationState, any global state could definitely move to the top-level browser state or even a BrowserState.translationState. If you went the latter route, the current TranslationState may make more sense as SessionTranslationState

MatthewTighe · 2024-02-12T19:28:18Z

...e/src/main/java/mozilla/components/browser/state/engine/middleware/TranslationsMiddleware.kt

+                    TranslationOperation.FETCH_IS_ENGINE_SUPPORTED -> {
+                        scope.launch {
+                            requestEngineSupport(context, action.tabId)
+                        }
+                    }


I'm only seeing that this operation is only really dispatched from this middleware, and only really used in this middleware outside of resetting the error state. I'm wondering if we need the additional operation/action dispatch, or if we can just call requestEngineSupport directly from things like AddTabAction.

If the isEngineSupported is global state as per the questions above, could we even maybe use BrowserAction.Init? I do seem to remember that you looked into that but can't recall the details

Thanks, updated the pattern!

I can use BrowserAction.Init now with the tab independent state. The earlier issue was I needed a tabId to set on, if I set per tab. I still might have to figure out alternatives for tab initialization, but BrowserAction.Init seems much more stable for this patch.

mergify · 2024-02-13T23:23:34Z

This pull request has conflicts when rebasing. Could you fix it @ohall-m? 🙏

ohall-m · 2024-02-14T15:45:47Z

...omponents/browser/state/src/main/java/mozilla/components/browser/state/state/BrowserState.kt

@@ -54,4 +55,5 @@ data class BrowserState(
    val showExtensionsProcessDisabledPrompt: Boolean = false,
    val extensionsProcessDisabled: Boolean = false,
    val awesomeBarState: AwesomeBarState = AwesomeBarState(),
+    val translationEngine: TranslationsBrowserState = TranslationsBrowserState(),


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

gabrielluong · 2024-02-14T18:05:44Z

...ponents/browser/state/src/main/java/mozilla/components/browser/state/action/BrowserAction.kt

+     * Indicates an app level translations error occurred and to set the error on
+     * [BrowserState.translationEngine].
+     *
+     * @property error The error that occurred.


Suggested change

* @property error The error that occurred.

* @property error The [TranslationError] that occurred.

We could reference the class where possible if the naming and wording just makes sense.

Thanks, good idea, updated!

gabrielluong · 2024-02-14T18:08:14Z

...omponents/browser/state/src/main/java/mozilla/components/browser/state/state/BrowserState.kt

@@ -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 state for translations that apply to the entire browser.


Suggested change

* @property translationEngine Holds state for translations that apply to the entire browser.

* @property translationEngine Holds translation state that applies to the browser.

Ty, updated

gabrielluong · 2024-02-14T18:12:02Z

...omponents/browser/state/src/main/java/mozilla/components/browser/state/state/BrowserState.kt

@@ -54,4 +55,5 @@ data class BrowserState(
    val showExtensionsProcessDisabledPrompt: Boolean = false,
    val extensionsProcessDisabled: Boolean = false,
    val awesomeBarState: AwesomeBarState = AwesomeBarState(),
+    val translationEngine: TranslationsBrowserState = TranslationsBrowserState(),


Suggested change

val translationEngine: TranslationsBrowserState = TranslationsBrowserState(),

val translationsEngineState: TranslationsEngineState = TranslationsEngineState(),

wdyt?

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

Currently have:

TranslationsState - This is the session one.

TranslationEngineState - This is information GeckoView reports about the session that is set on the session state.

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.

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

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.

gabrielluong · 2024-02-14T18:12:33Z

...owser/state/src/main/java/mozilla/components/browser/state/state/TranslationsBrowserState.kt

+/**
+ * Value type that represents the state of the translations engine within a [BrowserState].
+ *
+ * @property isEngineSupported The translations engine supports the device architecture.


Suggested change

* @property isEngineSupported The translations engine supports the device architecture.

* @property isEngineSupported Whether the translations engine supports the device architecture.

Just extra wording to indicate it is a boolean

Ty, nice wording

gabrielluong · 2024-02-14T18:13:14Z

...owser/state/src/main/java/mozilla/components/browser/state/state/TranslationsBrowserState.kt

+ * Value type that represents the state of the translations engine within a [BrowserState].
+ *
+ * @property isEngineSupported The translations engine supports the device architecture.
+ * @property engineError If the translations engine has a global error state.


Suggested change

* @property engineError If the translations engine has a global error state.

* @property engineError Holds the error state of the translations engine.

Good call, ty!

gabrielluong · 2024-02-14T18:14:04Z

...concept/engine/src/main/java/mozilla/components/concept/engine/translate/TranslationError.kt

+    /**
+     * 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.


Suggested change

* @param cause The original throwable before it was converted into this error state.

* @param cause The original [Throwable] before it was converted into this error state.

Updated, ty!

This patch supports the workflow for checking if the device architecture supports translations. This patch adds: * New `TranslationsBrowserState` to hold global translations engine state * New `SetEngineSupportedAction` to set the isEngineSupported value on `TranslationsBrowserState` * New `EngineExceptionAction` to set errors on `TranslationsBrowserState`

rvandermeulen · 2024-02-14T21:24:08Z

@Mergifyio refresh

mergify · 2024-02-14T21:24:15Z

refresh

✅ Pull request refreshed

github-actions bot added the 🕵️‍♀️ needs review PRs that need to be reviewed label Feb 9, 2024

ohall-m added work in progress Not ready to land yet. Work in progress (WIP). and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Feb 9, 2024

ohall-m closed this Feb 9, 2024

ohall-m reopened this Feb 9, 2024

github-actions bot added 🕵️‍♀️ needs review PRs that need to be reviewed and removed work in progress Not ready to land yet. Work in progress (WIP). labels Feb 9, 2024

ohall-m force-pushed the bug1877278 branch from 1784446 to f992e4e Compare February 9, 2024 20:03

ohall-m added work in progress Not ready to land yet. Work in progress (WIP). and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Feb 9, 2024

github-actions bot added 🕵️‍♀️ needs review PRs that need to be reviewed and removed work in progress Not ready to land yet. Work in progress (WIP). labels Feb 9, 2024

ohall-m closed this Feb 9, 2024

ohall-m reopened this Feb 9, 2024

ohall-m commented Feb 9, 2024

View reviewed changes

ohall-m requested review from MatthewTighe and calumozilla February 9, 2024 20:59

github-actions bot assigned calumozilla and MatthewTighe Feb 9, 2024

calumozilla reviewed Feb 12, 2024

View reviewed changes

...ser/state/src/main/java/mozilla/components/browser/state/reducer/TranslationsStateReducer.kt Outdated Show resolved Hide resolved

calumozilla approved these changes Feb 12, 2024

View reviewed changes

ohall-m force-pushed the bug1877278 branch from f992e4e to e211c10 Compare February 12, 2024 17:32

github-actions bot added approved PR that has been approved and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Feb 12, 2024

gabrielluong reviewed Feb 12, 2024

View reviewed changes

MatthewTighe reviewed Feb 12, 2024

View reviewed changes

ohall-m added work in progress Not ready to land yet. Work in progress (WIP). changes required PR that requires some changes before landing. and removed approved PR that has been approved labels Feb 12, 2024

ohall-m force-pushed the bug1877278 branch from e211c10 to a0e4e64 Compare February 13, 2024 18:33

ohall-m added 🕵️‍♀️ needs review PRs that need to be reviewed and removed approved PR that has been approved labels Feb 13, 2024

ohall-m mentioned this pull request Feb 13, 2024

Bug 1856979 - Translations UI Post-Translate Popup #5242

Merged

4 tasks

ohall-m force-pushed the bug1877278 branch from 1322455 to e6669f7 Compare February 13, 2024 19:55

github-actions bot added approved PR that has been approved and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Feb 13, 2024

ohall-m force-pushed the bug1877278 branch from e6669f7 to 6b4cad0 Compare February 13, 2024 21:02

ohall-m requested review from calumozilla, MatthewTighe and gabrielluong February 13, 2024 21:05

github-actions bot assigned gabrielluong Feb 13, 2024

ohall-m force-pushed the bug1877278 branch from 6b4cad0 to cbb7272 Compare February 14, 2024 15:42

ohall-m added 🕵️‍♀️ needs review PRs that need to be reviewed and removed approved PR that has been approved labels Feb 14, 2024

ohall-m commented Feb 14, 2024

View reviewed changes

gabrielluong approved these changes Feb 14, 2024

View reviewed changes

ohall-m force-pushed the bug1877278 branch from cbb7272 to 5ac5e17 Compare February 14, 2024 18:56

github-actions bot added approved PR that has been approved and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Feb 14, 2024

ohall-m force-pushed the bug1877278 branch from 5ac5e17 to c77a6e9 Compare February 14, 2024 19:27

ohall-m added the 🛬 needs landing PRs that are ready to land label Feb 14, 2024

Merge branch 'main' into bug1877278

ba9cf75

ohall-m closed this Feb 14, 2024

ohall-m reopened this Feb 14, 2024

Merge branch 'main' into bug1877278

32e2746

mergify bot merged commit b3aaa1e into mozilla-mobile:main Feb 14, 2024
111 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ohall-m commented Feb 9, 2024 •

edited by github-actions bot

Loading

ohall-m Feb 9, 2024 •

edited

Loading

ohall-m Feb 12, 2024

gabrielluong Feb 12, 2024 •

edited

Loading

gabrielluong Feb 12, 2024 •

edited

Loading

MatthewTighe Feb 12, 2024

ohall-m Feb 12, 2024 •

edited

Loading

ohall-m Feb 12, 2024

MatthewTighe Feb 12, 2024

ohall-m Feb 12, 2024

ohall-m Feb 13, 2024

gabrielluong Feb 12, 2024 •

edited

Loading

MatthewTighe Feb 12, 2024

MatthewTighe Feb 12, 2024

ohall-m Feb 13, 2024

mergify bot commented Feb 13, 2024

ohall-m Feb 14, 2024

gabrielluong Feb 14, 2024

ohall-m Feb 14, 2024

gabrielluong Feb 14, 2024

ohall-m Feb 14, 2024

gabrielluong Feb 14, 2024

ohall-m Feb 14, 2024

gabrielluong Feb 14, 2024

ohall-m Feb 14, 2024

gabrielluong Feb 14, 2024

ohall-m Feb 14, 2024

gabrielluong Feb 14, 2024

ohall-m Feb 14, 2024

gabrielluong Feb 14, 2024

ohall-m Feb 14, 2024

rvandermeulen commented Feb 14, 2024

mergify bot commented Feb 14, 2024

	* @property error The error that occurred.
	* @property error The [TranslationError] that occurred.

	* @property translationEngine Holds state for translations that apply to the entire browser.
	* @property translationEngine Holds translation state that applies to the browser.

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

	* @property isEngineSupported The translations engine supports the device architecture.
	* @property isEngineSupported Whether the translations engine supports the device architecture.

	* @property engineError If the translations engine has a global error state.
	* @property engineError Holds the error state of the translations engine.

	* @param cause The original throwable before it was converted into this error state.
	* @param cause The original [Throwable] before it was converted into this error state.

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

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

Conversation

ohall-m commented Feb 9, 2024 • edited by github-actions bot Loading

Pull Request checklist

After merge

To download an APK when reviewing a PR (after all CI tasks finished running):

GitHub Automation

ohall-m Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielluong Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

gabrielluong Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohall-m Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielluong Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Feb 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvandermeulen commented Feb 14, 2024

mergify bot commented Feb 14, 2024

✅ Pull request refreshed

ohall-m commented Feb 9, 2024 •

edited by github-actions bot

Loading

ohall-m Feb 9, 2024 •

edited

Loading

gabrielluong Feb 12, 2024 •

edited

Loading

gabrielluong Feb 12, 2024 •

edited

Loading

ohall-m Feb 12, 2024 •

edited

Loading

gabrielluong Feb 12, 2024 •

edited

Loading