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

Add option for changing windows icon #1

Merged
merged 1 commit into from
Apr 20, 2014
Merged

Add option for changing windows icon #1

merged 1 commit into from
Apr 20, 2014

Conversation

SamyPesse
Copy link
Contributor

This PR add an option winIco for changing the windows executable icon. This is working, I just test it using:

var NwBuilder = require('./');
var nw = new NwBuilder({
    files: '/Users/samypesse/Desktop/Projects/GitBook/editor/**/**', // use the glob format
    platforms: ['win'],
    winIco: '/Users/samypesse/Desktop/Projects/GitBook/editor/src/resources/images/icons/512.ico'
});

// Log stuff you you want
nw.on('log',  console.log);

// Build retruns a promise
nw.build().then(function () {
   console.log('all done!');
}).catch(function (error) {
    console.error(error);
});

I also fix a typo: "platform" instead of "plattform".

I have a PR pending on electron/node-rcedit#1 that adds the support of mac and linux to node-rcedit.

allDone.push(rcedit(
path.resolve(winPlatform.releasePath, _.first(winPlatform.files)),
{
icon: self.options.winIco
Copy link
Contributor

Choose a reason for hiding this comment

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

Does that work with relative paths? If not could you add a path.resolve

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should works fine with relative paths but I added a path.resolve just to be sure :)

@steffenmllr
Copy link
Contributor

Thanks for the PR and the the typo fix - in my defense it's spelled like this in German

@steffenmllr
Copy link
Contributor

Could you also pls squash your commits ?

@steffenmllr
Copy link
Contributor

@SamyPesse
Copy link
Contributor Author

Don't worry, I understand, I'm french ;)

I'll fix the travis issue and then squash my commit, but can I ask why do you want me to squash all my commits?

@SamyPesse
Copy link
Contributor Author

Ok, for travis, it's because we have to yet for the PR on node-rcedit to be merged since it's coffeescript.

@steffenmllr
Copy link
Contributor

it's just cleaner to have on PR eql one commit - german remember - but since this is alpha do and there is also typo fixing it's fine i'll test and merge it

@SamyPesse
Copy link
Contributor Author

Ok, I'll keep you update as soon as the other PR is merged.

When do you plan on updating grunt-node-webkit-builder with this module? And by the way: thank you for grunt-node-webkit-builder 👍

@steffenmllr
Copy link
Contributor

Thanks, there is a branch with the new module: https://github.com/mllrsohn/grunt-node-webkit-builder/blob/develp/tasks/node_webkit_builder.js

I want to release it as soon as possible, haven't had to time to test everything under linux + windows.

@steffenmllr
Copy link
Contributor

could you also please add an entry to the readme?

@SamyPesse
Copy link
Contributor Author

Done!

@ivantodorovich
Copy link
Contributor

Please merge this, I need the winIco feature 👍

Also, It might be good to update the npm package

@SamyPesse
Copy link
Contributor Author

We have to wait for electron/node-rcedit#1 to be merged.

@ivantodorovich
Copy link
Contributor

We could simply ignore winIco if process.platform !== 'win32', at least until that gets merged.

@steffenmllr
Copy link
Contributor

Going to publish the next version on npm next week - if it is not merged until then I'll consider doing a windows only version

@SamyPesse
Copy link
Contributor Author

OK, I can make it windows only right now and make another PR if/when electron/node-rcedit#1 is merged by the @atom team (@zcbenz or @kevinsawicki).

Another easy solution could be to use node-rcedit@0.1.2 dependency which contain the rcedit binary and run the binary from node-webkit-builder using the same code as this: https://github.com/SamyPesse/node-rcedit/blob/feature/maclinux/src/rcedit.coffee

@steffenmllr what do you prefer?

@steffenmllr
Copy link
Contributor

no rush, let's wait until next week. Also want to get the tests working on windows #5 until then.

@SamyPesse
Copy link
Contributor Author

The PR on node-rcedit has been merged!

@steffenmllr
Copy link
Contributor

@SamyPesse can you pls rebase master?

@SamyPesse
Copy link
Contributor Author

Done!

steffenmllr added a commit that referenced this pull request Apr 20, 2014
Add option for changing windows icon
@steffenmllr steffenmllr merged commit 206f33a into nwutils:master Apr 20, 2014
@steffenmllr
Copy link
Contributor

Thanks again for you PR

@steffenmllr
Copy link
Contributor

ping: @SamyPesse
#19

kessler pushed a commit that referenced this pull request May 28, 2018
[Snyk Update] New fixes for 3 vulnerable dependency paths
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants