999132 - Switch app.js to use mozL10n.ready

Status: RESOLVED FIXED

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

Description

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

Camera code currently works around bug 993188 here https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/app.js#L250-L254

This should be switched to mozL10n.ready once we fix bug 993188

Comment 1

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

The camera code has been refactored since the bug has been filed. Right now it's behavior is deterministic.

1) The app sets a listener on window.onlocalized
2) l10n.js is lazy_loaded
3) The app relies localized event to its modules

My three suggestions are:

1) Document this behavior in 

2) https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/controllers/settings.js#L289

This line should not be necessary, since the code is fired only after mozL10n fires 'localized' event, which means that the localization resources are available.

3) https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/app.js#L302-L305

I'm concerned about this code. L10n library has a fallback chain heuristics to react to missing entities. Right now it's pretty simple, but in the future we will try to get more out of it.

It also taps into user experience in case of errors.

The code here wraps mozL10n.get into a custom localization function that displays the key in case the translation is missing or the string is empty.

That also means that if the localizer will decide to return empty string for the given key, this code will show the key instead.

I'd suggest to remove that and use mozL10n.get.

Wilson, what do you think about those changes? I'll be happy to prepare pull request if you agree to them.

Comment 2

[Wilson Page [:wilsonpage]]

(In reply to Zbigniew Braniecki [:gandalf] from comment #1)
> The camera code has been refactored since the bug has been filed. Right now
> it's behavior is deterministic.
> 
> 1) The app sets a listener on window.onlocalized
> 2) l10n.js is lazy_loaded
> 3) The app relies localized event to its modules
> 
> My three suggestions are:
> 
> 1) Document this behavior in 
> 
> 2)
> https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/controllers/
> settings.js#L289
> 
> This line should not be necessary, since the code is fired only after
> mozL10n fires 'localized' event, which means that the localization resources
> are available.

The reason this line is here is because this function is run every time we get a new mozCamera object back. This can happen many times over the life-cycle of the app, not just on startup. The startup call does require this check as we don't yet know if l10n.js has been loaded, in this case it is called by the first 'localized' (ready) event.

> 
> 3)
> https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/app.js#L302-
> L305
> 
> I'm concerned about this code. L10n library has a fallback chain heuristics
> to react to missing entities. Right now it's pretty simple, but in the
> future we will try to get more out of it.
> 
> It also taps into user experience in case of errors.
> 
> The code here wraps mozL10n.get into a custom localization function that
> displays the key in case the translation is missing or the string is empty.
> 
> That also means that if the localizer will decide to return empty string for
> the given key, this code will show the key instead.
> 
> I'd suggest to remove that and use mozL10n.get.
> 
> Wilson, what do you think about those changes? I'll be happy to prepare pull
> request if you agree to them.

I'm happy to make changes to the `App.prototype.localized` method, but I'd prefer it to stay centralized if possible. This means that we can guard against the navigator.mozL10n being undefined and also quickly react to changes in l10n.js API in a single location instead of having to change code all over the app. This is also more convenient for us when unit-testing.

Comment 3

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

Attachment: pull request

Cool! I documented the behavior and removed the fallback where it was possible.

Comment 4

[Wilson Page [:wilsonpage]]

Comment on attachment 8416353 [details] [review]
pull request

- Breaks picture-size and recorder-profile title strings in settings menu (hidden by default, must be turned on in config.js)
- Some code changes appear to not change functionality.

Comment 5

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

(In reply to Wilson Page [:wilsonpage] from comment #4)
> Comment on attachment 8416353 [details] [review]
> pull request
> 
> - Breaks picture-size and recorder-profile title strings in settings menu
> (hidden by default, must be turned on in config.js)

As discussed in github review, I believe we should use localize:false for items that we don't want to localize.

Can you help me identify in which lists/items this flag should be added?

Comment 6

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

Wilson: another thing we noticed during the review is that you named the wrapper around .get "localized".

In L10n API we have another method called "localized". [1, 2] Can we rename your method to reduce confusion between L10n API and your wrappers?

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/L20n/Javascript_API#ctx.localize%28ids.3A_Array%3CString%3E.2C_callback.3A_Function%29
[2] https://github.com/mozilla-b2g/gaia/blob/master/shared/js/l10n.js#L1160-L1162

Comment 7

[Wilson Page [:wilsonpage]]

(In reply to Zibi Braniecki [:gandalf] from comment #6)
> Wilson: another thing we noticed during the review is that you named the
> wrapper around .get "localized".
> 
> In L10n API we have another method called "localized". [1, 2] Can we rename
> your method to reduce confusion between L10n API and your wrappers?

I think my function is called `.localize(<key>)`.

> As discussed in github review, I believe we should use localize:false for
> items that we don't want to localize.
> 
> Can you help me identify in which lists/items this flag should be added?

You should add the `localize: false` flag around the following areas:

pictureSizes: https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/controllers/settings.js#L296
recorderProfiles: https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/lib/format-recorder-profiles.js#L34

Then check for it before localizing in `SettingView` and `SettingOptionsView`. Let me know if you need anything else :)

Comment 8

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

(In reply to Wilson Page [:wilsonpage] from comment #7)
> (In reply to Zibi Braniecki [:gandalf] from comment #6)
> > Wilson: another thing we noticed during the review is that you named the
> > wrapper around .get "localized".
> > 
> > In L10n API we have another method called "localized". [1, 2] Can we rename
> > your method to reduce confusion between L10n API and your wrappers?
> 
> I think my function is called `.localize(<key>)`.

Spelling. The method on MozL10n is actually called "localize" and has different signature.
Would you be ok renaming it, to, for example App.prototype._ (common shortcut to l10n.get) or l10n_get ?

Comment 9

[Wilson Page [:wilsonpage]]

(In reply to Zibi Braniecki [:gandalf] from comment #8)
> Spelling. The method on MozL10n is actually called "localize" and has
> different signature.
> Would you be ok renaming it, to, for example App.prototype._ (common
> shortcut to l10n.get) or l10n_get ?

App.prototype.l10nGet() works for me.

Comment 10

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

Comment on attachment 8416353 [details] [review]
pull request

updated pull-request. re-requesting review

Comment 11

[Wilson Page [:wilsonpage]]

Comment on attachment 8416353 [details] [review]
pull request

I think the titles for pictureSize and recorderProfile will fail to get localized with this current implementation.

If you uncomment `pictureSize` and `recorderProfile` keys from within the `settingsMenu` configuration in apps/camera/js/config/config.js, you should see those items appear in the settings menu.

Comment 12

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

Comment on attachment 8416353 [details] [review]
pull request

aand fixed the review comments and re-requesting review

Comment 13

[Wilson Page [:wilsonpage]]

Comment on attachment 8416353 [details] [review]
pull request

- Patch breaks settings-menu

Comment 14

[Wilson Page [:wilsonpage]]

Attachment: 999132.png

Comment 15

[Wilson Page [:wilsonpage]]

gandalf: I checked out your patch and flashed my phone using the instructions in comment 11 and settings menu appears to be broken.

- Values for 'Camera Resolution' and 'Video Resolution' are missing
- Unable to view sub-menu after tapping a setting menu item

Comment 16

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

Ok, I identified the bug. I was trying to test for the localized flag in options, instead of config groups.

I fixed it and rebased the patch to master.

I see one bug which is that if I open the settings menu, and then change language, the settings stay in the old language. I have to close and reopen settings to get l10n updated. It's not a regression, but it would be nice to fix it.

Do you want me to try to fix it in this bug, or should I file a separate one?

Comment 17

[Wilson Page [:wilsonpage]]

Comment on attachment 8416353 [details] [review]
pull request

Cool, looks like we're finally there :) Just waiting on Travis now.

Comment 18

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

awesome. Do you want me to file a separate bug for the settings retranslation?

Comment 19

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

Patch: https://github.com/mozilla-b2g/gaia/commit/415316a4a2df61d9ee5ee15105ef56ed9900b97d
Merge: https://github.com/mozilla-b2g/gaia/commit/1a76ae328d057862e5107b7518ec1dc708376bdf

keeping open until we decide what to do with the settings pane

Comment 20

[Wilson Page [:wilsonpage]]

Yeah let's open another bug to track that edge case.

Comment 21

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

Filed bug 1013640.