1154980 - Localize first run pager titles

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

The title isn't localized - oops!

Comment 1

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: bz://1154980/liuche

/r/7103 - Bug 1154980 - Localize first run pager titles. r=ally

Pull down this commit:

hg pull -r 6320cbbd29f7166fd421d5ee60f31f16489acbd9 https://reviewboard-hg.mozilla.org/gecko/

Comment 2

[:Margaret Leibovic]

Does this also affect 38? Isn't that where this landed?

Comment 3

[:Margaret Leibovic]

It may be worth asking the l10n team if we can get a late string in, at least for 39.

Comment 4

[Chenxia Liu [:liuche]]

Flod: we're missing a localizable string in the first run experience that is riding the trains with 38 - how would you feel about a late string addition to 39?

Comment 5

[Allison Naaktgeboren :ally]

Comment on attachment 8593153 [details]
MozReview Request: bz://1154980/liuche

https://reviewboard.mozilla.org/r/7101/#review5947

::: mobile/android/base/firstrun/FirstrunPager.java
(Diff revision 1)
> -            final Fragment fragment = Fragment.instantiate(context, panels.get(i).getResource());
> +            final Fragment fragment = Fragment.instantiate(context, panels.get(i).getClassname());

Could you explain why you needed to change this line for my benefit?

::: mobile/android/base/firstrun/FirstrunPagerConfig.java
(Diff revision 1)
> -        private String title;
> +        private int titleRes;

Why do you need this variable?

I'm afraid I don't understand some of the mechanism for localization here, so can't in good conscious r+ it. f+ for now and we can talk about it tomorrow morning.

Comment 6

[Francesco Lodolo [:flod]]

(In reply to Chenxia Liu [:liuche] from comment #4)
> Flod: we're missing a localizable string in the first run experience that is
> riding the trains with 38 - how would you feel about a late string addition
> to 39?

I'm confused: 38 or 39 (aurora)? To be honest I'd be a lot happier with strings not landing on Aurora.

Counter question: how do you feel about a compromise reusing an existing string for the title, and fix it properly (new string) on 40? I'm looking at "Welcome to &brandShortName;"
http://hg.mozilla.org/releases/mozilla-aurora/file/8eab810238ea/mobile/android/base/locales/en-US/android_strings.dtd#l9

The pro is that the string would also be available on 38 if you need.

Comment 7

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/7101/#review5969

> Could you explain why you needed to change this line for my benefit?

This patch is mostly renaming some methods to be more explicit about what they're doing. The one substantial change is pulling a string resource from our localized resources instead of hard-coding the string in Java.

If you don't understand the Android code, definitely take a look at Android docs. This line is how you instantiate a Fragment:
http://developer.android.com/reference/android/app/Fragment.html#instantiate%28android.content.Context, java.lang.String%29

Here, I renamed the method to be more explicit, so instead of "getResource" I changed it to "getClassname".

> Why do you need this variable?

This variable is for storing the resource so we can fetch it in the PagerAdapter, which this is a subclass of. See the changes to firstrun/FirstrunPager.java. You'll see that it's a method change.

PagerAdapter: http://developer.android.com/reference/android/support/v4/view/PagerAdapter.html#getPageTitle%28int%29

Ok, let's talk about this over vidyo or something. The localization is basically the same as desktop localization, except that Android uses a strings.xml file, so we also need to reference the dtd strings in strings.xml.in, which gets preprocessed.

Basically, you add the actual strings to mobile/android/base/locales/en-US/android_strings.dtd, which will be localized. Then you add it to mobile/android/base/strings.xml.in so that they will be preprocessed so Android can use them.

Take a glance at this Android doc about resources, which should look familiar to our tree structure inside of mobile/android/base/resources:
http://developer.android.com/guide/topics/resources/providing-resources.html

Comment 8

[Jeff Beatty [:gueroJeff]]

I like flod's proposed compromise. If we can move on that, I think it's the best solution considering where we are in the cycle.

Comment 9

[Chenxia Liu [:liuche]]

(In reply to Francesco Lodolo [:flod] (UTC+2) from comment #6)
> (In reply to Chenxia Liu [:liuche] from comment #4)
> > Flod: we're missing a localizable string in the first run experience that is
> > riding the trains with 38 - how would you feel about a late string addition
> > to 39?
> 
> I'm confused: 38 or 39 (aurora)? To be honest I'd be a lot happier with
> strings not landing on Aurora.
> 
> Counter question: how do you feel about a compromise reusing an existing
> string for the title, and fix it properly (new string) on 40? I'm looking at
> "Welcome to &brandShortName;"
> http://hg.mozilla.org/releases/mozilla-aurora/file/8eab810238ea/mobile/
> android/base/locales/en-US/android_strings.dtd#l9
> 
> The pro is that the string would also be available on 38 if you need.

Good call, I think that will be okay. I'm needinfo-ing Matej just to give him a heads up that on 38 and 39, our title will be "Welcome to Firefox" instead of just "Welcome". (I assume this is better than the string not being localized at all.)

Comment 10

[Allison Naaktgeboren :ally]

Comment on attachment 8593153 [details]
MozReview Request: bz://1154980/liuche

https://reviewboard.mozilla.org/r/7101/#review5975

Ship It!

Comment 11

[Allison Naaktgeboren :ally]

viydo chat. r+

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3e73d9b001c3

Comment 13

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/fx-team/rev/3e73d9b001c3

Comment 14

[Chenxia Liu [:liuche]]

I'll also be adding an aurora and beta patch, to use onboard_start_message2, which is already localized.

Comment 15

[Chenxia Liu [:liuche]]

Attachment: Aurora/Beta patch: Use existing string.

Approval Request Comment
[Feature/regressing bug #]: Bug 1063844 didn't localize new string
[User impact if declined]: new non-English locale users will see an unlocalized title in the first run experience
[Describe test coverage new/current, TreeHerder]: local testing, try:
Beta: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ed8585cb6ba
Aurora: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a77a8bf18a9a
[Risks and why]: low, adding code path to use a resource instead of a hard-coded string
[String/UUID change made/needed]: none

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3e73d9b001c3

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8594253 [details] [diff] [review]
Aurora/Beta patch: Use existing string.

Should be in 38 beta 6

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3b137c13eec5

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/65cf03fc2bc9

Comment 20

[Catalin Suciu [:csuciu]]

Verified as fixed on Firefox 38.0b8.
Still needs to be verified on Nightly/Aurora

Comment 22

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1154980 - Localize first run pager titles. r=ally