897862 - Context::registerLocales should take the default language as the first argument

Status: RESOLVED FIXED

(L20n :: JS Library, defect, P1)

Description

[Staś Małolepszy :stas]

As per the discussion about language packs in https://groups.google.com/forum/#!topic/mozilla.tools.l10n/PeSbKFlAHRg, we want to change the semantics of Context::registerLocales.

Currently, it takes a list of negotiated languages with the last one being treated as the default.  After the change, it should take the default as the first argument, and all available locales as the second, optional, argument.

This will allow to move the language negotiation step into the language pack service in the future.

Comment 1

[Staś Małolepszy :stas]

I've been working on this today and I'm hoping to have a patch tomorrow.  Here's the summary of the API changes I'm planning:

ctx.registerLocales(defaultLocale, availableLocales)
ctx.registerLocaleNegotiator(negotiator)

where negotiator is a function which takes available locales, requested locales and the default locale and returns the final list of locales to use.

In order to support locale change, we will need one more method:

ctx.changeLocales(requestedLocales) or ctx.requestLocales(requestedLocales)

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Staś Małolepszy :stas from comment #1)
> In order to support locale change, we will need one more method:
> 
> ctx.changeLocales(requestedLocales) or ctx.requestLocales(requestedLocales)

What's the rationale behind it?

Comment 3

[Staś Małolepszy :stas]

We need a way to change the fallback chain of the context.  registerLocales' semantics have changed:  it's a static method which defines the default and the available locales.  So we need another method that we can call for example when listening to language change events.

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Staś Małolepszy :stas from comment #3)
> it's a static method which defines the default and
> the available locales.

Static method? You got me all confused here. I didn't know we use static methods on Context class at all!

> So we need another method that we can call for example when listening to language change events.

Why can't we reuse registerLocales for that?

If you want to avoid having to redefine default locale, we could allow for sth like:

ctx.registerLocales(null, localesArray); to avoid redefining default one (but still allow it if programmer wants)

Comment 5

[Staś Małolepszy :stas]

(In reply to Zbigniew Braniecki [:gandalf] from comment #4)
> (In reply to Staś Małolepszy :stas from comment #3)
> > it's a static method which defines the default and
> > the available locales.
> 
> Static method? You got me all confused here. I didn't know we use static
> methods on Context class at all!

Sorry, I totally misused "static" in that sentence.  I only meant that it's supposed to be used once and take the default locale and all available locales.  Then you freeze the context and never call registerLocales again.

> > So we need another method that we can call for example when listening to language change events.
> 
> Why can't we reuse registerLocales for that?
> 
> If you want to avoid having to redefine default locale, we could allow for
> sth like:
> 
> ctx.registerLocales(null, localesArray); to avoid redefining default one
> (but still allow it if programmer wants)

The thing is, localesArray shouldn't change (unless you update your code to a new version and add support for new languages).  I don't think overloading registerLocales is the right choice.  I't complex enough already, what with the null default (in the monolingual mode) and all.  In fact, I'm tempted to remove the monolingual mode after working with the code today, but that would mean always requiring a manifest or having to specify the default languages somehow in the HTML.

Comment 6

[Staś Małolepszy :stas]

One added benefit of changing registerLocales into: a call-once registerLocales and call-multiple-times requestLocales is that we get rid of the weird possibility of switching back and forth between the multilingual and monolingual modes.

Previously it was possible to do this:

  ctx.registerLocales('es-AR', 'es-ES', 'en-US');  // go into the multilingual mode
  ctx.registerLocales(null);                       // switch to monolingual
  ctx.registerLocales('fr-CA', 'fr', 'en-US');     // switch back to multilingual

With requestLocales, we can only do:

  ctx.registerLocales('en-US', ['es-AR', 'es-ES', 'en-US', 'fr', 'fr-CA']); 
  ctx.requestLocales(['es-AR', 'es']);    // get Spanish
  ctx.requestLocales(['fr-CA', 'fr']);    // get French
  ctx.requestLocales();                   // get the default

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Can we couple registerLocales into some other call?

I'm not an API expert, but it feels to me that having a method that we should avoid methods that are designed to be called only once.

Comment 8

[Staś Małolepszy :stas]

(In reply to Zbigniew Braniecki [:gandalf] from comment #7)
> Can we couple registerLocales into some other call?
> 
> I'm not an API expert, but it feels to me that we
> should avoid methods that are designed to be called only once.

We could consider extending L20n::getContext, or maybe exposing a ctx.locales object with "default" and "available" setters?

Comment 9

[Staś Małolepszy :stas]

That said, I'm not opposed to just leaving registerLocales as-is.  In the future we could deprecate it in favor of some other way of achieving the same thing.

Comment 10

[Staś Małolepszy :stas]

Attachment: WIP patch

Here's a work-in-progress patch, which works.  Tests are updated, but I should add a few more.

I'm not entirely happy with the logic related to __none__ and the way I create Locale objects now (createLocale, getLocale).  Maybe it's just a matter of renaming those functions.  I'll work on that tomorrow, together with more tests.

I used a documentation-driven approach for this patch, and here's the relevant excerpt of docs/api:


## ctx.registerLocales(defaultLocale: String, availableLocales: Array<String>?)

Register the default locale of the `Context` instance, as well as all other 
locales available to the `Context` instance before the language negotiation.  
Locales are referenced by their [BCP 47 language codes][].

[BCP 47 language codes]: http://tools.ietf.org/html/bcp47

```javascript
ctx.registerLocales('en-US', ['ar', 'es-AR', 'es-ES', 'en-US', 'fr', 'pl']);
```

`defaultLocale` is the original language of the `Context` instance and will be 
used as the last fallback locale if other locales are registered and used.  If 
given as a falsy value, or if `registerLocales` hasn't been called at all, the 
`Context` instance will create an unnamed locale called `__none__` to be used 
as the default.

`availableLocales` is an array of all locales available to the `Context` 
instance.  This array (with `defaultLocale` appended to it if it is not already 
present) will be used to negotiate the fallback chain for the user.  


### ctx.registerLocaleNegotiator(negotiator: Function)

Register a function which will be used to negotiate the locales supported by 
the `Context` instance.  If you don't call this function, L20n will use the 
built-in `Intl.prioritizeLocales` negotiator.

```javascript
ctx.registerLocaleNegotiator(function(available, requested, default) {
  return Intl.prioritizeLocales(available, requested, default);
});
```

`negotiator` is a function which takes three arguments:

 - `available` - all locales available to the `Context` instance,
 - `requested` - locales preferred by the user,
 - `default` - the default locale to be used as the ultimate fallback.


### ctx.requestLocales(...requestedLocales: String?)

Negotiate the preferred locales for the `Context` instance.  The final list of 
locales supported by the `Context` instance will be negotiated by the 
`negotiator` registered by `registerLocaleNegotiator`.

```javascript
ctx.requestLocales('pl');
```

```javascript
ctx.requestLocales('fr-CA', 'fr');
```

If `requestedLocales` is empty or falsy, the default locale from 
`registerLocales` will be used.

```javascript
ctx.requestLocales();
```

`requestLocales` can be called after `freeze` to change the current language 
fallback chain, for instance if requested by the user.

Comment 11

[Staś Małolepszy :stas]

Attachment: Add requestLocales, registerLocaleNegotiator

Here's a complete patch, with docs and tests updated.  I kept registerLocales even if it's to be called only once.  We could go for ctx.settings property with guarded setters, but I'm not a big fan of what the code looks like when using Object.defineProperty.  And it feels a little bit too magic to me.

It makes the entire language negotiation fully asynchronous, down the list of registered locales which could be extended by a language pack service in the future.

One thing that I wonder about is whether we should provide a facility to remember and restore user's preferences in case of explicit language changes.  I started a thread in the mailing list, we can tackle this in a separate bug:

  https://groups.google.com/forum/#!topic/mozilla.tools.l10n/cUTdZqGEA50

Comment 12

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 788147 [details] [diff] [review]
Add requestLocales, registerLocaleNegotiator

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

It looks good. I'm r+'ing it and feel free to carry it over to whatever changes you'll decide to take from my review.

::: lib/l20n/context.js
@@ +494,5 @@
> +      }
> +      _negotiator = negotiator;
> +    }
> +
> +    function requestLocales() {

Method with a name "requestX" that doesn't return anything is confusing. I'd love it to somehow indicate that it'll alter the state of the system and will result in actions.

Maybe:
 - setRequestedLocales() ?

@@ +540,5 @@
>          throw new ContextError('Context has no resources');
>        }
>  
> +      // the whole language negotiation process can be asynchronous;  for now 
> +      // we just use _registered as the list of all available locales, but in 

are we using _registered here?

@@ +547,5 @@
> +      // context;  hence the allLocales promise.
> +      var allLocales = new Promise();
> +      allLocales.then(function(available) {
> +        if (!_negotiator) {
> +          var Intl = require('./intl').Intl;

why do you use Intl here and in bindings? Can we not do language negotiation outside of bindings?

Comment 13

[Staś Małolepszy :stas]

(In reply to Zbigniew Braniecki [:gandalf] from comment #12)
> +    function requestLocales() {
> 
> Method with a name "requestX" that doesn't return anything is confusing. I'd
> love it to somehow indicate that it'll alter the state of the system and
> will result in actions.
> 
> Maybe:
>  - setRequestedLocales() ?

Yeah, good point.  I see why it would be confusing.  I went for requestLocales because you can also call it after freeze to change the language.  But you're right, it never returns anything immediately.  I'll think about it a bit more.

> 
> @@ +540,5 @@
> >          throw new ContextError('Context has no resources');
> >        }
> >  
> > +      // the whole language negotiation process can be asynchronous;  for now 
> > +      // we just use _registered as the list of all available locales, but in 
> 
> are we using _registered here?

Yes, down at the bottom of the then-chain, there's this:

  allLocales.fulfill(_registered);

> why do you use Intl here and in bindings? Can we not do language negotiation
> outside of bindings?

We still use Intl in the bindings because we need a custom negotiator to set the lang and dir attributes on document.  But, I also want to have a sane default for situations when someone doesn't use the HTML bindings and creates a custom contex and doesn't register their own negotiator.

Comment 14

[Staś Małolepszy :stas]

(In reply to Staś Małolepszy :stas from comment #13)
> (In reply to Zbigniew Braniecki [:gandalf] from comment #12)
> > +    function requestLocales() {
> > 
> > Method with a name "requestX" that doesn't return anything is confusing. I'd
> > love it to somehow indicate that it'll alter the state of the system and
> > will result in actions.
> > 
> > Maybe:
> >  - setRequestedLocales() ?
> 
> Yeah, good point.  I see why it would be confusing.  I went for
> requestLocales because you can also call it after freeze to change the
> language.  But you're right, it never returns anything immediately.  I'll
> think about it a bit more.

Following the naming scheme of http://www.ecma-international.org/ecma-402/1.0/#sec-9.2, how about these proposals (in each proposal, the first line is where the developer sets all available locales, i.e. locales for which they have resource files;  the second line is how the developer passes the users' preference which will be used in the language negotiation).

  A. ctx.registerAvailableLocales('en-US', ['ar', 'fr', 'en-US', 'pl'])
     ctx.registerRequestedLocales('fr-CA', 'fr')

  B. ctx.setAvailableLocales('en-US', ['ar', 'fr', 'en-US', 'pl'])
     ctx.setRequestedLocales('fr-CA', 'fr')

Or if we dropped "register" or "Locales" parts:

  C. ctx.registerAvailable('en-US', ['ar', 'fr', 'en-US', 'pl'])
     ctx.registerRequested('fr-CA', 'fr')

  D. ctx.availableLocales('en-US', ['ar', 'fr', 'en-US', 'pl'])
     ctx.requestedLocales('fr-CA', 'fr')

I think I personally like B.

Comment 15

[Staś Małolepszy :stas]

As a reminder, the patch currently has:

     ctx.registerLocales('en-US', ['ar', 'fr', 'en-US', 'pl'])
     ctx.requestLocales('fr-CA', 'fr')

Comment 16

[Staś Małolepszy :stas]

Landed in https://github.com/l20n/l20n.js/commit/3084e006694962a58022511288e22ca84102ca1a.  I filed bug 904618 to take care of the naming issues.