-
-
Notifications
You must be signed in to change notification settings - Fork 143
fix(android): show updates for lexical models #16266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
a332c0d
e3ed30a
0368a7b
6e83adb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ | |
| import androidx.annotation.NonNull; | ||
| import androidx.appcompat.app.AppCompatActivity; | ||
| import androidx.appcompat.widget.Toolbar; | ||
| import androidx.core.os.BundleCompat; | ||
|
|
||
| import com.keyman.engine.cloud.CloudApiTypes; | ||
| import com.keyman.engine.cloud.CloudDownloadMgr; | ||
|
|
@@ -69,14 +70,15 @@ public void onCreate(Bundle savedInstanceState) { | |
| TextView textView = (TextView) findViewById(R.id.bar_title); | ||
|
|
||
| Bundle bundle = getIntent().getExtras(); | ||
| String newLanguageID = bundle.getString(KMManager.KMKey_LanguageID); | ||
| String newCustomHelpLink = bundle.getString(KMManager.KMKey_CustomHelpLink, ""); | ||
| 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, ""); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| // Sometimes we need to re-initialize the list of models that are displayed in the ListView | ||
| languageID = newLanguageID; | ||
| customHelpLink = newCustomHelpLink; | ||
|
|
||
| final String languageName = bundle.getString(KMManager.KMKey_LanguageName); | ||
| textView.setText(String.format(getString(R.string.model_picker_header), languageName)); | ||
|
|
||
| listView = (ListView) findViewById(R.id.listView); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
| import com.keyman.engine.KeyboardPickerActivity; | ||
| import com.keyman.engine.R; | ||
| import com.keyman.engine.cloud.CloudApiTypes; | ||
| import com.keyman.engine.data.CloudRepository; | ||
| import com.keyman.engine.data.Keyboard; | ||
| import com.keyman.engine.data.KeyboardController; | ||
| import com.keyman.engine.data.LexicalModel; | ||
|
|
@@ -188,7 +189,6 @@ public static void processKeyboardPackageUpdateJSON(Context aContext, JSONObject | |
| } | ||
|
|
||
| public static void processLexicalModelPackageUpdateJSON(Context aContext, JSONObject pkgData, List<Bundle> updateBundles) { | ||
| boolean saveModelsList = false; | ||
| // Parse for lexical model package updates | ||
| if (pkgData.has(CDKey_Models)) { | ||
| try { | ||
|
|
@@ -201,32 +201,41 @@ public static void processLexicalModelPackageUpdateJSON(Context aContext, JSONOb | |
| String cloudVersion = cloudModelObj.getString(CDKey_Version); | ||
| String cloudKMP = cloudModelObj.getString(CDKey_KMP); | ||
| // Valid lexical model package exists. See if lexical model list needs to be updated | ||
| // Valid keyboard package exists. See if keyboard list needs to be updated | ||
| int index = KeyboardPickerActivity.getLexicalModelIndex(aContext, lexicalModelID); | ||
| 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); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is necessary to get matching models. The way we did it before ( |
||
| for (HashMap<String, String> lmInfo : lmInfos) { | ||
| String version = lmInfo.get(KMManager.KMKey_LexicalModelVersion); | ||
| if (lexicalModelID.equalsIgnoreCase(lmInfo.get(KMManager.KMKey_LexicalModelID)) && | ||
| (FileUtils.compareVersions(cloudVersion, version) == FileUtils.VERSION_GREATER) && | ||
| (!MapCompat.getOrDefault(lmInfo, KMManager.KMKey_KMPLink, "").equalsIgnoreCase(cloudKMP))) { | ||
| // Update keyboard with the latest KMP link | ||
| lmInfo.put(KMManager.KMKey_KMPLink, cloudKMP); | ||
| KeyboardPickerActivity.addLexicalModel(aContext, lmInfo); | ||
|
|
||
| (FileUtils.compareVersions(cloudVersion, version) == FileUtils.VERSION_GREATER)) { | ||
| // Update bundle list | ||
| LexicalModel lm = new LexicalModel( | ||
| lmInfo.get(KMManager.KMKey_PackageID), | ||
| lmInfo.get(KMManager.KMKey_LexicalModelID), | ||
| lmInfo.get(KMManager.KMKey_LexicalModelName), | ||
| lmInfo.get(KMManager.KMKey_LanguageID), | ||
| lmInfo.get(KMManager.KMKey_LanguageName), | ||
| lmInfo.get(KMManager.KMKey_Version), | ||
| lmInfo.get(KMManager.KMKey_LexicalModelVersion), | ||
| lmInfo.get(KMManager.KMKey_CustomHelpLink), | ||
| lmInfo.get(KMManager.KMKey_KMPLink)); | ||
| Bundle bundle = new Bundle(lm.buildDownloadBundle()); | ||
| updateBundles.add(bundle); | ||
| String updateKMP = lm.getUpdateKMP(); | ||
| if (cloudLinkIsNewer(updateKMP, cloudKMP)) { | ||
| // Update lexical model info with the latest KMP link after appending languageID | ||
| String languageID = lm.getLanguageID(); | ||
| String link = String.format("%s&bcp47=%s", cloudKMP, languageID); | ||
| lmInfo.put(KMManager.KMKey_KMPLink, link); | ||
| KeyboardPickerActivity.addLexicalModel(aContext, lmInfo); | ||
| lm.setUpdateKMP(link); | ||
| updateKMP = link; | ||
| LexicalModel cached = CloudRepository.shared.getLexicalModel(aContext, languageID, lexicalModelID); | ||
| if (cached != null) { | ||
| cached.setUpdateKMP(link); | ||
| } | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lines 228-231 are not necessary. Left-over from exploration. |
||
| } | ||
|
|
||
| saveModelsList = true; | ||
| if (updateKMP != null && !updateKMP.isEmpty()) { | ||
| // Update bundle list for update notifications | ||
| Bundle bundle = new Bundle(lm.buildDownloadBundle()); | ||
| updateBundles.add(bundle); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -367,14 +376,13 @@ public static boolean cloudLinkIsNewer(String updateKMP, String cloudKMP) { | |
| return false; | ||
| } | ||
|
|
||
| try { | ||
| Uri cloudLink = Uri.parse(cloudKMP); | ||
|
|
||
| if (updateKMP == null || updateKMP.isEmpty()) { | ||
| return true; | ||
| } | ||
| if (updateKMP == null || updateKMP.isEmpty()) { | ||
| return true; | ||
| } | ||
|
|
||
| try { | ||
| Uri localLink = Uri.parse(updateKMP); | ||
| Uri cloudLink = Uri.parse(cloudKMP); | ||
|
|
||
| boolean pathsMatch = localLink.getLastPathSegment().equalsIgnoreCase(cloudLink.getLastPathSegment()); | ||
| boolean cloudVersionNewer = FileUtils.compareVersions( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ | |
| import com.keyman.engine.packages.JSONUtils; | ||
| import com.keyman.engine.util.BCP47; | ||
| import com.keyman.engine.util.DownloadFileUtils; | ||
| import com.keyman.engine.util.FileUtils; | ||
| import com.keyman.engine.util.KMLog; | ||
| import com.keyman.engine.util.VersionUtils; | ||
|
|
||
|
|
@@ -158,10 +159,23 @@ private boolean shouldUseCache(Context context, File cacheFile) { | |
| * @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); | ||
| } | ||
|
|
||
| /** | ||
| * Get the lexical model with modelID associated with the given language ID. If | ||
| * modelID is null, return the first model associated with languageID. Available | ||
| * models are from the cloud catalog and locally installed models. | ||
| * @param context Context | ||
| * @param languageID String of the language ID to search | ||
| * @param modelID String of the lexical model ID. | ||
| * @return LexicalModel of an associated lexical model. Null if no match found. | ||
| */ | ||
| public LexicalModel getLexicalModel(@NonNull Context context, String languageID, String modelID) { | ||
| if (memCachedDataset != null) { | ||
| for (int i=0; i < memCachedDataset.lexicalModels.getCount(); i++) { | ||
| LexicalModel lm = memCachedDataset.lexicalModels.getItem(i); | ||
| if (BCP47.languageEquals(lm.getLanguageID(), languageID)) { | ||
| if (BCP47.languageEquals(lm.getLanguageID(), languageID) && (modelID == null || modelID.equals(lm.getLexicalModelID()))) { | ||
| return lm; | ||
| } | ||
| } | ||
|
|
@@ -253,6 +267,41 @@ public void updateDatasetIfNeeded(@NonNull Context context, UpdateHandler update | |
| downloadMetaDataFromServer(context,updateHandler,onSuccess,onFailure); | ||
| } | ||
|
|
||
| private void mergeLexicalModels(Dataset.LexicalModels datasetModels, List<LexicalModel> newModels) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doc-comments, please.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| if (newModels == null) { | ||
| return; | ||
| } | ||
|
|
||
| List<LexicalModel> existingModels = new ArrayList<>(datasetModels.asList()); | ||
| List<LexicalModel> mergedModels = new ArrayList<>(); | ||
|
|
||
| // Process all incoming models | ||
| for (LexicalModel newModel : newModels) { | ||
| LexicalModel existingMatch = null; | ||
| for (int i = 0; i < existingModels.size(); i++) { | ||
| if (newModel.equals(existingModels.get(i))) { | ||
| existingMatch = existingModels.remove(i); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (existingMatch != null) { | ||
| if (FileUtils.compareVersions(existingMatch.getVersion(), newModel.getVersion()) != FileUtils.VERSION_LOWER) { | ||
| mergedModels.add(existingMatch); | ||
| } else { | ||
| mergedModels.add(newModel); | ||
| } | ||
| } else { | ||
| mergedModels.add(newModel); | ||
| } | ||
| } | ||
|
|
||
| // Add remaining existing models that weren't matched | ||
| mergedModels.addAll(existingModels); | ||
|
|
||
| datasetModels.clear(); | ||
| datasetModels.addAll(mergedModels); | ||
| } | ||
|
|
||
| /** | ||
| * precache dataset and notify callbacks if no update from cloud api services is necessary. | ||
|
|
@@ -286,19 +335,21 @@ private void preCacheDataSet(@NonNull Context context, UpdateHandler updateHandl | |
| languageCodes.add(installedSet.getItem(i).code); | ||
| } | ||
|
|
||
| // add all models from installed set | ||
| memCachedDataset.lexicalModels.addAll(installedSet.lexicalModels.asList()); | ||
|
|
||
| // Get kmp.json info from installed (adhoc and cloud) models. | ||
| // Consolidate kmp.json info from packages/ | ||
| JSONObject kmpLanguagesArray = wrapKmpKeyboardJSON(JSONUtils.getLanguages()); | ||
| JSONArray kmpLexicalModelsArray = JSONUtils.getLexicalModels(); | ||
| final boolean fromKMP = true; | ||
|
|
||
| try { | ||
| if (kmpLanguagesArray.getJSONObject(KMKeyboardDownloaderActivity.KMKey_Languages). | ||
| getJSONArray(KMKeyboardDownloaderActivity.KMKey_Languages).length() > 0) { | ||
| memCachedDataset.keyboards.addAll(CloudDataJsonUtil.processKeyboardJSON(kmpLanguagesArray, true)); | ||
| } | ||
| if (kmpLexicalModelsArray.length() > 0) { | ||
| memCachedDataset.lexicalModels.addAll(CloudDataJsonUtil.processLexicalModelJSON(kmpLexicalModelsArray, fromKMP)); | ||
| mergeLexicalModels(memCachedDataset.lexicalModels, CloudDataJsonUtil.processLexicalModelJSON(kmpLexicalModelsArray, true)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 It's possible that just using |
||
| } | ||
| } catch (Exception e) { | ||
| KMLog.LogException(TAG, "preCacheDataSet error ", e); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
getAssociatedLexicalModelcall to fail? Why are we keeping thebundle.putStringapproach 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.