867175 - [suggestions] the suggestions does not follow the keyboard config

Status: RESOLVED DUPLICATE of bug 874011

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

Description

[Julien Wajsberg [:julienw]]

STR:
* enable both english and french settings for the keyboard (Settings > Keyboard > Keyboard layouts)
* open Sms and start writing a Sms 
* you can change the keyboard layout with the small icon in the bottom left corner

Expected:
* the suggestions language follows the keyboard layout

Actual:
* the suggestions language follows the language of the phone

Using different keyboard layouts makes it easy to write sms and emails in different languages, I use this a lot on my Android phone. That's why suggestions languages should follow the layout instead of the general language.

Comment 1

[David Flanagan [:djf]]

I totally agree with this. It only works if each keyboard layout is associated with only one language, however.

Comment 2

[Julien Wajsberg [:julienw]]

When I switch the keyboards (I have "other latin layout" activated), sometimes the suggestion line isn't displayed, _but_ the suggestions still work, and the words are replaced, which is quite strange/annoying (pick the word that fits the best).

Comment 3

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Attachment: Pull Request #9774 - Follow keyboard config for suggestions

Not sure if this is the best approach to tackle the problem, so feel free to suggest other ways of solving it.

In this patch the suggestions engine uses the keyboard language to load the proper dictionary (through inputMethod.activate) whenever the keyboard is changed, and the autocorrect feature (mentioned in comment 2) is deactivated through a new method 'disableAutoCorrection' which I introduced in latin.js.

Comment 4

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Since this affects user experience for languages in target locales, requesting blocking-b2g:leo

Comment 5

[David Flanagan [:djf]]

Comment on attachment 749611 [details]
Pull Request #9774 - Follow keyboard config for suggestions

There are a number of problems with the patch. The main one, however, is that you really can't call activate() again with an old inputstate object. It will get the input method out of sync with the content of the text field and auto-correct will be wrong.

See github for my suggestions on a different approach.

Comment 6

[David Scravaglieri [:scravag]]

We need a UX input about using a particular layout and a particular word suggestion language.

Comment 7

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

(In reply to David Flanagan [:djf] from comment #5)
> Comment on attachment 749611 [details]
> Pull Request #9774 - Follow keyboard config for suggestions
> 
> There are a number of problems with the patch. The main one, however, is
> that you really can't call activate() again with an old inputstate object.
> It will get the input method out of sync with the content of the text field
> and auto-correct will be wrong.
> 
> See github for my suggestions on a different approach.

Thanks for the suggestions, David. I have replied to your comments on GitHub, detailing why I chose the current approach, and asking about your thoughts on how to go about it.

Let me know what you think, thanks!

Comment 8

[Stephany Wilkes]

Assigning to Francis since this keyboard related, tracking+ and a leo nom.

Comment 9

[Leo]

Please check the modified patch uploaded for Duplicate Bug 874011.
Tested on V1-train. Works for Master Gaia as well.

Thanks,
Leo

Comment 10

[Rob MacDonald [:robmac]]

(In reply to David Scravaglieri [:scravag] from comment #6)
> We need a UX input about using a particular layout and a particular word
> suggestion language.

Hi David...

Thanks for flagging UX on this. I've posted my feedback to the duplicate bug 874011, comment #21. Please flag [e-mail] if there's anything I've missed.

- Rob

Comment 11

[David Flanagan [:djf]]

Mihai: I've replied to your comments on github.  I think the setLanguage() approach taken by Leo in bug 874011 is a good way to go. I've just added review comments to that patch as well.  If you can create a composite patch that follows that basic setLanguage() approach but fixes the flaws that your review and mine found, then I think we'll have something good.

Comment 12

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Comment on attachment 749611 [details]
Pull Request #9774 - Follow keyboard config for suggestions

Updated the pull request to use 'setLanguage' as you suggested and also refactored the disabling of auto-correction.

Let me know what you think about the current approach, thanks!

Comment 13

[David Flanagan [:djf]]

Comment on attachment 749611 [details]
Pull Request #9774 - Follow keyboard config for suggestions

This is very close!  But r-. See my comments on github.

Also please test your patch in the case where a dictionary doesn't exist (like push a version of gaia with a dictionary temporarily removed) and make sure that the keyboard still works in that case.  It is okay if the suggestions panel appears and stays blank. But we need to be sure the keyboard itself does not fail if the dictionary fails.

Comment 14

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Comment on attachment 749611 [details]
Pull Request #9774 - Follow keyboard config for suggestions

Refactored the patch based on your suggestions (check my comments on GitHub) and also extended the inputMethod with set/getState to properly pass the input state when switching keyboards.

Comment 15

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

(In reply to David Flanagan [:djf] from comment #13)
> Comment on attachment 749611 [details]
> Pull Request #9774 - Follow keyboard config for suggestions
>
> Also please test your patch in the case where a dictionary doesn't exist
> (like push a version of gaia with a dictionary temporarily removed) and make
> sure that the keyboard still works in that case.  It is okay if the
> suggestions panel appears and stays blank. But we need to be sure the
> keyboard itself does not fail if the dictionary fails.

Tested the patch for this case too, although the keyboard works well (with the suggestions panel blank), I do see the following errors in the console:

E/GeckoConsole( 4091): [JavaScript Error: "Exception thrown (nsresult = 0x80520012)." {file: "app://keyboard.gaiamobile.org/js/imes/latin/worker.js" line: 66}]
E/GeckoConsole( 4091): [JavaScript Error: "Error: not initialized" {file: "app://keyboard.gaiamobile.org/js/imes/latin/predictions.js" line: 330}]

I think catching the exception in worker.js and preventing further initialization can clear these error messages. Should I chase this in a follow-up bug? since it doesn't break the functionality from what I noticed.

Comment 16

[David Flanagan [:djf]]

Comment on attachment 749611 [details]
Pull Request #9774 - Follow keyboard config for suggestions

r- again, sorry.

The getState() function you've written doesn't look like it works because it just sends the same object back. The key piece of the state object is the part that maintains the state of the text and the position of the cursor, and those change with time.

And getting the state back only helps if all keyboards have an input method that maintains the state and can return it. So it doesn't fix the case where we have keyboards without input methods.

So I now think you're right that all of the latin keyboards should have the latin im specified.  I didn't want to have to do that because it seems like a larger change than we should be making for v1.1.  But I don't see what the alternative is.

I think we should go ahead and fix the errors in the no dictionary case in this patch and not defer it.

Comment 17

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Hi David, thank you for looking over these changes, please check my comments below which motivate my approach:

(In reply to David Flanagan [:djf] from comment #16)
> Comment on attachment 749611 [details]
> Pull Request #9774 - Follow keyboard config for suggestions
> 
> r- again, sorry.
> 
> The getState() function you've written doesn't look like it works because it
> just sends the same object back. The key piece of the state object is the
> part that maintains the state of the text and the position of the cursor,
> and those change with time.

The state obtained through getState is only relevant when switching from a keyboard that doesn't support suggestions (i.e. doesn't use the latin im) to one that does. In this case, what the user typed before and where the cursor is are not that relevant for suggesting new words. If the phone is booted with a current keyboard that doesn't use the latin im and the user tries to switch to a keyboard that offers suggestions (e.g. 'en-US') from the globe key, the suggestion engine is not activated if the state obtained from the onfocuschange event is not passed through like this.

If the user switches between latin im keyboards, the state object is maintained and suggestions update accordingly since only the dictionary is changed.

> 
> And getting the state back only helps if all keyboards have an input method
> that maintains the state and can return it. So it doesn't fix the case where
> we have keyboards without input methods.

This is true, if new input methods will follow the minimum interface that "defaultInputMethod" has, for which I added the set/getState logic to work with latin keyboards that don't use the latin im, then it shouldn't be a problem.

> 
> So I now think you're right that all of the latin keyboards should have the
> latin im specified.  I didn't want to have to do that because it seems like
> a larger change than we should be making for v1.1.  But I don't see what the
> alternative is.

All latin keyboards that don't have the latin im specified get the default one (defaultInputMethod), which, if set/getState are defined for it (already added in my patch), allows the proper activation of suggestions when the keyboard is switched to a layout that uses the latin im (and has a dictionary). This is only possible if the state is passed, as described in the previous paragraph.

> I think we should go ahead and fix the errors in the no dictionary case in
> this patch and not defer it.

I updated the patch with the proper handling of exceptions/errors in the suggestions worker (relevant commit: https://github.com/mcirlanaru/gaia/commit/b26a237eb241f60ae39c52e6df9525759da2edf5). Apparently the case where the file at url is not found can only be caught with a try-catch block, adding `xhr.onerror` to tackle this doesn't work.

Let me know how I can improve the patch given the considerations above.

Comment 18

[Leo]

Duplicate of Bug 874011