1113969 - The WAP Push app should be overhauled to properly use the l10n API

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Wappush, defect)

Description

[Gabriele Svelto [:gsvelto]]

Localization code in the WAP Push is all over the place. We should properly move it to using l10n-ids as well as adjusting non-obvious places where we might need localization such as notifications. See here for more information:

https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/localization_code_best_practices#Writing_APIs_that_operate_on_L10nIDs

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] gabrielesvelto:bug-1113969-wappush-l10n > mozilla-b2g:master

Comment 2

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8599280 [details] [review]
[gaia] gabrielesvelto:bug-1113969-wappush-l10n > mozilla-b2g:master

I've prepared a WIP that removes all uses of mozL10n.get() from the WAP Push app but run into a couple of problems:

- The first issue I've encountered is localizing the message title / notification. I'm using this as the l10n-id for the element

message-title-with-sim = ({{sim}}) {{title}}

And

sim = SIM {{id}}

Using this as the id/args couple doesn't seem to work though, the sim field is not expanded, instead I get the literal string {{sim}}

https://github.com/mozilla-b2g/gaia/pull/29795/files#diff-a63302e177cb67eee9c8356fe4f3fcaeR38

- The second issue I'm having is when using non-localized text. One way to do it is to removeAttribute(<node>, 'data-l10n-id') on the relevant element and then clear textContent but what about using a placeholder string instead with the text as an arg? I.e. something like this:

https://github.com/mozilla-b2g/gaia/pull/29795/files#diff-eee5c3abd3dc2488b100e49272a15cbfR13

The 'title' argument will always contain non-translated text (it's a phone number in this case) but using this id saves me from having a different code-path.

Comment 3

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

Great work!

(In reply to Gabriele Svelto [:gsvelto] from comment #2)
> - The first issue I've encountered is localizing the message title /
> notification. I'm using this as the l10n-id for the element
> 
> message-title-with-sim = ({{sim}}) {{title}}
> 
> And
> 
> sim = SIM {{id}}
> 
> Using this as the id/args couple doesn't seem to work though, the sim field
> is not expanded, instead I get the literal string {{sim}}
> 
> https://github.com/mozilla-b2g/gaia/pull/29795/files#diff-
> a63302e177cb67eee9c8356fe4f3fcaeR38

As I commented in the PR - just pass "id" and "title" as args and it will work.

> - The second issue I'm having is when using non-localized text. One way to
> do it is to removeAttribute(<node>, 'data-l10n-id') on the relevant element
> and then clear textContent but what about using a placeholder string instead
> with the text as an arg? I.e. something like this:
> 
> https://github.com/mozilla-b2g/gaia/pull/29795/files#diff-
> eee5c3abd3dc2488b100e49272a15cbfR13
> 
> The 'title' argument will always contain non-translated text (it's a phone
> number in this case) but using this id saves me from having a different
> code-path.

Yup, that's exactly pattern 1 here: https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/localization_code_best_practices#Pattern_1.3A

Pattern 2 is the one with removal of data-l10n-id and resetting textContent.

Both are viable solutions, so feel free to use the one you like more :)

Comment 4

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

Comment on attachment 8599280 [details] [review]
[gaia] gabrielesvelto:bug-1113969-wappush-l10n > mozilla-b2g:master

That looks awesome! Everything is clean and good to go! :)

Comment 5

[Gabriele Svelto [:gsvelto]]

Thanks, it's working like a charm now. I'll update the unit-tests and put it up for review.

Comment 6

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8599280 [details] [review]
[gaia] gabrielesvelto:bug-1113969-wappush-l10n > mozilla-b2g:master

Here's the complete patch with all the l10n-related changes plus fixes to all the unit-tests. One of the niceties of these changes is that all l10n IDs used for dynamic elements are now part of the unit-tests.

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 8599280 [details] [review]
[gaia] gabrielesvelto:bug-1113969-wappush-l10n > mozilla-b2g:master

looks mostly good, but I left too many nits to give r+ yet :)

Comment 8

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8599280 [details] [review]
[gaia] gabrielesvelto:bug-1113969-wappush-l10n > mozilla-b2g:master

I've addressed your review comments and pushed them in a separate patch on top of the previous one for ease of review.

Comment 9

[Julien Wajsberg [:julienw]]

Comment on attachment 8599280 [details] [review]
[gaia] gabrielesvelto:bug-1113969-wappush-l10n > mozilla-b2g:master

Some more nits, sorry about this !
(and especially sorry if I wasn't clear enough in my previous review)

Comment 10

[Gabriele Svelto [:gsvelto]]

(In reply to Julien Wajsberg [:julienw] (PTO May 8th -> May 17th) from comment #9)
> Some more nits, sorry about this !
> (and especially sorry if I wasn't clear enough in my previous review)

np, the longer the review process the better the code :)

Comment 11

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8599280 [details] [review]
[gaia] gabrielesvelto:bug-1113969-wappush-l10n > mozilla-b2g:master

Here's the fix for the notification helper, pushed on top of the other patches. I've left comments on GitHub about the other bits.

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 8599280 [details] [review]
[gaia] gabrielesvelto:bug-1113969-wappush-l10n > mozilla-b2g:master

let's land this :) r=me

Comment 13

[Gabriele Svelto [:gsvelto]]

Thanks, commits squashed and ready to land.

Comment 14

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/ac3a614d8ef17e964f70142aec5e5aac90bf989a

Comment 15

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

This is great!

I just published the latest stats on mozL10n.get deprecation (bug 1020138 comment 12) and Wappush is off the hook! Thanks a lot for help :gsvelto!

Comment 16

[Gabriele Svelto [:gsvelto]]

You're welcome! callscreen, dialer and the emergency-call app are next on my chopping block.