Skip to content
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 for issue 300: --icon option does not work under Electron 0.37.4 #301

Merged
merged 1 commit into from
Apr 5, 2016

Conversation

erkyrath
Copy link

@erkyrath erkyrath commented Apr 3, 2016

#300

This is an easy fix. It writes the icon to the filename defined in CFBundleIconFile, so it will work in Electron 0.37.4 as well as earlier versions.

Since the test/config.json specifies "0.35.6", there's no way to test this fix directly. Is that okay, or should we figure out how to test both configurations? (If so, how?)

@malept
Copy link
Member

malept commented Apr 3, 2016

I suggest creating a new (separate) test fixture project just for test/mac.js that has the Electron version set to 0.37.4, then add a testcase for the new .icns behavior.

@malept malept added tests-needed Pull request needs tests build-target:mac 🍎 Bundling an Electron app specifically for macOS labels Apr 3, 2016
@erkyrath erkyrath force-pushed the issue-300-iconname branch from e9d9507 to 1d02394 Compare April 4, 2016 00:54
Add a separate fixture for electron 0.37.4.

Drop in the el-0374 test.

Add pre-test setup for 0.37.4 tests.
@erkyrath
Copy link
Author

erkyrath commented Apr 4, 2016

Okay, added fixture and test as described.

series([
function (cb) {
console.log('Calling electron-download before running tests...')
util.downloadAll(config.version, cb)
}, function (cb) {
console.log('Calling electron-download 0.37.4 before running tests...')
util.downloadAll('0.37.4', cb)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't need to download all of the Electron zips for 0.37.4, just the OSX ones.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, there's more special changes in this release (see #298).

@feross
Copy link
Contributor

feross commented Apr 5, 2016

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build-target:mac 🍎 Bundling an Electron app specifically for macOS tests-needed Pull request needs tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants