-
-
Notifications
You must be signed in to change notification settings - Fork 103
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
feat(oem/fv/ios): Dictionary Support #6197
Conversation
User Test ResultsTest specification and instructions
Results Template
Test Artifacts |
UX question: Are the defaults on "Suggest Predictions" and "Suggest Corrections" off on Keyman for iPhone and iPad? I think on Android, we default to "On" (less clicking after the user installs a lexical-model) |
In Keyman for iOS, I installed a new keyboard, and the switches for Predictions and Corrections were active and enabled by default. That was even with no dictionary installed. That doesn't make much sense as those options should not be available until there is a dictionary, right? After a dictionary is installed, we can activate them and default them as enabled. Until then they should be disabled and inactive. |
A few notes on the details page:
The above changes differ from what we have in Keyman, but I think it would improve clarity of the UI. As for how the state of the controls depend on each other, there is a pretty clear hierarchy:
|
// try to read array | ||
if !jsonArray.isEmpty { | ||
let jsonModelMap = jsonArray.first | ||
let name = jsonModelMap!["description"] as! String |
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.
Shouldn't name be?
let name = jsonModelMap!["description"] as! String | |
let name = jsonModelMap!["name"] as! String | |
Write Preview |
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.
Good question. I used the description field because it had the more succinct name:
"description": "SEN\u0106O\u0166EN (Saanich Dialect)",
vs
"name": "SEN\u0106O\u0166EN (Saanich Dialect) Lexical Model"
Not sure what I can expect from each field of the API, but this text goes in the switch name, so I need something brief. Is the lexical model definition incorrect?
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.
You should be using name
. description
is a long form which may be several lines long. It just so happens that the SNECOTEN model has a overly short description:
vs name:
See for example https://api.keyman.com/model?q=bcp47:cmo-Khmr:
[
{
"license": "mit",
"languages": [
"cmo-khmr"
],
"description": "Bunong Wordlist is based on the Bible wordlist of 1,157 unique entries with their frequencies.",
"id": "sil.cmo.bw",
"name": "Bunong Wordlist",
"authorName": "[object Object]",
"lastModifiedDate": "2022-02-18T18:41:02.203Z",
"packageFilename": "https://keyman.com/go/package/download/model/sil.cmo.bw?version=1.1&update=1",
"packageFileSize": 107384,
"jsFilename": "https://downloads.keyman.com/models/sil.cmo.bw/1.1/sil.cmo.bw.model.js",
"jsFileSize": 286398,
"packageIncludes": [],
"version": "1.1",
"minKeymanVersion": "12.0",
"sourcePath": "release/sil/sil.cmo.bw"
}
]
|
||
class LexicalModelRepository { | ||
|
||
private static let keymanLexicalModelApiUrl = "https://api.keyman.com/model?q=bcp47:" |
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.
I wonder if some of this should use existing methods in
Like fetch()
iOS is using an old script to download fv_all.kmp keyman/oem/firstvoices/ios/build.sh Lines 9 to 10 in 7e43e37
We can remove . "$KEYMAN_ROOT/resources/build/build-download-resources.sh" and then do keyman/oem/firstvoices/android/build.sh Lines 95 to 101 in 7e43e37
|
… use for lexical model query
…et lexical models array
…move disabled keyboards
…s, moves keyboard list refresh to viewDidAppear
I think we aim to add Sentry to FV (I have created the fv-ios Sentry project at https://sentry.io/keyman/fv-ios/getting-started/apple-ios/). Otherwise, it is difficult, and I am certainly not keen on creating a new error-reporting mechanism for this. If adding Sentry is too big a task for now, then we can temporarily add an alert while diagnosing these issues and then remove it again? |
Yeah, we can consider that. I had a question on the SENCOTEN settings: "Keyboard Information" section. If there's multiple keyboards available for a language, which one would "Version" correspond to? |
@MakaraSok I've added some code temporarily for troubleshooting the issue you are have activating the keyboard. This new version will display an alert with error information. Please retest and tell me what you see. I cannot reproduce this, so let me know if you have any more details that will isolate the issue. @keymanapp-test-bot retest TEST_TURN_ON_KEYBOARD |
@darcywong00 Good question on the version number. I think it's broken. We could display the version of the active dictionary if there is one, but that's kind of weak and still not obvious. I'm not sure what the best solution is. |
This was obviously designed way back in the day with a one-keyboard-per-language pattern. Do we have any languages with more than one keyboard in the list? If not, then this is a brokenness that we can just ignore for now 😁 If we do, then I suggest for now just adding a version row for each keyboard and differentiating with the keyboard name in the caption (e.g. SENCOTEN version -> 9.1.1). The detail is of course important but we also need to balance that with addressing more serious issues... so I am comfortable with a less than 100% elegant answer for this edge case. |
Looking at keyboards.csv, I see "ikt-Latn: Inuinnuaqtun (Latin)" is used by fv_inuvialuktun and fv_uummartmitutun. But I'm thinking this should be fine because the FV apps separate Region --> Keyboard --> so each keyboard has its own row... |
Test Results
Screen.Recording.2022-03-17.at.9.26.47.AM.mov |
@mcdurdin So Makara is not seeing the alert that I added which implies that Keyman Engine is not throwing an error and it looks like a success to FV. Do you know of any other way to troubleshoot this in Makara's environment? |
I guess running Console on his machine and looking at logs from the simulator may provide some enlightenment? |
@MakaraSok @darcywong00 @mcdurdin I just realized that this problem Makara is seeing is the same as #6226 which appeared with the old FV iOS code. The change of Darcy's should not have broken the iOS FV app, should it? Or maybe that's why there was an iOS test case on #6226? |
Yes, I think that this problem is not new to this PR and is specific to the simulator. |
Retest all failed/blockedI've retested this again today and it doesn't work as expected like what was experienced prior. Note that the test was done on a simulator with its "all content and settings" erased being installing the .
FV.doesn.t.show.as.sys.kb.480p.mov
unable.to.install.dictionary.movThe second attempt to enable the dictionary got through without bouncing back, but it still doesn't show up when summoned. Please see the screen recording below. no.kb.shown.no.dictionary.visible.movThe blocked ones stay "Blocked". |
FYI: The build from here was used https://build.palaso.org/buildConfiguration/Keyman_iOS_TestPullRequests/310924?buildTab=artifacts#%2Fupload. |
Ready for retest, but need to have a way to either test a release version of FV on the simulator or a real device. Debug FV on the simulator works well from Xcode. @mcdurdin any ideas on how to change the relase build so that it will behave the same on simulator and device? @keymanapp-test-bot retest TEST_TURN_ON_KEYBOARD TEST_INSTALL_DICTIONARY |
It's really hard. The simulator does not appear to behave identically to devices in all circumstances -- particularly that boundary between apps and app extensions seems different. The best solution may be to merge to beta and do a beta release build which is then available in TestFlight. Otherwise, unless you are in the same room (where you can plug the device into your computer and do a build and run from XCode), I don't know how it can be done. |
Changes in this pull request will be available for download in Keyman version 15.0.224-beta |
Test ResultsTested on a physical device, iPad mini 6 running iOS 15.4.
Note that when removing a keyboard from the FV app, every keyboard was removed with it leaving only EuroLatin keyboard to choose from. |
Test Results
|
Adds dictionary/lexical model support to FirstVoices iOS app.
Tapping a keyboard in the list causes transition to a detail page where the keyboard is displayed with other settings, including downloadable dictionaries, if available.
User Testing
The changes to the app are substantial and tests amount to a regression test