976203 - [Camera] Localization (l10n) missing from settings UI

Status: VERIFIED FIXED

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

Comment 1

[Wilson Page [:wilsonpage]]

Landed in dmarcos/camera-dev

Comment 2

[Wilson Page [:wilsonpage]]

https://github.com/dmarcos/gaia/commit/a9cc39ecd7150daed92b0ec5ea2ff225b1756008

Comment 3

[David Flanagan [:djf]]

I know you haven't submitted a PR for this yet, but I'm preemptively giving r- to the commit shown above.

You should do the localization when the settings menu is rendered, not when the app starts up.

Do you create and destroy the menu each time it is shown? If so, then you'll automatically get the latest localization each time, so that if the user starts the camera, then switches languages, then displays the settings menu, it will do the right thing.

If you only create it once, then you should set the data-l10n-id attribute on each element you create so that their localizations will be performed (and updated) automatically.

Also, this patch does not address the other localization issues I pointed out like the localization of "MB" in picture size formatter

Comment 4

[Wilson Page [:wilsonpage]]

"You should do the localization when the settings menu is rendered, not when the app starts up."

Localization is not a concern of the view, but the underlying setting data model. The model could be represented in other views. If the changing of language is a concern, I suggest we 're-localize' the settings models on app 'focus'.

"...the localization of "MB" in picture size formatter"

I think you're referring to 'MP'. This seems a little like localization overkill. I don't imagine we localize strings like 'MP', 'KB', or 'GB', so don't think we should be concerned with 'MP' either. Willing to be proven wrong on this though.

Comment 5

[Wilson Page [:wilsonpage]]

Attachment: pull-request (camera-new-features)

Comment 6

[Wilson Page [:wilsonpage]]

Comments addressed.

Comment 7

[David Flanagan [:djf]]

(In reply to Wilson Page [:wilsonpage] from comment #4)
> "You should do the localization when the settings menu is rendered, not when
> the app starts up."
> 
> Localization is not a concern of the view, but the underlying setting data
> model. The model could be represented in other views. If the changing of
> language is a concern, I suggest we 're-localize' the settings models on app
> 'focus'.
> 

Actually, I would argue that localization is a concern of the view. Theoretically, what if you and one model and wanted to render it bi-lingually?  Obviously we don't do that in camera.

The simplest thing to do is to localize when you build the settings menu.  If the menu is created and destroyed each time it is displayed and hidden then this suffices.  If it is created lazily and retained, then you should set data-l10n properties on each element so they can be automatically localized on language changes, or you must listen for "localized" events.  (That might not be the right event name, so check on it.)  I don't think doing localization on visibility change is the right way to go.

> "...the localization of "MB" in picture size formatter"
> 
> I think you're referring to 'MP'. This seems a little like localization
> overkill. I don't imagine we localize strings like 'MP', 'KB', or 'GB', so
> don't think we should be concerned with 'MP' either. Willing to be proven
> wrong on this though.

I'll prove you wrong.  Look at apps/gallery/locales/gallery.en.properties and gallery.fr.properties and look at the strings byteUnit-B, byteUnit-KB, byteUnit-MB, etc.

When in doubt, localize!  If you need to localize a string that has parameters substituted into it, do that with the {{param}} syntax in the localization file.

Comment 8

[David Flanagan [:djf]]

Comment on attachment 8386998 [details] [review]
pull-request (camera-new-features)

For the record, I think it is an architectural mistake not to localize in the settings view class.  I'm willing to accept a patch that does it differently, but I think you'll be fighting the localization API if you try to do it that way.  Navigator.mozL10n caches all of the localized strings in memory. Caching another copy of them in your settings model is unnecessary.

Anyway: various comments on github.

Comment 9

[David Flanagan [:djf]]

Comment on attachment 8386998 [details] [review]
pull-request (camera-new-features)

For the record, I think it is an architectural mistake not to localize in the settings view class.  I'm willing to accept a patch that does it differently, but I think you'll be fighting the localization API if you try to do it that way.  Navigator.mozL10n caches all of the localized strings in memory. Caching another copy of them in your settings model is unnecessary.

Anyway: various comments here: https://github.com/wilsonpage/gaia/commit/8c49cd0726e02ecaecc36dee1a0111296829e1e5#diff-e4bc27afcad6f10d1abaf8eced7aa340R37

Comment 10

[Wilson Page [:wilsonpage]]

Comment on attachment 8386998 [details] [review]
pull-request (camera-new-features)

>https://github.com/mozilla-b2g/gaia/pull/16966

Comment 11

[David Flanagan [:djf]]

Comment on attachment 8386998 [details] [review]
pull-request (camera-new-features)

r+ with nits addressed.  Note that I have not run the code myself. I assume you have tested including the switching language case to verify that you get relocalized in that case.

I left comments on a couple of the commits then remembered that for this one I could just leave them on the PR itself.

You register the same localized event handler twice, and I think you need to fix that.

I suspect there is a pre-existing bug with localizing the overlay when the camera starts with no sdcard.  Please test that case if you can, and file a followup bug (if there is a bug).  And consider whether you need to change the architecture here to address that bug.

There's also a spot in the locale file where you define the same string twice.

Comment 12

[Wilson Page [:wilsonpage]]

Landed on 'camera-new-features': https://github.com/mozilla-b2g/gaia/commit/b9f025b09202a13545d29adc99d2b5b38b134d9f

Comment 13

[David Flanagan [:djf]]

Bulk edit for camera bugs.

If earlier comments do not show how this bug landed to master, it probably landed as part of https://github.com/mozilla-b2g/gaia/pull/17599 which merged the camera-new-features branch into master.

This bug was uplifted from master to v1.4 as part of https://github.com/mozilla-b2g/gaia/commit/a8190d08e61316a86bba572ba8d894d081a20530

Comment 14

[sarsenyev]

The issue is no longer reproduces on 1.4 and master build
Localization (l10n) presents in settings UI

1.4 Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140411000202
Gaia: 6c50349f41d40ba175ea0fc0c2c2cbd739ba7170
Gecko: 28b419f0e857
Version: 30.0a2
Firmware Version: v1.2-device.cfg

1.5 Environmental Variables:
Device: Buri 1.5 Master
BuildID: 20140411040203
Gaia: 1368d716072adf308e1b435ac828f97545a045f1
Gecko: d8c1b10c3a3d
Version: 31.0a1
Firmware Version: v1.2-device.cfg