Closed Bug 994519 Opened 10 years ago Closed 10 years ago

Clean up mozL10n.localize API

Categories

(Firefox OS Graveyard :: Gaia::L10n, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(2 files, 1 obsolete file)

Currently mozL10n.localize is used to localize DOMFragments, but its API is not logical. In particular, it can be also used to prevent DOMFragment localization (if l10n-id param is not given).

We should aim at cleaning it, especially after landing bug 992473.
Blocks: 994357
When bug 992473 lands, we can either remove mozL10n.localize completely, or rename it (to localizeNode?) in order to keep l10n.js compatible with browsers which don't support Mutation Observers.  What are your thoughts on this?
Blocks: 999779
Blocks: 1006359
Depends on: 992473
Priority: -- → P3
Assignee: nobody → stas
After giving it more thought and talking to Gandalf, we realized that we won't be able to remove mozL10n.localize completely.  We need a way to persist l10n args somehow before the mutation observer kicks in and localizes it (see bug 992473 comment 26 and bug 994357 comment 7).

Bug 992473 will land with this part removed:

https://github.com/mozilla-b2g/gaia/blob/83f622db256dad92e8c97cccf9c71611d0e5a737/shared/js/l10n.js#L1478-L1480

In this bug we can work on further cleaning up this API.  Specifically, we should probably remove the no-id path here:

https://github.com/mozilla-b2g/gaia/blob/83f622db256dad92e8c97cccf9c71611d0e5a737/shared/js/l10n.js#L1464-L1469

And rename the whole method to something that better describes its functionality.  My current ideas revolve around mozL10n.declare(node, id, args), mozL10n.declareNode(node, id, args) and mozL10n.registerNode(node, id, args).

Gandalf, any preferences?
Flags: needinfo?(gandalf)
(In reply to Staś Małolepszy :stas from bug 992473 comment #28)
> (In reply to Staś Małolepszy :stas from bug 992473 comment #26)
> 
> > With mutation observers we can at least get rid of the |if
> > (this.ctx.isReady)| part of the implementation, since the observer will take
> > care of that.
> 
> Actually, there is a use-case which I thought of that made me reconsider
> this statement.
> 
> See
> https://github.com/mozilla-b2g/gaia/blob/
> df05327a7fdb517ead5923f6b81e8284e440bc7d/apps/settings/js/battery.js
> 
> The element is already in the DOM and it already has data-l10n-id (so there
> is no mutation per se), and yet we need a method to insert new translation
> into it.

Maybe we should just react to changes to l10n-args the same way as we react to changes to l10n-id?
Flags: needinfo?(gandalf)
(In reply to Staś Małolepszy :stas from comment #2)
> And rename the whole method to something that better describes its
> functionality.  My current ideas revolve around mozL10n.declare(node, id,
> args), mozL10n.declareNode(node, id, args) and mozL10n.registerNode(node,
> id, args).
> 
> Gandalf, any preferences?

I like mozL10n.setNodeLocalization(node, l10nId, l10nArgs);

Will that work for you?
(In reply to Zibi Braniecki [:gandalf] from comment #3)

> Maybe we should just react to changes to l10n-args the same way as we react
> to changes to l10n-id?

Yes, that sounds like a good idea.  In the battery.js example above, you could then just do:

  batteryDesc.setAttribute('data-l10n-args', JSON.stringify({ level: Battery.level }));

And the Mutation Observer would take care of the rest, correct?

Alternatively, you could use a wrapper, as discussed below.

(In reply to Zibi Braniecki [:gandalf] from comment #4)
> (In reply to Staś Małolepszy :stas from comment #2)
> > And rename the whole method to something that better describes its
> > functionality.  My current ideas revolve around mozL10n.declare(node, id,
> > args), mozL10n.declareNode(node, id, args) and mozL10n.registerNode(node,
> > id, args).
> 
> I like mozL10n.setNodeLocalization(node, l10nId, l10nArgs);
> 
> Will that work for you?

I don't think "set" is the right action verb.  Technically, the method doesn't insert any kind of localization just yet.  The Observer will do it, asynchronously.

I think I prefer something that describes the fact that we're using the declarative API here.  In fact, all the wrapper should do, I believe, is:

  function(node, id, args) {
    node.setAttribute('data-l10n-id', id);
    node.setAttribute('data-l10n-args', JSON.stringify(args));
  }

One idea would be mozL10n.setAttributes or mozL10n.setL10nAttributes, but that ties us to the fact that the data happens to be stored as attributes.  In the future, this method could also do a little bit more work.

The idea I like best is mozL10n.declare:

  mozL10n.declare(batteryDesc, 'batteryLevel-percent-' + Battery.state, { level: Battery.level });

It's similar to 'register', but we use register already in different meanings (registerLocales).
I'd only like to be more descriptive in the way we name the function as to what it deal with.

declare is a universal term, but the function only declares localizability arguments for nodes.

Maybe mozL10n.setNodeAttributes(node, id, args) ? :)
I'd steer clear of using 'set' and 'attributes' in the name.  They suggest a specific implementation, which may not be accurate in the future.

How about:

  mozL10n.declareLocalizable(node, id, args)

I like 'declare' as a verb because it corresponds to the declarative nature of data-l10n-id and data-l10n-args.
(In reply to Staś Małolepszy :stas from comment #7)
> I'd steer clear of using 'set' and 'attributes' in the name.  They suggest a
> specific implementation, which may not be accurate in the future.
> 
> How about:
> 
>   mozL10n.declareLocalizable(node, id, args)
> 
> I like 'declare' as a verb because it corresponds to the declarative nature
> of data-l10n-id and data-l10n-args.

I like it.

Can we be more verbose - declareNodeLocalizable(node, id, args) ?

zb.
Blocks: 1020137
Blocks: 1020138
(In reply to Zibi Braniecki [:gandalf] from comment #8)

> Can we be more verbose - declareNodeLocalizable(node, id, args) ?

With declareLocalizable(obj, id, args), we're saying that Localizable is an interface and 'obj' will implement it.  It can be a node, or anything else, as long as it uses the id and the args to achieve localization.
Priority: P3 → P2
I just want to know why we can't just keep "localize" ? :)
Yes, the semantic of the function will be different than what it is now, but IMO it's simple and meaningful enough.

We could have "unlocalize" for the part that remove the data attributes, if you want to keep the functionality but have it in another function.
That's a great question :)

Our goal is to be on a patch to convergence of l10n.js and l20n.js.  The ultimate goal is to have a fully featured implementation of L20n in Gaia--but we're starting from small improvements and cleanups.

The localize() method works differently in L20n:

https://github.com/l20n/l20n.js/blob/986794bcd653b5b3c18a1a629852ec4c15612f22/docs/api.md#ctxlocalizeids-arraystring-callback-function

It's a combination of ready() and asynchronous get().
I'd suggest to split l20n's localize in 2 parts: have a "whenReady" or "ready" that takes the needed ids and return a promise; the promise's callback would get the values in an array (like Promise.all does with promises... this could even be implemented using Promise.all internally).

A callback-based API is so 2010. ;)
Julien: what you and stas are talking about is a different API. (which will need its own bug). This is about the current mozL10n.localize.

We came up with an updated API proposal for that:

 - introduce mozL10n.setAttributes(node, l10nId, l10nArgs);
 - introduce mozL10n.getAttributes(node); // -> [l10nId, l10nArgs]

This will mimic how node.setAttribute / node.getAttribute work. We want to stick as close to how DOM works as possible and centralize UI localization around DOM API.

setAttributes is a helper that will do node.setAttribute on data-l10n-id and data-l10n-args for you. You can run it yourself, and the only reason why we want to have it is that we find the fact that you have to JSON.stringify/JSON.parse the l10nArgs a bit of a hack that we'd like to cover.

In this bug I will introduce those two methods. In bug 1020137 we will migrate away from mozL10n.localize and remove it.
Assignee: stas → gandalf
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
This patch adds the API. In bug 1020137 we'll migrate the current uses of .localize including tests.
Attachment #8447326 - Flags: review?(stas)
Attached patch patch v2Splinter Review
updated patch. We already had getL10nAttributes that did the same except of returning empty object if node is empty.

There's no reason to keep it since there's no possible use of getL10nAttributes that can pass null.
Attachment #8447326 - Attachment is obsolete: true
Attachment #8447326 - Flags: review?(stas)
Attachment #8447337 - Flags: review?(stas)
Attached file pull request
Comment on attachment 8447337 [details] [diff] [review]
patch v2

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

r=me, thanks G.

::: shared/js/l10n.js
@@ +1489,4 @@
>      }
>    }
>  
> +  function setL10nAttributes(node, id, args) {

Let's use the word element here:

@@ +1496,5 @@
> +      node.setAttribute('data-l10n-args', JSON.stringify(args));
> +    }
> +  }
> +
> +  function getL10nAttributes(node) {

…and here.
Attachment #8447337 - Flags: review?(stas) → review+
FTR I like the simpler and closer-to-the-DOM approach here.
Depends on: 1041252
Depends on: 1043615
No longer depends on: 1043615
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: