905530 - [Keyboard][User Story] Add user dict support to Emscripten'd PinyinIME

Status: RESOLVED FIXED

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

Description

[Pin Zhang [:pzhang] (inactive)]

User story:

As a user, I want the PinyinIME to remember the new lexis I selected which is not shown as one of the candidates for the pinyin I typed, so next time I type the same pinyin, the lexis would be shown in the candidate list.

Comment 1

[Yuan Xulei [:yxl] (Leave from May 18)]

Current Pinyin IME has such feature, but it'll be broken after bug 900901 lands, as the emscripten'ed Pinyin engine cannot persist user dictionary.

We need figure out a way to resolve the persistence issue of emscripten filesystem.

Comment 2

[Pin Zhang [:pzhang] (inactive)]

Attachment: WIP for feedback

It's based on Luke's WIP branch for bug 900907.

Comment 3

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

Comment on attachment 792657 [details]
WIP for feedback

Looks good, I have no opinion about how the code works within Emscripten'd world as long as the following conditions are met, eventually:

-- People other than us have access to a build script where they could generate the exact same Javascript.
-- We could eventually move Emscripten'd code to a Web Workers*.

* Blocked by bug 701634, but we could implement postMessage interface in an iframe first.

Thanks!

Comment 4

[Pin Zhang [:pzhang] (inactive)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #3)
> Comment on attachment 792657 [details]
> WIP for feedback
> 
> Looks good, I have no opinion about how the code works within Emscripten'd
> world as long as the following conditions are met, eventually:
> 
> -- People other than us have access to a build script where they could
> generate the exact same Javascript.
Since Luke modified the original converted JS for performance tuning, we either need to provide a diff for the generated JS based on a specific commit of Emscripten or just use the raw generated file if the performance is acceptable.

> -- We could eventually move Emscripten'd code to a Web Workers*.
> 
> * Blocked by bug 701634, but we could implement postMessage interface in an
> iframe first.
Or we could post the user dictionary data back to the mainthread, and keep using the IndexedDB async API to it.

> 
> Thanks!

Comment 5

[Yuan Xulei [:yxl] (Leave from May 18)]

I suggest create a patch along with the README file to explain how to get the converted js code. It's very likely that we cannot avoid modifying the generated js for performance or other kind of issue.

Comment 6

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 792657 [details]
WIP for feedback

Looks good to me as well.
Thanks for the effort, some comments/suggestions on the pull request.

Comment 7

[Pin Zhang [:pzhang] (inactive)]

Attachment: PR 12022

Comment 8

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Yuan,

Are you available to review this change, I think you understand pinyin part better than me.
 :)

Comment 9

[Yuan Xulei [:yxl] (Leave from May 18)]

OK, I'll review it.

Comment 10

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 801423 [details]
PR 12022

Pin, please refer to my comments in the pull request. The patch works, but the user dictionary is saved to the DB each time user selects a sentence or word to commit. I concern about the performance and we don't need such frequent data saving operation. It is good for me if you save the dictionary to the DB when the IME is deactivated(hidden). You can add a `deactivated` method to IMEngine, which be called when the IME is deactivated and perform the data saving operation inside that method.

Comment 11

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 801423 [details]
PR 12022

PR is updated, please take a look again.

Comment 12

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 801423 [details]
PR 12022

r=me, but nits should be addressed before check-in. Please refer to the comments of the pull request.

Comment 13

[Pin Zhang [:pzhang] (inactive)]

(In reply to Yuan Xulei [:yxl] from comment #12)
> Comment on attachment 801423 [details]
> PR 12022
> 
> r=me, but nits should be addressed before check-in. Please refer to the
> comments of the pull request.

Nits are fixed, thanks!

@rudy, could you help to check in?

Comment 14

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Merged to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/5d8f81ebbba42afa0b1716367e5dc68199541118

Pin, Yuan,
Thanks for your help.

Do we need to follow up for the "uninit" part?

Comment 15

[Pin Zhang [:pzhang] (inactive)]

(In reply to Rudy Lu [:rudyl] from comment #14)
> Merged to Gaia master,
> https://github.com/mozilla-b2g/gaia/commit/
> 5d8f81ebbba42afa0b1716367e5dc68199541118
> 
> Pin, Yuan,
> Thanks for your help.
> 
> Do we need to follow up for the "uninit" part?

I think we should file a separate bug to tackle this issue.