vtourraine / AcknowList

Acknowledgements screen displaying a list of licenses, for example from CocoaPods and Swift Package Manager dependencies.
MIT License
784 stars 59 forks source link

Support SPM localized resources #72

Closed iDevelopper closed 3 years ago

iDevelopper commented 3 years ago
iDevelopper commented 3 years ago

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 commented 3 years ago

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

image
vtourraine commented 3 years ago

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 commented 3 years ago

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

vtourraine commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

iDevelopper commented 3 years ago

Do you think it could be simpler?

vtourraine commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

image

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

image
vtourraine commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

Got it!