fix Issue #8953- "Localizing package metadata in Extension Manager" #8987

bchintx · 2014-09-05T16:22:17Z

fix Issue #8953- "Localizing package metadata in Extension Manager"

This change adds Extension Manager support for localized extension package metadata. If the localized property is present (ie. property matching the short language name), then the additional provided properties will overlay any otherwise-defined properties. These localized strings will then be displayed in the Extension Manager dialog for those extensions that provide them.

note: The localized content is completely optional.

For example, in this code sample, the French title and description properties would be overlayed with the given, localized strings.

{
    "name": "localization-example",
    "title": "Localization Example",
    "description": "A guide on how to localize your extension.",
    ...
    "i18n": [
        "en", 
        "fr" 
    ],
    "fr": {
        "title": "fr title to replace the root title",
        "description": "fr description to replace the root description"
    }
}

…n package metadata.

marcelgerber · 2014-09-05T18:38:51Z

src/extensibility/ExtensionManagerView.js

+        var lang      = brackets.getLocale(),
+            shortLang = lang.split("-")[0];
+
+        // Extension metadata might have localized content.  If so, overlay those strings.


nit: two spaces

@marcelgerber Do you mean the spaces separating the two sentences? Ok.

marcelgerber · 2014-09-05T18:44:00Z

Hm, I wonder if we should have something like navigator.languages in Brackets (maybe in LocalizationUtils). It would make such use cases a lot easier.
Use cases I can think of:

UpdateNotification
package.json "i18n" parsing
package.json localization
...

marcelgerber · 2014-09-05T18:47:42Z

src/extensibility/ExtensionManagerView.js

+            var prop;
+            for (prop in info.metadata[shortLang]) {
+                if (info.metadata[shortLang].hasOwnProperty(prop)) {
+                    info.metadata[prop] = info.metadata[shortLang][prop];


For keywords, it's probably best to leave the original in so you can search in both languages.

Good idea. Will do.

bchintx · 2014-09-06T00:18:38Z

@marcelgerber navigator.languages looks like a good idea, but probably more of a bigger user story to tackle.

Pushed code changes per review. Ready for review.

le717 · 2014-09-06T00:20:03Z

src/extensibility/ExtensionManagerView.js

+            var key = useShortLang ? shortLang : lang,
+                prop;
+            for (prop in info.metadata[key]) {
+                if (info.metadata[key].hasOwnProperty(prop)) {


The coding conventions recommend using _.forEach() or _.some() instead of for-in.

https://github.com/adobe/brackets/wiki/Brackets-Coding-Conventions#apis-to-use-or-avoid

Good catch!

…age-metadata

bchintx · 2014-09-06T05:49:26Z

Great suggestion, @marcelgerber.
Pushed requested code changes. Ready for review.

marcelgerber · 2014-09-06T22:54:29Z

src/extensibility/ExtensionManagerView.js

+            _.forEach(info.metadata[key], function (value, prop) {
+                if (prop !== "keywords") {
+                    // overlay existing string w/ localized string
+                    info.metadata[prop] = info.metadata[key][prop];


You can now use value instead of info.metadata[key][prop]. Same below.

Ah, good catch! Will do.

marcelgerber · 2014-09-07T16:16:45Z

LGTM. Works great in practice.

TomMalbran · 2014-09-07T19:55:14Z

src/extensibility/ExtensionManagerView.js

+            shortLang       = lang.split("-")[0],
+            useShortLang    = info.metadata.hasOwnProperty(shortLang);
+        if (useShortLang || info.metadata.hasOwnProperty(lang)) {
+            var key = useShortLang ? shortLang : lang;


I think that we should replace the properties for both the short and the long language. Lets say that for some reason the title and description are in Spanish, so I then provide a title and description for "en", and I also provide just a title for "en-uk". If the language is "en-uk" we could first replace the title and description from "en" (as it is the shortLang) and then replace the title from "en-uk".

[shortLang, lang].forEach(function (locale) { if (info.metadata.hasOwnProperty(locale)) { _.forEach(info.metadata[locale], function (value, prop) { // Do the replacement ... }); } });

Oh? I hadn't encountered that before, so thanks for the suggestion. I'll add that.

bchintx · 2014-09-10T04:05:39Z

@peterflynn You mentioned earlier today that you had some concerns about the structure of the properties in the package.json? Can you please elaborate, and I can make the necessary changes. Thanks!

marcelgerber · 2014-09-10T12:07:42Z

Hm, it would be best to let the user search for both the original and the translated title & description, but that's probably out of scope.

peterflynn · 2014-09-11T03:28:14Z

src/extensions/samples/LocalizationExample/README.md

@@ -48,7 +48,7 @@ Move this plugin to the extensions\user\ folder to run the plugin. It will add a

 * main.js – loads the Strings module for the plugin and uses mustache to localize html content

-* package.json - add the translation languages as in the example: `"i18n: ["en", "fr" ]`
+* package.json - add the translation languages as in the example: `"i18n: ["en", "fr" ]`.  Also, add any localized metadata for displayed metadata in the Extension Manager, as in the example: `"fr": { "title": "localized title" }`.


In my proposed changes in #9028, this text is basically going away. It's most important to update the official package.json format docs, though.

Gotcha. Once this is merged, I can definitely update the official docs.

peterflynn · 2014-09-11T03:48:11Z

Some higher-level feedback...

1) We probably shouldn't allow fields like name (extension id) or version or engines to vary by locale, because it could create weird and unpredictable behavior if user switches locales. Also, some of these fields are covered by package validation and it could create loopholes if the values could be overridden post-validation.

I think we should have a whitelist of which properties can be overridden. Maybe just title, description, homepage, keywords?

2) Why special-case keywords such that searches cut across locales, without doing that for other fields like name & description? It's also odd that we single out the root language for special treatment -- if an extension has 5 translations, this code only searches two: the user's locale, and the original locale the author used when writing the extension. What makes that one more special than the remaining ones?

To keep it simple, I'd suggest we not try to do any special merging or concatenating at this point, for any properties.

3) Having all the locales sit as top-level keys in package.json risks having collisions, or at least is less structured than it could be. How about we put everything underneath a single top-level key, something like this:

{
    "name": "localization-example",
    "title": "Localization Example",
    "description": "A guide on how to localize your extension.",
    ...
    "i18n": [ "en", "fr" ],
    "package-i18n": {
        "fr": {
            "title": "fr title to replace the root title",
            "description": "fr description to replace the root description"
        }
    }
}

…adata

… root language

bchintx · 2014-09-12T03:18:14Z

@peterflynn Great observations. I can definitely do the first two.

For the third, is there a reason why you propose the additional i18n array? ie. do you foresee a need for the included languages?

bchintx · 2014-09-12T03:20:14Z

Whoops -- ignore that last question re: the third. I see what you're referring to in your earlier inline comment.

peterflynn · 2014-09-13T00:49:52Z

src/extensibility/ExtensionManagerView.js

@@ -28,7 +28,8 @@
 define(function (require, exports, module) {
    "use strict";

-    var Strings                   = require("strings"),
+    var _                         = require("thirdparty/lodash"),


Nit: no longer required

Whoops. Nice catch.

peterflynn · 2014-09-13T00:58:14Z

src/extensibility/ExtensionManagerView.js

+        var lang            = brackets.getLocale(),
+            shortLang       = lang.split("-")[0];
+        [shortLang, lang].forEach(function (locale) {
+            if (info.metadata["package-i18n"] !== undefined &&


Nit: this should just be if (info.metadata["package-i18n"] (without undefined check). Could also move that clause outside the forEach() if you want...

Good suggestions. Will do.

peterflynn · 2014-09-13T00:58:59Z

@bchintx Looks good -- just two small nits.

bchintx · 2014-09-15T16:13:24Z

I'd like to leave the property called package-i18n, rather than package-nls. With the already existing i18n property, using package-i18n IMHO helps to tie the connection between the two.

Pushing the rest of the requested code review changes.

bchintx · 2014-09-15T17:37:31Z

Added unit tests to confirm that the translated metadata actually appears in the Extension Manager view.

@peterflynn Ready for final review.

peterflynn · 2014-09-15T23:48:23Z

test/spec/ExtensionManager-test.js

+                runs(function () {
+                    spyOn(view.model, "dispose").andCallThrough();
+                });
+            }


This duplicates the code in describe("ExtensionManagerView") -- could you just hoist up that existing function so it can be called by both test suites?

Good call. I was trying out the Code Folding extension for the first time, and didn't notice that I had forgotten about copy-pasting from above.

peterflynn · 2014-09-15T23:50:55Z

@bchintx Sorry, one last request to reduce code duplication in the unit tests

peterflynn · 2014-09-15T23:51:33Z

test/spec/ExtensionManager-test.js

+                }
+            });
+
+            describe("when showing registry entries-i18n", function () {


Nit: we don't need another level of describe() here

ok. Was just matching the code block above.

bchintx · 2014-09-19T04:59:29Z

@peterflynn pushed requested changes. Thanks for the review.

peterflynn · 2014-09-19T08:25:53Z

@bchintx I had in mind to avoid duplicating toHaveText too... I've just added a commit to clean that up too -- hope you don't mind! I think this is good to merge now, and that last change is small enough that I don't think we need another round of review. Let me know if anything looks amiss in my commit post-facto!

fix Issue #8953- "Localizing package metadata in Extension Manager"

bchintx · 2014-09-19T18:14:58Z

Cool. Thanks, @peterflynn

bchintx added 3 commits September 5, 2014 11:06

fix Issue #8953- add Extension Manager support for localized extensio…

7cf0b40

…n package metadata.

update localization example for reference

8d47643

update localization readme for reference

d08d557

bchintx mentioned this pull request Sep 5, 2014

Localizing package metadata in Extension Manager #8953

Closed

marcelgerber reviewed Sep 5, 2014
View reviewed changes

as requested per code review

fbf682e

le717 reviewed Sep 6, 2014
View reviewed changes

bchintx added 2 commits September 6, 2014 00:47

changes per code review

dd6d579

Merge remote-tracking branch 'origin/master' into bchin/localize-pack…

019edbb

…age-metadata

marcelgerber reviewed Sep 6, 2014
View reviewed changes

TomMalbran reviewed Sep 7, 2014
View reviewed changes

per code review, overlay first by shortlang, then by lang.

da86d28

peterflynn reviewed Sep 11, 2014
View reviewed changes

bchintx added 2 commits September 11, 2014 16:12

Merge remote-tracking branch 'origin' into bchin/localize-package-met…

6aa3cea

…adata

whitelisting properties to overlay; remove keyword concatenation with…

cb01a98

… root language

add top-level key to package.json to avoid collisions

a7c8238

peterflynn reviewed Sep 13, 2014
View reviewed changes

marcelgerber mentioned this pull request Sep 14, 2014

Introduce LocalizationUtils.languages, .userLanguages #9104

Open

changes per code review

117b20c

adding unit tests

05d802d

peterflynn reviewed Sep 15, 2014
View reviewed changes

peterflynn self-assigned this Sep 17, 2014

changes as requested per code review

8fddc38

Further consolidate duplicated code in a unit test

c973447

peterflynn added a commit that referenced this pull request Sep 19, 2014

Merge pull request #8987 from adobe/bchin/localize-package-metadata

d1c8ce1

fix Issue #8953- "Localizing package metadata in Extension Manager"

peterflynn merged commit d1c8ce1 into master Sep 19, 2014

peterflynn deleted the bchin/localize-package-metadata branch September 19, 2014 08:26

peterflynn mentioned this pull request Nov 17, 2014

No substitution, author term when the language changes. #9933

Closed

fix Issue #8953- "Localizing package metadata in Extension Manager" #8987

fix Issue #8953- "Localizing package metadata in Extension Manager" #8987

Conversation

bchintx commented Sep 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelgerber commented Sep 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bchintx commented Sep 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bchintx commented Sep 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelgerber commented Sep 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bchintx commented Sep 10, 2014

marcelgerber commented Sep 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterflynn commented Sep 11, 2014

bchintx commented Sep 12, 2014

bchintx commented Sep 12, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterflynn commented Sep 13, 2014

bchintx commented Sep 15, 2014

bchintx commented Sep 15, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterflynn commented Sep 15, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bchintx commented Sep 19, 2014

peterflynn commented Sep 19, 2014

bchintx commented Sep 19, 2014