Bug 828237 - [Keyboard] Switch to new keyboard layout if language is cha... #9073

Merged
merged 1 commit into from May 8, 2013

Conversation

Projects
None yet
2 participants
Member

mihai commented Apr 9, 2013

...nged

Based on the current selected language from the Settings app: add the new keyboard layout group to the user's enabled keyboards and switch to the keyboard associated with the language.

+ console.warn('Exception in mozSettings.createLock():', ex);
+ }
+ });
+
@davidflanagan

davidflanagan Apr 25, 2013

I think it would be better if this code listened for changes to the language.current setting rather than relying on the localized event to trigger the keyboard layout change. That is, I'd probably put this code in or near to the fillLanguageList() function inside the lazyLoad() function.

@mihai

mihai Apr 25, 2013

Member

Makes sense, I will update this, thanks for pointing it out!

@mihai

mihai Apr 26, 2013

Member

Tried this approach out (i.e. placing the code near and also in the fillLanguageList function) and it does not seem to work -- after a more careful look and some debugging, the changes in the Language panel are actually done by the 'localized' listener and not the code in lazyLoad(). The latter only builds the select list of languages.

Based on these arguments I think there is no harm if the keyboard layout is set when 'localized'. In the end this is triggered only when the language is changed.

@davidflanagan

davidflanagan Apr 26, 2013

I don't understand why this wouldn't work. What happens when the user selects a new language in the Language panel? I'm not sure how it triggers a new localized event, but I'd assume that it also sets the language.current setting, doesn't it? It seems to me that you ought to be able to listen for changes to that setting (in the same way that the keyboard app does) and trigger your code from there.

I agree that there is no harm in doing it the way you suggest, but it just doesn't seem the cleanest to me. This is probably a decision for @evelynhung to make, since she's much more familiar with the app.

+const keyboardAlias = {
+ 'en-US': 'en',
+ 'pt-BR': 'pt_BR'
+};
@davidflanagan

davidflanagan Apr 25, 2013

I don't understand why you need this, but I don't like that you do. The mapping in shared/resources/keyboard_layouts.json should have all the data you need, and we shouldn't have to deal with aliases again here.

@mihai

mihai Apr 25, 2013

Member

I added this so that I don't read the keyboard_layouts.json in the Keyboard app. From what I investigated there seems to be no way to get these "non-standard" layout names for setting the current keyboard -- one solution I was thinking is to add them in layouts.js since en-US/pt-BR should be synonymous with en/pt_BR.

How should I go about it?

@mihai

mihai May 1, 2013

Member

From what I understand in your comment 28 on Bugzilla, the keyboardAlias should be fine for now.

apps/keyboard/js/keyboard.js
@@ -334,6 +341,8 @@ function initKeyboard() {
// don't need to tell the keyboard about the new value right away.
// We pass the value to the input method when the keyboard is displayed.
userLanguage = e.settingValue;
+ // Switch to the language associated keyboard
+ setKeyboardName(e.settingValue);
@davidflanagan

davidflanagan Apr 25, 2013

The keyboard app shouldn't have to have anything to do with switching layouts when the language changes.

When the user changes the language, the settings app will enable a new keyboard layout. And the keyboard app will be notified of that change.

I think you should just have the keyboard app switch to whatever keyboard layout is most recently enabled.

Also, if you do it this way, there might be a race condition... The keyboard will proabably be notified of the language change before it is notified of the keyboard layout enabled change. It could enable a new keyboard before that keyboard is added to the enabledKeyboardNames[] array. I don't know if that will cause bugs, but it makes me nervous.

@mihai

mihai Apr 25, 2013

Member

This is justified by the fact that a user would be interested in his language specific keyboard and not the first keyboard in the layout group -- one example is the otherlatins group, if you switch the language to French, you want the fr keyboard enabled and activated and not the cz one (the first in otherlatins).

For the prospective race condition I can add an extra check to see if the respective keyboard is enabled, and if not enable it, before setting the keyboard name.

What do you think about this?

@davidflanagan

davidflanagan Apr 25, 2013

The FTU app has the same problem, doesn't it? If you select French as your langauge, you're just going to get the otherlatins keyboard group enabled, right? Do you have to switch from cz to fr every time you reboot the phone?

I don't know why we have the keyboard groups at all. Is it common for multi-lingual Europeans to want multiple keyboards enabled at once? That is, I wonder if keyboard groups are there for some important reason or if it was just a hack from the early days that we're still left with?

If we could get rid of keyboard groups entirely, that would make everything simpler.

But if not, then what if we add a new setting keyboard.layout.default. This would be set to a specific layout, not to a layout group. There would not be any UX associated with the setting (though we could add that in future releases). But when the FTU and the setting app change the language.current setting, they would also change the keyboard.layout.default setting and not do anything at all with the enabled keyboards. Let's let the enabled keyboard groups settings be extras, and have a new setting for the default layout. Then we'd need some code changes in the keyboard app to handle the new setting, but it would be much easier to do the right thing.

How does that sound to you?

@mihai

mihai Apr 26, 2013

Member

Good point! A reboot does "reset" your currently activated keyboard (maybe this should be part of a separate bug?).

The keyboard layout groups was confusing for me too, and it indeed complicated everything. From the user point of view I also agree that you don't need a dozen keyboards enabled for only using the fr one. If the design decision would be to remove the keyboard groups I would be up to doing that :) -- again I suppose this should be a separate bug that will block this one.

Regarding your suggestion about a new setting that points to the active keyboard, there already is a keyboard.current setting, but it doesn't seem to be that widely used (only everything.me has an observer for it: DoATAPI.js#L87). I can try to build on that and integrate this setting better in the keyboard app. What do you think about this?

@davidflanagan

davidflanagan Apr 26, 2013

I didn't know we had that setting. Please use it!

@mihai

mihai Apr 27, 2013

Member

Updated the keyboard app to use keyboard.current setting and now the active keyboard is not affected by reboots.

apps/keyboard/js/keyboard.js
@@ -303,6 +311,7 @@ function getKeyboardSettings() {
// Copy settings values to the corresponding global variables.
userLanguage = values['language.current'];
+ keyboardName = values['keyboard.current'];
@davidflanagan

davidflanagan Apr 30, 2013

don't forget to declare this variable.

@mihai

mihai Apr 30, 2013

Member

This is a global variable (declared at #L136), same as userLanguage

@davidflanagan

davidflanagan Apr 30, 2013

Oh, I see now that this is an existing variable. I think you should either have a separate userKeyboard variable to store the preference, or you should call setKeyboardName() here. Given that there is a function for setting this variable, I suspect that setting it directly here is not the right thing to do. (Or if it is, add a comment explaining why you don't call setKeyboardName())

@mihai

mihai May 1, 2013

Member

Fixed this by using setKeyboardName() a couple of lines below

+ var newKb = keyboards[navigator.mozL10n.language.code];
+ var settingNewKeyboard = {};
+ var settingNewKeyboardLayout = {};
+ settingNewKeyboard['keyboard.current'] = navigator.mozL10n.language.code;
@davidflanagan

davidflanagan Apr 30, 2013

I think it would be better if the keyboard_layouts.json file included both the keyboard group name and the specific layout name. Using the language code directly here doesn't seem like the right thing because it it requires the ad-hoc keyboardAliases data in keyboard.js. I'd prefer to have that data elsewhere.

@mihai

mihai Apr 30, 2013

Member

The keyboardAliases that I added tries to fix the unfortunate naming for two keyboards in layouts.js (en and pt_BR instead of en-US and pt-BR) -- in all other places keyboard.current is set as the language code

apps/keyboard/js/keyboard.js
+ // Switch to the language associated keyboard
+ // everything.me also uses this setting to improve searches
+ setKeyboardName(e.settingValue);
+ handleNewKeyboards();
@davidflanagan

davidflanagan Apr 30, 2013

You call setKeyboardName() here, but above you just set keyboardName directly...

You call handleNewKeyboards(), but handleNewKeyboards() does not use the keyboardName variable.

I think you need to modify handleNewKeyboards() to add the value of this setting to the list of enabled keyboards, even when the keyboard group is not enabled.

@davidflanagan

davidflanagan Apr 30, 2013

When the user switches keyboard layouts (from the keyboard itself, not from the settings app) I think we should probably set the keyboard.current setting. That is code you'll need to add elsewhere. You'll need to be careful that we don't end up calling setKeyboardName and handleNewKeyboards() twice when that happens, though.

@davidflanagan

davidflanagan Apr 30, 2013

No, my previous comment isn't right. keyboard.current can not be both the default keyboard for the current language and the user's current keyboard preference. e.me probably wants to use it for the currently displayed keyboard, and that's what I'm proposing that you do above.

But we also need a way for the keyboard app to know what the default keyboard for the current language is, because I think that keyboard should always be available even if it is not in an currently enabled keyboard layout group. Maybe that means that keyboard.js has to read shared/resources/keyboard_layouts.json. (I think that's what the original version of your patch did... )
I don't like having to do XHR on startup, however. I wonder why this data is read with XHR instead of just being an ordinary file of JS code. If we could include the layout data just like we do keyboard/layout.js I'd be perfectly fine with that solution.

@mihai

mihai Apr 30, 2013

Member

handleNewKeyboards() makes sure that new enabled keyboards (i.e. keyboard.layout._) are marked in the global enabledKeyboardNames variable and thus I make sure the keyboard.current's keyboard group is enabled.

When the user switches keyboard layouts from the keyboard, the keyboard.current is set at #L729, I will check to make sure the observer doesn't run setKeyboardName() and handleNewKeyboards() twice.

apps/keyboard/js/keyboard.js
@@ -434,7 +450,9 @@ function handleKeyboardSound() {
}
function setKeyboardName(name) {
- var keyboard = Keyboards[name];
+ var keyboard =
+ (name in Keyboards) ? Keyboards[name] : Keyboards[keyboardAlias[name]];
@davidflanagan

davidflanagan Apr 30, 2013

You can simplify this code:

var keyboard = Keyboards[name] || Keyboards[keyboardAlias[name]];

@mihai

mihai Apr 30, 2013

Member

Good point :) thanks!

// This will also set the inputMethod
- if (!keyboardName)
+ if (enabledKeyboardNames.indexOf(keyboardName) == -1)
@davidflanagan

davidflanagan Apr 30, 2013

I don't think this line should be necessary. It is probably a bug if keyboardName is not a member of enabledKeyboardNames. But even if it isn't the user should always be able to use the keyboard.current keyboard even if it is not a member of an enabled keyboard group.

@mihai

mihai Apr 30, 2013

Member

This was indeed justified by the fact that keyboardName was not a member of the enabledKeyboardNames and an invalid keyboard was enabled.

+ var settingNewKeyboard = {};
+ var settingNewKeyboardLayout = {};
+ settingNewKeyboard['keyboard.current'] = navigator.mozL10n.language.code;
+ settingNewKeyboardLayout['keyboard.layouts.' + newKb] = true;
@davidflanagan

davidflanagan Apr 30, 2013

On second thought, what I'd like to see is changing keyboard_layouts.js so that it has nothing to do with keyboard groups. When the langugage changes, we change keyboard.current, but we never modify the keyboard.layouts.* properties. Let the user do that in the settings app.

Will that work if we just ignore keyboard groups here completely?

@mihai

mihai Apr 30, 2013

Member

This would work, but not enabling the associated keyboard group will not add the keyboard in the quick keyboard switch button (i.e. small globe on the keyboard). If other keyboards are enabled and the user clicks the quick switch button the keyboard.current will be lost.

apps/keyboard/js/keyboard.js
+ // everything.me also uses this setting to improve searches
+ if (keyboardName !== e.settingValue) {
+ setKeyboardName(e.settingValue);
+ handleNewKeyboards();
@davidflanagan

davidflanagan May 8, 2013

I think I asked you to add this line, because I was assuming that you would modify handleNewKeyboards to ensure that the keyboard.current setting was always in the list of enabled keyboards, even if the keyboard group that contains it was not enabled.
(that would be a way to work around the other latins thing).

But since handleNewKeyboards() never uses the keyboard.current value, you should remove this call to it, I think. Or maybe use the language.current setting to set defaultKeyboardNames[0] and then call handleNewKeyboards?

@mihai

mihai May 8, 2013

Member

I added this call to handleNewKeyboards() based on your suggestion in this outdated diff, where you were pointing out that there might be a race condition: the keyboard app could enable a new keyboard before that keyboard is added to the enabledKeyboardNames[] array, which is what handleNewKeyboards() takes care of.

The Settings app makes sure to set both keyboard.current and keyboard.layouts._, thus avoiding an unnecessary read of keyboard_layouts.json for the Keyboard app (which would have been required if only keyboard.current was set to activate the associated layout).

apps/communications/ftu/js/language.js
+ lock.set(settingOldKB);
+ }
+ }
+
@davidflanagan

davidflanagan May 8, 2013

It appears that FTU always lists English as the default language. So if I've got my language set to French, and then I do Settings->Device Information->More Information->Run FTU. I see English as the default language. So I just click Next to switch to engish. That works for switching the language, but it looks like no change event ever happens and your new code here never gets called. When I'm done with the FTU app, I still have all the otherlatins keyboards enabled.

I suggest that we just drop all the changes to the FTU app and only change the keyboard and settings app in this patch. This is because:

  1. I don't know anything about FTU and I'm very nervous about changing it list late.

  2. Users will almost never run it more than once, so there will never be multiple keyboard groups selected when it is run (that is, your new code here will rarely ever make a difference)

  3. It doesn't work in the "switch back to the default language" case anyway.

Bug 828237 - [Keyboard] Switch to new keyboard layout if language is …
…changed. r=djf

Based on the current selected language from the Settings app: add the new keyboard layout group
to the user's enabled keyboards and switch to the keyboard associated with the language
(i.e. `keyboard.current` setting).

mihai added a commit that referenced this pull request May 8, 2013

Merge pull request #9073 from mcirlanaru/bug_828237
Bug 828237 - [Keyboard] Switch to new keyboard layout if language is cha... r=@davidflanagan

@mihai mihai merged commit 74e3173 into mozilla-b2g:master May 8, 2013

1 check was pending

default The Travis CI build is in progress
Details

mihai added a commit that referenced this pull request May 8, 2013

Merge pull request #9073 from mcirlanaru/bug_828237
Bug 828237 - [Keyboard] Switch to new keyboard layout if language is cha... r=@davidflanagan(cherry picked from commit 74e3173)

davidflanagan pushed a commit to davidflanagan/gaia that referenced this pull request May 10, 2013

davidflanagan pushed a commit that referenced this pull request May 10, 2013

David Flanagan
Merge pull request #9667 from davidflanagan/backout-keyboard-patch-v1…
…-train

Revert "Merge pull request #9073 from mcirlanaru/bug_828237"

davidflanagan pushed a commit to davidflanagan/gaia that referenced this pull request May 15, 2013

Merge pull request #9073 from mcirlanaru/bug_828237
Bug 828237 - [Keyboard] Switch to new keyboard layout if language is cha... r=@davidflanagan(cherry picked from commit 74e3173)

change initialization and settings update logic r=@RudyLu

jrburke added a commit to jrburke/gaia that referenced this pull request May 16, 2013

Merge pull request #9073 from mcirlanaru/bug_828237
Bug 828237 - [Keyboard] Switch to new keyboard layout if language is cha... r=@davidflanagan(cherry picked from commit 74e3173)

change initialization and settings update logic r=@RudyLu

@mihai mihai deleted the mihai:bug_828237 branch May 17, 2013

sergi added a commit to comoyo/gaia that referenced this pull request Jul 2, 2013

Merge pull request #9073 from mcirlanaru/bug_828237
Bug 828237 - [Keyboard] Switch to new keyboard layout if language is cha... r=@davidflanagan(cherry picked from commit 74e3173)

change initialization and settings update logic r=@RudyLu
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment