fix(android): show updates for lexical models - #16266
Conversation
keymanapp-test-bot
Bot
added
the
user-test-missing
User Test ResultsTest specification and instructions
Test Artifacts |
8c5b51a to
0bc6a38
Compare
keymanapp-test-bot
Bot
added
has-user-test
user-test-required
88e18d5 to
f1adb22
Compare
f1adb22 to
ad1fbc2
Compare
With this change the model info activity will show an available update to the lexical model. Commit partially drafted by AI (Gemini 3 Flash Preview). Fixes: #16226 Build-bot: release:android
ad1fbc2 to
a332c0d
Compare
|
I am getting this error when dropping the apk file into the Android Emulator:
Can you help check? Updated: |
|
Physical device: Galaxy A05, Android v15 One UI v7.0 |
Test ResultsGROUP_OLDAPI:Test Specs
Test Prerequisites
GROUP_NEWAPI:Test Specs
Test Prerequisites
Note
Screen.Recording.2026-07-31.at.11.07.33.in.the.morning.mp4
I am pointing these out because the test cases and test results are a bit mismatched so I had to do some workaround. |
keymanapp-test-bot
Bot
removed
the
user-test-required
There was a problem hiding this comment.
It feels like there's extra work being included here past what's actually needed for a fix. Additional documentation may help further clarify the reasoning behind all of the changes.
Noting what a devin.ai review puts out:
This PR fixes the Android app to properly detect and display available updates for lexical models. The problem was multi-faceted:
- The processLexicalModelPackageUpdateJSON call was missing from the cloud catalog download callback, so model updates were never checked.
- The update check logic used a wrong version key (KMKey_Version instead of KMKey_LexicalModelVersion) and only looked up a single model by index instead of iterating all matching models.
- The model info activity didn't receive enough data to show update status because LanguageSettingsActivity wasn't passing the associated lexical model info.
- When merging cloud catalog models with installed models, addAll was used instead of a proper version-aware merge, causing duplicates or stale data.
From my current view, the actual core bug here was probably item 4. If an old version remains in the list, and comes first, it makes sense that the new version's entry never got reached. Items 1 and 2 do appear to support this as well.
I can see reasoning for item 3: if the cloud query operations suddenly decide that a different lexical model should be default for a language offering two or more, that could certainly pose an issue. Basically, when the user has one installed on-device, consider that one. The thing is... this case isn't currently being clearly or explicitly user-tested or unit-tested, so far as I can see, it wasn't noted in the base issue, and there's little in the PR description noting this aspect.
I would want to see, at minimum, actual user and/or unit testing for the case actually necessitating item 3 - the changes in how LanguageSettingsActivity passes data to ModelPickerActivity, in particular. I'd prefer spinning it off into a separate commit, if not PR and issue, unless it can be clearly shown to be necessary for a fix even for languages supported by a single model.
| i.addFlags(Intent.FLAG_ACTIVITY_NO_HISTORY); | ||
| i.putExtras(bundle); | ||
| startActivity(i); | ||
| HashMap<String, String> lmInfo = KMManager.getAssociatedLexicalModel(lgCode); |
There was a problem hiding this comment.
So, it looks like we're adding an alternate way to set the lexical model's data in place... but preserving the old one?
Are there any reasonable conditions for the getAssociatedLexicalModel call to fail? Why are we keeping the bundle.putString approach around, if not?
What advantages does the new approach offer? If the answer is mostly just "it's cleaner"... then why does this not clean up related code on the other side in ModelPickerActivity?
There was a problem hiding this comment.
Yes, I should have reverted this change. It's not necessary for this PR. I started out to do the cleanup but then discovered that it would require too many changes not related to fixing this bug, so I didn't continue the cleanup. As part of my exploration why it's not working I tried to make it more similar to how it's done for the keyboards.
| HashMap<String, String> lmInfo = BundleCompat.getSerializable(bundle, KMManager.KMKey_LexicalModel, HashMap.class); | ||
| final String newLanguageID = lmInfo != null ? lmInfo.get(KMManager.KMKey_LanguageID) : bundle.getString(KMManager.KMKey_LanguageID, ""); | ||
| final String newCustomHelpLink = lmInfo != null ? lmInfo.get(KMManager.KMKey_CustomHelpLink) : bundle.getString(KMManager.KMKey_CustomHelpLink, ""); | ||
| final String languageName = lmInfo != null ? lmInfo.get(KMManager.KMKey_LanguageName) : bundle.getString(KMManager.KMKey_LanguageName, ""); |
There was a problem hiding this comment.
This appears to be the "other side" of the previously-noted "alternate way" comment. The logic here is complexified due to selecting between two approaches, where just one appeared to work previously.
| downloadMetaDataFromServer(context,updateHandler,onSuccess,onFailure); | ||
| } | ||
|
|
||
| private void mergeLexicalModels(Dataset.LexicalModels datasetModels, List<LexicalModel> newModels) { |
| } | ||
| if (kmpLexicalModelsArray.length() > 0) { | ||
| memCachedDataset.lexicalModels.addAll(CloudDataJsonUtil.processLexicalModelJSON(kmpLexicalModelsArray, fromKMP)); | ||
| mergeLexicalModels(memCachedDataset.lexicalModels, CloudDataJsonUtil.processLexicalModelJSON(kmpLexicalModelsArray, true)); |
There was a problem hiding this comment.
I feel like this section could use some documentation.
So... this step aims to reconcile previously-cached data with the new incoming data from the cloud? Rather than replace the old data wholesale, we preserve entries for models that appear to no longer exist?
I do realize the old code was likely doing something similar here, but I feel like these lines would be well-served by comments providing additional context.
There was a problem hiding this comment.
Honestly, I don't fully understand why we're doing this here. Since it was here before I kept it.
All the models that we added from installedSet above DO exist - the set is based on the locally installed models. Here we add all models that exist locally - whether installed or not. The difference to installedSet is that only installedSet has the download link for the new version.
It's possible that just using installedSet would be sufficient - at least it works with just that, but I don't know which edge cases that would break, i.e. what we would loose if models that exit locally but are not installed wouldn't be in the dataset.
| i.addFlags(Intent.FLAG_ACTIVITY_NO_HISTORY); | ||
| i.putExtras(bundle); | ||
| startActivity(i); | ||
| HashMap<String, String> lmInfo = KMManager.getAssociatedLexicalModel(lgCode); |
There was a problem hiding this comment.
Yes, I should have reverted this change. It's not necessary for this PR. I started out to do the cleanup but then discovered that it would require too many changes not related to fixing this bug, so I didn't continue the cleanup. As part of my exploration why it's not working I tried to make it more similar to how it's done for the keyboards.
| LexicalModel cached = CloudRepository.shared.getLexicalModel(aContext, languageID, lexicalModelID); | ||
| if (cached != null) { | ||
| cached.setUpdateKMP(link); | ||
| } |
There was a problem hiding this comment.
Lines 228-231 are not necessary. Left-over from exploration.
| } | ||
| if (kmpLexicalModelsArray.length() > 0) { | ||
| memCachedDataset.lexicalModels.addAll(CloudDataJsonUtil.processLexicalModelJSON(kmpLexicalModelsArray, fromKMP)); | ||
| mergeLexicalModels(memCachedDataset.lexicalModels, CloudDataJsonUtil.processLexicalModelJSON(kmpLexicalModelsArray, true)); |
There was a problem hiding this comment.
Honestly, I don't fully understand why we're doing this here. Since it was here before I kept it.
All the models that we added from installedSet above DO exist - the set is based on the locally installed models. Here we add all models that exist locally - whether installed or not. The difference to installedSet is that only installedSet has the download link for the new version.
It's possible that just using installedSet would be sufficient - at least it works with just that, but I don't know which edge cases that would break, i.e. what we would loose if models that exit locally but are not installed wouldn't be in the dataset.
|
Yes, sorry for not better documenting the different changes. The commit/PR description should have been more extensive. I went through again and checked what's really necessary to fix the issue. Turns out most of the changes are needed. There were a few that are unnecessary, so I reverted those (particular related to item 3). |
a0b0738 to
6e83adb
Compare
| * @return LexicalModel of an associated lexical model. Null if no match found | ||
| */ | ||
| public LexicalModel getAssociatedLexicalModel(@NonNull Context context, String languageID) { | ||
| return getLexicalModel(context, languageID, null); |
| if (index != -1) { | ||
| HashMap<String, String> lmInfo = KeyboardPickerActivity.getLexicalModelInfo(aContext, index); | ||
| String version = lmInfo.get(KMManager.KMKey_Version); | ||
| ArrayList<HashMap<String, String>> lmInfos = KeyboardPickerActivity.getLexicalModelsMatchingId(aContext, lexicalModelID); |
There was a problem hiding this comment.
This change is necessary to get matching models. The way we did it before (KeyboardPickerActivity.getLexicalModelIndex(aContext, lexicalModelID)) never found anything because the the second parameter is expected to be the full model key, consisting of pkg id, language id and model id, but we passed in only the model id.
|
Changes in this pull request will be available for download in Keyman version 19.0.262-alpha |
4 participants
With this change the model info activity will show an available update to the lexical model.
PR partially drafted by AI (Gemini 3 Flash Preview).
Fixes: #16226
Build-bot: release:android
User Testing
Preparations
(If you have a newer version of the SIL Euro Latin keyboard or the nrc.en.mtnt model installed, uninstall them and install the older versions. Unfortunately you'll have to wait a day before running the tests because the update check runs only once a day, and if it was installed it'll already have checked for updates when you opened the app)
Tests
TEST_KM_MODEL: verify adding a model for Khmer works
TEST_EN_KBD: verify that an update is available for the EuroLatin (SIL) keyboard
TEST_EN_MODEL: verify that an update is available for the dictionary
TEST_UPDATE: verify installing updates