1040271 - [L10n][SMS] Clean up mozL10n.get uses in SMS app

Status: RESOLVED FIXED

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

Comment 1

[Julien Wajsberg [:julienw]]

We don't use a lot of mozL10n.get in the SMS app; the rare places we use it, I think we can't do otherwise (eg: notification API, window.confirm, window.alert).

But I'd be happy to be wrong :)

Comment 2

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

Let's see what Giovanni will find! :)

Comment 3

[Giovanni Charles]

Attachment: Some chnages to use node.setAttribute over mozL10n.get

Julien is correct, there was not much that could be done.

Much of the localisation is for dates - which I will not be focusing on - or alerts/confirmations.

The main thing blocking progress for this bug is notifications, they are not localised - see Bug 967925. That is something I am looking into now.

Comment 4

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

Comment on attachment 8458830 [details] [review]
Some chnages to use node.setAttribute over mozL10n.get

That looks good from l10n standpoint. let's get julien's r.

Comment 5

[Julien Wajsberg [:julienw]]

Comment on attachment 8458830 [details] [review]
Some chnages to use node.setAttribute over mozL10n.get

I'm fine with the changes for action_menu.js, but in information.js we miss one important case.

Added more comments on github.

Thanks !

Comment 6

[Julien Wajsberg [:julienw]]

Gandalf, can you comment on the RTL discussion here? I'd rather want the simpler solution but I fail to see if this messes up RTL. I think it doesn't but Guivanni thinks it does :)

You can find the discussion in the 3rd "outdated diff discussion".

Thanks !

Comment 7

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

Ahmed, can you look at https://github.com/mozilla-b2g/gaia/pull/21941#discussion_r15298765 ?

Comment 8

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

Can we move forward without help from Ahmed? Flod?

Comment 9

[Ahmed Nefzaoui [:Nefzaoui]]

Apologies for late reply
I don't see anything suspicious with the current version of the patch.
But just to note, AFAIK this:
> sim-detail = «end of list» {{detail-string}} {{sim}} SIM «start of list»
Doesn't happen at anyway because in RTL, Gecko will take care of reordering the context (and in the case of mixed RTL and LTR text this will be wrong too).
So if we originally have
> sim-detail = Sim {{sim}} {{detail-string}}
All we usually do is replace the word Sim with it's equivalent in the desired RTL language and that's enough.
This seems a little vague but that's what really happens, let's not forget that {{sim}} and {{detail-string}} will also be replaced by their correspondent text in the RTL locale so the final result is a complete RTL string, and because of that, changing the order of {{sim}} and {{detail-string}} will just wrongly reverse it.

Hope this is helpful and not making it more confusing.. :)

Comment 10

[Francesco Lodolo [:flod]]

(clearing NI, also considering I'm just starting to get a grasp of RTL)

Only request: please explain in localization comments what is going on with all those variables.

Comment 11

[Julien Wajsberg [:julienw]]

Thanks Ahmed, so this is what I thought too.

Giovanni, then I think we can use the simpler code :)

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 8458830 [details] [review]
Some chnages to use node.setAttribute over mozL10n.get

let's do the other approach then !

Thanks for your patience

Comment 13

[Julien Wajsberg [:julienw]]

Comment on attachment 8458830 [details] [review]
Some chnages to use node.setAttribute over mozL10n.get

one issue and some nits

(I actually couldn't try the SIM part on a device as I have no dual SIM device on central right now -- I'm doing one now so it should be ok tomorrow).

Thanks !

Comment 14

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

Giovanni, please, squash your commits into one, and rebase on top of the current master. It should help with the tests :)

Comment 15

[Julien Wajsberg [:julienw]]

Comment on attachment 8458830 [details] [review]
Some chnages to use node.setAttribute over mozL10n.get

Redirecting the review to Oleg, to check that the SIM information display correctly in the report panel on a dual SIM device (as I just broke my Flame...)

Comment 16

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8458830 [details] [review]
Some chnages to use node.setAttribute over mozL10n.get

(In reply to Julien Wajsberg [:julienw] from comment #15)
> Comment on attachment 8458830 [details] [review]
> Some chnages to use node.setAttribute over mozL10n.get
> 
> Redirecting the review to Oleg, to check that the SIM information display
> correctly in the report panel on a dual SIM device (as I just broke my
> Flame...)

SIM indicator in the report panel looks fine to me (basically as it was before + auto translation on language switch). So r+ for the report panel functional part.

Comment 17

[Julien Wajsberg [:julienw]]

Comment on attachment 8458830 [details] [review]
Some chnages to use node.setAttribute over mozL10n.get

Just put the r- to fix the remaining indentation issues.

Also please squash and add "r=gandalf,azasypkin,julien" in your commit log

Comment 19

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

Julien, I think we have everything ready for landing here. The try build is green except of one timed out test (I restarted the test).

Comment 20

[Julien Wajsberg [:julienw]]

Comment on attachment 8458830 [details] [review]
Some chnages to use node.setAttribute over mozL10n.get

I'm sorry but there are still indentation issues :(

Comment 21

[Julien Wajsberg [:julienw]]

master: c7f8938522a18454809fb6ea0fd3eddef10a73ea

Comment 22

[Julien Wajsberg [:julienw]]

Comment on attachment 8458830 [details] [review]
Some chnages to use node.setAttribute over mozL10n.get

r=me
thanks a lot for your patience !

Comment 23

[ankit93040]

Hi Julienw

Please uplift these to v2.0 as well.

Thanks!

Comment 24

[Francesco Lodolo [:flod]]

(In reply to ankit93040 from comment #23)
> Hi Julienw
> Please uplift these to v2.0 as well.

Please don't ;-)

New string in this patch, and I think we don't move these patches to 2.0, since it has a different l10n.js

Comment 25

[Francesco Lodolo [:flod]]

A couple more notes: 
* please add comments when adding non obvious variables (i.e. "detailString" in this case)
* is this hard-coded separator wanted?
https://github.com/giovannic/gaia/blob/ea9a3d91c564b2a0816106318477013edc721f17/apps/sms/js/information.js#L113

Comment 26

[Julien Wajsberg [:julienw]]

(In reply to Francesco Lodolo [:flod] from comment #25)
> A couple more notes: 
> * please add comments when adding non obvious variables (i.e. "detailString"
> in this case)

right, sorry :(

> * is this hard-coded separator wanted?
> https://github.com/giovannic/gaia/blob/
> ea9a3d91c564b2a0816106318477013edc721f17/apps/sms/js/information.js#L113

Yeah, it's been discussed with Pike AFAIK.