Closed Bug 1029951 Opened 10 years ago Closed 9 years ago

Allow built-in keyboard app to download latin-IMEngine dictionary dynamically

Categories

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

Product:
Firefox OS Graveyard
Component:
Gaia::Keyboard
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(feature-b2g:3.0?, tracking-b2g:+, b2g-master fixed)

Status:
RESOLVED FIXED
Milestone:
2.2 S2 (19dec)
Project Flags:
feature-b2g 3.0?
tracking-b2g +
Tracking Flags:
Tracking Status
b2g-master --- fixed

People

(Reporter: rudyl, Assigned: timdream)

References

Details

(Whiteboard: [p=3])

Attachments

(3 files, 1 obsolete file)

2014-08-15-23-32-24.png
67.85 KB, image/png
Details
VsD_Specifcation_Built-in_Keyobard.zip
933.84 KB, application/zip
Details
[PullReq] timdream:keyboard-dyn-download2 to mozilla-b2g:master
46 bytes, text/x-github-pull-request
mnjul
: review+
Omega
: ui-review+
timdream
: ui-review+
Details | Review 
+++ This bug was initially created as a clone of Bug #936724 +++

[Background story]
We cannot preload all keyboard layouts/dictionaries due to limited storage size.
(Each dictionary may cause 1 - 2 MB.)

[Proposed solution] (spawned from bug 936724 comment 8)
--
We could make the keyboard app still expose all the layouts in the (static) manifest.
And when the user triggers one of the layouts that lacks the dictionary, we can let the keyboard app download it by itself to devicestorage or something.

--
With this approach, we should not depend on any API change, like bug 936724.
See Also: → 1029539
If we just put every keyboard in the marketplace, wouldn't that be a good distribution platform? Operators can still preload the locales and dictionaries easily that way; plus we can update keyboards whenever we want; don't have to create a new download mechanism.
(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #1)
> If we just put every keyboard in the marketplace, wouldn't that be a good
> distribution platform? Operators can still preload the locales and
> dictionaries easily that way; plus we can update keyboards whenever we want;
> don't have to create a new download mechanism.

Yeah, we have been thinking about this as well.
This problem is, if it is one single keyboard app that includes all the layout/dictionaries, wouldn't it be too large and waste some space because some layouts/dicts have been pre-loaded  in built-in keyboard?

And if we split keyboard app into several apps, like Asian keyboard app, European keyboard app, that would make maintaining "the apps" kind-of cubersome and seems not a elegant way to solve this problem.
The above 2 options both have another issue that we would have duplicate layouts, like English, in built-in app and the "Marketplace-oriented" apps.
After a offline discussion with Tim, he suggest we store the downloaded dictionaries into indexedDB, because the dictionaries should be per-app data, and if we put these in device storage, then the user might be able to change/corrupt those dictionary files.
Another question raised in the previous discussion is that, after this change, we probably could include all the keyboard layouts we have ,and we need to decide whether we have to provide a mechanism for the partner to opt-out the layouts (for whatever reason, customization, or something), and if that happens, how could we make those layouts available to the user who needs them back?

--
These might be related to product requirement, so would need PM's comment.
Bruce, could you please help take a look at comment 4?
Thanks.
Flags: needinfo?(bhuang)
Backlogging, let's discuss details for 2.2.
blocking-b2g: --- → backlog
Flags: needinfo?(bhuang)
If we need someone to work on this, I'll volunteer.
(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #1)
> If we just put every keyboard in the marketplace, wouldn't that be a good
> distribution platform? Operators can still preload the locales and
> dictionaries easily that way; plus we can update keyboards whenever we want;
> don't have to create a new download mechanism.

The layouts would be lot more discoverable if all the available layouts were listed in the keyboard settings. Also, unless Marketplace adds a new type of downloadable thing (keyboard layout as opposed to a new keyboard app), there's a difference in the level of trust required. If it's just a layout, downloading one is harmless. OTOH, if you need to go download a full keyboard app, you need to be careful to pay attention to who made the keyboard app to make sure you aren't downloading a keylogger.

So my preference would be making all available layouts show up in the Settings app. If the user chooses a layout already on the phone, the selection would be instantaneous. If the user chooses a layout not already on the phone, there'd be a slight delay as the Settings app fetches a file from Mozilla.
Unfortunately, unless we have IndexedDB access within the worker, this feature will not be trivial to implement.

Currently the dict file is xhr'd from the package app package in worker.js

https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/imes/latin/worker.js#L69-L95

(Actually, it's even better if we could use Sync IndexedDB -- bug 798875, but that feature was being put on halt)
Depends on: AsyncIDB
After sit on this for the night and read some code, I think we can try to access IndexedDB from latin.js without getting the code really complex. I am going to write a prototype without UX spec for now and see how it goes.

I limit the scope of this bug to latin-IMEngine only because each of the IMEngine has to handle dynamically downloaded dictionary themselves, and Chinese IMEs are useless without dictionaries so it doesn't really make sense to add the feature there (only if we have bug 936724 where we could hide the layout if it's useless)

At the same time, we probably need to talk to UX on the interaction of this feature. Do we want to prompt the user and ask her/him to download the dictionary for example? What's the download UI is going to be like? I have some thoughts that I am going to implement in my own prototype and we can always get it checked-in and disabled before ui-review, I think.
Assignee: nobody → timdream
No longer depends on: AsyncIDB
Summary: Allow built-in keyboard app download dictionary dynamically → Allow built-in keyboard app to download latin-IMEngine dictionary dynamically
WIP https://github.com/timdream/gaia/tree/keyboard-dyn-download

I managed to create a framework around the latin.js for this feature.

* Besides GAIA_KEYBOARD_LAYOUTS, this patch introduce yet another variable called GAIA_KEYBOARD_PRELOAD_DICT_LAYOUTS and only includes dictionary of layouts specified in this variable.
* Information is passed to keyboard settings and latin.js so it will know where to pull the dictionaries, and whether or not to offer download
* A "Download Dictionaries" UI is written on Keyboard Settings and try to pull the dictionaries from Github, for now.

GAIA_KEYBOARD_PRELOAD_DICT_LAYOUTS is currently simply points to GAIA_KEYBOARD_LAYOUTS, and since there is no non-preloaded dictionary to offer for download, the UI will simply not shown. This is equivalent to not shipping this feature until it's complete.

To play around this patch, do 

GAIA_KEYBOARD_PRELOAD_DICT_LAYOUTS=en GAIA_KEYBOARD_LAYOUTS=* APP=keyboard make install-gaia

Noted that exposed layouts in manifest will not get picked up by Gecko until buildid change.

Missing bits:

* XHR is broken and I can't figured out why. Github returns 200 on the requests but with zero bytes.
* The actual step of saving the dictionary to IndexedDB is incomplete, nor the actual step to load it from latin.js
* lastly, of course, the unit tests.

I will be working on this on and off so if anyone interested please shoot me a pull request on that branch, since this feature is not trivial it would be optimal if we could land it on 2.2 after 2.1 FL and before 2.1 FC so it can be properly tested. Otherwise we would have to disable it for 2.2 if the quality is not up to the bar.
Attached image 2014-08-15-23-32-24.png — 
The UI in keyboard settings UI now works and correctly downloads databases into indexedDB, as evidenced by screenshot.

The 0-bytes download issue went away when I reboot the phone after updating package and kill the keyboard process (I was using |APP=keyboard make install-gaia|). It looks like a serious bug if an killed app can't download anything with systemXHR once re-launched; I might write a separate reduced test case for that.

The issue now before getting a full working prototype is to load the dictionary from database in latin.js. I think there is a bug around having |displaysCandidates| give the correct sync response before the database is loaded -- we might need to change the internal IME API for that.
After talking to :omega, he agreed we should land this bug disabled until UX can write up a spec of this feature. The current prototype UI is usable but suffers discovery issue -- user is not prompt anywhere when there is a yet-to-download dictionary.

The patch is already ready for review, with only unit tests missing.
Status: NEW → ASSIGNED
Whiteboard: [p=3]
Target Milestone: --- → 2.1 S3 (29aug)
David, Rudy, could you offer some feedback before I flag for review on this?

This will be landed disabled until UX update the UI and we have the CDN ready.
Attachment #8474385 - Flags: feedback?(rlu)
Attachment #8474385 - Flags: feedback?(dflanagan)
I don't have time to look at the code today, but will try to this week. If I recall correctly when we've discussed this in the past, you were reluctant to have mozilla host and maintain an authoritative set of dictionary files. How is this going to be handled, or is it something that vendors will be responsible for?
Flags: needinfo?(timdream)
(In reply to David Flanagan [:djf] from comment #16)
> I don't have time to look at the code today, but will try to this week. If I
> recall correctly when we've discussed this in the past, you were reluctant
> to have mozilla host and maintain an authoritative set of dictionary files.
> How is this going to be handled, or is it something that vendors will be
> responsible for?

I don't remember I have ever say that. Are you referring to someone else?

My previous thought was that we should never allow vendors to remove features from a phone branded with Mozilla Firefox OS. Given the fact that is already happening and disk size is a legitimate concern, the approach of this bug seems to be the next best thing.
Flags: needinfo?(timdream)
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #17)
> (In reply to David Flanagan [:djf] from comment #16)
> > I don't have time to look at the code today, but will try to this week. If I
> > recall correctly when we've discussed this in the past, you were reluctant
> > to have mozilla host and maintain an authoritative set of dictionary files.
> > How is this going to be handled, or is it something that vendors will be
> > responsible for?
> 
> I don't remember I have ever say that. Are you referring to someone else?
> 

Maybe I was thinking of someone else. Anyway, if mozilla can maintain a repository of dictionaries for download, then this seems like a great feature. Can the dictionaries be integrated with the marketplace somehow?

Unfortunately 2.0 blockers have prevented me from actually looking at the code to offer real feedback.  I will be on PTO until September 2nd and will try to get to this when I return if you still want feedback.
(In reply to David Flanagan [:djf] [OOO 'til Sept 2] from comment #18)
> Can the dictionaries be integrated with the marketplace somehow?

How would you like the dictionaries be integrated with Marketplace? For example, pointing people to built-in keyboard settings when she/he is looking for a spellcheck-capable keyboard app on the Marketplace? The discovery issue is something UX would probably need to figure out.
Target Milestone: 2.1 S3 (29aug) → 2.1 S4 (12sep)
Comment on attachment 8474385 [details] [review]
Github: https://github.com/mozilla-b2g/gaia/pull/22975

This patch is ready for review but it should not take any higher priority than feature-b2g and blocking-b2g.
Attachment #8474385 - Flags: feedback?(rlu) → review?(rlu)
Comment on attachment 8474385 [details] [review]
Github: https://github.com/mozilla-b2g/gaia/pull/22975

Sorry for the delay, but I have started to look at this patch.

Several questions/comments:
 1. It seems I cannot test this patch,
    I tried to modify the Makefile for the layout and preload dictionaries as follows, but cannot get Catalan to start downloading by clicking it in the keyboard settings page.
    Am I missing anything? 
=====
    GAIA_KEYBOARD_LAYOUTS?=en,pt-BR,es,de,fr,fr-CA,pl,ko,zh-Hans-Pinyin,en-Dvorak,ca
    GAIA_KEYBOARD_PRELOAD_DICT_LAYOUTS?=en,pt-BR,es,de,fr,fr-CA,pl,ko,zh-Hans-Pinyin,en-Dvorak

 2. I assume we need to do some checksum to make sure we download the correct binary from the CDN, but I did not see this implemented, is this going to be a follow-up?

 3. Guess we also need some version management to make sure we could update the dictionaries without the need to update the keyboard app itself, or maybe this is out of our current scope?

 4. I am not sure if I understand the purpose of Downloadable (and DownloadableList), for example, it seems it combines both UI manipulation and the business logic of downloading.
    Would suggest to separate these 2 concerns into 2 components.

Thanks.
Attachment #8474385 - Flags: review?(rlu)
feature-b2g: --- → 2.2?
See bug 983043 for UX spec update.
Public update:

This bug have changed direction and I would need to re-work on the previous disabled feature patch. We are going to deliver what's in the UX spec mentioned in comment 22.

Doing so require Gecko API & System app work in bug 936724 etc, which is currently being works on.

Also bug 701634 is flagged as dependency because we should avoid re-work on IMEngine logic if that could complete in time.
Depends on: 936724, AsyncIDB
Clarification:

This bug will cover the major flow of this spec

https://bug983043.bugzilla.mozilla.org/attachment.cgi?id=8519643#11

But checking mobile network and enabling the input for the user (when it's downloaded) will be follow-ups.
Depends on: keyboard-ux-update
Hi Omega, please put the UX spec in this bug, thanks.
feature-b2g: 2.2? → 2.2+
Flags: needinfo?(ofeng)
Never mind, just saw the spec is in comment 24
Flags: needinfo?(ofeng)
Target Milestone: 2.1 S4 (12sep) → 2.2 S2 (19dec)
QA Whiteboard: [COM=Gaia::Keyboard]
Mass-unassign feature-b2g: 2.2+ to re-scope 2.2 features.
feature-b2g: 2.2+ → ---
tracking-b2g: --- → +
Remove AsyncIDB dependency with arch change in bug 1110028.
No longer depends on: AsyncIDB
Omega, did you remember what we talk about, one-to-many mapping between a dictionary and layouts? Could you update the spec for that?
Flags: needinfo?(ofeng)
As discussion, the list shows all "layouts". If layout 1 and layout 2 both use dictionary A, downloading layout 1 won't make layout 2 downloaded visually. Dictionary A will be removed when both layout 1 and 2 are removed.
Flags: needinfo?(ofeng)
Attachment #8474385 - Attachment is obsolete: true
Attachment #8474385 - Flags: feedback?(dflanagan)
Update:

I am now working on this bug on this branch:
https://github.com/timdream/gaia/tree/keyboard-dyn-download2

The spec have changed (see comment 30) to a point that the underlining logic need some updates (expose the concept of "layout" to user while bookkeeping the "dictionaries") so it would take some additional time.

I might, or might not split the branch into more separate patches.
Marking 2.2 features for 3.0 considerations.
feature-b2g: --- → 3.0?
Omega, could you provide visual spec of this feature? I've just realized I have never received one.
Flags: needinfo?(ofeng)
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #31)
> Update:
> 
> I am now working on this bug on this branch:
> https://github.com/timdream/gaia/tree/keyboard-dyn-download2
> 

The branch now contains working patches, with two unit tests missing. I have also manually verified it works as intended.

I wonder if we should fix bug 1016268 before enabling this feature. The STR in that bug become very easy to reproduce with this feature (s/uninstall app/uninstall layout/).
Omega, there are some other issues as well. For layouts like Korean, it never actually comes with a dictionary and we preload it out right, what's the "size" of the layout we should show? Showing the size of the definition or script (e.g. 20KB) doesn't really make any sense.
Sorry for the delay, the visual spec has been updated, please take a look the attachment.
As discussion, for the Korean case, just show the definition/script size (like 20KB).
Flags: needinfo?(ofeng)
Comment on attachment 8556303 [details] [review]
[PullReq] timdream:keyboard-dyn-download2 to mozilla-b2g:master

Let's get this feedback'd first as the patch is already quite large.

To be completed before set as review:

- Split the list from "Keyboards" to "Installed keyboards" and "More keyboards".

Incomplete from spec (follow-up bugs to be filed):

- Notification when downloading had failed.
- Dialog & enable layout (not just addInput()) when a layout is installed
- Data connection warning and "remember my choice"
Attachment #8556303 - Flags: feedback?(jlu)
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #40)
> To be completed before set as review:
> 
> - Split the list from "Keyboards" to "Installed keyboards" and "More
> keyboards".

... and comment 38, set the size of layouts w/o dictionaries as the size of the preloaded engine scripts (and data file(s) of the engine script).
Comment on attachment 8556303 [details] [review]
[PullReq] timdream:keyboard-dyn-download2 to mozilla-b2g:master

Patch is updated with everything intended to be included.
Will start filing follow-up bugs according to the spec next.

We probably need some RTL love in the UI but let's first make sure LTR is correct.
Attachment #8556303 - Flags: ui-review?(ofeng)
Attachment #8556303 - Flags: review?(jlu)
Attachment #8556303 - Flags: feedback?(jlu)
There is another question to be asked. The patch currently set the build script in a way that:

A) If a layout that doesn't require a download is set to be built-in, it will always marked as preloaded
B) If one of the layout marked as built-in comes with a dictionary, all layouts with the same dictionary is marked as preloaded.

These rules are in-place so we don't require user to "Install" a layout in the Keyboard settings page if a layout has everything it needed in the package -- but it might be desirable to centralize the layout management there by allowing user to "uninstall" (in essences, disable) these layouts in the same place.

Omega, should I remove these rules or we should keep them?
Flags: needinfo?(ofeng)
To better illustrate the above question, consider Korean layout, which does not require a dictionary (yet). If we allow the user to tap uninstall/install on the page, we essentially are enabling/disabling the layout w/o downloading anything.
Tim: I'm in the midst of reviewing the patch. But some heads-up: your Gb is failing there.
Comment on attachment 8556303 [details] [review]
[PullReq] timdream:keyboard-dyn-download2 to mozilla-b2g:master

The patch is awesome!

This all looks very good except for a few minor nits and the Gb failures. I've also tested the patch. I didn't check the tests to the very detail, though.

I believe that the complexity of the modules warrants some documentation now. Either we write some comments in the beginning of the files or we create some wiki/MDN page, to document the interplay and responsibility of: InpuMethodDatabaseLoader (not a new module but with a lot of new codes), LayoutDictionary, LayoutDictionaryDownloader, LayoutDictionaryList, LayoutItem{List}{View}. How do you think about this?
Attachment #8556303 - Flags: review?(jlu) → review+
Comment on attachment 8556303 [details] [review]
[PullReq] timdream:keyboard-dyn-download2 to mozilla-b2g:master

UX looks good. Add Helen for visual review.
Flags: needinfo?(ofeng)
Attachment #8556303 - Flags: ui-review?(ofeng)
Attachment #8556303 - Flags: ui-review?(hhuang)
Attachment #8556303 - Flags: ui-review+
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #43)
> A) If a layout that doesn't require a download is set to be built-in, it
> will always marked as preloaded
> B) If one of the layout marked as built-in comes with a dictionary, all
> layouts with the same dictionary is marked as preloaded.
> Omega, should I remove these rules or we should keep them?

They look fine. Just keep these two rules.
Comment on attachment 8556303 [details] [review]
[PullReq] timdream:keyboard-dyn-download2 to mozilla-b2g:master

Thanks for the implement, everything looks good for me, there is only one thing that the space from the right side to the icons should be 1.5 rem, it's too much now.
Attachment #8556303 - Flags: ui-review?(hhuang) → ui-review-
(In reply to Helen Huang from comment #49)
> Comment on attachment 8556303 [details] [review]
> [PullReq] timdream:keyboard-dyn-download2 to mozilla-b2g:master
> 
> Thanks for the implement, everything looks good for me, there is only one
> thing that the space from the right side to the icons should be 1.5 rem,
> it's too much now.

Thanks for review, just push a new commit to fix this!
Attachment #8556303 - Flags: ui-review- → ui-review+
blocking-b2g: backlog → ---
Fixed only in 3.0? For 2.2 not? :P
(In reply to Raul Malea from comment #52)
> Fixed only in 3.0? For 2.2 not? :P

We are not likely to uplift this feature to v2.2...
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: