Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support SPM localized resources #72

Merged
merged 9 commits into from
Feb 4, 2021
Merged

Support SPM localized resources #72

merged 9 commits into from
Feb 4, 2021

Conversation

iDevelopper
Copy link
Contributor

@iDevelopper iDevelopper commented Feb 1, 2021

  • Update SPM package definition to support localization
  • Add manual configuration example project
  • Add SPM configuration example project

Base automatically changed from master to main February 1, 2021 13:59
Copy link
Owner

@vtourraine vtourraine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for preparing this pull request 👍

Package.swift Outdated Show resolved Hide resolved
AcknowList.podspec.json Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Sources/AcknowLocalization.swift Show resolved Hide resolved
@iDevelopper
Copy link
Contributor Author

Hello @vtourraine,

What’s the reason for adding macOS? I’d love to support it too, but as of today, the library requires UIKit

I thought it was useful for Mac Catalyst, but I'm not sure now. I will test.

Moving the “resources” folder inside of “sources” seems counterintuitive. Is this required for SPM?

From the documentation:
https://developer.apple.com/documentation/swift_packages/localizing_package_resources

Where does this SWIFT_PACKAGE value come from? The last time I tried to add SPM support, there was no such mechanism to dynamically check whether the Bundle.module object was available or not.

https://github.com/apple/swift-package-manager/blob/main/Documentation/Usage.md#packaging-legacy-code

@iDevelopper
Copy link
Contributor Author

I pulled your changes. Why do I have this AcknowList group in red again? I had deleted it. An idea ?

image

@vtourraine
Copy link
Owner

Why do I have this AcknowList group in red again? I had deleted it. An idea ?

That’s because it’s currently mapped to the “Source” folder, which it can no longer find (renamed to “Sources”). You can select the folder in red, then click on the “location” folder button in the Inspectors pane, to pick a relevant folder.

Screenshot 2021-02-02 at 14 59 38

@iDevelopper
Copy link
Contributor Author

Yes but before your changes I had already done it. Is it related to the deletion of the xcworkspacedata?

@vtourraine
Copy link
Owner

Yes but before your changes I had already done it. Is it related to the deletion of the xcworkspacedata?

That’s possible, I think that depends on how you opened the Xcode project. The problem with the xcworkspacedata file in your original commit was that is was part of the .swiftpm meta project, which we don’t need here.

@vtourraine
Copy link
Owner

Where does this SWIFT_PACKAGE value come from? The last time I tried to add SPM support, there was no such mechanism to dynamically check whether the Bundle.module object was available or not.

https://github.com/apple/swift-package-manager/blob/main/Documentation/Usage.md#packaging-legacy-code

Oh wow, that’s awesome! I had never heard of this definition before, thanks for bringing this up!

@vtourraine
Copy link
Owner

Thanks for addressing my previous questions.

There’s just one last thing that I need to understand: is there a reason for rewriting the localizedString(forKey key: String, defaultString: String) function? The new implementation has a bit too many levels of conditionals to my taste, and the code was already finicky. Why can’t we just call your new resourcesBundle() method, and leave the rest of the function as-is?

@iDevelopper
Copy link
Contributor Author

I thought your function was a bit complicated and not very pretty! (Sorry!) :-) So I simplified it.

Why can’t we just call your new resourcesBundle() method, and leave the rest of the function as-is?

You are the owner!

@vtourraine
Copy link
Owner

Ha ha, that’s fair! If I remember correctly, I originally copy-pasted this code from another popular repo. I’m sure it could be improved, but for now I would prefer to minimize the impact of this pull request.

@vtourraine
Copy link
Owner

I’ll have another look at this part of the code, thanks for the feedback.

@iDevelopper
Copy link
Contributor Author

Do you think it could be simpler?

Add SPM example project
Add CocoaPods example project
@vtourraine
Copy link
Owner

Yes, significantly simpler!

Upon investigation, I’ve realized that the current localization implementation was based on old CocoaPods mechanisms. Still working, but there’s a better solution now with the resource_bundles key in the Podspec file.

The real difficulty here is to find a system that works for CocoaPods, SPM, and standalone Xcode project configuration (including Carthage). I think I’ve figured it out, following Apple’s SPM guidelines, with inspiration from https://github.com/TimOliver/TOCropViewController.

Please have a look at my latest commit. I’ve added dedicated example projects for SPM and CocoaPods, so it’s easier to check they all work as expected.

@iDevelopper
Copy link
Contributor Author

Yes, great!

This is what I wanted to do at the beginning following Apple's documentation (https://developer.apple.com/documentation/swift_packages/localizing_package_resources), but I mistakenly thought that you wanted to keep the AcknowList.bundle file!

Maybe it would be better to use the Pods-acknowledgments.plist in AcknowExampleCocoaPods too, no?

@iDevelopper
Copy link
Contributor Author

iDevelopper commented Feb 4, 2021

I don't quite understand why, but it's not working well. Try the Spanish language.

image

(lldb) po Bundle.module.preferredLocalizations
▿ 1 element

  • 0 : "fr"

image

@vtourraine
Copy link
Owner

With the example projects? I think that’s actually the correct behavior. These projects support English (as Base) and French localization. So the AcknowList title should not be localised if the phone is set in Spanish, for instance, because that would lead to half-translated apps.

You can try adding Spanish to the list of supported languages to see the difference.

Screenshot 2021-02-04 at 09 25 04

@vtourraine
Copy link
Owner

Maybe it would be better to use the Pods-acknowledgments.plist in AcknowExampleCocoaPods too, no?

I’ve updated the CocoaPods project to use its own acknowledgements file (Pods-AcknowExampleCocoaPods-acknowledgements.plist), which is probably a better example in this case.

@iDevelopper
Copy link
Contributor Author

You can try adding Spanish to the list of supported languages to see the difference.

Got it!

@vtourraine vtourraine merged commit 559f4a6 into vtourraine:main Feb 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants