Closed Bug 594017 Opened 9 years ago Closed 9 years ago

localize android java files

Categories

(Core :: Widget: Android, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0b4+ ---

People

(Reporter: blassey, Assigned: mwu)

References

Details

Attachments

(4 files, 5 obsolete files)

we have some strings in the embedding/android/*.java files that need to be localized. Information about the android l10n system can be found here http://developer.android.com/guide/topics/resources/localization.html and here http://developer.android.com/resources/tutorials/localization/index.html.

From Axel on irc:
Pike: blassey: seems like they hard-core discourage their old .properties style, which is OK, they came up with this strings xml replacement. I suspect that you should be fine with putting the xml in an xml.in, and use a localized strings.inc or so and the PreProcessor.py to fold them together into a localized dir
Pike: for the multi build, for single locale builds, you can probably just go with the default dir
tracking-fennec: --- → 2.0b2+
Assignee: nobody → blassey.bugs
Attached patch patch to use strings.xml (obsolete) — Splinter Review
Attachment #476925 - Flags: review?(mwu)
Attachment #476925 - Flags: review?(l10n)
Axel, one issue with this patch is it just uses Fennec in the splash screen string. We couldn't figure out how to stick the content of MOZ_APP_DISPLAYNAME in there.
I'm wondering if we could just make the xml file #include android.dtd and brand.dtd, something like

<!DOCTYPE resources [
#include brand.dtd
#include android.dtd
]>

where you'd copy both files to the working dir, with picking up the right file in the l10n-merge case.

Then you could just use &brandShortName; in android.dtd. Depends a bit on how the java parser is set up.
Comment on attachment 476925 [details] [diff] [review]
patch to use strings.xml

The general approach looks fine to me. The opinion of someone doing l10n is probably more important. Just two things:

1. Can we use MOZ_APP_DISPLAYNAME in splash_screen_label?
2. Can we use AB_CD instead of NO_JA_JP_MAC_AB_CD?
Attachment #476925 - Flags: review?(mwu) → review+
I don't see AB_CD used at all?

Also, what does 

b.setText("Launch"); // don't need to localize

say? I lack context on that comment.
(In reply to comment #5)
> I don't see AB_CD used at all?
> 
This is true.

> Also, what does 
> 
> b.setText("Launch"); // don't need to localize
> 
> say? I lack context on that comment.
This is text for a button that is showed in a special debugging mode. It lets us control when gecko gets started so we can attach gdb and debug startup problems.
(In reply to comment #6)
> (In reply to comment #5)
> > I don't see AB_CD used at all?
> > 
> This is true.
> 
> > Also, what does 
> > 
> > b.setText("Launch"); // don't need to localize
> > 
> > say? I lack context on that comment.
> This is text for a button that is showed in a special debugging mode. It lets
> us control when gecko gets started so we can attach gdb and debug startup
> problems.

This is a debug only screen used for attaching gdb. I don't think we localize those sorts of things.
Comment on attachment 476925 [details] [diff] [review]
patch to use strings.xml

I'm noting this as a feedback-, as I think we'll need to answer Brad's question about the brand names differently.

Re the comment on "Lauch", make that more explicit so folks can understand that without reading the whole method? I do agree with not localizing that string, though.
Attachment #476925 - Flags: review?(l10n) → review-
(In reply to comment #3)
> I'm wondering if we could just make the xml file #include android.dtd and
> brand.dtd, something like
> 
> <!DOCTYPE resources [
> #include brand.dtd
> #include android.dtd
> ]>
> 
> where you'd copy both files to the working dir, with picking up the right file
> in the l10n-merge case.
> 
> Then you could just use &brandShortName; in android.dtd. Depends a bit on how
> the java parser is set up.

This works if I use an absolute path to brand.dtd. How do I set the search path so it can just being "brand.dtd" rather than "/home/blassey/src/mozilla-central/mobile/branding/nightly/locales/en-US/brand.dtd"
Basically two options:

copy all three files into one dir, as PreProcessor.py checks relative to DIRECTORY, 

or

pass the full paths via -DBRAND_PATH=..., and use 
#includesubst @BRAND_PATH@
Attached patch patch v.2 (obsolete) — Splinter Review
Attachment #476925 - Attachment is obsolete: true
Attachment #477580 - Flags: review?(l10n)
Comment on attachment 477580 [details] [diff] [review]
patch v.2

carrying mwu's review
Attachment #477580 - Flags: review+
Comment on attachment 477580 [details] [diff] [review]
patch v.2

r-.

In this case, strings.inc should be a DTD, and included like brand.dtd. That way, we can deduce the variable format from the file.

Also, @MOZ_ABS_BRANDING_DIRECTORY@/locales/@AB_CD@/brand.dtd is not going to work for l10n. Look at http://mxr.mozilla.org/mozilla-central/source/browser/locales/Makefile.in#51 or http://mxr.mozilla.org/mozilla-central/source/browser/locales/Makefile.in#89 on how to resolve those right, and make l10n-merge builds work right. The first is for something like

res/values/strings.xml: strings.dtd brand.dtd strings.xml.in
	mkdir -p res/values
	cp $^ .
	PreProcessor.py strings.xml.in > res/values/strings.xml

with vpaths set for %.dtd,

the other is for 
BRANDPATH = $(firstword $(wildcard $(LOCALE_MERGEDIR)/brand.dtd) 
                        $(wildcard $(LOCALE_SRCDIR)/brand.dtd) 
                        $(srcdir)/en-US/brand.dtd )
(this needs to be tweaked to actually resolve brand.dtd)
and then do
res/values/strings.xml:
	PreProcessor.py -DBRANDPATH=$(BRANDPATH) strings.xml.in > res/values/strings.xml

I tend to think the vpath one is slicker for your use case.
Attachment #477580 - Flags: review?(l10n) → review-
Attached patch patch (obsolete) — Splinter Review
the vpath thing doesn't work for dtd's because the preprocessor looks in the srcdir for dtd's and we really don't want to be copying things into the srcdir.
Attachment #477580 - Attachment is obsolete: true
Attachment #477745 - Flags: review?(l10n)
Comment on attachment 477745 [details] [diff] [review]
patch


>+        outFile.setLastModified(fileEntry.getTime());
>     }
> 
>-        outFile.setLastModified(fileEntry.getTime());

please ignore this
Comment on attachment 477745 [details] [diff] [review]
patch

r-, for a few:

+abs_topsrcdir   = `cd $(topsrcdir) && pwd`

is frowned upon, there are other places where we need abs topsrc dirs that do .... forgot. Ask ted or khuey?

+BRANDPATH = $(firstword $(wildcard $(LOCALE_MERGEDIR)/brand.dtd) \
+	$(wildcard $(LOCALE_SRCDIR)/brand.dtd) \
+	$(DEPTH)/$(MOZ_BRANDING_DIRECTORY)/locales/$(AB_CD)/brand.dtd)

should be in an #ifdef LOCALE_MERGEDIR and not merge else.

You'll want another one like this for strings.dtd, and get rid of strings.inc, like I said in my earlier review. 

Also, don't define AB_CD in the preprocessor line if you don't use it.

And, the generation of strings.xml should be done always, the dependencies aren't strong enough to only do that "when needed". In particular, when locale changes or such, you end up depending on different files than before.
Attachment #477745 - Flags: review?(l10n) → review-
Attached patch patch (obsolete) — Splinter Review
Attachment #477745 - Attachment is obsolete: true
Attachment #478312 - Flags: review?(l10n)
Comment on attachment 478312 [details] [diff] [review]
patch

grrr, I suck at this review. I find more and more stuff piecemeal-wise, sorry.

strings.dtd will need to be in locales/en-US, I guess locales/en-US/android/strings.dtd or installer/android.dtd?

The target path of strings.xml should probably depend on the locale code. How exactly depends on the locale lookup code on android. Probably some s/-/_/ business, at least. Probably wanna use the no-ja-JP-mac one, too. Maybe just for en-US copy it to res/values/strings.xml, too, as fallback?

/locales/$(AB_CD)/brand.dtd
won't work for anything but en-US. you wanna hardcode that to en-US, LOCALE_SRCDIR is the one that depends on AB_CD (and RELATIVE_SRCDIR (should be mobile)).

The rule to put the strings.xml in the right place may want to be in locales/Makefile.in so that we can use it for repacks.

Not exactly sure how this will go for the multi-locale builds.

Did we at all start to repack android builds yet? At least locally in some proof-of-concept? Really hard to flesh this out without a build to test this in, I guess.
Attachment #478312 - Flags: review?(l10n) → review-
Not sure if this is the question your asking, but for android packages the "defaults" go in values/*. Other languages go in values-AB_CD/* and mask what's in values/*.

I would assume for a multilocale build we'd have our defaults in values/strings.xml and all the other languages in values-AB_CD/strings.xml.
Attached patch patch (obsolete) — Splinter Review
Attachment #478312 - Attachment is obsolete: true
Attachment #478967 - Flags: review?(l10n)
Comment on attachment 478967 [details] [diff] [review]
patch

This patch doesn't work, for reasons stated before (dtd file in a non-localizable path, for example) and new ones.

I don't see us getting anywhere by guessing on what might work and what might not.

Can we actually have some localized builds to test this in before making more iterations?
Attachment #478967 - Flags: review?(l10n) → review-
Depends on: 600214
Assignee: blassey.bugs → l10n
Assignee: l10n → nobody
Assignee: nobody → blassey.bugs
Assignee: blassey.bugs → nobody
Axel, given that the android specific bits have landed in bug 600214, can you own this bug?
Sorry, can't. I don't have free cycles, nor a setup for android builds, nor something to test the results. I'm hacking on bug 525438 to make l10n-merge less cumbersome, but I'm currently running into a wall with the fx windows installer, so no eta on that patch. It'd bring macros for picking up the right files from the right dirs, possibly I can factor out a patch-let of that to re-use.
I'd be willing to own this, if no one else is moving forward on it...
That'd be great from my POV.

http://tests.themasta.com/tinderboxpushlog/?tree=MozillaTry&rev=4cfb3752fab4 isn't looking too bad right now, fyi.
Taking this bug for now since I'm writing the patches that will make l10n repacks on Android possible (bug 567873), so I might know how to get this part going too.
Assignee: nobody → mwu
Assignee: mwu → pavlov
Assignee: pavlov → l10n
Assignee: l10n → nobody
FWIW, the patches in bug 525438 won't land in time for this bug, I don't have the cycles ad-hoc, nor am I confident that it's good to land this while B7 is on the bastard branch.

https://bugzilla.mozilla.org/attachment.cgi?id=482213&action=diff#a/config/config.mk_sec1 shows the code snippet to get the right fallback code though, so you can use the result of the MERGE_FILE macro to just do the right thing for the android DTD file.
tracking-fennec: 2.0b2+ → 2.0b3+
Assignee: nobody → mwu
Duplicate of this bug: 612992
Android unfortunately compiles all strings and code together into a binary blob. This requires us to move more of the build process to package time. Additionally, I am having problems getting compare-locales to pick up the android_strings dtd which I guess may be related to Pike's comment. Too tired to figure it out right now.
Patch produced with pike's help.
Attachment #478967 - Attachment is obsolete: true
Attachment #494213 - Flags: review?(mark.finkle)
This moves the generation of gecko.ap_ to packaging so we can pick up new strings added with make chrome-%. It also creates a fake chrome directory for these strings so our current l10n code dealing with LOCALE_MERGEDIR can automatically drop in the appropriate localized dtds when chrome-% is run.
Attachment #494216 - Flags: review?(ted.mielczarek)
Attachment #494217 - Flags: review?(blassey.bugs)
Attachment #494217 - Flags: review?(blassey.bugs) → review+
Attachment #494213 - Flags: review?(mark.finkle) → review+
tracking-fennec: 2.0b3+ → 2.0b4+
Blocks: 615389
Comment on attachment 494216 [details] [diff] [review]
2. Support localization of strings in embedding/android code

rs=me. I already hate this Makefile and I hate l10n in the build system, so whatever.
Attachment #494216 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 494216 [details] [diff] [review]
2. Support localization of strings in embedding/android code

>-DEFAULT_BRANDPATH = $(DEPTH)/$(MOZ_BRANDING_DIRECTORY)/locales/en-US/brand.dtd
>+DEFAULT_BRANDPATH = $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/locales/en-US/brand.dtd
This broke the build for any objdir which isn't a sibling of the src dir
Attachment #499673 - Flags: review+
You need to log in before you can comment on or make changes to this bug.