Closed Bug 920431 Opened 11 years ago Closed 10 years ago

[keyboard] keyboard_layout.json should be customizable

Categories

(Firefox OS Graveyard :: Gaia::System::Input Mgmt, defect)

Product:
Firefox OS Graveyard
Component:
Gaia::System::Input Mgmt
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(tracking-b2g:backlog)

Status:
RESOLVED FIXED
Project Flags:
tracking-b2g backlog

People

(Reporter: GaryChen, Assigned: rudyl)

References

Details

(Whiteboard: [3rd-party-keyboard], [in-bubble-tea])

Attachments

(2 files, 1 obsolete file)

Pull Request #12560 - Update keyboard_layouts.json structure
(deleted), text/html
Details
Patch V1.4 - pull request 16404
(deleted), text/x-github-pull-request
yurenju
: review+
Details 
Since build time (script), FTU and Settings app need to reuse |keyboard_layout.json| file and we also need to consider about keyboard app might be a 3rd-party preloaded app by partner.

So we need to figure out how to make keyboard_layout.json can be customizable, especially in appName and appOrign field.

Here is Rudy's comment in my patch.
https://github.com/mozilla-b2g/gaia/pull/12420#discussion-diff-6565700
blocking-b2g: --- → koi?
A new format that might solve this issue has been introduced with the patch for bug 913782 which got feedback+ from :RudyL -- it already landed to master.
(In reply to Mihai Cirlanaru [:mihai][:mcirlanaru] from comment #1)
> A new format that might solve this issue has been introduced with the patch
> for bug 913782 which got feedback+ from :RudyL -- it already landed to
> master.

Just realised that the discussions are already based on the layout introduced by the patch for bug 913782 :)

I can take this if no one already started on it.
(In reply to Mihai Cirlanaru [:mihai][:mcirlanaru] from comment #2)
> (In reply to Mihai Cirlanaru [:mihai][:mcirlanaru] from comment #1)
> > A new format that might solve this issue has been introduced with the patch
> > for bug 913782 which got feedback+ from :RudyL -- it already landed to
> > master.
> 
> Just realised that the discussions are already based on the layout
> introduced by the patch for bug 913782 :)
> 
> I can take this if no one already started on it.

Sure, you can take it.

I forgot to mention one more thing, 
keyboard_manager.js use layoutID and appOrign to match which keyboard layout is enabled in MozSettings,
but each app has different origin between devices and browser.
for example:
    In keyboard apps:
    Device: app://keyboard.gaiamobile.org
    Browser: http://keyboard.gaiamobile.org:8000

So we need to figure out how to fix this issue in new format.
You can see more detail on bug 920394.

If you have any problem, please feel free to talk with Rudy or me.
Thanks.
Taking it, thanks for the info Gary!
Assignee: nobody → mihai
Depends on: 913782
Although the changes to 'keyboard_layouts.json' introduced by the patch for bug 920394 make sense for fixing that particular issue (my bad for hard coding 'app://' there...), they don't scale for the FTU (bug 913783) and Settings (bug 913784) apps: having 'appName' for Gaia domain and 'appOrigin' for 3rd party is not that intuitive given that we can simply have 'appOrigin' for both and figure the URL schema properly when setting them -- this is how the keyboard setting were originally set in 'keyboard_helper.js' (https://github.com/mozilla-b2g/gaia/blob/master/shared/js/keyboard_helper.js#L95-L107)

Rudy, what do you think of this approach (i.e. use 'appOrigin' and set schema properly)?
Flags: needinfo?(rlu)
Since build in keyboard can be 3rd party one. Need to fix it to allow 3rd party to be pre-loaded
blocking-b2g: koi? → koi+
No longer depends on: 913782
Hi Mihai,
  
   3rd-party preload apps get app.manifestURL and app.origin from metadate.json and their manifestURL and origin are fixed value on browser and devices , they do not like core apps have alternative value between browser and devices.
   
   For example, 
    
   marketplace (preload package app)
    Devices
     https://marketplace.firefox.com/packaged.webapp
     app://marketplace.firefox.com
    Browser
     https://marketplace.firefox.com/packaged.webapp
     app://marketplace.firefox.com" keyboard_helper.js

   Camera
    Devices
     app://camera.gaiamobile.org/manifest.webapp
     app://camera.gaiamobile.org
    Browser
     http://camera.gaiamobile.org:8080/manifest.webapp
     http://camera.gaiamobile.org:8080

As discuss with Rudy, we can reuse homescreen.json structure as the following link:
https://github.com/telefonicaid/firefoxos-gaia-spain/blob/master/homescreens.json.

If default keyboard is 3rd-party preload app we can get app.origin from its metadate.json in build time.
And if default keyboard is build-in keyboard app we can use |utils.gaiaOriginURL| to composite app.origin.

So here is my propose keyboard_layout.json.
     "as": [
        {"layoutId": "en", "appPath": "apps/keyboard"}
     ],
     "bn-BD": [
        {"layoutId": "en", "appPath": "apps/keyboard"}
     ],
     "ca": [
        {"layoutId": "en", "appPath": "apps/keyboard"}
     ],
     "cs": [
        {"layoutId": "en", "appPath": "external-apps/3rd-partykeyboard"}
     ],
Flags: needinfo?(rlu)
Updated according to suggestions (thanks Gary!)
Attachment #812377 - Flags: review?(rlu)
Comment on attachment 812377 [details]
Pull Request #12560 - Update keyboard_layouts.json structure

Hi Gary, Mihai,

For this work to move on, I think what Gary proposed make sense.
And we should mimic the way we customize homescreen icon arrangement.
(https://wiki.mozilla.org/B2G/MarketCustomizations#homescreens)
 
So we will need, 
   1. keyboards.json in our customization package (distribution)
      - The format should look similar to what Gary proposed here.
   2. a script to convert #1 to keyboard_layout.json 

--
BTW, according to Fabrice's comment (quoted below), we would need to identify an app by "manifest URL" instead of "app origin". 

===================
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.

Also, we want to get rid of entry points. Do you really launch keyboards
based on their entry points, or is that just a convenience to list them
in the settings app? If the later, you should switch to a different
manifest property.
Attachment #812377 - Flags: review?(rlu)
Hey Rudy, thanks for the info!

Just to make sure I get the desired approach right: from what I noticed, a distribution folder is used for build time customization with default values in "build/applications-data.js" and your proposed approach would imply:

1. adding the built-in keyboard layout data which is currently in "shared/resources/keyboard-layouts.json" in "build/applications-data.js" (i.e. something like https://github.com/mozilla-b2g/gaia/blob/master/build/applications-data.js#L160-L197)

2. parse the keyboard layout data from distribution package or default values and generate "keyboard-layouts.json"

3. use the newly generated "keyboard-layouts.json" to setup the keyboard during build time/FTE/Settings

Does this make sense? Or am I missing something?
Flags: needinfo?(rlu)
Functionally work just need some fine-tuned on the customization of keyboard selection. Move it to v1.3
blocking-b2g: koi+ → 1.3?
(In reply to Mihai Cirlanaru [:mihai][:mcirlanaru] from comment #10)
> Hey Rudy, thanks for the info!
> 
> Just to make sure I get the desired approach right: from what I noticed, a
> distribution folder is used for build time customization with default values
> in "build/applications-data.js" and your proposed approach would imply:
> 
> 1. adding the built-in keyboard layout data which is currently in
> "shared/resources/keyboard-layouts.json" in "build/applications-data.js"
> (i.e. something like
> https://github.com/mozilla-b2g/gaia/blob/master/build/applications-data.
> js#L160-L197)
> 
> 2. parse the keyboard layout data from distribution package or default
> values and generate "keyboard-layouts.json"
> 
> 3. use the newly generated "keyboard-layouts.json" to setup the keyboard
> during build time/FTE/Settings
> 
> Does this make sense? Or am I missing something?

Yes, this is consistent to what I understand.
Thank you.

Please help let Gary and me know if there is anything you need our help.
Flags: needinfo?(rlu)
Since this blocks bug 913784 we need to koi+ it. Sorry about the little oversight.

Mihai, what's the status here? Are you getting all the help you need?
blocking-b2g: 1.3? → koi+
Flags: needinfo?(mihai)
Hi Tim, this refactoring of keyboard_layouts.json is affecting a couple of other koi+ bugs (mainly the children of bug 863719). It wasn't quite clear to me what the approach with this should be when it was pushed to 1.3 and bug 913782 was taken by :gnarf, so I see a couple of options here:

1. I fix the minor regression my initial patch for bug 913782 caused and go with that keyboard_layouts.json structure for all other bug 863719 children bugs

2. Wait for :gnarf's solution on bug 913782 (which will involve refactoring of keyboard_layouts.json) and go with that structure for bug 913784, bug 913783, etc.

3. Focus on fixing this bug and use the solution for the others, though this will most probably affect :gnarf's work on bug 912010 and bug 913782 if he is still taking care of it

Let me know what option you think is best?
Flags: needinfo?(mihai) → needinfo?(timdream)
(In reply to Mihai Cirlanaru [:mihai][:mcirlanaru] from comment #14)
> Let me know what option you think is best?

Option #3 is probably the best, we need to get that's need in v1.2 as fast as possible BY ALL MEANS. If it can be done with a bandage patch, do it and provide a proper fix later. Thanks!
Flags: needinfo?(timdream)
(In reply to Mihai Cirlanaru [:mihai][:mcirlanaru] from comment #14)
> Hi Tim, this refactoring of keyboard_layouts.json is affecting a couple of
> other koi+ bugs (mainly the children of bug 863719). It wasn't quite clear
> to me what the approach with this should be when it was pushed to 1.3 and
> bug 913782 was taken by :gnarf, so I see a couple of options here:
> 
> 1. I fix the minor regression my initial patch for bug 913782 caused and go
> with that keyboard_layouts.json structure for all other bug 863719 children
> bugs
> 
> 2. Wait for :gnarf's solution on bug 913782 (which will involve refactoring
> of keyboard_layouts.json) and go with that structure for bug 913784, bug
> 913783, etc.
> 
> 3. Focus on fixing this bug and use the solution for the others, though this
> will most probably affect :gnarf's work on bug 912010 and bug 913782 if he
> is still taking care of it
> 
> Let me know what option you think is best?

Hi Mihai,

I am not sure of what Option 3 is going to do, so could you elaborate this and/or coordinate with Corey and me on this topic?
Thanks.
Flags: needinfo?(mihai)
Whiteboard: [3rd-party-keyboard]
> > 3. Focus on fixing this bug and use the solution for the others, though this
> > will most probably affect :gnarf's work on bug 912010 and bug 913782 if he
> > is still taking care of it
> > 
> 
> Hi Mihai,
> 
> I am not sure of what Option 3 is going to do, so could you elaborate this
> and/or coordinate with Corey and me on this topic?
> Thanks.

Hey Rudy, for option 3 I will need to fix this bug following the approach detailed in comment 10 and then, based on the keyboard_layouts.json introduced here, the following bugs can be fixed: bug 913782, bug 913783, bug 913784 -- for which I already have pending patches that can be quickly updated
Flags: needinfo?(mihai)
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.
blocking-b2g: koi+ → 1.3+
Hey Rudy, from what I notice the updates that this bug propose are already tackled in your patch for bug 913784, should we close this as WFM (since desired keyboard_layouts.json structure is now in place)?
Flags: needinfo?(rlu)
As comment 9 said, I think we could leave this bug open for "customization" that if a partner want to put some 3rd-party keyboard apps built-in, and the partner could specify which layouts in each app should be enabled by default for different languages.

This has been put into v1.3, since we have not received any committed requests to support this feature.

Let me know if you need more details.
Thanks.
Flags: needinfo?(rlu)
Since partner can still change the setting code to select default keyboard before build, this one can be addressed/enhanced in next release.
blocking-b2g: 1.3+ → madai?
Is this bug still valid? What is left to be done here?
Assignee: mihai → nobody
Component: Gaia::Keyboard → Gaia::System::Input Mgmt
Flags: needinfo?(rlu)
As comment 20 stated, this could be part of the customization (or distribution) work.
 1. Find out pre-installed 3rd-party IME apps if any.
 2. Read a config file, say default_ime_mapping.json in distribution package, which will store:
    language => ime app + layoutId
 3. Generate 'shared/resources/keyboard_layouts.json', based on #1 and #2.
Flags: needinfo?(rlu)
Correct the blocking-b2g flag to 1.4?
blocking-b2g: madai? → 1.4?
Assignee: nobody → rlu
Attached file Patch V1.1 (obsolete) (deleted) — 
This is a WIP.

Yuren,

This is the work as our offline talk, please help take a look if you have time.
I'll refine it to read in distribution data and fix up the tests.

Thanks.
Attachment #8361053 - Flags: feedback?(yurenju.mozilla)
Comment on attachment 8361053 [details]
Patch V1.1

looks good to me and I suggest move |setDefaultKeyboardLayouts()| into keyboard-config.js to make it clearer.
Attachment #8361053 - Flags: feedback?(yurenju.mozilla) → feedback+
Comment on attachment 8361053 [details]
Patch V1.1

Hi Yuren,

The patch has been updated to include the unit tests for both of the added functions.
Could you please help review this?

Thanks.
Attachment #8361053 - Attachment description: WIP V1 → Patch V1.1
Attachment #8361053 - Flags: review?(yurenju.mozilla)
Yuren,

BTW, I have tested this on windows, and it works fine.
Thanks for the reminding.
Comment on attachment 8361053 [details]
Patch V1.1

looks good to me and r=yurenju if nits is addressed.
Attachment #8361053 - Flags: review?(yurenju.mozilla) → review+
Merged to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/cffbe7a643525f479a1e20538b38d442c73fa849

Yuren,

Thanks for the review.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Sorry, I am reopening this due to backing this out here: https://github.com/mozilla-b2g/gaia/commit/2e1a63ffc3ce108f522d6d69239aa876a97c7a99

I am reopening this because this is breaking common gaia workflows. The specific one I noticed as being broken is: "make install-gaia APP=communications". The specific error is as follows: 

Exception: Error: Using inexistent shared resource: keyboard_layouts.json from: communications.gaiamobile.org

execute/</<@resource://gre/modules/commonjs/toolkit/loader.js -> file:///Users/kevin/workspace/gaia/build/webapp-zip.js:581
execute/<@resource://gre/modules/commonjs/toolkit/loader.js -> file:///Users/kevin/workspace/gaia/build/webapp-zip.js:569
makeWebappsObject/<.forEach/<@resource://gre/modules/commonjs/toolkit/loader.js -> file:///Users/kevin/workspace/gaia/build/utils-xpc.js:250
makeWebappsObject/<.forEach@resource://gre/modules/commonjs/toolkit/loader.js -> file:///Users/kevin/workspace/gaia/build/utils-xpc.js:190
execute@resource://gre/modules/commonjs/toolkit/loader.js -> file:///Users/kevin/workspace/gaia/build/webapp-zip.js:329
@-e:1

make: *** [webapp-zip] Error 3
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: needinfo?(rlu)
Re-landed without removing the file, shared/resource/keyboard_layouts.json,
https://github.com/mozilla-b2g/gaia/commit/d79ca8856b056e24ed465bc02810c925266b3969


--
Kevin,

Sorry about breaking your workflow without knowing it would.
Have relanded the patch and tested that it won't break it again.

Thanks for the heads-up.
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Flags: needinfo?(rlu)
Resolution: --- → FIXED
Now this breaks DEBUG=1 make because the generated file uses http: instead of app: as checked in, so after building there is a modified file in the source tree. Kevin is backing it out again.

I think the original patch, which removed keyboard_layouts.json from the repo was the correct way to go. But because the FTU app also requires this file, we need to ensure that the file exists when that app is built. I think the settings app may be the same (or maybe that was only before the 3rd party framework).

I don't understand the build system well enough to know what the right fix is here.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Thanks for the details David. Backout commit is here: https://github.com/mozilla-b2g/gaia/commit/61866f17977a440a3297aa8ade8f410c68d2cdd7

Regarding the correct fix, I am sure you guys will figure something out, but I guess there are a few options. If we do end up removing it from the shared/ folder, I guess it would make the most sense to not access it by shared/ or have some one-off check for the filepath in the build script. Not the prettiest solution but might might be a quick fix.
(In reply to David Flanagan [:djf] from comment #33)
> Now this breaks DEBUG=1 make because the generated file uses http: instead
> of app: as checked in, so after building there is a modified file in the
> source tree. Kevin is backing it out again.
> 

May I confirm with you or Kevin that the build process could succeed, though extra modified file is generated, but everything else could work?

I tried to avoid this "modified file", but could not think of a better way per my poor knowledge on build system. will try to consult the expert to come out with a fix plan.

Thanks.
Flags: needinfo?(dflanagan)
(In reply to Rudy Lu [:rudyl] from comment #35)
> (In reply to David Flanagan [:djf] from comment #33)
> > Now this breaks DEBUG=1 make because the generated file uses http: instead
> > of app: as checked in, so after building there is a modified file in the
> > source tree. Kevin is backing it out again.
> > 
> 
> May I confirm with you or Kevin that the build process could succeed, though
> extra modified file is generated, but everything else could work?

Yes, as I recall, the build worked.  But after doing DEBUG=1 make, running git status would show a modified file.  If I had then made a patch and done a git commit -a, I would have accidentally added that a modified keyboard_layout.json file to my patch.

> I tried to avoid this "modified file", but could not think of a better way
> per my poor knowledge on build system. will try to consult the expert to
> come out with a fix plan.

It looks like the reason the original patch got backed out was that it broke the APP=communications case.  If the developer had done a make install-gaia or make profile first, then I suspect the subsequent builds would have worked fine.  

Kevin: could it be that for the original patch all you needed to do was to do a full build before doing an APP=communications build?  That seems like a general shortcoming of the build system.  Can we live with it? Or can we modify the build system to run all the build-time configuration steps even when the APP environment variable is set?  (I just faced this same proble for a build-time configuration change I landed that affected the Camera app.)

> 

> Thanks.
Flags: needinfo?(yurenju.mozilla)
Flags: needinfo?(kgrandon)
Flags: needinfo?(dflanagan)
I don't recall how I ran the install, as this was a while ago. I haven't really looked at the code here in depth yet to know what the problem is.

It should be totally fine to generate a file if needed, but just be sure to include it in the .gitignore so it's not in the git stage.

I think if you just make sure the following is met you'll be fine to land:

1 - You are able to install individual apps (including contacts) with make install-gaia APP=communications

2 - You are able to perform DEBUG=1 builds without dirtying the local tree.
Flags: needinfo?(kgrandon)
Rudy discussed with me, since applications-data.js will be removed after refactoring[1] and we will have a new module webapp-shared.js to handle all build process of shared directory, I recommand add a new makefile rule to build keyboard_layout.json and this part of code will be migrated to webapp-shared.js.

[1] https://wiki.mozilla.org/Gaia/Build/Refactoring_Plan
Flags: needinfo?(yurenju.mozilla)
Triage: 1.4 feature work.
blocking-b2g: 1.4? → 1.4+
Attached file Patch V1.4 - pull request 16404 (deleted) — 
Hi Yuren, George,

Please help review this patch about generate "shared/resources/keyboard_layouts.json".

The logic was kept unchanged, I only added the following file, so that the procedure would be run in a separate build target.
build/shared-makefiles.js

I know that we should not put the file in source dir, and should put it in build_stage/, however after talking about this with Yuren, maybe we can land this first and then refine it (That's why I set George as feedback needed).

Thanks.
Attachment #8361053 - Attachment is obsolete: true
Attachment #8377514 - Flags: review?(yurenju.mozilla)
Attachment #8377514 - Flags: feedback?(gduan)
This doesn't fall under the QC blocking feature list & DSDS feature list. Renoming.
blocking-b2g: 1.4+ → 1.4?
Comment on attachment 8377514 [details]
Patch V1.4 - pull request 16404

code in build system looks good if nits are addressed but as George said we need integration test for this feature in distribution.test.js, come to find me if you need any help.
Attachment #8377514 - Flags: review?(yurenju.mozilla)
Comment on attachment 8377514 [details]
Patch V1.4 - pull request 16404

Pull request updated to address review comments.
But it is still a WIP, since I need to add distribution integration test and check unit tests are ok after the modification.
Attachment #8377514 - Attachment description: Patch V1.2 - pull request 16404 → Patch V1.3 - pull request 16404
Moving to backlog - third party keyboard is being moved out of scope for 1.4.
blocking-b2g: 1.4? → backlog
Comment on attachment 8377514 [details]
Patch V1.4 - pull request 16404

pull request updated to add the distribution test.

Yuren,

Could you please review it again?
Let me know if there is anything else I need to address.
Thanks.
Attachment #8377514 - Attachment description: Patch V1.3 - pull request 16404 → Patch V1.4 - pull request 16404
Attachment #8377514 - Flags: feedback?(gduan) → review?(yurenju.mozilla)
Comment on attachment 8377514 [details]
Patch V1.4 - pull request 16404

looks good to me, please make sure you get green light on travis-ci!
Attachment #8377514 - Flags: review?(yurenju.mozilla) → review+
Landed to bubble-tea first,
https://github.com/mozilla-b2g/gaia/commit/20954fbbe8e6b3761d9e55caf485220399fbf202
Whiteboard: [3rd-party-keyboard] → [3rd-party-keyboard], [in-bubble-tea]
travis and try server are green, merged.

https://github.com/mozilla-b2g/gaia/commit/f406b7ec0b73999aeb62979a9f6226a6edade2b5
Status: REOPENED → RESOLVED
Closed: 11 years ago → 10 years ago
Resolution: --- → FIXED
Yuren,

Thanks for your great help to land this.
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: