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

RESOLVED FIXED

Status

Firefox OS
Gaia::First Time Experience
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mihai, Assigned: mihai)

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(b2g18?)

Details

Attachments

(1 attachment)

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.
(Assignee)

Updated

5 years ago
tracking-b2g18: --- → ?
See Also: → bug 828237
Created attachment 747111 [details]
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.
Attachment #747111 - Flags: review?(fernando.campo)
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?
Flags: needinfo?(mihai)
(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.
Flags: needinfo?(mihai)
(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 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.
Attachment #747111 - Flags: review?(fernando.campo) → review+
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.