Closed Bug 956175 Opened 6 years ago Closed 6 years ago

[keyboard refactor] handle language switching

Categories

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

All
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: djf, Assigned: rudyl)

References

Details

Attachments

(1 file)

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.
Depends on: 985385
Rudy, this bug is on your weekly report, so please please give a brief update on the progress last week.
Flags: needinfo?(rlu)
Attached file 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.
Attachment #8396956 - Flags: feedback?(timdream)
Flags: needinfo?(rlu)
Comment on attachment 8396956 [details] [review]
Pull request 17629

Please talk to me with a KeyboardLayoutManager proposal and WIP patch.
Attachment #8396956 - Flags: feedback?(timdream) → feedback-
Assignee: nobody → rlu
Status: NEW → ASSIGNED
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.
Attachment #8396956 - Flags: feedback- → feedback?(timdream)
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.
Attachment #8396956 - Flags: feedback?(timdream) → feedback-
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.
Attachment #8396956 - Flags: review?(timdream)
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → All
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?
Attachment #8396956 - Flags: review?(timdream)
Attachment #8396956 - Flags: feedback-
Attachment #8396956 - Flags: feedback+
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.)
Attachment #8396956 - Flags: review?(timdream)
Comment on attachment 8396956 [details] [review]
Pull request 17629

Well done!
Attachment #8396956 - Flags: review?(timdream) → review+
Landed to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/4645b8da5a9cf25795313b20a56bad67225786c5

Tim,

Thanks for the review!
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.