Mihai Cirlanaru [:mihai][:mcirlanaru] Reporter
	Description • 11 years ago

When changing the system language from the Settings app, the language-associated built-in keyboard layout should also be enabled. If the language has a non-latin keyboard layout, also enable the default latin layout (English) if not already enabled.

+++ This bug was initially created as a clone of Bug #863719 +++

	Mihai Cirlanaru [:mihai][:mcirlanaru] Reporter
	Comment 1 • 11 years ago

Enable the language relevant keyboard layout and switch to it if the system language is changed from the Settings app.

	Rudy Lu [:rudyl] (inactive after 2015/7/3) Assignee
	Comment 2 • 11 years ago

Comment on attachment 801332 [details]
Pull Request #12014 - Enable language-associated kb

Clear the flag first since we have some discussion there and this patch should be modified accordingly.

Mihai, if you're still working on this after Bug 913782, please help set review to me and also settings peer, like :arthurcc.

Thanks.

	Ivan Tsay (:ITsay)
	Comment 3 • 11 years ago

Rudy, should this one be fixed in v1.2.

	Mihai Cirlanaru [:mihai][:mcirlanaru] Reporter
	Comment 4 • 11 years ago

Rudy, Ivan, note this is blocked by bug 920431 -- that has to go first before I can update the patch for this one.

	Rudy Lu [:rudyl] (inactive after 2015/7/3) Assignee
	Comment 5 • 11 years ago

Yes, this is part of Bug 863719. Please Bug 863719 Comment 2 & 3 for why we need this.

	Mihai Cirlanaru [:mihai][:mcirlanaru] Reporter
	Comment 6 • 11 years ago

Hey Rudy, I can still work on this if the structure for keyboard_layouts.json is in place (bug 920431 or 913782) -- at that point I will only need to update the patch that I already submitted

	bhavana bajaj [:bajaj]
	Comment 7 • 11 years ago

can we please have a target milestone set here ?

	Rudy Lu [:rudyl] (inactive after 2015/7/3) Assignee
	Comment 8 • 11 years ago

Hi, this is a WIP for this issue.
 - Not sure if it is ok to load keyboard_helper.js in settings.js, so send out the WIP first to get Arthur's feedback.
 - unit test is not ready yet, will work on that.

	Corey Frang [:gnarf]
	Comment 9 • 11 years ago

Comment on attachment 826693 [details]
Patch v2

Added some feedback on the pull, I think we should move this process into KeyboardHelper itself.

Let's create a method that keyboard_manager can call that will setup a settings watch on language.current and change the "default" flags to be correct as well as turning on "enabled" for the new language whenever it changes.  Then FTU, Settings, etc, wherever you set your language, the right thing will be done because keyboard_manager is always loaded.

f? me again with updates?

	Rudy Lu [:rudyl] (inactive after 2015/7/3) Assignee
	Comment 10 • 11 years ago

Comment on attachment 826693 [details]
Patch v2

Pull request updated.

 1. In order to minimize the duplicate code, move the logic into KeyboardHelper.changeDefaultLayouts() and this will be invoked by both settings app and FTU app.
 
 2. We cannot listen to "language.current" only, since keyboard_manager will also include keyboard_helper and it won't be able to distinguish if this language change is caused by FTU or settings. (We need different behaviors here, FTU also needs to reset the originally enabled layouts).

	Arthur Chen [:arthurcc][inactive after 6/30]
	Comment 11 • 11 years ago

Comment on attachment 826693 [details]
Patch v2

r=me for the settings part. Thanks!

	Rudy Lu [:rudyl] (inactive after 2015/7/3) Assignee
	Comment 12 • 11 years ago

Comment on attachment 826693 [details]
Patch v2

pull request updated to add unit test.

Corey,

Could you help review this change to keyboard_helper?

	Corey Frang [:gnarf]
	Comment 13 • 11 years ago

Comment on attachment 826693 [details]
Patch v2

Left some comments on the pull - reflag me when ready.

	Rudy Lu [:rudyl] (inactive after 2015/7/3) Assignee
	Comment 14 • 11 years ago

Comment on attachment 826693 [details]
Patch v2

Patch updated.
 - remove callback parameter for changeDefaultLayouts()
 - add a test case for nonLatin handling

Please be informed I have not removed the nonLatin definition in keyboard_layouts.json because that would involve some changes in build script, which will be handled in Bug 913782.

Corey, please help review again.
Thanks.

	Corey Frang [:gnarf]
	Comment 15 • 11 years ago

Comment on attachment 826693 [details]
Patch v2

r=me with nits

	Rudy Lu [:rudyl] (inactive after 2015/7/3) Assignee
	Comment 16 • 11 years ago

Gaia master,
https://github.com/mozilla-b2g/gaia/commit/5cf3e2972c4d623ab0759ad85f6c4673ed9ed75f

--
Corey, Arthur, thanks for the review.

	John Ford [:jhford] CET/CEST Berlin Time
	Comment 17 • 11 years ago

I was not able to uplift this bug to v1.2.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.2, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.2
  git cherry-pick -x -m1 5cf3e2972c4d623ab0759ad85f6c4673ed9ed75f
  <RESOLVE MERGE CONFLICTS>
  git commit

	Rudy Lu [:rudyl] (inactive after 2015/7/3) Assignee
	Comment 18 • 11 years ago

uplifted to v1.2,
06bf995d66331aed1535e668d1f731cb38956e78

The travis result is here,
https://travis-ci.org/mozilla-b2g/gaia/builds/13849372
The marionette_js error seems to be irrelevant.