macPlist option improvements #96

Merged
merged 1 commit into from Nov 13, 2014

Conversation

Projects
None yet
4 participants
@bastimeyer
Contributor

bastimeyer commented Oct 27, 2014

This change makes it easier if you want to add a single value to the Info.plist file. Right now, the only choice you have is replacing the whole plist file, which is inconvenient.
I've changed the editPlist utility method, so you can use an object with custom values which will overwrite the parsed ones.

@bastimeyer

This comment has been minimized.

Show comment
Hide comment
@bastimeyer

bastimeyer Nov 3, 2014

Contributor

Could I please get a response? 😕

Contributor

bastimeyer commented Nov 3, 2014

Could I please get a response? 😕

@adam-lynch

This comment has been minimized.

Show comment
Hide comment
Member

adam-lynch commented Nov 3, 2014

@steffenmllr

This comment has been minimized.

Show comment
Hide comment
@steffenmllr

steffenmllr Nov 3, 2014

Contributor

LGTM - it would be great if the object properties are documented in the readme

Contributor

steffenmllr commented Nov 3, 2014

LGTM - it would be great if the object properties are documented in the readme

@bastimeyer

This comment has been minimized.

Show comment
Hide comment
@bastimeyer

bastimeyer Nov 3, 2014

Contributor

@steffenmllr
Are you talking about the appName, appVersion, copyright, etc. properties? I don't think that these custom auxiliary properties are a good idea at all. I didn't touch those, but in my opinion these should be removed.
Right now, you're unable to set the CFBundleDisplayName property, because it will be overwritten by appName later on (see index.js#387 and utils.js#159).
It would be much better to create a default object with the needed CFBundleName, CFBundleDisplayName, CFBundleVersion, CFBundleShortVersionString and NSHumanReadableCopyright properties. Then it can be modified by all user defined properties instead of using auxiliary properties.

Do you want me to make further changes and update my pull request? (I've also seen, that I messed up some line endings in the readme file, so let me fix these too)

Contributor

bastimeyer commented Nov 3, 2014

@steffenmllr
Are you talking about the appName, appVersion, copyright, etc. properties? I don't think that these custom auxiliary properties are a good idea at all. I didn't touch those, but in my opinion these should be removed.
Right now, you're unable to set the CFBundleDisplayName property, because it will be overwritten by appName later on (see index.js#387 and utils.js#159).
It would be much better to create a default object with the needed CFBundleName, CFBundleDisplayName, CFBundleVersion, CFBundleShortVersionString and NSHumanReadableCopyright properties. Then it can be modified by all user defined properties instead of using auxiliary properties.

Do you want me to make further changes and update my pull request? (I've also seen, that I messed up some line endings in the readme file, so let me fix these too)

@bastimeyer

This comment has been minimized.

Show comment
Hide comment
@bastimeyer

bastimeyer Nov 6, 2014

Contributor

PR is updated and squashed...
I would really appreciate it if you could merge these changes and bump the version of this and the grunt repo. Thanks!

Contributor

bastimeyer commented Nov 6, 2014

PR is updated and squashed...
I would really appreciate it if you could merge these changes and bump the version of this and the grunt repo. Thanks!

@steffenmllr

This comment has been minimized.

Show comment
Hide comment
@steffenmllr

steffenmllr Nov 6, 2014

Contributor

@adam-lynch could you please take a look at it and merge it if you are ok with it

Contributor

steffenmllr commented Nov 6, 2014

@adam-lynch could you please take a look at it and merge it if you are ok with it

@bastimeyer bastimeyer referenced this pull request in streamlink/streamlink-twitch-gui Nov 9, 2014

Closed

Fails to find livestreamer #24

@adam-lynch

This comment has been minimized.

Show comment
Hide comment
@adam-lynch

adam-lynch Nov 9, 2014

Member

I have no idea about macPlist to be honest. Sorry for taking a few days to respond, was out of the country.

Member

adam-lynch commented Nov 9, 2014

I have no idea about macPlist to be honest. Sorry for taking a few days to respond, was out of the country.

@majodev

This comment has been minimized.

Show comment
Hide comment
@majodev

majodev Nov 10, 2014

Would also love to see these changes within node-webkit-builder. Updating the Info.plist afterwards is a pain (as is replacing the whole file). This will make it easier to replace one single value.

majodev commented Nov 10, 2014

Would also love to see these changes within node-webkit-builder. Updating the Info.plist afterwards is a pain (as is replacing the whole file). This will make it easier to replace one single value.

@bastimeyer

This comment has been minimized.

Show comment
Hide comment
@bastimeyer

bastimeyer Nov 13, 2014

Contributor

I have no idea about macPlist to be honest.

If the motivation of this pull request is still unclear, just have a look at the modified test case. These changes are actually fairly simple and make using the macPlist option much, much easier, so I don't know why nobody feels responsible to quickly look into this for a merge.
As you can see, I'm not the only one who's having some issues with the current implementation.
Thank you...

Contributor

bastimeyer commented Nov 13, 2014

I have no idea about macPlist to be honest.

If the motivation of this pull request is still unclear, just have a look at the modified test case. These changes are actually fairly simple and make using the macPlist option much, much easier, so I don't know why nobody feels responsible to quickly look into this for a merge.
As you can see, I'm not the only one who's having some issues with the current implementation.
Thank you...

@adam-lynch

This comment has been minimized.

Show comment
Hide comment
@adam-lynch

adam-lynch Nov 13, 2014

Member

@bastimeyer you have a point 😄

I had another there now. Looks good to me. One small thing I'd suggest is not having more than three params per function. So for getPlistOptions, if it was me I'd have it accept a single object argument, which would then contain appName, appVersion, copyright and custom as properties.

Also, for the README, how about something like this?

Pass a string if you want to supply exactly what will be written to the plist file. If a string isn't passed, a plist file will be generated from your package.json. Pass an object to overwrite or add properties to the generated plist file.

Member

adam-lynch commented Nov 13, 2014

@bastimeyer you have a point 😄

I had another there now. Looks good to me. One small thing I'd suggest is not having more than three params per function. So for getPlistOptions, if it was me I'd have it accept a single object argument, which would then contain appName, appVersion, copyright and custom as properties.

Also, for the README, how about something like this?

Pass a string if you want to supply exactly what will be written to the plist file. If a string isn't passed, a plist file will be generated from your package.json. Pass an object to overwrite or add properties to the generated plist file.

Added custom macPlist options support
* Added support for a custom macPlist object so plist properties can be
changed or added to the generated file.
* Removed auxiliary properties appName, appVersion and copyright
* Added checks for mandatory plist properties
* Fixed asynchronicity issues
* Modified plist test to reflect these changes
@bastimeyer

This comment has been minimized.

Show comment
Hide comment
@bastimeyer

bastimeyer Nov 13, 2014

Contributor

PR is again updated... 87ee185

Contributor

bastimeyer commented Nov 13, 2014

PR is again updated... 87ee185

adam-lynch added a commit that referenced this pull request Nov 13, 2014

@adam-lynch adam-lynch merged commit 55da009 into nwjs-community:master Nov 13, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@adam-lynch

This comment has been minimized.

Show comment
Hide comment
@adam-lynch

adam-lynch Nov 13, 2014

Member

Thanks 👍 Sorry for taking so long 😄

Member

adam-lynch commented Nov 13, 2014

Thanks 👍 Sorry for taking so long 😄

@bastimeyer

This comment has been minimized.

Show comment
Hide comment
@bastimeyer

bastimeyer Nov 13, 2014

Contributor

Thanks! Would be nice if you could also bump the version and publish it on npm (also the grunt repo), so this can be used. 😃

Contributor

bastimeyer commented Nov 13, 2014

Thanks! Would be nice if you could also bump the version and publish it on npm (also the grunt repo), so this can be used. 😃

@adam-lynch

This comment has been minimized.

Show comment
Hide comment
@adam-lynch

adam-lynch Nov 13, 2014

Member

Will do tomorrow unless someone else gets to it before me.

Why are you suggesting to bump the grunt module's version though? Won't anyone installing it fresh / updating their install of the grunt module automatically get the latest version of the dependences (i.e. this module)? That's what I assume, unless we bump the version by its major version.

Member

adam-lynch commented Nov 13, 2014

Will do tomorrow unless someone else gets to it before me.

Why are you suggesting to bump the grunt module's version though? Won't anyone installing it fresh / updating their install of the grunt module automatically get the latest version of the dependences (i.e. this module)? That's what I assume, unless we bump the version by its major version.

@bastimeyer

This comment has been minimized.

Show comment
Hide comment
@bastimeyer

bastimeyer Nov 13, 2014

Contributor

installing it fresh

Then yes. But if you've already installed the current version, then you'd need to clear your npm cache first before installing/updating the package or else it will be copied from there. That's why I'm suggesting to bump the patch version number...

Contributor

bastimeyer commented Nov 13, 2014

installing it fresh

Then yes. But if you've already installed the current version, then you'd need to clear your npm cache first before installing/updating the package or else it will be copied from there. That's why I'm suggesting to bump the patch version number...

@adam-lynch

This comment has been minimized.

Show comment
Hide comment
@adam-lynch

adam-lynch Nov 14, 2014

Member

K thanks

Member

adam-lynch commented Nov 14, 2014

K thanks

@adam-lynch

This comment has been minimized.

Show comment
Hide comment
@adam-lynch

adam-lynch Nov 14, 2014

Member

Done. Both at 0.3.0 now

Member

adam-lynch commented Nov 14, 2014

Done. Both at 0.3.0 now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment