Staś Małolepszy :stas Assignee
	Description • 10 years ago

With bug 1015041, pseudolocales are generated on the fly, with no buildtime dependencies.  This should make it possible to introduce a setting in the Developer panel in the Settings app with turns pseudolocales on and off.

The setting should be on by default in custom/dev/eng builds, and off in prod builds.

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 1 • 10 years ago

Would it require l10n.js to have Settings access?

	Staś Małolepszy :stas Assignee
	Comment 2 • 10 years ago

No, I have an idea about how to work around this.  In essence, I want all apps to list qps locales in their manifests, which means that they are available in pseudolocales.  Then, in the Settings app I'd only expose qps locales as options in the language selection panel if the setting is turned on.  So navigator.language can only be set to one of the qps locales if the setting is on.

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 3 • 10 years ago

One implication of that, if I understand correctly, is that apps like Fireext will not be able to show pseudolocales in their custom locale selection. Could we do something for that scenario?

	Staś Małolepszy :stas Assignee
	Comment 4 • 10 years ago

If third-party apps don't want to use the system-wide language settings, I'd expect them not to use the system-provided pseudolanguage solution.  They can maintain their own setting, local to the app. As long as they use l10n.js *and* qps langs are listed in available langs *and* the user can set their preferred lang to qps, things should work.

	Staś Małolepszy :stas Assignee
	Comment 5 • 10 years ago

Pull request: https://github.com/mozilla-b2g/gaia/pull/22026 (also includes the fix from bug 1036380)
TBPL: https://tbpl.mozilla.org/?tree=Gaia-Try&rev=b860ca9bb5ad

Here's a WIP patch.  I named the setting in question 'devtools.qps.enabled' (bool) and put it under the Devtools header in the Developer panel.

Arthur, can you take a look and tell me what you think?  I'm also not sure how to write tests for this.  I don't know how to manipulate the settings from Marionette tests.  As far as unit tests are concerned, it looks like the languages panel is empty in the test and I don't know yet how to test if it includes the right set of languages.  Arthur, do you have any pointers for me?

	Arthur Chen [:arthurcc][inactive after 6/30]
	Comment 6 • 10 years ago

Comment on attachment 8460138 [details] [diff] [review]
WIP: Added devtools.qps.enabled setting

EJ, could you help check this patch? Thanks.

	EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)
	Comment 7 • 10 years ago

Comment on attachment 8460138 [details] [diff] [review]
WIP: Added devtools.qps.enabled setting

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

Hi Małolepszy, 

thanks for the patch and I already did a full test on it and it did work nice.

But, I noticed there is a bug.

If you checked the Pseudo Language checkbox and changed the language to one of pseudo languages, next time, when you are going to change the language to **any option** below the checked one (the pseudo language) and changed back (the pseudo language), we would choose to the **wrong** language which means that you can't change back to pseudo language.

And about unit test, you have to make sure you did cover all changes you made in languages.js. There is already one test case there and you can check it for reference.

While for marionette test, you just have to use `client.settings.set('language.current')` before testing your code, to make sure when this mozSettings key is set, related language list would be updated at the same time. But to be honest, I am not sure whether pseudo language would exist on b2g desktop or not, you can give it a try and let's discuss here.

Overall this patch is good for me, thanks for the patch and feel free to ni? or r? again. thanks ! :P

::: apps/settings/elements/developer.html
@@ +32,5 @@
>          </li>
> +        <li>
> +          <label class="pack-checkbox">
> +            <input type="checkbox" name="devtools.qps.enabled">
> +            <span data-l10n-id="dev-tools-qps">Pseudolanguages</span>

Pseudo Languages ?

::: apps/settings/js/panels/languages/languages.js
@@ +11,5 @@
>      init: function() {
>        this.langSel.innerHTML = '';
> +
> +      function fillLanguageList(qpsEnabled, languages) {
> +        /* jshint -W040 */

we can use `self` or `that` here to avoid this inline comment for jshint.

@@ +17,3 @@
>          for (var lang in languages) {
> +          var isCurrent = (lang === navigator.mozL10n.language.code);
> +          if (lang.indexOf('qps') === 0 && !qpsEnabled && !isCurrent) {

Please put some comments here, it's hard to understand when reading that at the first time because there are too many judgement at one line.

@@ +29,5 @@
>            // Use script direction control-characters to wrap the text labels
>            // since markup (i.e. <bdo>) does not work inside <option> tags
>            // http://www.w3.org/International/tutorials/bidi-xhtml/#nomarkup
>            var lEmbedBegin =
>                (rtlList.indexOf(lang) >= 0) ? '&#x202B;' : '&#x202A;';

nit: wrong indentation.

@@ +34,5 @@
>            var lEmbedEnd = '&#x202C;';
>            // The control-characters enforce the language-specific script
>            // direction to correctly display the text label (Bug #851457)
>            option.innerHTML = lEmbedBegin + languages[lang] + lEmbedEnd;
> +          option.selected = isCurrent;

Can we wrap this part into a function so that we can get a generated option easily ? In this way, we can add unit test for this function to make sure we do wrap them correctly.

@@ +56,5 @@
>        if (this.panel.children.length) {
>          var d = new Date();
>          var f = new navigator.mozL10n.DateTimeFormat();
>          var _ = navigator.mozL10n.get;
>          this.panel.querySelector('#region-date').textContent =

nit: wrong indentation.

@@ +62,2 @@
>          this.panel.querySelector('#region-time').textContent =
>              f.localeFormat(d, _('shortTimeFormat'));

same here.

::: apps/settings/js/panels/languages/panel.js
@@ +9,5 @@
>      var localizedEventListener;
>  
>      return SettingsPanel({
>        onBeforeShow: function() {
> +        languages.init();

This is confusing because now we have languages.init() and languages.onInit(). After checking your code, right now when calling init(), update() would definitely be called inside, so I propose to rename `init()` to `update()`, and original `update()` would become `updateDateTime()`.

In this way, we can get the meaning quickly at the first glimpse. WDYT ?

::: apps/settings/locales/settings.en-US.properties
@@ +1011,5 @@
>  draw-layer-borders=Draw layer borders
>  log-animations=Log slow animations
>  remote-debugging-usb=Debugging via USB
>  dev-tools-wifi=DevTools via Wi-Fi
> +dev-tools-qps=Pseudolanguages

Pseudo Languages ?

	Staś Małolepszy :stas Assignee
	Comment 8 • 10 years ago

Great review, thank you, EJ!  I'll work on the patch tomorrow and also try to write the tests as you instructed.

(In reply to EJ Chen [:eragonj][:小龍哥] from comment #7)

> But, I noticed there is a bug.
> 
> If you checked the Pseudo Language checkbox and changed the language to one
> of pseudo languages, next time, when you are going to change the language to
> **any option** below the checked one (the pseudo language) and changed back
> (the pseudo language), we would choose to the **wrong** language which means
> that you can't change back to pseudo language.

I'm wasn't able to reproduce this locally.  After changing to Accented English, should I change to zh-TW, and then back to Accented English?

> While for marionette test, you just have to use
> `client.settings.set('language.current')` before testing your code, to make
> sure when this mozSettings key is set, related language list would be
> updated at the same time. But to be honest, I am not sure whether pseudo
> language would exist on b2g desktop or not, you can give it a try and let's
> discuss here.

I had no idea about client.settings.  This will make the test writing much easier indeed!

> Pseudo Languages ?

I'm not a native English speaker, but a quick dictionary search suggested that the pseudo- prefix is spelled without spaces or hyphens. 

> Can we wrap this part into a function so that we can get a generated option
> easily ? In this way, we can add unit test for this function to make sure we
> do wrap them correctly.

Good idea.

> This is confusing because now we have languages.init() and
> languages.onInit(). After checking your code, right now when calling init(),
> update() would definitely be called inside, so I propose to rename `init()`
> to `update()`, and original `update()` would become `updateDateTime()`.
> 
> In this way, we can get the meaning quickly at the first glimpse. WDYT ?

Makes total sense, I'll change it.

	EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)
	Comment 9 • 10 years ago

(In reply to Staś Małolepszy :stas from comment #8)
> Great review, thank you, EJ!  I'll work on the patch tomorrow and also try
> to write the tests as you instructed.

Thanks :P

> > But, I noticed there is a bug.
> > 
> > If you checked the Pseudo Language checkbox and changed the language to one
> > of pseudo languages, next time, when you are going to change the language to
> > **any option** below the checked one (the pseudo language) and changed back
> > (the pseudo language), we would choose to the **wrong** language which means
> > that you can't change back to pseudo language.
> 
> I'm wasn't able to reproduce this locally.  After changing to Accented
> English, should I change to zh-TW, and then back to Accented English?
> 

I will try to upload a screencast for you about this bug.

> > While for marionette test, you just have to use
> > `client.settings.set('language.current')` before testing your code, to make
> > sure when this mozSettings key is set, related language list would be
> > updated at the same time. But to be honest, I am not sure whether pseudo
> > language would exist on b2g desktop or not, you can give it a try and let's
> > discuss here.
> 
> I had no idea about client.settings.  This will make the test writing much
> easier indeed!
> 

If you want to know more about them, just go ahead to check the other marionette tests under settings and I think that would definitely help !

> > Pseudo Languages ?
> 
> I'm not a native English speaker, but a quick dictionary search suggested
> that the pseudo- prefix is spelled without spaces or hyphens. 

Because this string will be exposed to users, it will be more readable if we can make a space for it.

	EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)
	Comment 10 • 10 years ago

Hi Małolepszy,

just uploaded a quick demo about the bug on MEGA. Please give it a try.

Hope this helps you find the problem !

	Staś Małolepszy :stas Assignee
	Comment 11 • 10 years ago

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

Hi EJ, here is a revised patch which addresses your comments and adds tests.  A few notes:

1. I moved the whole logic to a shared library because I will also want the FTU app to use it (I'll file a bug once we have the setting).  I feel like it simplified and made the code clearer; what do you think?

2. Thanks for discovering the bug and recording a video!  I found out that it is currently impossible to manipulate the DOM of a <select> attribute and have the changes reflected live.  The DOM changes fine (I can remove pseudolanguage options dynamically), but the UI is not updated.  I think this is a platform bug.  The workaround is to rebuild the list of <option>s only when the user re-opens the Language panel.

This means that it will be possible to 1) change the current language to a pseudolocale, 2) turn the qps.enabled setting off, 3) go to the Language panel, tap the dropdown; the pseudolocale is still listed (but only the one which is the current one), 4) tap English, 5) the pseudolocale is still listed, 6) tap OK and tap the dropdown again; the pseudolocale is *still* listed, 7) go back to the root panel and 8) open the Languages panel again, 9) tap the dropdown: the pseudolocale is not listed this time.

3. Do I need a special review regarding the name of the setting (devtools.qps.enabled) and the position of the checkbox in the developer panel?  Also, I left the English name 'Pseudolanguages' in the patch, but I can change it to something else.

4. In Marionette tests I need a way to assert that an element is *not* present.  (Alternatively, I could try to count elements, although that's less reliable.) For that I used client.helper.waitForElementToDisappear, which under the hood just wait for an NoSuchElement exception to be thrown and handles it gracefully. At least on my machine, however, it takes a lot of time: around 20 seconds for each waitForElementToDisappear.  Consequently, the test case that I added takes more than 60 seconds to complete. I tried to split it, but I realized that it was important to run all the actions in this specific sequence, so it makes sense to have the in a single test, I think.

	EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)
	Comment 12 • 10 years ago

(In reply to Staś Małolepszy :stas from comment #11)
> Created attachment 8467477 [details] [diff] [review]
> Patch
> 
> https://github.com/mozilla-b2g/gaia/pull/22026
> https://tbpl.mozilla.org/?rev=acf77829b158&tree=Gaia-Try
>

Geeeeeeen !
 
> Hi EJ, here is a revised patch which addresses your comments and adds tests.
> A few notes:
> 
> 1. I moved the whole logic to a shared library because I will also want the
> FTU app to use it (I'll file a bug once we have the setting).  I feel like
> it simplified and made the code clearer; what do you think?
> 

AWESOME !!! let's just move it ! and please help to open another bug for FTU, thanks !

> 2. Thanks for discovering the bug and recording a video!  I found out that
> it is currently impossible to manipulate the DOM of a <select> attribute and
> have the changes reflected live.  The DOM changes fine (I can remove
> pseudolanguage options dynamically), but the UI is not updated.  I think
> this is a platform bug.  The workaround is to rebuild the list of <option>s
> only when the user re-opens the Language panel.
> 

I commented an idea for this on github, you can go check it and let's discuss.

> This means that it will be possible to 1) change the current language to a
> pseudolocale, 2) turn the qps.enabled setting off, 3) go to the Language
> panel, tap the dropdown; the pseudolocale is still listed (but only the one
> which is the current one), 4) tap English, 5) the pseudolocale is still
> listed, 6) tap OK and tap the dropdown again; the pseudolocale is *still*
> listed, 7) go back to the root panel and 8) open the Languages panel again,
> 9) tap the dropdown: the pseudolocale is not listed this time.
> 

check my previous comment !

> 3. Do I need a special review regarding the name of the setting
> (devtools.qps.enabled) and the position of the checkbox in the developer
> panel?

let's ask UX for more details !

> Also, I left the English name 'Pseudolanguages' in the patch, but I
> can change it to something else.
> 

ok, let's leave this to UX.

> 4. In Marionette tests I need a way to assert that an element is *not*
> present.  (Alternatively, I could try to count elements, although that's
> less reliable.) For that I used client.helper.waitForElementToDisappear,
> which under the hood just wait for an NoSuchElement exception to be thrown
> and handles it gracefully. At least on my machine, however, it takes a lot
> of time: around 20 seconds for each waitForElementToDisappear. 
> Consequently, the test case that I added takes more than 60 seconds to
> complete. I tried to split it, but I realized that it was important to run
> all the actions in this specific sequence, so it makes sense to have the in
> a single test, I think.

I left comments about this on Github  !

	EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)
	Comment 13 • 10 years ago

Comment on attachment 8467477 [details] [diff] [review]
Patch

Hi :stas,

I already checked your patch and left some comments on Github, so please go check it ! Please set r? flag again when you are ready again ! Thanks for the hard works, we are almost there !

And by the way, I am not familiar with build part and l10n.js, you may need to find the others to double check this part !

	EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)
	Comment 14 • 10 years ago

Hi UX team,

please check comment 11 and question 3 ! In this patch, we are going to add one more checkbox for pseudo languages, but we want to make sure what look & feel is ok to you guys. Can you guys give us some information about this ? 

Thanks !

	Staś Małolepszy :stas Assignee
	Comment 17 • 10 years ago

(In reply to EJ Chen [:eragonj][:小龍哥] from comment #13)

> And by the way, I am not familiar with build part and l10n.js, you may need
> to find the others to double check this part !

I showed my patch to Gandalf last week and he r+'ed it on IRC for changes in l10n.js.

	Staś Małolepszy :stas Assignee
	Comment 18 • 10 years ago

Thanks for a quick review! I addressed the comments.

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

With Julien's help I found out that you can reduce the timeout for Marionette's searching commands with client.scope({ searchTimeout: 50 });.  This speeds up my testcases from 66 seconds to 5 seconds, which I think should be okay.  I didn't find any other way to check if elements don't exist in Marionette, or to even count them.

Regarding buildList(), I think this is a platform bug.  I tried doing what you suggested and it reintroduced the bug that you had found previously. The DOM is updated correctly, but the displayed UI is not. So you tap English first to change the language once, the DOM is updated and the options list is rebuilt. But the UI is not updated, so Accented English is still on the list and tapping English selects French.

	EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)
	Comment 19 • 10 years ago

Hi :stas,

I checked Marionette part with Even and it seems this temporary solution is ok, so let's just use searchTimeout !

And for buildList problem, I came up with a workable solution and please give it a try in languages.js with following snippet. BTW, if possible, can we get pseudo-langauge lists from l10n.js or some other places ? If yes, this will make our code more clean !

//////
    onInit: function(panel) {
      var self = this;

      this.panel = panel;
      this.langSel =
        this.panel.querySelector('select[name="language.current"]');

      this.langSel.addEventListener('blur', this._rebuildListAfterBlur);
    },
    _rebuildListAfterBlur: function() {
      var pseudoLanguages = ['qps-ploc', 'qps-plocm'];
      var selectedLang = navigator.mozL10n.language.code;

      /*
       * We will only re-build list if selectedLang
       * is not qps-ploc and qps-plocm.
       */
      if (selectedLang !== 'qps-ploc' && selectedLang !== 'qps-plocm') {
        self.buildList(); 
      }
    },
//////

For me, I think they are ok enough and I'll hold the review process after UX's responses. 

Ahhh, forgot one thing, can you push your patch on github ? I prefer to use github to review codes and CI will help us find any mistakes at the same time ! Thanks  !!!

	Staś Małolepszy :stas Assignee
	Comment 20 • 10 years ago

(In reply to Staś Małolepszy :stas from comment #11)

> 1. I moved the whole logic to a shared library because I will also want the
> FTU app to use it (I'll file a bug once we have the setting).  I feel like
> it simplified and made the code clearer; what do you think?

I filed bug 1048851.

(In reply to EJ Chen [:eragonj][:小龍哥] from comment #19)
> I checked Marionette part with Even and it seems this temporary solution is
> ok, so let's just use searchTimeout !

Great, thanks!

> And for buildList problem, I came up with a workable solution and please
> give it a try in languages.js with following snippet. BTW, if possible, can
> we get pseudo-langauge lists from l10n.js or some other places ? If yes,
> this will make our code more clean !

OK, I'll give it a try, thanks for the code snippet.  And yes, the list of pseudolanguages is available as the keys of the navigator.mozL10n.qps object.  But maybe I won't even need that, since the logic in LanguageList.exted already handles the case when the current lang is one of the pseudos.
> 
> Ahhh, forgot one thing, can you push your patch on github ? I prefer to use
> github to review codes and CI will help us find any mistakes at the same
> time ! Thanks  !!!

Sure, I keep pushing to the pull request as I work: 

https://github.com/mozilla-b2g/gaia/pull/22026/commits

I also like attaching plain-text patches to Bugzilla since it helps me keep track of my progress.

	Staś Małolepszy :stas Assignee
	Comment 21 • 10 years ago

Hi EJ.  The 'blur' listener works well, thanks for the tip!  Unfortunately, I'm not able to test it in Marionette, I'm not even sure if the event fires correctly :(

I pushed the latest commit to my branch:

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

	Stephany Wilkes
	Comment 22 • 10 years ago

Flagging Jacqueline to see if she can review this, though it's a bit off our beaten path. :) Jacqueline if not, Jenny and/or Omega may be able to take it. This will help us have settings to do better L10N testing on device.

	Staś Małolepszy :stas Assignee
	Comment 23 • 10 years ago

Latest Try push: https://tbpl.mozilla.org/?rev=f24e8cc70208&tree=Gaia-Try (green).

	EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)
	Comment 24 • 10 years ago

Great ! Let's wait for ux-review and then I'll do a final review again. Thanks Stats!

	Staś Małolepszy :stas Assignee
	Comment 25 • 10 years ago

Here are my specific questions to UX:

  1. What should be the label of this setting?  'Pseudo Languages', 'Pseudolanguages', some other?
  2. Where in the Developer panel should the checkbox be placed?  Please see attachment 8467576 [details] for the current proposal.

EJ, are you happy with 'devtools.qps.enabled' as the name of the setting?  I noticed there's a number of settings which just use the 'X.enabled' pattern, like keyboard.enabled or rocketbar.enabled.  Should this setting be simply called qps.enabled?

	Staś Małolepszy :stas Assignee
	Comment 26 • 10 years ago

Comment on attachment 8467813 [details] [diff] [review]
Patch 3

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

::: apps/settings/test/marionette/tests/language_settings_test.js
@@ +110,5 @@
> +      // XXX This currently doesn't work because the blur event is not
> +      // emitted in Marionette.
> +      //
> +      // quickly.helper.waitForElementToDisappear('option[value="qps-ploc"]');
> +      // assert.ok(true, 'qps-ploc is not listed');

With EJ's help I was able to work around this with:

  var selectEl = languagePanel.findElement('languageChangeSelect');
  selectEl.scriptWith(function(selectEl) {
    var evt = new Event('blur', {
      'view': window,
      'bubbles': true,
      'cancelable': true
    });
    selectEl.dispatchEvent(evt);
  });

	Staś Małolepszy :stas Assignee
	Comment 27 • 10 years ago

I've applied all feedback and rebased the patch;  I think this is now complete except for the UX questions.

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

	Stephany Wilkes
	Comment 28 • 10 years ago

Hi Stas - Sorry for the delay here. Jacqueline was on PTO and I forgot that was the case when I flagged her. 

I think the current placement in the panel works fine. It fits better under "Developer Tools" than any of the other categories, conceptually.

I am wondering if there isn't a clearer label, though. Those without prior L10N familiarity may not realize from the "pseudolocales" label what a neat tool this is and what it enables. Do any of the following fit?
* Pseudo-localization (<- my preference)
* Pseudo-L10N 
* Language test
* Localization test

	Staś Małolepszy :stas Assignee
	Comment 29 • 10 years ago

Thanks, Stephany!

I too like the 'Pseudo-localization' label.  Here's what it looks like in the App.  It fits in English, and, a bit ironically, forces a line-break (thanks to bug 892700) in Accented English which is designed to be longer.

If you're happy with this result, I'll go ahead and change the patch.

	Stephany Wilkes
	Comment 30 • 10 years ago

Comment on attachment 8469470 [details]
Screenshot of Pseudo-localization checkbox

Looks great. Ship it! :)

	EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)
	Comment 31 • 10 years ago

Comment on attachment 8469258 [details] [diff] [review]
Patch 4

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

Thanks for the works, :stas !

Have a nice vacation by the way :P

	Staś Małolepszy :stas Assignee
	Comment 32 • 10 years ago

Commit: https://github.com/mozilla-b2g/gaia/commit/87902322cdabcde520f6128cc719208e56affbaf
Master: https://github.com/mozilla-b2g/gaia/commit/fa71786a9cd1db40f653fd4aae8f00a4192adbc7

\o/

Thanks for helping me land this, :eragonj!

	Staś Małolepszy :stas Assignee
	Comment 33 • 10 years ago

L20n.js: https://github.com/l20n/l20n.js/commit/ee646b97bf826970b16c9f5fa0b148a6c593ef26