870039 - [FTU] Choosing a language should enable the associated keyboard and disable ALL other enabled keyboards

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::First Time Experience, defect)

Description

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

Based on the discussion from bug 828237, when the user selects a language from the language selection screen of the FTU app, the keyboard associated to the selected language should be enabled and ALL other enabled keyboards should be disabled (cf. bug 828237 comment 14).

The current implementation only disables the latest keyboard.

Comment 1

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

Attachment: Pull Request #9630 - Disable all other enabled keyboards when language is selected

Once the user selects a language, enable the associated keyboard and disable all other keyboards.

Comment 2

[Fernando Campo (:fcampo)]

Hi Mihai, just a comment.

Even though the patch is technically correct (I already approved the same changes for bug 828237), I have some doubts about if this changes are really necessary. 
During FTU, by default, only one keyboard is active. When we change language, we de-activate previous keyboard, and activate the new one. There's no use case for when we have more than one keyboard active, so I don't see a reason for disable all the keyboards from the list every time we choose a new language, as we do it one by one. 

Am I missing something on the process?

Comment 3

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

(In reply to Fernando Campo (:fcampo) from comment #2)
> Hi Mihai, just a comment.
> 
> Even though the patch is technically correct (I already approved the same
> changes for bug 828237), I have some doubts about if this changes are really
> necessary. 
> During FTU, by default, only one keyboard is active. When we change
> language, we de-activate previous keyboard, and activate the new one.
> There's no use case for when we have more than one keyboard active, so I
> don't see a reason for disable all the keyboards from the list every time we
> choose a new language, as we do it one by one. 
> 
> Am I missing something on the process?

Hi Fernando,

These changes were detached from bug 828237 since they were not directly connected to the requirements of that bug. As Josh pointed out in bug 828237 comment 14, the desired behavior for the FTU app is to enable the language associated keyboard and disable all others, and this is the main reason I filed this bug.

You are definitely right about the use case you highlighted. As I was also discussing with David Flanagan, in practice, the current implementation should be sufficient since only one language/keyboard ('en' I believe) is active when running the FTU.

In the end, this patch would simply be a correctness fix and would prevent future bugs when, for example, multiple keyboard layouts would be enabled by default, by 3rd parties builds.

Comment 4

[Fernando Campo (:fcampo)]

(In reply to Mihai Cirlanaru [:mcirlanaru] from comment #3)
> (In reply to Fernando Campo (:fcampo) from comment #2)
> > Hi Mihai, just a comment.
> > 
> > Even though the patch is technically correct (I already approved the same
> > changes for bug 828237), I have some doubts about if this changes are really
> > necessary. 
> > During FTU, by default, only one keyboard is active. When we change
> > language, we de-activate previous keyboard, and activate the new one.
> > There's no use case for when we have more than one keyboard active, so I
> > don't see a reason for disable all the keyboards from the list every time we
> > choose a new language, as we do it one by one. 
> > 
> > Am I missing something on the process?
> 
> Hi Fernando,
> 
> These changes were detached from bug 828237 since they were not directly
> connected to the requirements of that bug. As Josh pointed out in bug 828237
> comment 14, the desired behavior for the FTU app is to enable the language
> associated keyboard and disable all others, and this is the main reason I
> filed this bug.
> 
> You are definitely right about the use case you highlighted. As I was also
> discussing with David Flanagan, in practice, the current implementation
> should be sufficient since only one language/keyboard ('en' I believe) is
> active when running the FTU.
> 
> In the end, this patch would simply be a correctness fix and would prevent
> future bugs when, for example, multiple keyboard layouts would be enabled by
> default, by 3rd parties builds.

Oh, I didn't think about 3rd party keyboards possibility, I see your point now. So, I'm going ahead with it, being my only concern the performance issues (looping through the whole list and setting all to false in every single language change), but as we are not focusing or even measuring it for FTU at the moment, there's no immediate worries.

On a side note, this will most definitely crash with bug 868001, but again, will be a future issue (IF it becomes an issue finally).

Comment 5

[Fernando Campo (:fcampo)]

Comment on attachment 747111 [details]
Pull Request #9630 - Disable all other enabled keyboards when language is selected

Nice work Mihai, thanks for taking care of it.

Comment 6

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

Thanks for the review, Fernando! I was not aware of bug 868001, it does seem like a special case would be required for non-latin keyboards.

Landed on master:
https://github.com/mozilla-b2g/gaia/commit/69695837996f5694a0a455699ff5ef763e17fa35