-
-
Couldn't load subscription status.
- Fork 1.8k
feat: support Icon Composer icons for macOS #9279
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: master
Are you sure you want to change the base?
feat: support Icon Composer icons for macOS #9279
Conversation
🦋 Changeset detectedLatest commit: 0ef3fde The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
would this support shipping both a |
@Kilian Do you know if those are separate "fields" perceived by mac? e.g., if we package both an I haven't gotten around to updating my mac to Tahoe yet as I'm afraid it'll brick my current dev environment while I'm mid-project. This is indeed on the todo list though |
|
@mmaietta @Kilian I have tested on macOS Sequoia and with both the Update 1:
The plist property seems to be available since macOS 10.13+, which should mean that the Liquid Glass icon will be visible since that macOS version. Update 2: Tested with macOS Monterey and confirmed that it does show there.
|
|
Thank you @iamEvanYT ! In this case then, I'd like to refactor this PR to instead leverage the Is there a minimum xcode or macos version required for supporting this CLI tool? Wondering if we need to throw a more descriptive error if the tool command doesn't exist |
|
@mmaietta I did a few commits a few days ago to change to the |
|
This is looking solid! Any chance we can add a |
|
@mmaietta There, the tests passed on my computer, so it should theoretically pass on the CI. However, it may not pass as it still uses the old Xcode SDK and not Xcode 26. In that case, we might have to change the CI runner from Edit: Huh, it seems like the I've added a commit that runs the macIconTests on |
|
I took a look at Thank you for your great work on this PR and I appreciate your flexibility in working through these iterations |
|
FYI, this LGTM, but I need to fix the core |
|
I tried this out and fixed the following problems:
Feel free to cherry-pick from c4ccbb5 Other problems I ran into and worked around:
|
|
@erikolofsson, thanks for helping test out this PR!
Good catch, thank you!
This isn't needed as the Liquid Glass icon will show on macOS versions 10.13+ and the latest chromium versions won't support macOS with such old versions. Screenshot from Latest Chromium Version's
I agree that the macOS version check could be removed, but the actool version check shouldn't, as removing it could lead to unexpected errors.
What was the problem? How did you work around it?
I didn't know that it generats a
What difference does this change make? Does icons not show on older versions with this set to |
Ok, granted that it works. You can however look at the partial plist that actool generates, where it seems Apple thinks you should set the CFBundleIconFile and include the icns file in Resources. Also if you don't set it you end up with this, which seems unecessary: Could you also use older Electron versions with electron-builder where older OSes are supported? When you try to run it on an older OS version you still see the icon with a stop sign overlayed, so that would also be a scenario. The oldest VM I have is 10.13, so I can't check what happens on older OSes.
If you have macOS 15 to try on you can try to debug it, but it could also have been the environment I was running in, maybe you can run the check after actool has failed instead and do it as an extra explanation? Because now it could be an unnecessary source of errors if actool would have worked.
It tried to extract an icon from the file in mac.icon it seems. Got the same error when trying to generate a DMG without setting dmg.icon. I had it set as: My workaround was to create separate config file when I built for Linux which set mac.icon to icon.png.
I tried it out now and it seems that it generates the .icns file anyway when set to 26.0. You should probably set it to the version supported by Electron bundled in the build. |
Example: |
|
Quite a conversation happening here. @iamEvanYT, is this ready for review? |
|
@mmaietta not yet, i'll try to get the improvements ready as soon as possible. |
|
@erikolofsson I should have fixed most of the problems that you mentioned now. Thank you for flagging them!
@erikolofsson Would you mind testing the linux builds for me? I don't have a linux machine so I couldn't. @mmaietta The PR ready for review now. |
|
We end up with this: <key>CFBundleIconFile</key>
<string>icon.icns</string>
<key>CFBundleIconName</key>
<string>Icon</string>It doesn't match up with convention and what is output from the partial plist where Also the version parsing is still failing on my macOS 15 build server: • spawning command=actool --version
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<key>com.apple.actool.version</key>
<dict>
<key>bundle-version</key>
<string>24128</string>
<key>short-bundle-version</key>
<string>26.0.1</string>
</dict>
</dict>
</plist>
• exited command=actool code=0 pid=8366
[xmldom error] invalid doc source
@#[line:0,col:undefined]
⨯ Cannot read properties of undefined (reading 'documentElement') failedTask=build stackTrace=TypeError: Cannot read properties of undefined (reading 'documentElement')
at Object.parse (.../node_modules/plist/lib/parse.js:68:9)
at generateAssetCatalogForIcon (.../node_modules/app-builder-lib/src/util/macosIconComposer.ts:12:29)
at processTicksAndRejections (node:internal/process/task_queues:105:5)
at MacPackager.applyCommonInfo (.../node_modules/app-builder-lib/src/macPackager.ts:558:44)
at createMacApp (.../node_modules/app-builder-lib/src/electron/electronMac.ts:118:3)
at beforeCopyExtraFiles (.../node_modules/app-builder-lib/src/electron/ElectronFramework.ts:85:5)
at MacPackager.doPack (.../node_modules/app-builder-lib/src/platformPackager.ts:321:7)
at MacPackager.doPack (.../node_modules/app-builder-lib/src/macPackager.ts:156:9)
at MacPackager.pack (.../node_modules/app-builder-lib/src/macPackager.ts:254:9)
at Packager.doBuild (.../node_modules/app-builder-lib/src/packager.ts:507:11) |
|
This fixes the macOS 15 version parsing: Also universal builds are broken again. Guessing because of this: The problem is that the arm64 build now builds without the .icon file so Info.plist is different between the builds. |
@erikolofsson For the For the other issues, are they fixed now? |
macOS builds correctly now. Still have the Linux failure: I'm building the Linux build from a macOS host |
@erikolofsson Does it work now? |
Yes, everything works now. |
|
@mmaietta Any idea what's causing the tests to fail? I don't think I have changed anything relating to those. |




closes #9254
closes #9278
This PR updates the "icon" property to the electron-builder configuration which adds support for icons made with Icon Composer (
.iconformat).On building, it will create an
Assets.carwith the icon which will be bundled inside the app's Resources folder.