Pass UI language to BuildComponents().

Pass UI language to BuildComponents().

This patch removes language_tag parameter from
Localization::SetGetter(), because it's not required for retrieving
strings. BuildComponents() now requires a |ui_language_tag| parameter.

R=lararennie@google.com

Committed: https://code.google.com/p/libaddressinput/source/detail?r=292

[Rouslan Solomakhin, 2014-06-13 18:21:02]

Lara: PTAL Patch Set 1. Evan pointed out that this comment is not helpful when
he's reviewing using libaddressinput in Chrome. I think this comment is an
improvement. WDYT?

Evan: Does this comment clarify the purpose of |language_tag| parameter?

[estade, 2014-06-13 19:41:24]

On 2014/06/13 18:21:02, Rouslan Solomakhin wrote:
> Lara: PTAL Patch Set 1. Evan pointed out that this comment is not helpful when
> he's reviewing using libaddressinput in Chrome. I think this comment is an
> improvement. WDYT?
> 
> Evan: Does this comment clarify the purpose of |language_tag| parameter?

still confused: does the language_tag have anything to do with the string
getter? Localization doesn't seem to ever use this value; why not make it a
parameter to BuildComponents or whatever piece of code actually needs it?

[Rouslan Solomakhin, 2014-06-13 20:04:01]

On 2014/06/13 19:41:24, estade wrote:
> On 2014/06/13 18:21:02, Rouslan Solomakhin wrote:
> > Lara: PTAL Patch Set 1. Evan pointed out that this comment is not helpful
when
> > he's reviewing using libaddressinput in Chrome. I think this comment is an
> > improvement. WDYT?
> > 
> > Evan: Does this comment clarify the purpose of |language_tag| parameter?
> 
> still confused: does the language_tag have anything to do with the string
> getter? Localization doesn't seem to ever use this value; why not make it a
> parameter to BuildComponents or whatever piece of code actually needs it?

If you use Localization::SetString(langauge_tag) instead of
Localization::SetGetter(getter, langauge_tag), then the language_tag is used
both to retrieve built-in translations and to select the best format for the
address. BuildComponents() already needs the Localization object to set the
labels on the UI components. The API of BuildComponents() is more clear when it
uses Localization::GetLanguage() instead of taking a Localization object and
also a ui_language_tag, no?

[estade, 2014-06-13 20:13:20]

On 2014/06/13 20:04:01, Rouslan Solomakhin wrote:
> On 2014/06/13 19:41:24, estade wrote:
> > On 2014/06/13 18:21:02, Rouslan Solomakhin wrote:
> > > Lara: PTAL Patch Set 1. Evan pointed out that this comment is not helpful
> when
> > > he's reviewing using libaddressinput in Chrome. I think this comment is an
> > > improvement. WDYT?
> > > 
> > > Evan: Does this comment clarify the purpose of |language_tag| parameter?
> > 
> > still confused: does the language_tag have anything to do with the string
> > getter? Localization doesn't seem to ever use this value; why not make it a
> > parameter to BuildComponents or whatever piece of code actually needs it?
> 
> If you use Localization::SetString(langauge_tag) instead of
> Localization::SetGetter(getter, langauge_tag), then the language_tag is used
> both to retrieve built-in translations and to select the best format for the
> address. BuildComponents() already needs the Localization object to set the
> labels on the UI components. The API of BuildComponents() is more clear when
it
> uses Localization::GetLanguage() instead of taking a Localization object and
> also a ui_language_tag, no?

I see what you're saying, but I think SetGetterFromLanguageTag() would be a
better name for SetLanguage(), and passing language tag to both that and
BuildComponents is sensible. The alternative that we seem to be running into is

a) SetGetter takes a parameter that doesn't have to do with the getter, which is
confusing
b) it's possible that parameter is never even used

[Rouslan Solomakhin, 2014-06-13 21:05:51]

On 2014/06/13 20:13:20, estade wrote:
> On 2014/06/13 20:04:01, Rouslan Solomakhin wrote:
> > On 2014/06/13 19:41:24, estade wrote:
> > > On 2014/06/13 18:21:02, Rouslan Solomakhin wrote:
> > > > Lara: PTAL Patch Set 1. Evan pointed out that this comment is not
helpful
> > when
> > > > he's reviewing using libaddressinput in Chrome. I think this comment is
an
> > > > improvement. WDYT?
> > > > 
> > > > Evan: Does this comment clarify the purpose of |language_tag| parameter?
> > > 
> > > still confused: does the language_tag have anything to do with the string
> > > getter? Localization doesn't seem to ever use this value; why not make it
a
> > > parameter to BuildComponents or whatever piece of code actually needs it?
> > 
> > If you use Localization::SetString(langauge_tag) instead of
> > Localization::SetGetter(getter, langauge_tag), then the language_tag is used
> > both to retrieve built-in translations and to select the best format for the
> > address. BuildComponents() already needs the Localization object to set the
> > labels on the UI components. The API of BuildComponents() is more clear when
> it
> > uses Localization::GetLanguage() instead of taking a Localization object and
> > also a ui_language_tag, no?
> 
> I see what you're saying, but I think SetGetterFromLanguageTag() would be a
> better name for SetLanguage(), and passing language tag to both that and
> BuildComponents is sensible. The alternative that we seem to be running into
is
> 
> a) SetGetter takes a parameter that doesn't have to do with the getter, which
is
> confusing
> b) it's possible that parameter is never even used

Evan: I'm OK with going this route. See Patch Set 2. Is that what you had in
mind?

Lara: PTAL Patch Set 2. We think that it makes the API cleaner and easier to
understand.

[lararennie, 2014-06-16 07:44:21]

I think the concern here was that the localisation might end up being set up
with a different UI language to the language that we use when calculating the
best language we are using.

However I'll take a look at the proposed changes and see what I think.

[Rouslan Solomakhin, 2014-06-16 14:44:52]

FYI

[lararennie, 2014-06-17 21:18:50]

https://codereview.appspot.com/109940045/diff/40001/cpp/include/libaddressinp...
File cpp/include/libaddressinput/localization.h (right):

https://codereview.appspot.com/109940045/diff/40001/cpp/include/libaddressinp...
cpp/include/libaddressinput/localization.h:31: //   
localization.SetGetterFromLanguageTag("en");
So, just to clarify: Chrome has a method that gets, e.g., French translations?
And you set that below? I'm wondering:

a) Do we need SetGetterFromLanguageTag? Should it just be that we use the
default getter, which gets English, unless SetGetter is called to override it?

b) Alternatively, could we provide the localization with information about what
how to get translations based on language, and then we set the language for the
localization but not an individual getter - rather, all the getters? And then
the localization would either: call the language-specific getter (if that's what
Chrome has) or call the getter *with* the language code passed in (if Chrome has
one getter, and it does the language resolution part inside it...)

If we did (b) then we would only need to change the localization, since the
address UI can just ask for the language like it did before.

If we did (a) instead I think it'd make the API cleaner than it is in this patch
(but I'm not sure, maybe there is a reason why you specifically have that
method). It does mean it's possible for UI language to get out of sync with the
two components...

[Rouslan Solomakhin, 2014-06-17 22:50:31]

On 2014/06/17 21:18:50, lararennie wrote:
>
https://codereview.appspot.com/109940045/diff/40001/cpp/include/libaddressinp...
> File cpp/include/libaddressinput/localization.h (right):
> 
>
https://codereview.appspot.com/109940045/diff/40001/cpp/include/libaddressinp...
> cpp/include/libaddressinput/localization.h:31: //   
> localization.SetGetterFromLanguageTag("en");
> So, just to clarify: Chrome has a method that gets, e.g., French translations?

Chrome has l10nutil::GetStringUTF8(int) which returns strings in the current
application locale. Changing application locale requires a restart of the
browser, which loads only the strings for the current locale. Thus there's no
way to get French strings while your UI is in English, for example.

[Rouslan Solomakhin, 2014-06-17 22:50:54]

On 2014/06/17 21:18:50, lararennie wrote:
> a) Do we need SetGetterFromLanguageTag? Should it just be that we use the
> default getter, which gets English, unless SetGetter is called to override it?

This approach is the simplest, which makes me like it.

[Rouslan Solomakhin, 2014-06-17 22:52:31]

On 2014/06/17 21:18:50, lararennie wrote:
>
https://codereview.appspot.com/109940045/diff/40001/cpp/include/libaddressinp...
> File cpp/include/libaddressinput/localization.h (right):
> 
>
https://codereview.appspot.com/109940045/diff/40001/cpp/include/libaddressinp...
> cpp/include/libaddressinput/localization.h:31: //   
> localization.SetGetterFromLanguageTag("en");
> So, just to clarify: Chrome has a method that gets, e.g., French translations?
> And you set that below? I'm wondering:
> 
> a) Do we need SetGetterFromLanguageTag? Should it just be that we use the
> default getter, which gets English, unless SetGetter is called to override it?
> 
> b) Alternatively, could we provide the localization with information about
what
> how to get translations based on language, and then we set the language for
the
> localization but not an individual getter - rather, all the getters? And then
> the localization would either: call the language-specific getter (if that's
what
> Chrome has) or call the getter *with* the language code passed in (if Chrome
has
> one getter, and it does the language resolution part inside it...)

Option (b) does not work because GetStringUTF8() in Chrome always returns the
same string: the one in the current application locale. Changing the locale
requires a restart of the browser.

[Rouslan Solomakhin, 2014-06-17 22:54:30]

On 2014/06/17 21:18:50, lararennie wrote:
> If we did (a) instead I think it'd make the API cleaner than it is in this
patch
> (but I'm not sure, maybe there is a reason why you specifically have that
> method). It does mean it's possible for UI language to get out of sync with
the
> two components...

The two components will be in sync in Chrome always because GetStringUTF8(int)
always returns strings in the current application locale, which is what I pass
as the UI language to libaddressinput. It's possible that other users of the
library have different properties.

[Rouslan Solomakhin, 2014-06-18 00:29:07]

Lara: PTAL Patch Set 3.

[roubert (google), 2014-06-18 10:19:59]

https://codereview.appspot.com/109940045/diff/80001/cpp/include/libaddressinp...
File cpp/include/libaddressinput/localization.h (right):

https://codereview.appspot.com/109940045/diff/80001/cpp/include/libaddressinp...
cpp/include/libaddressinput/localization.h:84: // The string getter, not owned.
You can't allocate functions on the heap, can you? There is no concept of
ownership for functions (they're always there; even pointers to functions in
dynamically loaded libraries don't point to structures that need to be
deallocated).

[lararennie, 2014-06-18 12:37:41]

LGTM

https://codereview.appspot.com/109940045/diff/80001/cpp/include/libaddressinp...
File cpp/include/libaddressinput/localization.h (right):

https://codereview.appspot.com/109940045/diff/80001/cpp/include/libaddressinp...
cpp/include/libaddressinput/localization.h:32: //   
Process(BuildComponents("CA", localization, "en-US", &best_language_tag));
Add a comment here to say it'll return English by default?

[Rouslan Solomakhin, 2014-06-18 16:27:39]

https://codereview.appspot.com/109940045/diff/80001/cpp/include/libaddressinp...
File cpp/include/libaddressinput/localization.h (right):

https://codereview.appspot.com/109940045/diff/80001/cpp/include/libaddressinp...
cpp/include/libaddressinput/localization.h:32: //   
Process(BuildComponents("CA", localization, "en-US", &best_language_tag));
On 2014/06/18 12:37:41, lararennie wrote:
> Add a comment here to say it'll return English by default?

Done.

https://codereview.appspot.com/109940045/diff/80001/cpp/include/libaddressinp...
cpp/include/libaddressinput/localization.h:84: // The string getter, not owned.
On 2014/06/18 10:19:59, roubert wrote:
> You can't allocate functions on the heap, can you? There is no concept of
> ownership for functions (they're always there; even pointers to functions in
> dynamically loaded libraries don't point to structures that need to be
> deallocated).

Good point. I removed the ownership part of the comment.

[Rouslan Solomakhin, 2014-06-18 16:27:58]

Committed patchset #4 manually as r292 (presubmit successful).