1102834 - [UserDictionary] Implement dictionary binary blob generation & storage to KeyboardSettingsApp

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

Description

[John Lu [:mnjul][Please NI or I'll miss]]

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 .

Comment 1

[John Lu [:mnjul][Please NI or I'll miss]]

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.

Comment 2

[John Lu [:mnjul][Please NI or I'll miss]]

Attachment: Patch (PR @ GitHub)

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 [1] so I'll take extra care of that).

[1] 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!

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 4

[John Lu [:mnjul][Please NI or I'll miss]]

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 5

[John Lu [:mnjul][Please NI or I'll miss]]

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.

Comment 6

[John Lu [:mnjul][Please NI or I'll miss]]

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.

Comment 7

[John Lu [:mnjul][Please NI or I'll miss]]

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.

Comment 8

[John Lu [:mnjul][Please NI or I'll miss]]

Try run is good but Gaia seems closed and I don't have the nice big green button to merge. Setting the flag.

Comment 9

[GitHub Autolander Bot]

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.

Comment 10

[John Lu [:mnjul][Please NI or I'll miss]]

Oops...guess that's the fate for a PR opened before Autolander's activation...

Comment 11

[John Lu [:mnjul][Please NI or I'll miss]]

Setting NI on myself so I can remember to merge this when tree is open.

Comment 12

[John Lu [:mnjul][Please NI or I'll miss]]

Master: https://github.com/mozilla-b2g/gaia/commit/813eda1a3c9764880bd245f28aafbd85ea8927af