keyboard build-time customization

RESOLVED FIXED in 1.3 Sprint 3 - 10/25

Status

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: gal, Assigned: djf)

Tracking

unspecified
1.3 Sprint 3 - 10/25
x86
macOS
Dependency tree / graph

Firefox Tracking Flags

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

Details

Attachments

(1 attachment, 2 obsolete attachments)

46 bytes, text/x-github-pull-request
rudyl
: review+
yurenju
: review+
Pike
: review+
Pike
: feedback+
Details | Review
Reporter

Description

6 years ago
This doesn't change the dictionaries we are actually using, only the source dictionaries. We should test the old and new .dict files and if there is no regression, we can switch over.
Assignee

Comment 2

6 years ago
Taking this from Andreas at his request. I'll create a new pull request that actually updates the compiled dictionary files.
Assignee: nobody → dflanagan
Assignee

Comment 3

6 years ago
Andreas,

My PR is going to include dictionary files for all of the new wordlists. This means that the size of the keyboard app goes from 10mb to 30mb. I suppose that vendors may have a way to customize the build so that it only includes the dictionaries they want.

Are you okay with this increase in the keyboard app file size?
Flags: needinfo?(gal)
Assignee

Comment 4

6 years ago
Posted file link to patch on github (obsolete) —
Rudy,

Andreas noticed that there are new versions of the Android wordlists, and a number of new languages as well.  This PR includes the new wordlists and does some organization on the way the dictionary files are built.

If it helps your review you might also see Andreas's initial PR for this bug here https://github.com/mozilla-b2g/gaia/pull/10484

Andreas: the only critical thing I changed from your pr is to switch back to python3. With python2, the generated dictionary files are corrupted in some subtle way so that they give plausible but poor suggestions.
Attachment #765803 - Flags: review?(rlu)
Attachment #765803 - Flags: feedback?(gal)
Assignee

Comment 5

6 years ago
Comment on attachment 765803 [details]
link to patch on github

This is not a blocker, but it seems like we might as well ship 1.1 with the latest set of dictionaries. This PR adds support for a whole lot of European languages, and it will be really nice to land this along with bug 874011, which is a Leo+ blocker.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:
Attachment #765803 - Flags: approval-mozilla-b2g18?
Reporter

Comment 6

6 years ago
We should probably not include all languages, or at least make the OEM aware that they can change how many langues they want to include in a build since 30MB is a lot.
Flags: needinfo?(gal)
Assignee

Comment 7

6 years ago
See https://bugzilla.mozilla.org/show_bug.cgi?id=874011#c29 for my plan to only include the dictionaries that are needed in the build.
Assignee

Comment 8

6 years ago
Comment on attachment 765803 [details]
link to patch on github

Cancelling the review request based on Andreas's feedback. I'll make the list of keyboards and dictionaries configurable so that only the ones needed for a build are included.
Attachment #765803 - Flags: review?(rlu)
Attachment #765803 - Flags: feedback?(gal)
Comment on attachment 765803 [details]
link to patch on github

At this point we are no longer doing approvals to b2g18 branch unless the bug is a security issue - all other bugs must request leo+ status to be uplifted. If there's a partner champion for this issue, they can push for it to be a blocker otherwise this will have to ride the trains and be in v1.2.
Attachment #765803 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18-
Assignee

Updated

6 years ago
Duplicate of this bug: 902120

Updated

6 years ago
Duplicate of this bug: 815966
Reporter

Updated

6 years ago
blocking-b2g: --- → koi+
Assignee

Comment 12

6 years ago
I think we may need to fix this for leo, nominating.

Bug 908286 added a bunch of autocorrect dictionaries for 1.1, but it has broken the build for Buri which apparently has a pretty small partition (see bug 909392)

The only solution I can think of is to make the list of dictionaries configurable at runtime.  Which is what this bug is about.
blocking-b2g: koi+ → leo?
Assignee

Comment 13

6 years ago
Looks like this was a false alarm. Bug 909392 appears to have been a build script problem. Setting this back to koi?
blocking-b2g: leo? → koi?
blocking-b2g: koi? → koi+
Blocks: 915697
Blocks: 907444
Assignee

Comment 14

6 years ago
This bug has taken on new urgency now that we have so many autocorrect dictionaries that our builds have gotten too big.

We need a solution that allows us (and our partners) to specify at build time, which keyboard layouts and autocorrect dictionaries should be included in the build.  We already have this for locales, and need something similar for keyboards.

This is mostly a matter of customizing the keyboard app. (Though the settings app and maybe the FTU app might be affected as well.)

Locale customization is a system-wide thing and not the same as the app-specific customization we need here, so I don't think it makes sense to try to tie keyboard customization to the locale customization.

As a general thing, we need a per-app build-time customization mechanism. In a number of our apps we do this weird thing where we make them use XHR to read a json configuration file on startup, or make them read a customization from the settings db on startup.  But these customizations are hard-coded at build time, and there is no reason the customizations couldn't take the form of .js files that were incorporated directly into the application.  This would allow us to cut-down on async code during startup and everyone would be happier.

I imagine a build mechanism where we'd be able to specify multiple customizations by name and have them applied to the build:

  CUSTOM=eastern_european_keyboards,hdscreen,hires_camera make

I'm not proposing to do all that here, but I want to keep a general-purpose customization mechanism in mind while working on this keyboard customization.
Reporter

Comment 15

6 years ago
How far are we from being able to ship keyboard/dictionary/layout for each locale as a separate app? Then we could have a basic set, and you can download more from the marketplace.
Assignee

Comment 16

6 years ago
(In reply to Andreas Gal :gal from comment #15)
> How far are we from being able to ship keyboard/dictionary/layout for each
> locale as a separate app? Then we could have a basic set, and you can
> download more from the marketplace.

My understanding is that we can do that now, though it requires duplicating all of the keyboard app code for each separate keyboard.  See also this thread on dev-gaia about runtime customization of layouts: https://groups.google.com/forum/#!topic/mozilla.dev.gaia/-uBm7XvGq_U  Basically, it might be nice to be able to add layouts and dictionaries in whatever way we allow the download of new themes.  Maybe we can make this even simpler than downloading an app.

But we still need this bug for the ability to customize which dictionaries go into a build, however.
Assignee

Comment 17

6 years ago
Posted file link to patch on github (obsolete) —
Dave and Jason,

The attachment is a link to a work-in-progress patch on github.  I'm asking for your feedback to see if I'm no the right track here: will this help with the bugs you're working on?

What I've done here is remove the dictionaries from the keyboard app, and modify gaia/Makefile to copy a specified set of dictionaries back into the app at build time.  By default I only copy 5 or 6 of them, but that can be customized by setting an environment variable before running make so that vendors can create builds with whatever set of dictionaries they want.

The reason that this is a work in progress is that keyboard layouts that were expecting to find dictionaries generate some error messages in logcat and still display the autocorrect keyboard area even though they don't have a dictionary.
So I'll have to do some more work before we can land this, but wanted you to see what I have so far.
Attachment #807477 - Flags: feedback?(jsmith)
Attachment #807477 - Flags: feedback?(dhylands)
Yeah - that would definitely help. I did some analysis and discovered that effect of the compressed dictionaries on the image size is:

     ca.dict    897,962
     cs.dict  1,027,499
     de.dict  1,495,362
     el.dict  1,294,557
  en_us.dict  1,008,487
     es.dict  1,107,500
     fr.dict  1,284,774
     hr.dict  1,201,274
     hu.dict  2,664,987
     nl.dict  1,322,853
     pl.dict  1,176,933
  pt_br.dict  1,071,540
     ro.dict    677,943
     ru.dict  1,390,883
     sk.dict  1,159,719
sr-Cyrl.dict  1,133,617
sr-Latn.dict  1,090,746
     tr.dict  1,099,920

jszhuyin phrases.json:        1,500,764
jspinyin empinyin_files.data:   703,033
jspinyin libpinyin.js:          186,668

So even being able to configure just the latin dictionaries could yield a significant size reduction (and the reduction is multipled by 2 since the base and the OTA are both smaller by the same amount)
Attachment #807477 - Flags: feedback?(dhylands) → feedback+
Comment on attachment 807477 [details]
link to patch on github

I'm probably not the right person to give feedback on something like this implementation-wise. I could give input to check and see if this allows the unagi engineering build to be flashed or not though.
Attachment #807477 - Flags: feedback?(jsmith)
Assignee

Comment 20

6 years ago
Continuing to work on this. I'm splitting all the known keyboard layouts from the layouts.js file into separate js files that can be dynamically loaded individually. 

We'll have a top-level gaia/keyboards directory that has dictionaries/ and layouts/ subdirectories.

The build process will copy a specified set of layouts and dictionaries into the keyboard app at build time.

In order to make this work, it will also have to modify apps/keyboard/manifest.webapp at build time, since that is now how the keyboard framework finds out what layouts are supported by a given keyboard app.

So I probably won't do this with just a Makefile. I'll need to create a script in build/keyboard_layouts.js or something.  If I can do this intelligently, it can do everything it needs with just the list of layouts, and can read the individual layout files to determine the display of the layout and the dictionary it needs.  If I can't make the script that intelligent, then the configuration will have to be based on a keyboard_layouts.json file that defines layouts, dictionaries and language names.
Reporter

Comment 21

6 years ago
It seems weird to have different manifests for the same app. Even though we currently install locally in the future we want to host things things (logically) and offline cache. Any other approaches here? Something that gets us closer to language packs?
(In reply to Andreas Gal :gal from comment #21)
> It seems weird to have different manifests for the same app. Even though we
> currently install locally in the future we want to host things things
> (logically) and offline cache. Any other approaches here? Something that
> gets us closer to language packs?

For 1.2 we should just do what David is proposing. For 1.3 we need to figure out a more flexible approach that let users customize their keyboards, and not just people doing the builds.
Assignee

Comment 23

6 years ago
Comment on attachment 807477 [details]
link to patch on github

Rudy,

This patch modifies the keyboard app so that the set of layouts (and dictionaries) can be set at build time.

Aus: I see that you've worked on the build/ directory recently so I'm also asking for your review of the build-time configuration mechanism here.

Dave and Stephen: this patch isn't reviewed yet, but if you apply it and build gaia, you'll find that the keyboard app goes from 25mb to 10mb because now it only builds wiht 6 keyboard layouts and dictionaries. Maybe this will help wiht your build-too-big bugs.  Set GAIA_KEYBOARD_LAYOUTS to a list of layout names at build time to configure.
Attachment #807477 - Flags: review?(rlu)
Attachment #807477 - Flags: review?(aus)
Attachment #807477 - Flags: feedback?(stephen.donner)
Attachment #807477 - Flags: feedback?(dhylands)
Attachment #807477 - Flags: feedback+
Comment on attachment 807477 [details]
link to patch on github

A 15 MB reduction (is this in appliction.zip?) translates to a 30 Mb savings (since it will reduce the size of the image flashed as well as the size of the image OTA'd, so that will be fantastic.
Attachment #807477 - Flags: feedback?(dhylands) → feedback+
Thanks, Dave + David -- we don't self-build, especially not engineering builds, but I'm a +1 to this change too!
Attachment #807477 - Flags: feedback?(stephen.donner) → feedback+
Assignee

Comment 26

6 years ago
(In reply to Andreas Gal :gal from comment #21)
> It seems weird to have different manifests for the same app. Even though we
> currently install locally in the future we want to host things things
> (logically) and offline cache. Any other approaches here? Something that
> gets us closer to language packs?

The thing I dislike most about this patch is that it has to modify manifest.webapp. The 3rd party keyboard framework that just landed needs a way to know what layouts each keyboard app supports.  And it does that through the entry_points property of the manifest. I only just learned about this myself, but it means that we can't add new layouts at runtime.  I agree with Fabrice that this is something we need to fix in 1.3.  

Downloading new layouts and dictionaries ought to be as simple as downloading a pdf file and launching the pdf.js viewer app.  You ought to be able to just click a link and get a new layout or new autocorrect dictionary.

But we'll have to solve the issue of having apps list their layouts.
Comment on attachment 807477 [details]
link to patch on github

The build bits looks good to me David :)
Attachment #807477 - Flags: review?(aus) → review+
Assignee

Updated

6 years ago
Attachment #807477 - Flags: review?(rlu) → review?(janjongboom)
Can we get this landed and uplifted soon?  This is blocking bug 907444, which prevents us from testing OTA testing on the 1.2 and master branch.
Comment on attachment 807477 [details]
link to patch on github

Guess you didn't check in build/configure-keyboard.js.

Exception: Module `configure-keyboard` is not found at file:///Users/janjongboom/repos/gaia/build/configure-keyboard.js when running make...
Attachment #807477 - Flags: review?(janjongboom) → review-
Assignee

Comment 30

6 years ago
Comment on attachment 807477 [details]
link to patch on github

Jan,

I've updated the pull request to include the missing file and have also rebased so it should apply cleanly now.  Sorry about that!
Attachment #807477 - Attachment description: work in progress patch → link to patch on github
Attachment #807477 - Flags: review- → review?
Attachment #807477 - Flags: review? → review?(janjongboom)
Comment on attachment 807477 [details]
link to patch on github

Few nits on GH. The make command still fails, `make: *** No rule to make target `build-config-js', needed by `keyboard'.  Stop.` Changed config myself and got the thing to run and seems to be all alright. The naming convention of dictionaries that are all lowercase (pt-br) vs. layout files which aren't (pt-BR) feels a bit weird but soit.
Attachment #807477 - Flags: review?(janjongboom) → review-
FYI, if make works, the config changes based on the env variable and the things on GH are addressed the patch is fine by me. :-)
Assignee

Comment 33

6 years ago
Jan: I've updated again, so the make file works. I've also modified the build script to include error messages for missing layout files and dictionaries.

The patch appears to break pinyin input, but I don't know why.  We may need to break that on master for now in order to get this patch uplifted to 1.2...

Rudy: do you know why the pinyin keyboard does not with this patch?  Note that to build with the pinyin layout files it is now necessary to do:

  GAIA_KEYBOARD_LAYOUTS=en,fr,zh-Hans-Pinyin make install-gaia

Actually a make reset gaia may be needed since this rebuilds the keyboard manifest file.  If you do that, you'll see pinyin as one of the layout options, but the IM doesn't work.  Here's the logcat:

E/GeckoConsole(  826): [JavaScript Warning: "Error: successfully compiled asm.js code (total compilation time 1669ms; 1 functions compiled slowly: _malloc:21024:9 (255ms))" {file: "app://keyboard.gaiamobile.org/js/imes/jspinyin/libpinyin.js" line: 0}]
E/GeckoConsole(  826): [JavaScript Error: "uncaught exception: abort() at abort@app://keyboard.gaiamobile.org/js/imes/jspinyin/libpinyin.js:28509
E/GeckoConsole(  826): assert@app://keyboard.gaiamobile.org/js/imes/jspinyin/libpinyin.js:441
E/GeckoConsole(  826): addRunDependency@app://keyboard.gaiamobile.org/js/imes/jspinyin/libpinyin.js:884
E/GeckoConsole(  826): @app://keyboard.gaiamobile.org/js/imes/jspinyin/user_dict.js:26
E/GeckoConsole(  826): callRuntimeCallbacks@app://keyboard.gaiamobile.org/js/imes/jspinyin/libpinyin.js:724
E/GeckoConsole(  826): preRun@app://keyboard.gaiamobile.org/js/imes/jspinyin/libpinyin.js:753
E/GeckoConsole(  826): run@app://keyboard.gaiamobile.org/js/imes/jspinyin/libpinyin.js:28461
E/GeckoConsole(  826): @app://keyboard.gaiamobile.org/js/imes/jspinyin/libpinyin.js:28524
E/GeckoConsole(  826): @app://keyboard.gaiamobile.org/js/imes/jspinyin/libpinyin.js:28527
E/GeckoConsole(  826): "]
Flags: needinfo?(rlu)
Assignee

Updated

6 years ago
Attachment #807477 - Flags: review- → review?(janjongboom)
Comment on attachment 807477 [details]
link to patch on github

I'll r+ it, but... If you build without english keyboard enabled keyboard's won't work at all. We'll need a follow up bug for it.

Pinyin works for me by the way (make clean first).

Also the ~/keyboards folder doesn't pass linting, see github.
Attachment #807477 - Flags: review?(janjongboom) → review+
Assignee

Comment 35

6 years ago
Jan: can you actually input Chinese text with the Pinyin keyboard, or can you just see the layout? For me, the layout was working, but the IME was not and no input was visible...
I have asked Luke's help to take a look, since he is the expert on our pinyin input method.
Flags: needinfo?(rlu) → needinfo?(lchang)
Yeah, I don't know if it makes sense what it inputs, but I saw characters. I think it was on Fx Nightly though, haven't tested on device.
Hi David,

It appears that pinyin also works fine for me on either devices or firefox nightly.

Once in a while, however, I was confronted with an issue that I can't enable pinyin layout (or any other layouts which are not in default set) from settings app until I reset-gaia but I can't reproduce it. I always use "make clean" and "make reset-gaia" so I think it is not caused by manifest.

BTW, I met some minor issue when I tried to build the gaia with this patch so I left some personal opinions in the pull request. I hope this will be helpful.
Flags: needinfo?(lchang)
Assignee

Comment 40

6 years ago
I'm guessing that this is going to require a tricky manual uplift.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Looks like it was trickier than that, that broke gaia-ui-tests, https://tbpl.mozilla.org/php/getParsedLog.php?id=29339449&full=1&branch=mozilla-aurora (timeout in test_settings_change_keyboard_language.py)
(In reply to Phil Ringnalda (:philor) from comment #42)
> Looks like it was trickier than that, that broke gaia-ui-tests,
> https://tbpl.mozilla.org/php/getParsedLog.
> php?id=29339449&full=1&branch=mozilla-aurora (timeout in
> test_settings_change_keyboard_language.py)

Do we need to backout?
Yes please.
And let me know to let the pushbot through, since mozilla-aurora is closed because of this.
David - looks like we need to back this out on 1.2. Can you back it out?
Flags: needinfo?(dflanagan)
Reporter

Comment 48

6 years ago
Nothing to be sorry about. When David is back I am sure he will thank you for having his back here! Remember to always reopen a bug when you back out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: needinfo?(dflanagan)
Assignee

Comment 49

6 years ago
Sorry everyone!

Ben: Thanks for backing out.

Phil: did the tests fail only on the m-a tree?  So not on the original patch but only on the uplift?

It appears that Travis never ran any tests on my pull request. I have no idea why. But in any case, I wasn't seeing any failures on github.  And Travis only runs against master, right? So I wouldn't have expected Travis to catch test failures for my uplift...  I guess that means that I need to figure out how to run tests manually.
Assignee

Comment 50

6 years ago
I'm going to need some help figuring these test failures out. It doesn't look like there are specific keyboard tests that are failing. Just a bunch of tests that bring up the keyboard are failing with:

ERROR file:///builds/slave/test/build/b2g/components/MozKeyboard.js:360

My patch is pure gaia and does not touch that gecko file.  There used to be a b2g/components/MozKeyboard.js file. Looks like it is called dom/inputmethod/MozKeyboard.js now. I don't get how that ends up in b2g/components/ but whatever.

Jan, or Yuan: hg log tells me that you have both worked on MozKeyboard.js.  Can either of you help me interpret these test failures?
Flags: needinfo?(xyuan)
Flags: needinfo?(janjongboom)
They did indeed fail on the trunk as well, but until bug 927404 landed, gaia-ui-test was extremely green on tbpl because if it failed, it stayed green and didn't put the failure in the log at all, or only stuck one "INFO - ERROR" line in with nothing more than that. https://tbpl.mozilla.org/php/getParsedLog.php?id=29352095&tree=B2g-Inbound#error19 was the very first run where an error actually made it as far as the log.
Hi David, I just realize that this is conflicting with making the keyboard entrypoints localizable, too. See bug 927855.

Please don't move localizable data into new files without me knowing, it breaks my automation to expose strings to localizers.

Your configure-keyboards.js doesn't support localized entrypoints, too.

Can you rewrite this so that you don't need to change file types in the source? Yuren is rewriting some of these parts in webapp-zip in bug 922463, which might be helpful.
This problem was addressed 25 days ago, but not right...

https://github.com/mozilla/mozilla-central/commit/e58a21d0cee1506ccf156c50544cc5b5202007f6

We should fix the check there, because this makes no sense.
Flags: needinfo?(janjongboom)
Assignee

Comment 55

6 years ago
(In reply to Axel Hecht [:Pike] from comment #52)
> Hi David, I just realize that this is conflicting with making the keyboard
> entrypoints localizable, too. See bug 927855.
> 
> Please don't move localizable data into new files without me knowing, it
> breaks my automation to expose strings to localizers.
> 
> Your configure-keyboards.js doesn't support localized entrypoints, too.

In 1.1 we decided not to localize the name of keyboard layouts and just have them displayed in their own locale. The English layout would be "English" everywhere. The French layout would be "Français" everywhere, etc.  The original 3rd party keyboard framework patch broke this completely, and all layouts are displayed in English everywhere.

My patch restores the settings app to 1.1 behavior: keyboard layouts are named in the locale that uses the layout.  Its is not perfect, but is much better than what we have now, and matches 1.1.  Will that be good enough for 1.2, or are we aiming for full localization in that release?

> 
> Can you rewrite this so that you don't need to change file types in the
> source? 

I have no idea what you mean by this.

In any case, OTA updates are failing until this bug lands, so I think we should land it ASAP, and then file followup bugs to modify it as needed.

Also note that listing keyboard entry points in the manifest is kind of a hack, in my opinion, and it means that keboard apps can not dynamically load new layouts.  In 1.3 (or 1.4?) we are presumably going to have to switch to using DataStore instead of the manifest, and this will all change again.

Yuren is rewriting some of these parts in webapp-zip in bug 922463,
> which might be helpful.

That bug is not koi+, and this one is, so
Assignee

Comment 56

6 years ago
Ah. I see that in Phil's original comment he said that the test failure was: "timeout in test_settings_change_keyboard_language.py".  I missed that the first time I read it and got hung up on the MozKeyboard.js :360 thing

This patch changes thing so that the settings app now displays the name of each keyboard layout in the language of the layout.  (That's how we do it in 1.1, I think.) It looks like maybe the test is looking for the string "Spanish" to appear, but with this patch the expected string should be "Español".  That's my guess anyway, from looking at the error log.

Unfortunately, all I know about test_settings_change_keyboard_language.py is that it is not part of gaia.  I don't know where that test lives, who wrote it, how to run it manually, or how to get it changed.

Given that this test is a python file, I'm assuming that it is in the gecko tree somewhere. I could probably find it with mxr and try to fix it. But if I modify the test in gecko before I land the gaia patch, then the test will fail and my patch would be backed out.  And if I land my gaia patch first, then the test will fail and my gaia patch will be backed out.

Who knows what to do here?
Yeah, as a matter of fact I *do* wish I'd read that page before pasting the links, since as it says, it is in gaia, https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/tests/functional/settings/test_settings_change_language.py
djf, it would be great if you can modify and copy files into zip file, then you don't need to change anything in gaia source tree, you can reference webapp-zip.js

and please set review flag to Alexandre Poirot if you modify build system. and you also can set feedback flag to me since I have modified build scripts a lot.
Btw - seeing as these changes require gaia ui test updates, feel free to work with Zac on this.
(In reply to David Flanagan [:djf] from comment #55)
> (In reply to Axel Hecht [:Pike] from comment #52)
> > Hi David, I just realize that this is conflicting with making the keyboard
> > entrypoints localizable, too. See bug 927855.
> > 
> > Please don't move localizable data into new files without me knowing, it
> > breaks my automation to expose strings to localizers.
> > 
> > Your configure-keyboards.js doesn't support localized entrypoints, too.
> 
> In 1.1 we decided not to localize the name of keyboard layouts and just have
> them displayed in their own locale. The English layout would be "English"
> everywhere. The French layout would be "Français" everywhere, etc.  The
> original 3rd party keyboard framework patch broke this completely, and all
> layouts are displayed in English everywhere.
> 
> My patch restores the settings app to 1.1 behavior: keyboard layouts are
> named in the locale that uses the layout.  Its is not perfect, but is much
> better than what we have now, and matches 1.1.  Will that be good enough for
> 1.2, or are we aiming for full localization in that release?

So the data in the entrypoint entries isn't used at all? If so, why is it there, then? Right now, the entrypoints are in English, not in the local language.

> > 
> > Can you rewrite this so that you don't need to change file types in the
> > source? 
> 
> I have no idea what you mean by this.
> 
> In any case, OTA updates are failing until this bug lands, so I think we
> should land it ASAP, and then file followup bugs to modify it as needed.
> 
> Also note that listing keyboard entry points in the manifest is kind of a
> hack, in my opinion, and it means that keboard apps can not dynamically load
> new layouts.  In 1.3 (or 1.4?) we are presumably going to have to switch to
> using DataStore instead of the manifest, and this will all change again.
> 

You're changing the location of localizable resources from manifest.webapp to manifest.template. That breaks. If you want to expose new locations of localizable resource, I need to know that, and approve that before things land. That is the same in all our products, btw.
Assignee

Comment 62

6 years ago
(In reply to Axel Hecht [:Pike] from comment #61)
> 
> So the data in the entrypoint entries isn't used at all? If so, why is it
> there, then? Right now, the entrypoints are in English, not in the local
> language.
> 

But this patch fixes that bug. The script generates entry points in the manifest file that uses the local language for each keyboard layout.  (Though that isn't the case for the "number" layout).

> > > 
> > > Can you rewrite this so that you don't need to change file types in the
> > > source? 
> > 
> > I have no idea what you mean by this.
> > 
> 
> You're changing the location of localizable resources from manifest.webapp
> to manifest.template. That breaks. If you want to expose new locations of
> localizable resource, I need to know that, and approve that before things
> land. That is the same in all our products, btw.

Ah. I get it now, and I see why that is a problem.  Sorry!

We could tweak the build scripts so that this patch does not need to change the name of the manifest.webapp file.  (I'd just need another step to clean up after building the manifest).

Going forward, though, if we are going to allow the keyboard app to be customzied at build time, we should treat the keyboard layout files as the source of truth, not the manifest file.

Also, if we are going for full localization of the keyboard layout names, it would actually be easiest to do that localization in the settings app rather than the keyboard app. We could localize each of the entry points in the generated manifest file. But since they are not real entry points, the localization would not be automatic and the settings app would end up having to include custom localization code.  It would be less error prone to give each keyboard layout (and therefore each entry point) a localization key and handle the strings in apps/settings/locales/

I'm not going to have time to work on this bug for a few days at least. If anyone can move it forward, please feel free to take it.
I'm slowly starting to grok this patch. There's a lot of data in the way between me and the truth, which confuses me, sorry.

I think it's cool to have the keyboard layout names be only in their native language.

I think it's OK to read the keyboard names from the keyboard js files, didn't find the getLayoutNameAndDict in https://github.com/davidflanagan/gaia/blob/61c539f489342d7f085dc1f9c661f879cac5eb91/build/configure-keyboard.js when I read the code.

We should remove the entrypoints completely in the source manifest then, though, shouldn't we? If they're the source of truth, let's make it so.

We should avoid the patching of the source dir, we can create the manifest.webapp via a hook in webapp-manifests.js, I think. communications already does that. I don't think we need the full monty of Yuren's work for that.
Target Milestone: --- → 1.3 Sprint 3 - 10/25

Updated

6 years ago
Blocks: 927855
In v1.1, we show each keyboard layout in its native language because we have several late-coming requirements to add some new keyboard layouts that we cannot afford to localize (will block the release).

Since that is a late change with some compromise, I would assume that we want to get the full localization back in v1.2.
On second thought and a few offline discussion with Yuren and Tim, here is is another proposal that might be simplify this patch here a lot,

We could make this build time keyboard config to take care of dictionaries only and leave all the layouts  still available on the system.
This way we don't have to modify the manifest and can avoid the localization issue.
Actually, this is what we did for v1.1 that our partner might have removed some dictionaries from their builds so that it can fit into the limited storage.

I admit this is not elegant, but given we are not far from v1.2 code freeze, I would suggest we take this approach (again) and land this first.

David, do you agree on this?
Flags: needinfo?(dflanagan)
Flags: needinfo?(yurenju.mozilla)
Assignee

Comment 66

6 years ago
(In reply to Rudy Lu [:rudyl] from comment #65)
> On second thought and a few offline discussion with Yuren and Tim, here is
> is another proposal that might be simplify this patch here a lot,
> 
> We could make this build time keyboard config to take care of dictionaries
> only and leave all the layouts  still available on the system.
> This way we don't have to modify the manifest and can avoid the localization
> issue.

And our partners will have to edit the manifest by hand to add or remove layouts.

> Actually, this is what we did for v1.1 that our partner might have removed
> some dictionaries from their builds so that it can fit into the limited
> storage.
> 

And if we're not going to allow build-time customization of layouts, I don't know if it is worth the effort for dictionaries.  Maybe we can just have partners add and remove dictionaries as they please.

> I admit this is not elegant, but given we are not far from v1.2 code freeze,
> I would suggest we take this approach (again) and land this first.
> 
> David, do you agree on this?

I'd prefer to either do nothing here, or to solve the problem correctly.

Eventually, we want to be able to dynamically add new layouts (by downloading them, for example). So all of the layout localization information is going to have to live in the layout file eventually.  I suggest that we do that now.  I've been meaning to talk this through with Axel, but haven't done that yet.

Its not that it is a difficult patch, it is just that I haven't had much time.  And we have to make sure that what we do works for localizers.
Flags: needinfo?(dflanagan)
FWIW, I also like the ability to configure the keyboard layouts. Much better than to have hidden features like keyboards that work one way in one build and in another way in other builds, IMHO.
Assignee

Comment 68

6 years ago
I've started to work on this again in a new pull request here: https://github.com/mozilla-b2g/gaia/pull/13106

I've attempted to fix the test and will see if the travis build is green this time.
Assignee

Comment 69

6 years ago
I've attached a new pull request.

This one fixes the test that was not passing.

And it moves the build-time configuration into build/webapp-manifests.js as someone (Axel?) suggested above.  This means that there is no longer a manifest.template file making l10n confusing.

manifest.webapp now only has a single entry point hardcoded in it for the numeric layout.  This will need localization.

All the other layouts (for text) are defined in separate layout files in gaia/keyboard/layouts/.  These layout files are used to generate entry points in the manifest file at build time.

Each layout file includes a "menuLabel" property, and that is used as the name of the keyboard layout.  Each one is only localized in its own language, but Axel said above that he was okay with that.
Attachment #765803 - Attachment is obsolete: true
Attachment #807477 - Attachment is obsolete: true
Attachment #822659 - Flags: review?(rlu)
Attachment #822659 - Flags: review?(l10n)
Assignee

Comment 70

6 years ago
Comment on attachment 822659 [details] [review]
revised pull request

Yuren,

Would you review the build system modifications, please?
Attachment #822659 - Flags: review?(yurenju.mozilla)
Flags: needinfo?(yurenju.mozilla)
Comment on attachment 822659 [details] [review]
revised pull request

|configureKeyboard| in webapp-manifest.js do two things: copy js into keyboard app and modify manifest. however the first one isn't a part of job for webapp-manifest.js, we should move those code to an indiviual module, seperate to two function and use |require| to use it in webapp-manifest.js and webapp-zip.js (or applications-data.js if you want to use it in DEBUG=1 mode).
Attachment #822659 - Flags: review?(yurenju.mozilla) → review-
Assignee

Comment 72

6 years ago
You're right. Copying the files in webapp-manifest.js is a hack. I did it because figuring out what files to copy and figuring out how to modify the manifest both require me to read the selected keyboard layout files and I didn't want to do that twice...

You're right though. And I like the suggestion of putting this code in a separate module.  I'll do that.

Meanwhile, Rudy and Axel, it should still be possible for you to review the l10n implications of this patch even before I modify the build system stuff.
Assignee

Comment 73

6 years ago
Comment on attachment 822659 [details] [review]
revised pull request

Yuren,

I've modified the build system code as you suggested.  The keyboard configuration code is in build/keyboard-config.js, which is require()'ed into applications-data.js and webapps-manifest.js when it is needed.

Please take another look.
Attachment #822659 - Flags: review- → review?(yurenju.mozilla)
Comment on attachment 822659 [details] [review]
revised pull request

hi djf, I left some comments in github, getting two differnent manifest files  and using javascript for layout but not JSON are the major issues. I will give r+ if we fix those two issues, thanks!
Attachment #822659 - Flags: review?(yurenju.mozilla)
Comment on attachment 822659 [details] [review]
revised pull request

r=me for the keyboard app related part.

David, thanks for your work.
Attachment #822659 - Flags: review?(rlu) → review+
I got comments on the comments that yuren has ;-)

I don't think there's a great factorization to use the webapp object instead of constructing the path itself. The only way to those seems through utils.getGaia(), which hides each app in a massive iterator, and gets way more data than we need. Also, it seems that the shared logic between that and the path glue is coming out of Makefile right now.

I guess that the js piece for the keyboard layouts is OK, too, as much as I managed to digest the code. Helps that I made a local checkout of that branch, too. I wish there was a less brutal way for this than eval(), but it pairs correctly with loading the keyboard layouts via a <script> tag in the app.

If I read the code correctly, you'll still need to modify webapp-zip.js to have the correct entrypoints in application.zip#!/manifest.webapp. I don't even know if the plaintext one is used.

Also, it took me a while to see that webapp.buildDirectoryFile isn't the one in the profile, that's what clock and email use. Not sure if it'd be easier to use a similar approach for keyboard. Yuren?
Comment on attachment 822659 [details] [review]
revised pull request

Flagging this with a feedback+, because the approach is heading in the right direction. Needs further work, though.
Attachment #822659 - Flags: review?(l10n) → feedback+
Assignee

Comment 78

6 years ago
Comment on attachment 822659 [details] [review]
revised pull request

Yuren, Axel,

I've updated the pull request to address the review comments.

I can't change the layout files to JSON because they rely on things like KeyEvent being defined. They weren't JSON to begin with, and it is out of scope for this bug to convert them to JSON.

I modified webapps-zip.js to remove manifest.webapp from the zip file and then add the customized version.  This is different than what Yuren seemed to be suggesting, but I couldn't figure out any other way to do it.
Attachment #822659 - Flags: review?(yurenju.mozilla)
Attachment #822659 - Flags: review?(l10n)
Comment on attachment 822659 [details] [review]
revised pull request

Looks good to me, Thanks for your effort! r=yurenju
Attachment #822659 - Flags: review?(yurenju.mozilla) → review+
Comment on attachment 822659 [details] [review]
revised pull request

Nice, thanks for hanging in there, I'm really happy with the outcome here.

r=me
Attachment #822659 - Flags: review?(l10n) → review+
Assignee

Comment 82

6 years ago
This is almost certainly going to need manual uplift to v1.2.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Flags: needinfo?(xyuan)
Resolution: --- → FIXED
Summary: updated dictionaries from JB (24 dictionaries) → keyboard build-time customization
David,

I am very confused on what exactly being done in this bug. The original summary from Andreas was "updated dictionaries from JB (24 dictionaries)" and the first patch is indeed made some modification on the *_wordlist.xml and *.dict files. But the patch that actually lands didn't made any of these modifications, and instead it was a big patch on supporting build-time customization. Did I miss anything? Should we file another koi+ bug that update the dictionaries for real?
Flags: needinfo?(dflanagan)
Assignee

Comment 85

6 years ago
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #84)
> David,
> 
> I am very confused on what exactly being done in this bug. The original
> summary from Andreas was "updated dictionaries from JB (24 dictionaries)"
> and the first patch is indeed made some modification on the *_wordlist.xml
> and *.dict files. But the patch that actually lands didn't made any of these
> modifications, and instead it was a big patch on supporting build-time
> customization. Did I miss anything? Should we file another koi+ bug that
> update the dictionaries for real?

The bug morphed along the way. The intent of the bug was primarily to support more languages. And it turned out that the only way to do that was to make it configurable at build time.

No one is dissatisfied with the words in the existing wordlists: I should have updated them as part of this bug, but it is not urgent and I forgot. The blocking bug was that we needed to be able to add new word lists.

Luigi's bug 902120 seems like a good one for updating the wordlists to the most recent versions. But I don't think that needs to be a blocker.
Flags: needinfo?(dflanagan)
Hello, Is there a reason for ar language not being
> GAIA_KEYBOARD_LAYOUTS?=en,pt-BR,es,de,fr,pl
here?

I mean, It is true that bug 906270 is a blocker for not having RTL languages as a system lang.. But the layout is something else. (As I user I could be using my phone in English, but needs to send text messages, emails in ar.. Which will require an Arabic keyboard aka layout)
? :-)
Flags: needinfo?(dflanagan)
Assignee

Comment 87

6 years ago
Ahmed,

Most layouts are tied to an autocorrect dictionary, which are large.  If we put them all in the build gets too big for the hardware. Vendors creating FirefoxOS builds will have to choose which layouts they want to include, in the same way that they choose which locales they want to include in the build. The default value is almost arbitrary, and I expect that anyone producing a commercial build with customize this variable.

I have no idea whether our Arabic support is actually any good.  Unless it is production-ready, I suspect that our partners won't include an arabic layout in the phones they sell.

If you are producing your own custom build, all you have to do is set the GAIA_KEYBOARD_LAYOUTS environment variable to specify the layouts you want.
Flags: needinfo?(dflanagan)
Reporter

Comment 88

6 years ago
Hi Ahmed. Our goal is to have 1.2 localized in most languages, so we will have to generate localization itself, and we also need help generating keyboard layouts and predictive text dictionaries. Arabic might miss the mark with 1.2 because of RTL and I think also our current word prediction algorithm might need tuning to perform well with Arabic, but we should file any issues we know and ideally we want to get test devices to our Arabic community so they can start helping us find what needs fixing. Chris Hofmann is coordinating these efforts.
Understood.. Thank you Andreas and David for the responses :)

Updated

6 years ago
Blocks: 938052
No longer blocks: 900626
Just want to mention here that I got this error when flashing a keyboard app (without any GAIA_KEYBOARD_LAYOUTS environment variable) over a 1.3 build from geeskphone:

03-29 11:31:38.229   125   125 E GeckoConsole: Content JS ERROR at app://keyboard.gaiamobile.org/js/keyboard.js:479 in setKeyboardName/loadLayout/script.onerror: Cannot load keyboard layout js/layouts/en.js

I also think that GAIA_KEYBOARD_LAYOUTS is not used if we use "APP=" to flash only one app.

If you want I can file individual bugs for this.
And I forgot to say this disables all non-number layouts, otherwise it wouldn't be funny ;)
You need to log in before you can comment on or make changes to this bug.