Skip to content

OSX compatibility and updated for ARC#5

Merged
ap4y merged 8 commits into
ap4y:masterfrom
mredig:master
Oct 12, 2014
Merged

OSX compatibility and updated for ARC#5
ap4y merged 8 commits into
ap4y:masterfrom
mredig:master

Conversation

@mredig

@mredig mredig commented Sep 29, 2014

Copy link
Copy Markdown

I updated the code to be compatible with ARC and also added the ability to use the same class categories on OSX. Sample OSX code also provided.

@ap4y

ap4y commented Sep 30, 2014

Copy link
Copy Markdown
Owner

Hey @mredig thanks for the awesome PR, I just wanted to ask you to update a code a bit to match code style in repo? Also it would be awesome to squash commits a bit into 1-2 commits with detailed message. Let me know if it's ok.

Comment thread UIBezierPath+SVG.m Outdated

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Probably a typo here

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Oh yes. That is. I just copied and pasted the comment data from the header without thinking about that.

@ap4y

ap4y commented Sep 30, 2014

Copy link
Copy Markdown
Owner

I also not sure about SKU prefix. I think current naming is more idiomatic for obj-c categories. What do you think?

@mredig

mredig commented Sep 30, 2014

Copy link
Copy Markdown
Author

I'm not sure how I would go about reducing the number of commits. My work style is to get to a checkpoint, commit, go crazy with experimentation, get something that works, commit, etc. Still pretty new at this whole programming and git thing!

As for SKU, that's just what I'm using in my SpriteKit Utilities library. I basically just modified it to fit into that, and pulled the code out to contribute back to this project. The only reason I went with that is because OSX uses NSBezierPath, while iOS uses UIBezierPath. They are very similar, but not quite the same (you'll see I also had to add a category for NSBezierPath to be able to handle quadratic points, as well as the ability to handle a CGPath property). While I was experimenting, I simply defined NSBezierPath to UIBezierPath as well as the similar methods (NS - lineToPoint/UI - addLineToPoint, etc), but had a gut feeling that that could potentially cause problems down the road. So after I got to a place where it was working well, swapped it out for SKU as a prefix. It wouldn't be too hard to swap it back to what it was before, but I don't know if that's a good idea or not.

@ap4y

ap4y commented Oct 1, 2014

Copy link
Copy Markdown
Owner

Ok, I agree renaming makes sense. The only thing I still want to fix is a formatting consistency. I would like to preserve current code formatting style. Would you mind fixing it? Or I can fix it myself and we will discuss my changes, if it will be ok with you we will merge it. If you prefer me fixing formatting, then we have several options:

  • You can give me access into your branch. where I can fix it;
  • I can create a new branch and we will merge your PR into temporary branch. I will change formatting there and merge it into master;
  • I can cherry pick your changes. Not the best options as your direct contribution will be lost and it won't be easy to cherry pick it.

Let me know what you think.

@mredig

mredig commented Oct 2, 2014

Copy link
Copy Markdown
Author

I'll do what I can to get it as close as possible. If I miss anything, you are more than welcome to finish it. :)

@mredig

mredig commented Oct 2, 2014

Copy link
Copy Markdown
Author

Okay, I did what you asked to the best of my abilities. If you'd rather do it another way, you are more than welcome to follow one of your alternative strategies. I don't care too much about credit, but if you decide to throw my name and/or a link to my github in the comments, I won't complain at all. :)

I hope these modifications were helpful!

ap4y added a commit that referenced this pull request Oct 12, 2014
OSX compatibility and updated for ARC
@ap4y ap4y merged commit 26db16d into ap4y:master Oct 12, 2014
@ap4y

ap4y commented Oct 13, 2014

Copy link
Copy Markdown
Owner

Sorry about delay, was a bit busy last week. PR looks awesome, I cleaned a minor things in examples and added contributors information to the README. Thanks again for the PR.

@mredig

mredig commented Oct 14, 2014

Copy link
Copy Markdown
Author

No problem whatsoever! I'm glad I could help!

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.

2 participants