Sort Working Files locale-aware #8971

Merged
merged 7 commits into from Sep 24, 2014

Projects

None yet

3 participants

@MarcelGerber
Member

For #8955.
Tested with German and Swedish locale.

@pthiess
Member
pthiess commented Sep 5, 2014

@ingorichter Looks like a good enhancement to me.

@MarcelGerber MarcelGerber and 1 other commented on an outdated diff Sep 5, 2014
src/file/FileUtils.js
@@ -459,14 +459,15 @@ define(function (require, exports, module) {
function compareFilenames(filename1, filename2, extFirst) {
var ext1 = getFileExtension(filename1),
ext2 = getFileExtension(filename2),
- cmpExt = ext1.toLocaleLowerCase().localeCompare(ext2.toLocaleLowerCase(), undefined, {numeric: true}),
+ lang = brackets.getLocale() === "root" ? "en-us" : brackets.getLocale(),
@MarcelGerber
MarcelGerber Sep 5, 2014 Member

I wonder if we should make this a function in LocalizationUtils? May be useful elsewhere.

@ingorichter
ingorichter Sep 5, 2014 Contributor

Yes, that sounds like a good idea. Perhaps there are other use cases for it.

@MarcelGerber
MarcelGerber Sep 5, 2014 Member

One is probably #8977 :) I'll change it soon.

@MarcelGerber
MarcelGerber Sep 5, 2014 Member

Oops. I actually found out that brackets.getLocale() already returns "en" instead of "root"...

@MarcelGerber MarcelGerber commented on the diff Sep 5, 2014
src/extensibility/ExtensionManagerView.js
@@ -230,6 +230,9 @@ define(function (require, exports, module) {
context.translated = true;
context.translatedLangs =
info.metadata.i18n.map(function (value) {
+ if (value === "root") {
@MarcelGerber
MarcelGerber Sep 5, 2014 Member

This is a little offtopic, but it normalizes "i18n": ["root"] to "i18n": ["en"] in package.json

@ingorichter

Does this by intent return only en instead of en-us?

Owner

Yes, en-us can't be translated as the only nls entry is LOCALE_EN.

@ingorichter ingorichter was assigned by pthiess Sep 15, 2014
@ingorichter
Contributor

@MarcelGerber could you please add a couple of tests to FileUtils-tests.js to verify that this fixes #8955? Thanks.

@MarcelGerber
Member

Added unit tests.

@ingorichter ingorichter and 1 other commented on an outdated diff Sep 24, 2014
test/spec/FileUtils-test.js
@@ -182,5 +182,23 @@ define(function (require, exports, module) {
expect(FileUtils.getSmartFileExtension("foo.bar.php.scss.erb")).toBe("php.scss.erb");
});
});
+
+ describe("compareFilenames", function () {
+
@ingorichter
ingorichter Sep 24, 2014 Contributor

nit: would you mind removing all these empty lines after the describe function? Thanks.

@MarcelGerber
MarcelGerber Sep 24, 2014 Member

That's actually rather common in the Brackets code...

@ingorichter ingorichter commented on an outdated diff Sep 24, 2014
test/spec/FileUtils-test.js
@@ -182,5 +182,23 @@ define(function (require, exports, module) {
expect(FileUtils.getSmartFileExtension("foo.bar.php.scss.erb")).toBe("php.scss.erb");
});
});
+
+ describe("compareFilenames", function () {
+
+ it("should compare filenames using German rules", function () {
+ spyOn(brackets, "getLocale").andReturn("de-DE");
+ // Should be like this: Äckerman, Adler, Rossi, Xavier
+ expect(FileUtils.compareFilenames("Äckerman", "Adler", false)).toBeLessThan(0);
+ expect(FileUtils.compareFilenames("Adler", "Rossi", false)).toBeLessThan(0);
+ expect(FileUtils.compareFilenames("Rossi", "Xavier", false)).toBeLessThan(0);
+ });
+ it("should compare filenames using Swedish rules", function () {
@ingorichter
ingorichter Sep 24, 2014 Contributor

nit: please a line break here.

@ingorichter
Contributor

@MarcelGerber I'm done with the review. Looks good to me and is definitely a great improvement.

@MarcelGerber
Member

@ingorichter I'm done. Please let me know if that newline should be removed.

@ingorichter
Contributor

@MarcelGerber we need to get better with coding and formatting conventions in general. Thanks for changing this.

@ingorichter ingorichter merged commit 9abc718 into adobe:master Sep 24, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@MarcelGerber MarcelGerber deleted the MarcelGerber:workingset-sort-locale branch Sep 24, 2014
@pthiess pthiess added this to the Release 0.44 milestone Sep 25, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment