Skip to content
This repository has been archived by the owner on Jun 18, 2019. It is now read-only.

App still uses default Electron icon #23

Closed
conorsch opened this issue Jun 8, 2017 · 6 comments · Fixed by #24
Closed

App still uses default Electron icon #23

conorsch opened this issue Jun 8, 2017 · 6 comments · Fixed by #24

Comments

@conorsch
Copy link
Contributor

conorsch commented Jun 8, 2017

Despite new asset changes in #17, the built application still uses the default app icon for Electron:

sunder-default-electron-icon

Output of npm run dist on Mac includes:

⚠️ Application icon is not set, default Electron icon will be used

Notably that message does not display when building under Linux (including changes proposed in #22), but I also don't see a custom icon under Linux:

sunder-linux-default-electron-icon

So looks like we have some pathing issues to sort out. Will hit the docs.

@conorsch
Copy link
Contributor Author

conorsch commented Jun 8, 2017

Applying this diff removes the warning on Mac builds:

diff --git a/package.json b/package.json
index 21c409e..4f9a7e6 100644
--- a/package.json
+++ b/package.json
@@ -27,6 +27,9 @@
     "linux": {
       "icon": "build/icons/",
       "target": ["deb"]
+    },
+    "mac": {
+      "icon": "build/icons/app.icns"
     }
   },
   "bin": {

but the icon does not change. Quoth the docs:

The path to icon (.icns for MacOS and .ico for Windows), relative to build (build resources directory).

So likely a simple pathing problem.

@conorsch
Copy link
Contributor Author

conorsch commented Jun 8, 2017

The diff above seems to be what we want. The changes in #17 provided a Sunder-specific app icon (3457a3b), but that file is no longer in master. Must have been a mistake caused by my resolution of merge conflicts between #16 and #17.

Will try to pluck out of history and add again.

@GabeIsman
Copy link
Contributor

Is this diff still necessary? The Mac icon was working for me on #17 without. That is, I think build/icons/app.icns is the default location that electron-builder looks. Wouldn't hurt to be explicit of course!

@conorsch
Copy link
Contributor Author

conorsch commented Jun 9, 2017

@GabeIsman No, the diff above is superseded entirely by #24, which solves the real porblem, which was I screwed up a complicated merge and rewound the clock on your asset integration.

@GabeIsman
Copy link
Contributor

Gotcha. Closing this one then!

@conorsch
Copy link
Contributor Author

conorsch commented Jun 9, 2017

Still an issue on Linux—it's a quick fix, PR incoming.

@conorsch conorsch reopened this Jun 9, 2017
conorsch pushed a commit that referenced this issue Jun 9, 2017
The `icns2png` command was referencing the old path to the `app.icns`
file, which is no longer valid as of #17 and #24. Fixing that command
allows the icns -> png format conversion work as intended. No changes
required to `package.json`, since the paths there are already correct.

Closes #23.

Signed-off-by: Conor Schaefer <conor@freedom.press>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants