1048851 - FTU should use the qps.enabled setting

Status: RESOLVED FIXED

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

Description

[Staś Małolepszy :stas]

In bug 1041565 I'm working on adding a devtools.qps.enabled setting (exact name TBD).  The setting controls whether the pseudolocales are available to the user as choices for language selection.

I have a WIP patch to add the same functionality to FTU's language selection panel.

Comment 1

[Staś Małolepszy :stas]

WIP branch: https://github.com/mozilla-b2g/gaia/pull/22516

It also includes all my work from bug 1041565 for now;  I'll rebase it when qps.enabled lands.

TBPL: https://tbpl.mozilla.org/?rev=4b804d7789a6&tree=Gaia-Try

Comment 2

[Staś Małolepszy :stas]

Green try: https://tbpl.mozilla.org/?rev=8d925f6d4451&tree=Gaia-Try

Comment 3

[Staś Małolepszy :stas]

Attachment: Patch 1

In bug 1015041 the pseudo languages were changed to be generated dynamically on runtime.  In bug 1041565 I'm currently working on introducing a developer setting to turn them on and off.

Depending on the value of the setting, the list of languages available on the device will include pseudo languages or not.

In bug 1041565 I abstracted this logic into LanguageList in shared/js/language_list.js anticipating that I might want to use it in FTU as well.

Here is a first patch.  It depends and requires the patch from bug 1041565, which should land very shortly.  The pull request: https://github.com/mozilla-b2g/gaia/pull/22516

Fernando, could look at this for me please and give me feedback?  Thanks!

Comment 4

[Fernando Campo (:fcampo)]

Comment on attachment 8469524 [details] [diff] [review]
Patch 1

Review of attachment 8469524 [details] [diff] [review]:
-----------------------------------------------------------------

Definitely a good idea :). Just a couple of nits (naming and comments only), and I'll try to test the patch soon (now that bug 1041565 landed) and give full review.

::: apps/ftu/js/language.js
@@ +27,3 @@
>      var container = document.querySelector('#languages ul');
>      container.innerHTML = '';
> +    LanguageList.get(function fillLanguageList(languages, uiLanguage) {

'uiLanguage' is a little ambiguous name, maybe better to change the name? If I'm not milead, it is the currently chosen language (from settings), hence is the one that we have to 'tick' on the ui, right?

Maybe it's easier to name them 'languageFullList' and 'currentLanguage' ?

@@ +35,5 @@
>          input.checked = (lang == uiLanguage);
>  
>          var span = document.createElement('span');
>          var p = document.createElement('p');
> +        p.textContent = LanguageList.wrapBidi(lang, languages[lang]);

Better to add a comment to explain that this is made for RTL languages. 'wrapBiDi' is not very clear.

Comment 5

[Staś Małolepszy :stas]

Thanks Fernando!  You're right about the naming and comments, I'll apply your feedback.  I'll also add a Marionette test and reattach a new patch later today for your review.

Comment 6

[Staś Małolepszy :stas]

Attachment: Patch 2

https://github.com/mozilla-b2g/gaia/pull/22516
https://tbpl.mozilla.org/?rev=eeb883879c52&tree=Gaia-Try

Here's an updated patch with your comments addressed and two Marionette tests.

(In reply to Fernando Campo (:fcampo) from comment #4)
> 'uiLanguage' is a little ambiguous name, maybe better to change the name? If
> I'm not milead, it is the currently chosen language (from settings), hence
> is the one that we have to 'tick' on the ui, right?

Yes, precisely.

> Maybe it's easier to name them 'languageFullList' and 'currentLanguage' ?

I went for allLanguages and currentLanguage, to avoid a line-break.

Comment 7

[Fernando Campo (:fcampo)]

Comment on attachment 8470008 [details] [diff] [review]
Patch 2

just a small nit, and all perfect :)

thanks for taking care of it

Comment 8

[Staś Małolepszy :stas]

Fixed the nit and landed:

Commit: https://github.com/mozilla-b2g/gaia/commit/efa42ba3d3a639b2b26505fd53629d356b9bf5f5
Master: https://github.com/mozilla-b2g/gaia/commit/2d2475b521351e200136e463358e6c8e91957702

Thanks for a quick review, Fernando!