Issue 1166 - Import strings from ABP for Firefox

Issue 1166 - Import strings from ABP for Firefox

[Felix Dahlke, 2014-08-05 16:14:16]

http://codereview.adblockplus.org/5748296123416576/diff/5668600916475904/impo...
File import_locales.py (right):

http://codereview.adblockplus.org/5748296123416576/diff/5668600916475904/impo...
import_locales.py:51: "en": "en-US",
There are two other locales we probably need to map:

ur-PK (Urdu, Pakistan):
We have "ur" in ABP for Firefox - do we want to read the ur-PK strings from
there?

fil (Filipino):
Couldn't even find the Mozilla locale code for this one. Ideas?

http://codereview.adblockplus.org/5748296123416576/diff/5668600916475904/impo...
import_locales.py:79: gecko_first_run_path = "%s/%s/firstRun.properties" % (
I've only looked at first run page strings we can reuse for now. We can probably
fetch a few more strings from Firefox if we look at all of them, but that's
something I'd rather do after the release.

[Felix Dahlke, 2014-08-05 16:38:59]

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
File import_locales.py (right):

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
import_locales.py:51: "en": "en-US",
There are two other locales we probably need to map:

ur-PK (Urdu, Pakistan):
We have "ur" in ABP for Firefox - do we want to read the ur-PK strings from
there?

fil (Filipino):
Couldn't even find the Mozilla locale code for this one. Ideas?

[Wladimir Palant, 2014-08-06 06:09:04]

What happened to "import at build time"? The current solution is what we used
for Chrome originally and it has several disadvantages:

1) String synchronization has to be done explicitly, manually.
2) We duplicate data between repositories.
3) Uploading strings for translations becomes more complicated since we need to
exclude the strings that we imported from Firefox.

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
File import_locales.py (right):

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
import_locales.py:4: from ConfigParser import ConfigParser
We generally use SafeConfigParser

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
import_locales.py:48: ]
What is this list based on? It seems to be rather short. We shouldn't need to
adjust this script whenever we add more locale files.
http://www.microsoft.com/en-us/download/details.aspx?id=36804 lists 94 languages
(plus en-US which appears to be the default).
http://blogs.msdn.com/b/ie/archive/2011/05/25/internet-explorer-9-now-availab...
lists 93.

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
import_locales.py:51: "en": "en-US",
Right now, Firefox isn't available in either Urdu or Filipino - that's why we
aren't asking for ABP/Firefox translations in these languages. We need an
adblockpluscore translation project, this will fix it...

It's surprising that Internet Explorer uses ur-PK given that most Urdu speakers
are in India (ur-IN).

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
import_locales.py:53: "nb": "nb-NO"
These are locales that *we* named, IE itself always uses the full locale names.
My impression here is that we misnamed English, Hindi and Bokmål locales - these
are country-specific and shouldn't be shortened. So I'd rather see the names
fixed instead of the mapping here. This can be done separately if it is too
complicated.

Please note that the locales under installer/src/msi/locale/ follow a different
naming scheme (of course :-( ).

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
import_locales.py:63: locale_base_path =
"libadblockplus/adblockplus/chrome/locale"
Define this along with the other constants above instead of hardcoding it in the
function?

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
import_locales.py:64: locale_files = ["firstRun.properties", "filters.dtd"]
Extract this list from strings_to_import?

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
import_locales.py:68: locale_base_path, locale, locale_file)
Please use os.path.join(locale_base_path, locale, locale_file).

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
import_locales.py:84: file.write("%s=%s\n" % item)
Given that the translations are coming from Crowdin, please make sure to handle
newlines. SafeConfigParser inserts a tab character after newlines, this should
indicate that what comes there still belongs to the same option. Our INI parser
isn't quite as advanced so re.sub(r'\s+', ' ', re.S) should do.

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
import_locales.py:90: gecko_locale = ie_locale
This block can be written as:

  gecko_locale = locale_mapping.get(ie_locale, ie_locale)

[Wladimir Palant, 2014-08-06 06:11:04]

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
File import_locales.py (right):

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
import_locales.py:104: value = value.replace("&", "")
This won't do the job for CJK locales. The strings there use Latin access keys
like this: "blablabla (&x)". You need to remove " (&x)" at the end of the string
and not just &.

[Felix Dahlke, 2014-08-06 10:32:07]

New patch set, the remaining issue now is to list all the possible locales.

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
File import_locales.py (right):

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
import_locales.py:4: from ConfigParser import ConfigParser
On 2014/08/06 06:09:04, Wladimir Palant wrote:
> We generally use SafeConfigParser

Done.

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
import_locales.py:48: ]
On 2014/08/06 06:09:04, Wladimir Palant wrote:
> What is this list based on? It seems to be rather short. We shouldn't need to
> adjust this script whenever we add more locale files.
> http://www.microsoft.com/en-us/download/details.aspx?id=36804 lists 94
languages
> (plus en-US which appears to be the default).
>
http://blogs.msdn.com/b/ie/archive/2011/05/25/internet-explorer-9-now-availab...
> lists 93.

Yes, I just took the list of locales we currently have, since I wasn't sure
where to get the others.

On second thought, I suppose I can just use the codes we use on Crowding for
these? Anything supported by IE but missing from Crowding should probably be
added there too, right?

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
import_locales.py:53: "nb": "nb-NO"
On 2014/08/06 06:09:04, Wladimir Palant wrote:
> These are locales that *we* named, IE itself always uses the full locale
names.
> My impression here is that we misnamed English, Hindi and Bokmål locales -
these
> are country-specific and shouldn't be shortened. So I'd rather see the names
> fixed instead of the mapping here. This can be done separately if it is too
> complicated.
> 
> Please note that the locales under installer/src/msi/locale/ follow a
different
> naming scheme (of course :-( ).

Here's a follow-up for that:
https://issues.adblockplus.org/ticket/1177

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
import_locales.py:63: locale_base_path =
"libadblockplus/adblockplus/chrome/locale"
On 2014/08/06 06:09:04, Wladimir Palant wrote:
> Define this along with the other constants above instead of hardcoding it in
the
> function?

Done.

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
import_locales.py:64: locale_files = ["firstRun.properties", "filters.dtd"]
On 2014/08/06 06:09:04, Wladimir Palant wrote:
> Extract this list from strings_to_import?

Done.

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
import_locales.py:68: locale_base_path, locale, locale_file)
On 2014/08/06 06:09:04, Wladimir Palant wrote:
> Please use os.path.join(locale_base_path, locale, locale_file).

Done.

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
import_locales.py:84: file.write("%s=%s\n" % item)
On 2014/08/06 06:09:04, Wladimir Palant wrote:
> Given that the translations are coming from Crowdin, please make sure to
handle
> newlines. SafeConfigParser inserts a tab character after newlines, this should
> indicate that what comes there still belongs to the same option. Our INI
parser
> isn't quite as advanced so re.sub(r'\s+', ' ', re.S) should do.

Done.

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
import_locales.py:90: gecko_locale = ie_locale
On 2014/08/06 06:09:04, Wladimir Palant wrote:
> This block can be written as:
> 
>   gecko_locale = locale_mapping.get(ie_locale, ie_locale)

Done.

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
import_locales.py:104: value = value.replace("&", "")
On 2014/08/06 06:11:04, Wladimir Palant wrote:
> This won't do the job for CJK locales. The strings there use Latin access keys
> like this: "blablabla (&x)". You need to remove " (&x)" at the end of the
string
> and not just &. 

Done.

[Wladimir Palant, 2014-08-06 10:50:58]

LGTM

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
File import_locales.py (right):

http://codereview.adblockplus.org/5748296123416576/diff/5685265389584384/impo...
import_locales.py:53: "nb": "nb-NO"
On 2014/08/06 10:32:08, Felix H. Dahlke wrote:
> Here's a follow-up for that:
> https://issues.adblockplus.org/ticket/1177

I also created https://issues.adblockplus.org/ticket/1178 on the locale names in
the installer.

[Felix Dahlke, 2014-08-06 12:14:05]

Uploaded a new patch set that doesn't hard code the IE locales, and that uses
langMappingGecko from localeTools.

I've also created a follow-up for doing all this at build time:
https://issues.adblockplus.org/ticket/1179

[Wladimir Palant, 2014-08-06 15:10:55]

LGTM