-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
fix(app): Electron - missing icons folder in build (#11087) #11088
Conversation
The fixes include: 1. 'process.platform' is undefined if `nodeIntegration` is not true. Fall back to using `os.platform()` 2. Resolve which icon to use based on platform Electron is running on 3. Inject icon path into BrowserWindow (with previous commit this now works with both dev and build)
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.
Not ok.
- If there would be something to copy then it should be hooked into CopyWebpackPlugin in create-chain.js.
- Is it really always needed? electron-builder embeds the icons automatically into the executable (but can't remember what it does on Linux);
- Is this related to building on Linux? Is there something we can do to alleviate the Linux icons problem? Then add it to the electron template.
- Please test behavior with and without the changes for electron-packager and electron-builder.
Done. Using CopyWebpackPlugin to copy the icons.
You are referring to this code: cfg.electron = merge({
packager: {
asar: true,
icon: appPaths.resolve.electron('icons/icon'), // <-- here
overwrite: true
},
builder: {
appId: 'quasar-app',
icon: appPaths.resolve.electron('icons/icon'), // <-- here
productName: this.pkg.productName || this.pkg.name || 'Quasar App',
directories: {
buildResources: appPaths.resolve.electron('')
}
} This does not work with Ubuntu 20.04.3 or Windows (11). Here, I even tried being more specific by adding the file extension.
This is related to building with both Linux (Ubuntu) and Windows (11). Given enough time, I can set up our Mac for development and also test that. However, if the previous code no longer works as expected, this works 100% for Linux and Windows (and I assume Mac).
Tested both Here is some more information after further research: Builder: Also, there's this note:
This implies that the icon used currently is for the file output (exe, setup, etc) and not specifically the tray icon. Based on this information I tried this: builder: {
appId: 'quasar-app',
linux: {
icon: appPaths.resolve.electron('icons/linux-512x512.png')
},
win: {
icon: appPaths.resolve.electron('icons/icon.ico')
},
mac: {
icon: appPaths.resolve.electron('icons/icon.icns')
},
// icon: appPaths.resolve.electron('icons/icon'),
productName: this.pkg.productName || this.pkg.name || 'Quasar App',
directories: {
buildResources: appPaths.resolve.electron(''),
files: ['icons/icon.', 'icons/linux-512x512.png']
}
} Which did not work. Packager: https://electron.github.io/electron-packager/main/interfaces/electronpackager.options.html#icon
|
I don't know how to get rid of the "changes requested" flag. :( |
This fix should get rid of this PR: #10142 |
Also, if you create a clean Electron app under Ubuntu, then
|
One last thing. This is what happens when you build under Ubuntu 20.04.3 using "builder" The bottom vscode is where the build occurred. The top vscode is showing an issue with When I comment out those two lines in |
Merged and pushed another commit on top of this to add to CopyWebpackPlugin from the main electron build config. |
Fixes #11087