Closed Bug 888253 Opened 11 years ago Closed 10 years ago

Latin keyboard split might break locale settings after FOTA

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

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

RESOLVED FIXED
blocking-b2g koi+
Tracking Status
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed

People

(Reporter: gerard-majax, Assigned: rudyl)

References

Details

Attachments

(1 file)

After bug 867883 landed, gaia master update made my french keyboard not visible anymore, being reverted to the default english one.

Going to the settings is very confusing in this case, since bug 821646 makes it not clear that "first keyboard is the default one but it might be deduced from your locale instead of the real setting value".

According to :mihai, in my case, since I was using the old setting previously, then the system did not enabled the "French" keyboard, but made me think so because my phone locale was french.

I fear this might cause issue for FOTA.
For backward compatibility with previously enabled keyboard layout groups (i.e. 'otherlatins', 'cyrillic') in the case of a FOTA update, we most probably need to keep their mapping logic in the Keyboard, without needing the option to activate/deactivate the whole group in Settings > Keyboard.
Assignee: nobody → mihai
(In reply to Mihai Cirlanaru [:mihai][:mcirlanaru] from comment #1)
> For backward compatibility with previously enabled keyboard layout groups
> (i.e. 'otherlatins', 'cyrillic') in the case of a FOTA update, we most
> probably need to keep their mapping logic in the Keyboard, without needing
> the option to activate/deactivate the whole group in Settings > Keyboard.

Isn't it possible and/or best to provide a migration of settings?
Fabien, do you know of any mechanism that would perform a migration of settings during a OTA/FOTA update? I couldn't finding anything meaningful at the gaia level.

In this particular case, if the user has the setting 'keyboard.layouts.otherlatins' set before the update, we want to activate a couple other settings (e.g. 'keyboard.layouts.french', 'keyboard.layouts.german', etc.) and remove the 'otherlatins' one with the update.
Flags: needinfo?(kaze)
Mihai, I don’t think we have any dedicated system to do this in Gaia (I know there are some for Firefox). Implementing this would be neat, I guess it would belong to the System app though — Tim, would you have a pattern in mind to implement this?

In the short term, I guess we could add an app/system/js/settings_migration.js file to check these obsolete settings one by one when the system starts up… again, that sounds like a question for Tim.
Flags: needinfo?(kaze) → needinfo?(timdream)
(In reply to Fabien Cazenave [:kaze] from comment #4)
> In the short term, I guess we could add an
> app/system/js/settings_migration.js file to check these obsolete settings
> one by one when the system starts up… again, that sounds like a question for
> Tim.

If there is a need this sounds like the best solution out there. I might be wrong though so we should probably bring this to dev-gaia to gather opinions from wider audiences.

Thanks for the needinfo.
Flags: needinfo?(timdream)
Unassigning myself for now, as I will probably not work on this any time soon.
Assignee: mihai → nobody
With the new keyboard manager mechanism, Bug 816912, it should be able to at least activate the default keyboard layout, so this might be not a blocker.
However, the settings for the enabled keyboard layouts won't be kept after update, and this sounds like a general issue of mozSettings migration that we have to address.

I am going to initiate a discussion on #dev-gaia if no one beats me to it.
Not a blocker.
blocking-b2g: koi? → -
Moving flags over from the dupe. We concluded this was a blocker for data loss risk involved here.
Assignee: nobody → rlu
blocking-b2g: - → koi+
Hi all,

This is an attempt to address the data migration issue for keyboard layouts, from v1.1 -> v1.2+.

Corey,

I think you're the best person to review the changes I made to keyboard_helper to handle this case.
Please note that we store the old settings as:
 keyboard.layouts.english: true,
 keyboard.layouts.french: false,
 ...

And in this case, we're going to read these settings out and convert these to the new format.

If we're going to land this soon, since tomorrow is the code freeze date for v1.2, I'll ask Gary to take a look at this part.

--
Yuren,

I also removed these old setting entries and the logic to set it according to the locale, since it is deprecated as well.
Please help review this.
Attachment #8344371 - Flags: review?(yurenju.mozilla)
Attachment #8344371 - Flags: review?(gnarf37)
Attachment #8344371 - Flags: review?(gchen)
Comment on attachment 8344371 [details] [review]
Patch V1 - pull request 14488

Rudy, nice work!
r=me, looks good,

thanks.
Attachment #8344371 - Flags: review?(gchen) → review+
Comment on attachment 8344371 [details] [review]
Patch V1 - pull request 14488

Couple of comments:

* The settings process that reads the "legacy" or "deprecated" values should not be a part of the standard kh_getSettings (this is called everytime the enabled keyboards changes)

* The process should delete all of the "migrated" values from the settings db whenever it completes to stop it from constantly re-enabling the keyboards listed in the old settings

* I would suggest making the "deprecated check" kick off right after the first settings needed for the app have loaded by queuing up a kh_withSettings(kh_checkDeprecated) in the init method.  This will ensure that the enabled keyboards has been loaded by the time you request your deprecated settings
Attachment #8344371 - Flags: review?(gnarf37) → review-
Also, can I just add that it kind of offends me that this would be considered koi+ but the code to make the keyboard helper use appManifestURL instead of appOrigin wasn't.  This keyboard_helper rewrite for 1.2 should have used appManifestURL from the get-go, but rudy assured me we would fix it in another ticket (which suddenly lost it's koi+ priority)

It also means that you should probably make a second version of this patch which can actually apply to 1.2, and then another version to convert from appOrigin to appManifestURL on loading of settings in 1.3 as well.

Or force them to take the patch for bug 930358 into 1.2.  I'm adding it as a blocker to this one.
Depends on: 930358
Comment on attachment 8344371 [details] [review]
Patch V1 - pull request 14488

Discussed with Rudy and sounds we don't need this part of code in settings.js anymore, but I would like to know the risk for l10n team if we remove this code, set feedback flag to Pike.
Attachment #8344371 - Flags: review?(yurenju.mozilla)
Attachment #8344371 - Flags: review+
Attachment #8344371 - Flags: feedback?(l10n)
Comment on attachment 8344371 [details] [review]
Patch V1 - pull request 14488

I'm afraid I don't know what this code does. I'm on a work week this week so I won't have much time to try to understand this code in depth.

Is there anything in particular that you think might break l10n?
Attachment #8344371 - Flags: feedback?(l10n)
Comment on attachment 8344371 [details] [review]
Patch V1 - pull request 14488

Hi Corey,

I just updated the patch as our discussion.
Could you help take a look if this is ideal per your understanding?

Thanks a lot.
Attachment #8344371 - Flags: review- → review?(gnarf37)
(In reply to Axel Hecht [:Pike] from comment #16)
> Comment on attachment 8344371 [details] [review]
> Patch V1 - pull request 14488
> 
> I'm afraid I don't know what this code does. I'm on a work week this week so
> I won't have much time to try to understand this code in depth.
> 
> Is there anything in particular that you think might break l10n?

That code segment is used to set the default keyboard layout according to the default locale at build time. We had this feature before, but it has been not working since we modify the format we store keyboard layout settings in mozSettings.

It should not break anything if we would go through FTU, since in that language setting page, we would do the same keyboard layout setup as well.
Comment on attachment 8344371 [details] [review]
Patch V1 - pull request 14488

looks so much cleaner! thanks for attacking this again!
Attachment #8344371 - Flags: review?(gnarf37) → review+
Merged to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/51ed5c6b6bda46284e797648b4b44dc9f804b14b

Guys, thanks for the review!
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Set ni to Fabrice to ask for his approval to land this into v1.3.

--
Fabrice,

Please be informed we need this in v1.3 as well for data migration of keyboard layouts, when we upgrade from v1.1 to v1.2+.
Flags: needinfo?(fabrice)
(In reply to Rudy Lu [:rudyl] from comment #21)
> Set ni to Fabrice to ask for his approval to land this into v1.3.
> 
> --
> Fabrice,
> 
> Please be informed we need this in v1.3 as well for data migration of
> keyboard layouts, when we upgrade from v1.1 to v1.2+.

This has a koi+ - it'll automatically land on 1.2 & 1.3.
I like when the decision is easy to take!
Thanks for the clarification.
Flags: needinfo?(fabrice)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: