Fix localisation variant groups #70

ryohey · 2017-09-25T13:54:40Z

Add localized resources are added to one variant group.
described in #69

Create variant groups of the base localization
Add references to localized resources into base localization variant groups

yonaskolb · 2017-09-25T14:27:53Z

Thanks @ryohey, this is great!
Before I have a proper look at this, are you able to run swift test to generate the fixture project again and commit the changes (if there are any).

I actually just added a commit to master that fails the travis test if the generated fixture changes and wasn't committed, but it's after you created your branch

ryohey · 2017-09-25T14:50:07Z

OK thanks!
By the way I want to write some tests for this changes.
How do you think I can do that properly? (Is FixtureTests.swift good for that?)

yonaskolb · 2017-09-25T17:18:10Z

Yeah, FixtureTests would be a good place. You can make a new Fixture project, but I think it would just be easier to edit the current TestProject and localise the storyboard

yonaskolb · 2017-09-25T17:32:18Z

Sources/XcodeGenKit/PBXProjGenerator.swift

-            let fileReference = getFileReference(path: filePath, inPath: path)
-            groupChildren.append(fileReference)
+        // create variant groups of the base localisation first
+        var baseLocalisationVariantGroups = [PBXVariantGroup]()


minor: the rest of the codebase uses var baseLocalisationVariantGroups: [String] = [] syntax

yonaskolb · 2017-09-25T17:33:19Z

Sources/XcodeGenKit/PBXProjGenerator.swift

+        var baseLocalisationVariantGroups = [PBXVariantGroup]()
+        if let baseLocalisedDirectory = localisedDirectories.first(where: { $0.lastComponent == "Base.lproj" }) {
+            for filePath in try baseLocalisedDirectory.children() {
+                let variantGroup = PBXVariantGroup(reference: generateUUID(PBXVariantGroup.self, filePath.lastComponent),


You can pass the whole path.string into generateUUID so it's more deterministic

yonaskolb · 2017-09-25T17:35:28Z

Sources/XcodeGenKit/PBXProjGenerator.swift

+        if let baseLocalisedDirectory = localisedDirectories.first(where: { $0.lastComponent == "Base.lproj" }) {
+            for filePath in try baseLocalisedDirectory.children() {
+                let variantGroup = PBXVariantGroup(reference: generateUUID(PBXVariantGroup.self, filePath.lastComponent),
+                                                   children: Set(),


children can use []. This will also be changing to an array in the next xcodeproj version anyway tuist/XcodeProj#87

yonaskolb · 2017-09-25T17:38:51Z

Sources/XcodeGenKit/PBXProjGenerator.swift

+        var baseLocalisationVariantGroups = [PBXVariantGroup]()
+        if let baseLocalisedDirectory = localisedDirectories.first(where: { $0.lastComponent == "Base.lproj" }) {
+            for filePath in try baseLocalisedDirectory.children() {
+                let variantGroup = PBXVariantGroup(reference: generateUUID(PBXVariantGroup.self, filePath.lastComponent),


What happens if more than 1 target has these files in it's sources? Do the variant groups need to be cached like the groupsByPath are?

That's right. I will add the cache for variant groups.

yonaskolb · 2017-09-25T17:43:39Z

Sources/XcodeGenKit/PBXProjGenerator.swift

-                allFilePaths.append(path)
+                // find base localisation variant group
+                let name = filePath.lastComponentWithoutExtension
+                guard let variantGroup = baseLocalisationVariantGroups.first(where: { Path($0.name).lastComponentWithoutExtension == name }) else {


what if a localised file isn't inside a Base.lproj? Does this then not add the file reference anymore?

Yes It will do not add the reference. It can be a problem in the case that resources placed in en.lproj but not in Base.lproj. For example a case an user removed resource in Base.lproj but not one in en.lproj.
I will fix it.

If the project was created in the usual way, it looks like Base.lproj is included.

The project folder should contain one folder named Base.lproj and other language-specific folders with the .lproj extension. The prefix for the language folders is the language ID, as described in Language and Locale IDs.

https://developer.apple.com/library/content/documentation/MacOSX/Conceptual/BPInternational/MaintaingYourOwnStringsFiles/MaintaingYourOwnStringsFiles.html

ryohey · 2017-09-28T07:43:20Z

@yonaskolb I fixed issues you reviewed and I ran swift test then commit the updated project, but CI was failure. What should I do?

yonaskolb · 2017-09-28T08:33:00Z

Thats interesting. If you've really committed the current version of the generated project I'm not sure why it's generating a different on on travis.
One thing to make sure of is to not have the GeneratedProject open in Xcode, as Xcode will resave the project with some small formatting differences. If it's open close it and then run swift test again

ryohey · 2017-09-29T06:49:13Z

@yonaskolb Thanks I resolved. I used full path to generate UUID. I changed it to remove basePath. Could you review it?

ryohey · 2017-10-01T15:33:38Z

rebased onto master and resolve conflicts

ryohey · 2017-10-03T00:48:13Z

@yonaskolb are there any problems?

yonaskolb · 2017-10-03T21:11:12Z

Hi @ryohey, sorry I haven't had time to look at this in depth. I'll review it as soon as I have some time

toshi0383

👍👍

yonaskolb · 2017-10-13T11:10:22Z

Sources/XcodeGenKit/PBXProjGenerator.swift

            for path in try localisedDirectory.children().sorted { $0.lastComponent < $1.lastComponent } {
+                guard fileReferencesByPath[path] == nil else {


What happens when multiple targets share localised sources that aren't in a Base.lproj directory? It looks like the second target won't get a build file, as this path is skipped

You’re right. I will fix it.

@yonaskolb I fixed this. 8596091#diff-2f3c621620623f55e7ff42cc32198a10R540

yonaskolb · 2017-10-13T11:10:50Z

Sources/XcodeGenKit/PBXProjGenerator.swift

-                addObject(variantGroup)
+                // find base localisation variant group
+                let name = path.lastComponentWithoutExtension
+                let variantGroup = baseLocalisationVariantGroups.first(where: { Path($0.name).lastComponentWithoutExtension == name })


nitpick: could use trailing closure syntax here

yonaskolb · 2017-10-13T11:15:34Z

Tests/XcodeGenKitTests/FixtureTests.swift

+
+            guard let variableGroup = getVariableGroups(resourceName).first else { throw failure("Couldn't find the variable group") }
+
+            do {


Is this just for visual grouping? Otherwise the it function is throwing so you don't need to wrap in a do

Yes it's just for visual grouping.

ryohey · 2017-10-16T03:10:38Z

I rebased this branch, but the code change is only 8596091, which fixed the problem pointed out in the review.

yonaskolb · 2017-10-16T18:48:03Z

Nice one @ryohey! 🎉

keith · 2017-10-17T06:32:26Z

I noticed this only seems to work if the default localization is in Base.lproj, not if you just have one in en.lproj. I'm not really sure if that's expected, or if it's worth solving for *.lproj, but it might be.

keith · 2017-10-17T22:49:24Z

Actually looks like there might be another issue with this, I get this error on CI:

2017-10-17 14:19:02.197 xcodebuild[1357:7330] warning:  The file reference for "Base.lproj/Localizable.strings" is a member of multiple groups ("Localizable.strings" and "Localizable.strings"); this indicates a malformed project.  Only the membership in one of the groups will be preserved (but membership in targets will be unaffected).  If you want a reference to the same file in more than one group, please add another reference to the same path.

yonaskolb · 2017-10-20T11:59:29Z

@ryohey, are you able to take a look at this?

ryohey · 2017-10-21T16:28:42Z

@keith If only one * .lproj other than Base.lproj is included in the project, It does not create a Variant Group but add a reference to the file directly, so I think it's not a problem.

However, if there are only en.lproj and ja.lproj, for example, they are added directly, so there is a problem. It's necessary to disable Base Localization to fix the issue but the project currently output by XcodeGen is unconditionally enabled Base Internationalization by specifing Base in knownRegions.

XcodeGen/Sources/XcodeGenKit/PBXProjGenerator.swift

Line 126 in 09209a4

let knownRegions: [String] = ["en", "Base"]

(I just noticed that it might necessary to add the name of * .proj to knownRegions now)

If there is no base.lproj, it seems correct to use PBXProject's developmentRegion as Base Language, but developmentRegion is fixed to English, It may be necessary to adding syntax so that it can be specified developmentRegion from spec.

XcodeGen/Sources/XcodeGenKit/PBXProjGenerator.swift

Line 133 in 09209a4

developmentRegion: "English",

As for the CI warnings please let me know your project's spec because I could not reproduced.

keith · 2017-10-22T17:37:58Z

Here's an example project with the warning. You need to build with the newest commit of XcodeGen for it to work at all. Run make to see the warning output in the console.

XcodeGenLocalizationWarnings.zip

ryohey · 2017-10-23T03:03:17Z

@keith Thanks I got the warning.
fixed in #102

keith · 2017-10-23T03:09:56Z

Awesome thanks so much!

ryohey force-pushed the fix-localize branch from 7e158d5 to 91611e4 Compare September 25, 2017 14:39

yonaskolb reviewed Sep 25, 2017

View reviewed changes

ryohey force-pushed the fix-localize branch 3 times, most recently from c720d9e to d8aa1e2 Compare September 28, 2017 02:58

ryohey force-pushed the fix-localize branch 3 times, most recently from 7ddc1b2 to a5256f1 Compare September 29, 2017 06:33

ryohey force-pushed the fix-localize branch 2 times, most recently from a0c15a9 to 9cc24bb Compare October 1, 2017 15:28

toshi0383 mentioned this pull request Oct 4, 2017

Adding common Localizable.strings files #80

Closed

toshi0383 approved these changes Oct 4, 2017

View reviewed changes

yonaskolb reviewed Oct 13, 2017

View reviewed changes

ryohey added 4 commits October 16, 2017 11:51

fix localisation variant groups

2f2c89e

add test for variant groups

bc9c007

fix localized sources

8596091

update fixture

f2c440d

ryohey force-pushed the fix-localize branch from 9cc24bb to f2c440d Compare October 16, 2017 03:00

yonaskolb merged commit 98706a3 into yonaskolb:master Oct 16, 2017

keith mentioned this pull request Oct 16, 2017

Resource files included in fileGroups don't get correctly added to targets #90

Closed

ryohey mentioned this pull request Oct 17, 2017

Add localized resources to a variant group #69

Closed

ryohey mentioned this pull request Oct 23, 2017

fix duplicate file reference #102

Merged

yonaskolb mentioned this pull request Nov 10, 2017

Bug .lproj is broken if Base.lproj is missing #143

Closed

bonkey mentioned this pull request Oct 2, 2019

2.8.0: createIntermediateGroups puts localized resources to "Recovered References" #674

Closed

ikesyo mentioned this pull request Feb 7, 2020

Localized strings resources are not detected correctly #763

Open

		for path in try localisedDirectory.children().sorted { $0.lastComponent < $1.lastComponent } {
		guard fileReferencesByPath[path] == nil else {


		guard let variableGroup = getVariableGroups(resourceName).first else { throw failure("Couldn't find the variable group") }

		do {

Fix localisation variant groups #70

Fix localisation variant groups #70

Conversation

ryohey commented Sep 25, 2017

yonaskolb commented Sep 25, 2017

ryohey commented Sep 25, 2017

yonaskolb commented Sep 25, 2017

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryohey commented Sep 28, 2017

yonaskolb commented Sep 28, 2017

ryohey commented Sep 29, 2017

ryohey commented Oct 1, 2017

ryohey commented Oct 3, 2017

yonaskolb commented Oct 3, 2017

toshi0383 left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryohey commented Oct 16, 2017

yonaskolb commented Oct 16, 2017

keith commented Oct 17, 2017

keith commented Oct 17, 2017 • edited Loading

yonaskolb commented Oct 20, 2017

ryohey commented Oct 21, 2017

keith commented Oct 22, 2017

ryohey commented Oct 23, 2017

keith commented Oct 23, 2017

keith commented Oct 17, 2017 •

edited

Loading