661083 - Let developers localize add-on metadata with cfx

Status: RESOLVED WONTFIX

(Add-on SDK Graveyard :: General, enhancement, P1)

Description

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: WIP patch

It'd be nice if developers had a way to specify localized add-on metadata (like name, description, etc) directly from package.json without having to manually edit install.rdf in the generated XPI files.

This is a work in progress patch to try to add this.

The patch adds an em:localized element (and child elements) to the app-extension copy of install.rdf, which is used as the SDK's template when creating an extension's XPI file.

In packaging.py, the 'localized' property is added so that it is recognized in package.json.

The rest of the patch is in rdf.py:

It makes a tiny change to the remove() method, making sure that the "localized" versions of elements don't get deleted when the unlocalized versions are. (I'm not sure if this is really necessary.)

It adds a "localize()" method, which takes in the package.json attributes and adds them to install.rdf correctly.

It takes the em:localized element that was added to app-extension/install.rdf up above, and inserts a copy of it back into the working copy of the rdf file (because there can be more than one localization, I couldn't just add the properties directly to the template). One copy gets attached for every localization specified in package.json.

One a copy has been attached, it looks at each of the child elements and checks for a corresponding property in package.json. If a property exists in there, that property is inserted into the rdf file. If that property doesn't exist, that child element is removed.

After all of the locales have been added, the template version of em:localized gets removed.







I'd like some feedback on how this is being done.

As it stands, the patch works completely (as far as I can tell, at least. haven't tried running any tests). Installing the generated XPI picks up the localizations for the name, description and other metadata.

It still needs to add to the package specification documentation to show the new stuff, but I don't want to do that until the format of the new stuff gets more finalized.

(The generated install.rdf doesn't look very pretty, though. Open to suggestions!)

Comment 1

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: sample package.json

This is the sample package.json contents that I used to test this.

Comment 2

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: generated install.rdf

And here's what the generated install.rdf looks like from that sample.

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Right now, for convenience (to me) the patch uses the rdf element names for the new package.json property names, not the names already in package.json. (All it has to do is strip off the "em:" and then the names match up.)

The final version of this should probably use the same names as the rest of package.json.

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 536521 [details]
generated install.rdf

Changing the attached install.rdf's mimetype so you can see the not-so-pretty output easier.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Our vision for addon localization is that addon developers shouldn't have to do any work to solicit and package localizations.  Rather, they should just use strings in their addon (identifying them as needing localization as appropriate) and submit their addon to AMO for distribution, at which point AMO should automatically solicit localizations from localizers and package the localizations that localizers contribute.

So I don't want to add functionality that enables addon developers to manually provide localizations.  Rather, we should make sure that the project to provide a "common pool" service for addon localization includes the ability to localize metadata in addition to strings embedded in code.

cc:ing gandalf so he's aware of this additional requirement for the common pool localization service (which he may already be aware of!).

Comment 6

[Alexandre Poirot [:ochameau]]

Reopening this as we didn't got resources to release commonpool.

We can now implement this through existing properties file.

Comment 7

[simon.lachinger]

Until this is fixed, can you add a hint to the documentation that the name and description cannot be localized using the l10n mechanisms provided by the addon-sdk? It is bad enough to have to unzip the xpi, manually edit install.rdf and zip it again. But I, (and others) have spent hours trying to figure this out because no one assumes that this most basic functionallity is actually missing.

https://addons.mozilla.org/en-US/developers/docs/sdk/latest/dev-guide/tutorials/l10n.html

Thx!

Comment 8

[Erik Vold [:erikvold] (please needinfo? me)]

Sorry for the delay in getting to this.  I think Alex suggests in (comment 6) that we provide these localizations via the properties files using the method described here https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localizing_extension_descriptions.

This would help us achieve what Myk describes in comment 5 too I believe.

Comment 9

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/818

Looks good! we would need some tests here tho, we should probably just patch jpm to handle this use case though, we are going to try to terminate cfx.

https://github.com/mozilla/jpm

Sorry we didn't see this sooner :(

Comment 10

[TGOS]

> Looks good! we would need some tests here tho, we should probably just patch
> jpm to handle this use case though, we are going to try to terminate cfx.

Eric, if it looks good, just approve it. We developers are waiting for a fix almost three years and now there are two fixes. The one suggested here by Wes Kocher and the new one by kukushechkin (which I personally prefer); that last one is just waiting for approval. I don't care if cfx has a feature or if some tool named jpm will solve the problem one day (maybe in another 3 years). We developers need solutions to problems and we don't need them the day after tomorrow but the day before yesterday. Most of the time people here complain that nobody writes code, well, we have code, now it all fails because somebody fails to approve it.

Comment 11

[Dave Townsend [:mossop]]

(In reply to TGOS from comment #10)
> > Looks good! we would need some tests here tho, we should probably just patch
> > jpm to handle this use case though, we are going to try to terminate cfx.
> 
> Eric, if it looks good, just approve it. We developers are waiting for a fix
> almost three years and now there are two fixes. The one suggested here by
> Wes Kocher and the new one by kukushechkin (which I personally prefer); that
> last one is just waiting for approval. I don't care if cfx has a feature or
> if some tool named jpm will solve the problem one day (maybe in another 3
> years). We developers need solutions to problems and we don't need them the
> day after tomorrow but the day before yesterday. Most of the time people
> here complain that nobody writes code, well, we have code, now it all fails
> because somebody fails to approve it.

As a rule all changes must be accompanied by tests, this is important as it stops us from accidentally regressing functionality in the future. If a feature is important enough to fix then it is important enough that we shouldn't unintentionally break it. If you're willing to provide test code then we can land this change, otherwise it will have to wait for one of the team to do so.

Comment 12

[TGOS]

Okay, I re-implemented the fix, because I didn't like that the original fix used install.rdf names for localization keys instead of packages.json names (e.g. it used "addon_creator" instead of "addon_author"). Also the original fix broke two other unit tests, which I had to fix as well. Other than that my fix works pretty much the same. A unit test which tests for localized output in the install.rdf file is included.

https://github.com/mozilla/addon-sdk/pull/1594

I think not too bad for the first Python code I ever wrote myself.

Comment 13

[Dave Townsend [:mossop]]

Attachment: new patch

Up to Erik whether he wants to re-review the code for cfx

Comment 14

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8475332 [details] [review]
new patch

So the format that we want to support is defined here https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localizing_extension_descriptions#Localizing_before_Gecko_1.9

"addon_name" for example should be "extensions.EXTENSION_ID.name" and there needs to be a preference set like `pref("extensions.EXTENSION_ID.name", "PATH_TO_LOCALIZATION_FILE");`

Comment 15

[TGOS]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #14)

> So the format that we want to support is defined here
> https://developer.mozilla.org/en-US/docs/Mozilla/Localization/
> Localizing_extension_descriptions#Localizing_before_Gecko_1.9

Who is "we"? What you've been linking is the Gecko pre-1.9 method. Gecko 1.9 is Firefox 3 and SDK based add-ons don't support Firefox 2, so they always target a Gecko version for sure 1.9 or later.

To quote from that same page:

>> Gecko 1.9 includes a new, more robust method for localizing add-on
>> descriptions and other metadata. All of the different descriptions
>> now appear in the install.rdf file using em:localized properties. 
>> Each has at least one em:locale property marking the locale that 
>> the information should be used for and then all the various strings
>> for the locale.

So why exactly do you want to support a legacy localization scheme which I have not seen in use for ages at any existing add-on and that is only required for Firefox version that cannot even run SKD based add-ons produced by the cfx tool?

Despite the fact that it is pretty annoying to write string keys with the "extension ID" (which may change a couple of times during early development), this scheme uses the install.rdf field names, which do not match the package.json field names (which is even more annoying to most developers, especially add-on beginners that know package.json but have no idea what install.rdf is):

package.json -> install.rdf
---------------------------
title -> name
author -> creator
description -> description
homepage -> homepageURL

So why not using the "more robust" method, that add-on developers are using today, unfortunately by unpacking the XPI created by cfx, making the changes by hand and repacking it? (see comment 7)

First you complain no unit test but you said the code looks good. Now there is a unit test but all of a sudden the code that looked good before is wrong. If I do it exactly as you described, you'd find another excuse. Geeez, this bug is from 2011! You force us developers through this repacking hell for 3 years already, just because you have a great vision. People also want a working solution for add-ons hosted on their own webpages or elsewhere in the web, and they want be able to test localization locally, so a solution that relies on some automatic AMO localization service is no solution at all IMHO, because it is no generic solution suitable for everyone. Despite this bug is named "Let developers localize add-on metadata.", *developers* and not some unknown people using some AMO localization service.

Honestly, I give it up. Coding for Mozilla is the biggest waste of time ever. I've done it in the past for Firefox, many years ago and instead of taking my working code every user was happy with, the code was rejected out of reasons I'd call nitpicking and the bug has never been fixed up to today. From now on I will maintain my private cfx fork, implement the features I've been missing forever (I have some others, e.g. <em:targetPlatform> support) and keep using this one for add-on development. Alternatively, I may write a cfx-wrapper/post-processor, that later on "fixes" the generated XPIs (automatically unpack, modify and repack the content); advantage would be that it could also fix XPIs produced by jpm that will probably lack the same set of fundamental features as cfx. I'm sick waiting decades for the simplest bugs getting fixed and the simplest features getting implemented.

If anybody comes here and needs localization support for the install.rdf today, just download the current official cfx release, apply my patch to it (I can also provide a patched branch on request for download at GitHub) and use it. It works great, you will love it. No XPI repacking any longer. Don't wait for this bug to be fixed, it won't be fixed within the next three years either.

Comment 16

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1597

Comment 17

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/b55b79799c6fb2ab4c554578b7f73e7534247808
Fix for bug 661083: "Let developers localize add-on metadata."
https://bugzilla.mozilla.org/show_bug.cgi?id=661083

The package.json keys "title", "author", "description", and "homepage"
can be localized using the property file keys "addon_title",
"addon_author", "addon_description", and "addon_homepage".

Conflicts:
	python-lib/cuddlefish/rdf.py

https://github.com/mozilla/addon-sdk/commit/f1e469cd2765c4f00e97f9222dffc8cccda5f961
Bug 661083 using "extensions.<EXTENSION_ID>." instead of "addon_" as a prefix for key names

https://github.com/mozilla/addon-sdk/commit/7ba6834d74a413d50e902026cc568647c334b453
Bug 661083 adding a testaddon for the manifest localization

https://github.com/mozilla/addon-sdk/commit/2acf356359a5c311ba8255b4dff584b4d1fbb851
Merge pull request #1597 from erikvold/661083

Bug 661083 manifest localization r=@gozala

Comment 18

[Erik Vold [:erikvold] (please needinfo? me)]

Sorry we won't be releasing any new versions of cfx, jpm is the replacement https://www.npmjs.com/package/jpm

I've reverted this change in https://github.com/mozilla/addon-sdk/commit/0ccba4c256ada078cb2eff8fd77a8543b5a3ee87 so that this bug does not delay native-jetpacks (bug 915376)

Bug 1123428 has been opened for add-on manifest localization support for jpm.