refactor(android/engine): Parse keyboards.json for FirstVoices app #11943

darcywong00 · 2024-07-09T20:14:41Z

Relates to #2036

This PR migrates the FirstVoices for Android app from using keyboards.csv to using the keyboards.json metadata file published within fv_all.kmp (as of keymanapp/keyboards#2872)

All the keyboard info along with region names are included in keyboards.json, so startup is just a matter of populating the keyboard info from it.

Note: I'm leaving keyboards.csv in the repo because the FirstVoices app for iOS still uses it.

User Testing

Setup - Install the PR build of FirstVoices for Android on an Android device/emulator

TEST_FV_ANDROID - Verifies the FirstVoices for Android app runs

Launch the FirstVoices for Android app
From the main FV Setup menu

Select keyboards --> Select a region --> select a keyboard --> Enable the keyboard
Setup --> enable FirstVoices and set as the current input method

Launch Chrome browser and select a text area
Select FirstVoices as system keyboard
Verify the installed keyboard displays and functions

keymanapp-test-bot · 2024-07-09T20:14:46Z

User Test Results

Test specification and instructions

✅ TEST_FV_ANDROID (PASSED): I tested this issue with the attached "FirstVoices-18.0.81-alpha-test-11943" build on the Android 14(Physical Device) environment: Here is my observation. (notes)

Test Artifacts

Android

darcywong00 · 2024-07-09T20:17:47Z

I note the keyboard names now come straight from the individual keyboard kmn store(&NAME), so they'll vary slightly from existing keyboard names in the keyboards.csv column.

dinakaranr · 2024-07-10T12:13:55Z

Test Results

TEST_FV_ANDROID (Passed): I tested this issue with the attached "FirstVoices-18.0.71-alpha-test-11943" build on the Android 12(emulator) and 14(Physical Device) environment: Here is my observation.

Install the FirstVoices for Android app
Open the FirstVoice app.
Select keyboards --> Select a region --> Select a keyboard --> Enable the keyboard
Setup --> Enable FirstVoices and set as the current input method
Launch the Chrome browser.
Open the Google Docs or Google search box.
Select FirstVoices as the system keyboard
Type some text sentences using the FirstVoice keyboard.
Actual Results: The first Voice keyboard was installed successfully and performed with any text area in the Chrome browser.

mcdurdin

I like the way this is going. But I think we need to have better error handling and a unit test for the new function.

https://keyman.sentry.io/issues/?project=5983532&statsPeriod=90d shows that we are collecting Sentry data for FV Keyboards (this report needs auditing by the way), so let's leverage that in this function as well -- let exceptions propagate to Sentry.

mcdurdin · 2024-07-29T02:21:18Z

oem/firstvoices/android/app/src/main/java/com/firstvoices/keyboards/FVShared.java

+      if (!jsonFile.exists()) {
+          return list;
+      }


This seems like it should be fatal because if this cannot be loaded, we don't get any data for the keyboards, right?

Updated to throw fatal Error if keyboards.json doesn't exist

mcdurdin · 2024-07-29T02:21:30Z

oem/firstvoices/android/app/src/main/java/com/firstvoices/keyboards/FVShared.java

+          if (keyboardsArray == null) {
+              Log.d("loadRegionList", "unable to load keyboards.json");
+              return list;
+          }


Malformed data should at the very least be reporting to sentry, or killing the app so we know that there is something wrong.

I've updated the following to use KMLog to sentry.

Are you wanting to throw fatal errors for all of them?

Are you wanting to throw fatal errors for all of them?

I think so -- if our keyboards.json is invalid, we should reject the build at startup.

mcdurdin · 2024-07-29T02:21:55Z

oem/firstvoices/android/app/src/main/java/com/firstvoices/keyboards/FVShared.java

+              if (keyboardObj == null) {
+                  Log.d("loadRegionList", "keyboard Obj is null");
+                  continue;
+              }
+              JSONArray languageArray = keyboardObj.getJSONArray("languages");
+              if (languageArray == null) {
+                  Log.d("loadRegionList", "language Array is null");
+                  continue;
+              }


mcdurdin · 2024-07-29T02:22:57Z

oem/firstvoices/android/app/src/main/java/com/firstvoices/keyboards/FVShared.java

+      File resourceRoot = new File(getResourceRoot());
+      PackageProcessor kmpProcessor = new PackageProcessor(resourceRoot);


This doesn't seem to be used?

good catch. removed

mcdurdin · 2024-07-29T02:23:13Z

oem/firstvoices/android/app/src/main/java/com/firstvoices/keyboards/FVShared.java

+      File resourceRoot = new File(getResourceRoot());
+      PackageProcessor kmpProcessor = new PackageProcessor(resourceRoot);
+      JSONParser parser = new JSONParser();
+      File jsonFile = new File(getPackagesDir() + FVDefault_PackageID + File.separator + FVKeyboards_JSON);


If we pass this path in as a parameter, then we could add a unit test for this quite easily I think?

I could add the keyboards.json path as a parameter to loadRegionList().

I don't think the FV project is running any unit tests, so I'd have to add some plumbing for that.

I could add the keyboards.json path as a parameter to loadRegionList().

Good stuff.

I don't think the FV project is running any unit tests, so I'd have to add some plumbing for that.

Yep, future project :)

Yep, future project :)

Tracked as #12098

mcdurdin · 2024-07-29T02:24:34Z

oem/firstvoices/android/app/src/main/java/com/firstvoices/keyboards/FVShared.java

+      } catch (Exception e) {
+          Log.e("createKeyboardList", "Error: " + e);
+          // We'll return a malformed list for now in this situation
+      }


We should be crashing the app so we can detect this -- a malformed list will make the app unusable anyway, won't it?

This block was pre-existing.
I've updated to throw fatal error

mcdurdin · 2024-07-29T02:25:07Z

oem/firstvoices/android/app/src/main/java/com/firstvoices/keyboards/MainActivity.java

+        if (BuildConfig.DEBUG) {
+          KMManager.setDebugMode(true);
+        }
+        KMManager.initialize(getApplicationContext(), KMManager.KeyboardType.KEYBOARD_TYPE_INAPP);
+
        FVShared.getInstance().initialize(this);

        FVShared.getInstance().upgradeTo12();
        FVShared.getInstance().upgradeTo14();
        FVShared.getInstance().preloadPackages();

-        if (BuildConfig.DEBUG) {
-            KMManager.setDebugMode(true);
-        }
-        KMManager.initialize(getApplicationContext(), KMManager.KeyboardType.KEYBOARD_TYPE_INAPP);


Will this change in initialization order cause problems for upgrade scenarios?

Unsure - atm I don't have a clear way to test upgrade scenarios.

What was the purpose of changing the order? Was it needed for the load?

Re testing upgrade scenarios, can't we just get user testers to install an old version of say 12.x or 14.x and then upgrade it?

Previously, KMManager.initialize() hasn't happened yet, so the fv_all.kmp file hasn't been extracted.
This is why we've been copying keyboards.csv into the assets folder during the build.sh process (at the same folder as fv_all.kmp).

mcdurdin

One small change requested but apart from that LGTM

mcdurdin · 2024-08-05T22:25:57Z

oem/firstvoices/android/app/src/main/java/com/firstvoices/keyboards/FVShared.java

+          String errorMsg = "loadRegionList had malformed keyboard list";
+          KMLog.LogException(TAG, errorMsg, e);
+          // Crash the app
+          throw new Error(errorMsg);


This is an antipattern -- masking the original error in the thrown exception. Should re-throw e.

darcywong00 · 2024-08-06T07:20:28Z

After cleanup, ready for re-testing
@keymanapp-test-bot retest

dinakaranr · 2024-08-07T14:50:57Z

Test Results

TEST_FV_ANDROID (Passed): I tested this issue with the attached "FirstVoices-18.0.81-alpha-test-11943" build on the Android 14(Physical Device) environment: Here is my observation.

Install the FirstVoices for Android app.
Open the FirstVoice app.
Select keyboards --> Select a region --> Select a keyboard --> Enable the keyboard.
Setup --> Enable FirstVoices and set as the current input method.
Launch the Chrome browser.
Open the Google Docs or Google search box.
FirstVoices appears as the system keyboard.
Type some text sentences using the FirstVoice keyboard.
Actual Results: The first voice keyboard was installed successfully. Text appears in the search box using the firstvoice/keyboard. Online editor(editpad.org): Text appears using the firstvoice/keyboard.
It works well. Thank you.

keyman-server · 2024-08-08T18:02:56Z

Changes in this pull request will be available for download in Keyman version 18.0.85-alpha

fix(android/engine): Parse keyboards.json for FirstVoices app

4c5b621

darcywong00 requested a review from sgschantz as a code owner July 9, 2024 20:14

keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels Jul 9, 2024

keymanapp-test-bot bot added this to the A18S6 milestone Jul 9, 2024

github-actions bot added oem/ oem/fv/ oem/fv/android/ android/ refactor labels Jul 9, 2024

keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Jul 10, 2024

darcywong00 modified the milestones: A18S6, A18S7 Jul 19, 2024

Merge branch 'master' into fv-all/keyboards-json

2718f36

github-actions bot added android/ and removed android/ labels Jul 20, 2024

mcdurdin requested changes Jul 29, 2024

View reviewed changes

Merge remote-tracking branch 'origin/master' into fv-all/keyboards-json

0c570a1

github-actions bot added android/ and removed android/ labels Aug 2, 2024

darcywong00 modified the milestones: A18S7, A18S8 Aug 2, 2024

darcywong00 added 3 commits August 5, 2024 07:41

Merge remote-tracking branch 'origin/master' into fv-all/keyboards-json

5fca4c7

change(oem/fv/android): Add keyboard JSON path to loadRegionList()

fe758cb

fix(oem/fv/android): Add error logging

564308c

github-actions bot added android/ and removed android/ labels Aug 5, 2024

darcywong00 mentioned this pull request Aug 5, 2024

feat(oem/fv/android): Add unit tests #12098

Open

mcdurdin approved these changes Aug 5, 2024

View reviewed changes

fix(oem/fv/android): Throw errors if keyboards.json can't be read

bf86d67

github-actions bot added android/ and removed android/ labels Aug 6, 2024

keymanapp-test-bot bot added the user-test-required User tests have not been completed label Aug 6, 2024

keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Aug 7, 2024

darcywong00 merged commit 464c6f9 into master Aug 8, 2024
5 checks passed

darcywong00 deleted the fv-all/keyboards-json branch August 8, 2024 00:44

darcywong00 mentioned this pull request Aug 8, 2024

feat(android): Firstvoices: Use the metadata in kmp.json to associate with languages and versions #2036

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(android/engine): Parse keyboards.json for FirstVoices app #11943

refactor(android/engine): Parse keyboards.json for FirstVoices app #11943

darcywong00 commented Jul 9, 2024 •

edited

Loading

keymanapp-test-bot bot commented Jul 9, 2024 •

edited

Loading

darcywong00 commented Jul 9, 2024

dinakaranr commented Jul 10, 2024

mcdurdin left a comment

mcdurdin Jul 29, 2024

darcywong00 Aug 5, 2024

mcdurdin Jul 29, 2024

darcywong00 Aug 5, 2024

mcdurdin Aug 5, 2024

mcdurdin Jul 29, 2024

mcdurdin Jul 29, 2024

darcywong00 Aug 5, 2024

mcdurdin Jul 29, 2024

darcywong00 Aug 5, 2024

mcdurdin Aug 5, 2024

darcywong00 Aug 5, 2024

mcdurdin Jul 29, 2024

darcywong00 Aug 5, 2024

mcdurdin Jul 29, 2024

darcywong00 Aug 5, 2024

mcdurdin Aug 5, 2024

darcywong00 Aug 5, 2024

mcdurdin left a comment

mcdurdin Aug 5, 2024

darcywong00 commented Aug 6, 2024

dinakaranr commented Aug 7, 2024

keyman-server commented Aug 8, 2024

		File resourceRoot = new File(getResourceRoot());
		PackageProcessor kmpProcessor = new PackageProcessor(resourceRoot);

refactor(android/engine): Parse keyboards.json for FirstVoices app #11943

refactor(android/engine): Parse keyboards.json for FirstVoices app #11943

Conversation

darcywong00 commented Jul 9, 2024 • edited Loading

User Testing

keymanapp-test-bot bot commented Jul 9, 2024 • edited Loading

User Test Results

Test Artifacts

darcywong00 commented Jul 9, 2024

dinakaranr commented Jul 10, 2024

Test Results

mcdurdin left a 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

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

mcdurdin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darcywong00 commented Aug 6, 2024

dinakaranr commented Aug 7, 2024

Test Results

keyman-server commented Aug 8, 2024

darcywong00 commented Jul 9, 2024 •

edited

Loading

keymanapp-test-bot bot commented Jul 9, 2024 •

edited

Loading