Closed Bug 1102834 Opened 5 years ago Closed 5 years ago
Dictionary] Implement dictionary binary blob generation & storage to Keyboard Settings App
And we'll add the user dictionary binary blob generation code after we're done with bug 1102831. I've already have experimental codes at https://github.com/mnjul/gaia-kb-dict-tools, which rewrote xml2dict.py into list2dict.js .
Assigning this to myself. Will work on bug 1124150 at the same time and won't report as ASSIGNED until I have a WIP patch.
Assignee: nobody → jlu
Target Milestone: --- → 2.2 S6 (20feb)
Tim, please check if the patch looks good so far. The internal code of TSTConverter is roughly statement-to-statement translation from xml2dict.py. I believe TSTConverter can be more blackboxed and we probably can expose an object with only one toBlob() callable method, aside from the constructor. If you agree on this then I'll modify the code accordingly. For testing TSTConverter, I also intend to test it in a blackboxed/integrated fashion (i.e. "module as a unit" in "unit test", as opposed to "function as a unit"), instead of testing each of its internal functions, such as |rotateRight|. We'll just compare the binary blob the object returns, against a known result, for a handful test cases (not sure if I'll bump onto something like  so I'll take extra care of that).  https://wiki.mozilla.org/Gaia/System/Keyboard/IME/Latin/Dictionary#Same_XML.2C_Different_Dict_Blob Also, test strategy of the saveDict function in UserDictionary has been modified a bit: now, the second call's MockPromise chain is triggered from the last MP in the first call, instead of being triggered from the MP returned by second call. (uh....the code says a thousand words, I know, so you can just check the code). We do also have a couple optimization opportunities and I'll file follow-up bugs for them. They're XXX'ed in the codes. Thanks!
Attachment #8556361 - Flags: feedback?(timdream)
Comment on attachment 8556361 [details] [review] Patch (PR @ GitHub) Discussed on wrapping offline and left some comment on Github. I didn't look into TSTConverter -- I think having tests to assert the output is enough.
Attachment #8556361 - Flags: feedback?(timdream) → feedback+
Sitrep: Feedback comments from Tim have been addressed. The problem of encapsulation of TSTConverter was kinda "worked-around" by decomposing it: TSTConverter can actually be divided into subcomponents: one that builds the tree and book-keeps some metadata, one that serializes ("flattens") the tree, and one that produces the blob bytes from thr metadata and the nodes. We now have one minimal "stub" object exposed to the outside world whose sole purpose is coordinate those subcomponents. I haven't written the test for the converter, though.
Comment on attachment 8556361 [details] [review] Patch (PR @ GitHub) Alright we're good to go. - PromiseStorage#setItems is now implemented & used. - Use real Promise (instead of MockPromise) to test UserDictionary#saveDict to ease things up. - Encapsulation of the converter (as in comment 4). - Tests for the converter (now named WordListConverter) have been added. Tim, could you check the patch out again? Thanks. Follow-up bugs will be filed when this is greenlighted.
Attachment #8556361 - Flags: review?(timdream)
Attachment #8556361 - Attachment description: WIP Patch (PR @ GitHub) → Patch (PR @ GitHub)
I think another set of test case is worth adding here: Emojis. So we can see how characters from different unicode planes behave with the converter.
Attachment #8556361 - Flags: review?(timdream) → review+
I'm waiting for Gaia-Try. Side note: we're kinda getting into no man's land with those Emojis since the original xml2dict.py can't even properly encode a unicode char outside BMP as it expects that every input char's code point "value" is no more than 2 bytes large.
Try run is good but Gaia seems closed and I don't have the nice big green button to merge. Setting the flag.
https://github.com/mozilla-b2g/gaia/pull/27769 The pull request could not be applied to the integration branch. Please try again after current integration is complete.
Oops...guess that's the fate for a PR opened before Autolander's activation...
Setting NI on myself so I can remember to merge this when tree is open.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.