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

Enable the appropriate language-associated keyboard during build time (i.e. through build/settings.js depending on GAIA_DEFAULT_LOCALE parameter) and enable latin keyboard layout if non-latin layout set as default.

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

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

Enable the GAIA_DEFAULT_LOCALE associated keyboard during build time.

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

Comment on attachment 801194 [details]
Pull Request #12005 - Set build-time keyboard

Hi Mihai,

Thanks for patching this up.
However, originally I would expect we put both "appOrigin" and "layoutId" info into "shared/resources/keyboard_layouts.json", this way we could enable customization for our partner if they want to have their own 3rd-party IME app preloaded and can enable some of layouts of it.

Do you think this makes sense?

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

Comment on attachment 801194 [details]
Pull Request #12005 - Set build-time keyboard

Thanks for the feedback, Rudy! I updated the patch to include the appOrigin information in keyboard_layouts.json, let me know if this is what you had in mind, or if there are any other changes needed.

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

Mihai, thanks for the update and sorry for the delay to look at this patch.
Right now, I am going to review and test this.

Please stay tuned. :)

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

Comment on attachment 801194 [details]
Pull Request #12005 - Set build-time keyboard

Hi Mihai,

I've done a quick round of review, and want to confirm with you on the format in 
shared/resources/keyboard_layouts.json.

Please help take a look and after that I might ask for Yuren's help to have a detailed review on build/settings.js.

Thanks.

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

Hey Rudy, I have left a comment on GitHub regarding "keyboard_layouts.json" structure, let me know if that sounds good to you and I can update the patch. Thanks!

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

(In reply to Mihai Cirlanaru [:mihai][:mcirlanaru] from comment #6)
> Hey Rudy, I have left a comment on GitHub regarding "keyboard_layouts.json"
> structure, let me know if that sounds good to you and I can update the
> patch. Thanks!

(Put the comments here for the record)

What is in my mind, is this file defines the association between language to keyboard layouts, and it can be defined by our partner in customization build.

Our party could preload a 3rd-party IME app at build time and specify that it should be enabled when the corresponding language is the default language or selected in FTU/settings app.
Hence, I don't think we have to separate "built-in" and "3rd-party" in this file, it should tell build time/FTU/settings which layouts we should enable when the language is changed.

If we need another level of granularity, like some layouts should be enabled in build time but not for FTU/settings app, then I guess we should define that in another file and add this logic, but I don't see this requirement.

Do you think this makes sense?

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

Comment on attachment 801194 [details]
Pull Request #12005 - Set build-time keyboard

Thanks for the info, Rudy! It does indeed make sense to use 'keyboard_layouts.json' for this purpose.

The patch is now updated according to your suggestions, feel free to check it out, thanks!

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

Comment on attachment 801194 [details]
Pull Request #12005 - Set build-time keyboard

Hi Mihai,

Thanks for the update.
Generally, now it looks quite good.

Before merging this code, please help refine it according to the comments on github.
Besides, I think we would need one more pair of eyes for build script related changes.

Yuren, can you help us here?
Thank you.

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

Hey Rudy, the PR is now updated (including rebased on master) integrating your feedback. I will be waiting for Yuren on this. Feel free to leave more comments if further changes need to be done.

Thanks!

	Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)
	Comment 11 • 11 years ago

Comment on attachment 801194 [details]
Pull Request #12005 - Set build-time keyboard

r=yurenju if you use map() instead of reduce(), please read comment on github, thanks!

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

Thanks Yuren! Patch is updated, feel free to merge it for me

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

Landed on master:
https://github.com/mozilla-b2g/gaia/commit/51eb595572dfde7e3af451a9b30b3e22db1fdab2

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

This patch just got backed out due to bug 920588 with this commit.
https://github.com/mozilla-b2g/gaia/commit/73460187f0db40198dee1592a441076b29f2c850

--
And we should handle the concerns mentioned in bug 920394 that we should avoid hard-coding the appOrigin in the shared/resources/keyboard_layouts.json.

	Jason Smith [:jsmith]
	Comment 16 • 11 years ago

Reopening per backout in comment 15

	GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]
	Comment 17 • 11 years ago

Hi Mihai, 
   I found the root cause,
   build/settings.js pushed 'en' layout twice.
   {"layoutId":"en","appOrigin":"http://keyboard.gaiamobile.org:8080","enabled":true},
   {"layoutId":"en","appOrigin":"http://keyboard.gaiamobile.org:8080","enabled":false}]" keyboard_helper.js:125

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

(In reply to GaryChen [:GaryChen] from comment #17)
> Created attachment 810471 [details]
> Bug_913782_root_acuse.txt
> 
> Hi Mihai, 
>    I found the root cause,
>    build/settings.js pushed 'en' layout twice.
>   
> {"layoutId":"en","appOrigin":"http://keyboard.gaiamobile.org:8080","enabled":
> true},
>   
> {"layoutId":"en","appOrigin":"http://keyboard.gaiamobile.org:8080","enabled":
> false}]" keyboard_helper.js:125

Thanks for finding this subtle issue -- I remember double checking that, but somehow it still remained...

I will solve bug 920431 first to have a clear commit that updates 'keyboard_layouts.json' structure, and then update this and submit a new PR.

	Preeti Raghunath(:Preeti)
	Comment 19 • 11 years ago

Mihai,

Is this bug ready to be uplifted to 1.2?

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

(In reply to Preeti Raghunath(:Preeti) from comment #19)
> Mihai,
> 
> Is this bug ready to be uplifted to 1.2?

Hi Preeti, the fix for this is blocked by bug 920431, for which I am waiting on Rudy's input.

If a quick fix is required and Rudy is fine with it (adding needinfo? for him), I can update my initial patch without restructuring keyboard_layouts.json (which is part of bug 920431), thus fixing the regression it initially caused -- which proves to be easy to tackle (see comment 17).

	Jan Jongboom [:janjongboom] (inactive)
	Comment 21 • 11 years ago

bug 920431 has been moved to 1.3.

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

I'm working on bug 912010 - and proposing changing the format of this setting.  The current one requires that all keyboards are listed.  I think we should instead set:

{"http://keyboard.gaiamobile.org:8080": {"en": true, "number": true}}

as a plain object in two different keys - the keyboard.enabled-layouts and keyboard.default-layouts - 912010 takes care of reading these formats.

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

I'd be happy to take this bug and finish it as well

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

Hi Corey,

Please help take this koi+ bug if you have free cycles to work on it. 

Please be informed that we want to change from using "appOrigin" to "manifestURL" to identify a keyboard app.
Quote what Fabrice responded in a mail. 
-----------
The manifest URL must be used to identify an app. Currently we support
only one app per origin, but that will change so the origin won't be
appropriate anymore.
-----------

Thanks.

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

(In reply to Corey Frang [:gnarf] from comment #23)
> I'd be happy to take this bug and finish it as well

Thanks for checking this bug, Corey. Since I won't have the time to update my initial patch (which you can base your work on -- see comment 20) this week, feel free to take it.

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

Please be informed that we should take care of Bug 930358, 930371 before getting this bug fixed.

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

Hi Corey,

Please assign one of the target milestone (1.2 C3 ~ 1.2 C6) for this bug based on the effort you need. Thank you! I've assigned the target milestone of the dependency bug 930358, 930371 to 1.2 C4 (Nov. 8). We need the milestone flag for tracking purpose.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 28 • 11 years ago

Reduce v1.2 scope as there is no partner request on shipping a phone with preloaded 3rd-party keyboards.

If this patch is completed in time and needed for other koi+ bug, or it is deemed safer to uplift to v1.2 than not (judging by the maintenance cost of the branch, stability etc), I will be happy to a+ the patch.

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

Pointer to Github pull-request

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

stepping out since this moved to 1.3 though I might pick it back up, just wanted it open for someone to take if they had free cycles

	Jason Smith [:jsmith]
	Comment 31 • 11 years ago

This is not a committed feature for 1.3, so this should not block the release. 3rd party keyboard support can ride the trains to 1.3 if it's safe enough risk-wise.

	Jason Smith [:jsmith]
	Comment 32 • 11 years ago

Moving to 1.4? per offline discussion - we'll be keeping this preffed off for 1.3.

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

Hi Yuren,

This is the first part of the changes for default keyboard layout setup at build time and I hope to get an early feedback from you.

 1. This patch resume the work that settings.js now will read in the default layout config in 'shared/resource/keyboard_layouts.json' and setup the correct default layouts in mozSettings.
 2. It also removed the unused info, "nonLatin" entry in 'keyboard_layouts.json'.

The 2nd part of the patch would be done with Bug 920431 and checked in together.

	Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)
	Comment 34 • 11 years ago

Comment on attachment 8359128 [details]
Patch part 1

Rudy, this commit looks good to me.

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

Merged to Gaia master with bug 920431,
https://github.com/mozilla-b2g/gaia/commit/cffbe7a643525f479a1e20538b38d442c73fa849

Yuren,

Thanks for the review.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 36 • 11 years ago

Triage: 1.4 feature work.