-
-
Notifications
You must be signed in to change notification settings - Fork 20
Add .icon (icon composer) icon support
#119
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
base: main
Are you sure you want to change the base?
Conversation
stackotter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! And thanks for documenting all your changes so nicely.
Most of my comments are just requesting small style changes or documentation wording/formatting tweaks.
My main question is regarding your changes to fixAndCopyResourceBundle. To summarise the relevant comment: why does using a layered icon (as opposed to an icns icon) require this additional resource bundle fixing logic? If it's necessary for the layered icon to work at all, then I think this should be handled differently; I don't like that layered icons are getting double handled (once in DarwinBundler to compile them there, and once here to compile them into every resource bundle with an asset catalog).
Sources/SwiftBundler/Bundler/SwiftPackageManager/ApplePlatform.swift
Outdated
Show resolved
Hide resolved
|
I'm happy with everything other than that addition ResourceBundler asset catalog logic now. I looked into it, and it seems like actool doesn't support taking files other than asset catalogs as inputs in compilation mode, so it doesn't seem like your approach to getting the layered icon into the Assets.car would work. Have you confirmed that it does in practice? Also, if the app doesn't have any resources, then there won't be a resource bundle to fix, and the icon won't end up in the app's Assets.car (it won't have one). And the icon will be inserted into every single resource bundle's Assets.car as far as I can tell, whereas I believe that you only want it in the main Assets.car file. As an aside, what additional functionality does putting the layered icon in the Assets.car do? I assume it's to allow iOS/macOS to relight the icon and add depth effects? But I dunno exactly where that would happen I think that ResourceBundler should insert the icon into the xcassets directory before compilation, and it should limit itself to only doing that when isMainBundle is true. Additionally, DarwinBundler should check for the presence of a main Assets.car file, and if it doesn't exist then it should synthesise an xcassets catalog with the layered icon and then compile it into the correct place. Also, it feels weird having layered icon specific logic in ResourceBundler. But inserting synthesised assets such as layered icons or accent colors into the main resource bundle's Assets.car feels like a generic operation, so maybe the API surface can take an array of 'synthesised assets' instead of an optional layered icon url. It would then insert those synthesised assets into the main bundle's xcassets catalog before compilation. And then if there isn't a main bundle it should synthesise one with those assets. Note that when inserting assets into the main bundle's xcassets, you may need to make a temporary copy as to not confuse swiftpm's build caching. |
…equirement when compiling icons
|
Thanks for the detailed review. I want to add more context about what I tested, because several of the concerns raised do not match the behavior I am seeing in practice.
I agree that thinking about a more general mechanism for synthesized assets could be interesting as a longer-term improvement. But this PR is focused on solving a concrete breakage that has been blocking layered icon support. Given the above, my view is that:
If there is a specific technical case I have not covered, I am happy to test it. Otherwise, I would prefer to land this fix so that layered icon support is unblocked. |
I know that it's synthesising an asset catalog when one doesn't exist, but it only does that if there is a main resource bundle in the first place. If the app doesn't have any resources, then I'm pretty sure (from some quick testing on my laptop) that SwiftPM doesn't produce a resource bundle for it, so this resource bundle fixing code doesn't get run.
I don't believe that's so. I know that the |
Closes #118
Copilot generated summary:
This pull request adds support for using
.iconfiles (layered icon format) as app icons on Apple platforms, in addition to the existing support for.pngand.icnsfiles. It introduces a newLayeredIconCreatorutility for converting.iconfiles to.icns, updates the resource bundling logic to handle these new icon types, and improves the documentation to reflect the expanded icon format support.Support for new icon formats:
.iconfiles as app icons on Apple platforms, alongside.pngand.icnsfiles, by updating thecompileAppIconmethod inDarwinBundlerto handle.iconfiles and call the newLayeredIconCreatorutility. [1] [2] [3]LayeredIconCreatorutility for converting.iconfiles to.icnsusingactool, along with a dedicated error type for handling related errors. [1] [2]Resource bundling and asset catalog improvements:
ResourceBundlerto compile asset catalogs and.iconfiles together when present, refactored parameters to use the fullBundlerContext, and improved handling of temporary files and resource copying. [1] [2] [3] [4] [5] [6]actoolbased on the Apple platform, ensuring proper icon generation for all supported targets.Documentation updates: