956175 - [keyboard refactor] handle language switching

Status: RESOLVED FIXED

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

Description

[David Flanagan [:djf]]

The refactored keyboard in test_apps/demo-keyboard/ does not yet handle multiple languages. It needs to be able to switch languages.

Also, it should not display the language switch key if there are no other languages available.

Also, see bug 936581: there should be some user visible feedback when the language changes.

Comment 1

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

Rudy, this bug is on your weekly report, so please please give a brief update on the progress last week.

Comment 2

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

Attachment: Pull request 17629

Hi Tim,

Here is a WIP that could dynamically load the layout definition for different languages.

Somethings not working that I am still working on,
 1. The suggestion language would not be changed after you switch layouts.

Please help give feedback if you can.
Thanks.

Comment 3

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

Comment on attachment 8396956 [details] [review]
Pull request 17629

Please talk to me with a KeyboardLayoutManager proposal and WIP patch.

Comment 4

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

Comment on attachment 8396956 [details] [review]
Pull request 17629

Tim,

This WIP is in a early stage, but I'd like to hear your feedback on this.
 - Add KeyboardLayoutManager, and use observer pattern (events) instead of callback to interact with KeyboardApp.

Thanks.

Comment 5

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

Comment on attachment 8396956 [details] [review]
Pull request 17629

Please use

this.layoutManager = new KeyboardLayoutManager(this);

and 

function KeyboardLayoutManager(app) {
  this.app = app;
}

and 

  ...
  this.app.handleLayoutLoaded(layout);
  ...

instead, since it is unlikely KeyboardLayoutManager need to tell anyone other than KeyboardApp that a keyboard is loaded.

Comment 6

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

Comment on attachment 8396956 [details] [review]
Pull request 17629

Pull request updated to:
 1. Address the previous review comment about how KeyboardLayoutManager interact with KeyboardApp.
 2. Change the format of the layout definition to json.

--
Tim, could you please help review it again?
Thank you.

--
Note:
I plan to separate language switching for suggestion engine to another bug, so that we could land this first without conflicting with the ongoing changes to autoCorrection modules.

Comment 7

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

Comment on attachment 8396956 [details] [review]
Pull request 17629

This is only half done.

-- Async xhr need better handling. I recommend you centralize event handlers from simple `xhr.onXXX` to `instance.handleEvent`.
-- What if there is a new loadLayout() request and the previous one is not done yet?
-- What if the module is stopped before xhr is finished?
-- Where are the test cases?

Comment 8

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

Comment on attachment 8396956 [details] [review]
Pull request 17629

Patch updated to address the review comments and add the unit test for KeyboardLayoutManager.

Tim,

Please help review it again.
Thank you. (Sorry it took so many iterations.)

Comment 9

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

Comment on attachment 8396956 [details] [review]
Pull request 17629

Well done!

Comment 10

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

Landed to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/4645b8da5a9cf25795313b20a56bad67225786c5

Tim,

Thanks for the review!