[Settings][Keyboard] Language selection should also enable associated built-in keyboard layout (for 3rd-party keyboard support)

RESOLVED FIXED in 1.2 C4(Nov8)

Status

defect
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mihai, Assigned: rudyl)

Tracking

unspecified
1.2 C4(Nov8)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:koi+, b2g-v1.2 fixed)

Details

(Whiteboard: [FT:System-Platform], [Sprint: 2], [3rd-party-keyboard])

Attachments

(2 attachments)

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 +++
See Also: → 828237
Depends on: 913782
Enable the language relevant keyboard layout and switch to it if the system language is changed from the Settings app.
Attachment #801332 - Flags: review?(rlu)
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.
Attachment #801332 - Flags: review?(rlu)
Rudy, should this one be fixed in v1.2.
Flags: needinfo?(rlu)
Rudy, Ivan, note this is blocked by bug 920431 -- that has to go first before I can update the patch for this one.
Yes, this is part of Bug 863719. Please Bug 863719 Comment 2 & 3 for why we need this.
Assignee: mihai → rlu
Status: NEW → ASSIGNED
Flags: needinfo?(rlu)
Whiteboard: [FT:System-Platform], [Sprint: 2] → [FT:System-Platform], [Sprint: 2], [3rd-party-keyboard]
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
can we please have a target milestone set here ?
Flags: needinfo?(rlu)
Flags: needinfo?(rlu)
Target Milestone: --- → 1.2 C4(Nov8)
Posted file Patch v2
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.
Attachment #826693 - Flags: review?(arthur.chen)
Attachment #826693 - Flags: feedback?(gnarf37)
Comment on attachment 826693 [details] [review]
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?
Attachment #826693 - Flags: feedback?(gnarf37) → feedback-
Comment on attachment 826693 [details] [review]
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).
Attachment #826693 - Flags: feedback- → feedback?(gnarf37)
Comment on attachment 826693 [details] [review]
Patch v2

r=me for the settings part. Thanks!
Attachment #826693 - Flags: review?(arthur.chen) → review+
Comment on attachment 826693 [details] [review]
Patch v2

pull request updated to add unit test.

Corey,

Could you help review this change to keyboard_helper?
Attachment #826693 - Flags: feedback?(gnarf37) → review?(gnarf37)
Attachment #826693 - Attachment description: Patch v1 - WIP → Patch v1
Comment on attachment 826693 [details] [review]
Patch v2

Left some comments on the pull - reflag me when ready.
Attachment #826693 - Flags: review?(gnarf37)
Comment on attachment 826693 [details] [review]
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.
Attachment #826693 - Attachment description: Patch v1 → Patch v2
Attachment #826693 - Flags: review?(gnarf37)
Comment on attachment 826693 [details] [review]
Patch v2

r=me with nits
Attachment #826693 - Flags: review?(gnarf37) → review+
Gaia master,
https://github.com/mozilla-b2g/gaia/commit/5cf3e2972c4d623ab0759ad85f6c4673ed9ed75f

--
Corey, Arthur, thanks for the review.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 937289
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
Flags: needinfo?(rlu)
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.
Flags: needinfo?(rlu)
You need to log in before you can comment on or make changes to this bug.