Switch app.js to use mozL10n.ready

RESOLVED FIXED

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: zbraniecki, Assigned: zbraniecki)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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
Depends on: 993188
Blocks: 1000593
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.
Flags: needinfo?(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.
Flags: needinfo?(wilsonpage)
Posted file pull request
Cool! I documented the behavior and removed the fallback where it was possible.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Attachment #8416353 - Flags: review?(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.
Attachment #8416353 - Flags: review?(wilsonpage) → review-
(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?
Flags: needinfo?(wilsonpage)
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
(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 :)
Flags: needinfo?(wilsonpage)
(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 ?
Flags: needinfo?(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.
Flags: needinfo?(wilsonpage)
Comment on attachment 8416353 [details] [review]
pull request

updated pull-request. re-requesting review
Attachment #8416353 - Flags: review- → review?(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.
Attachment #8416353 - Flags: review?(wilsonpage) → review-
Comment on attachment 8416353 [details] [review]
pull request

aand fixed the review comments and re-requesting review
Attachment #8416353 - Flags: review- → review?(wilsonpage)
Comment on attachment 8416353 [details] [review]
pull request

- Patch breaks settings-menu
Attachment #8416353 - Flags: review?(wilsonpage) → review-
Posted image 999132.png
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
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?
Flags: needinfo?(wilsonpage)
Comment on attachment 8416353 [details] [review]
pull request

Cool, looks like we're finally there :) Just waiting on Travis now.
Attachment #8416353 - Flags: review- → review+
Flags: needinfo?(wilsonpage)
awesome. Do you want me to file a separate bug for the settings retranslation?
Flags: needinfo?(wilsonpage)
Yeah let's open another bug to track that edge case.
Flags: needinfo?(wilsonpage)
Filed bug 1013640.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.