Fixed compiler warnings with some casts and little changes like that #68

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

cjhanson commented Apr 4, 2013

No description provided.

cjhanson added some commits Apr 4, 2013

@cjhanson cjhanson Fixed compiler warnings with some casts and little changes like that 7945167
@cjhanson cjhanson Added Mac support
Removed auto scaling of bitmap on iOS. Now you get the bitmap at as many pixels as your size.
Fixed compiler warnings
34f9d77
@cjhanson cjhanson Fixed warning on deallocation of SVGKImage about observers. (It neede…
…d to remove observer in dealloc)
8fea71d
@cjhanson cjhanson Added xcconfig files (though not all settings have been pulled out of…
… the xcode project panel yet)

Added conditional to only enable NSLog on debug builds
b9d69c9
Contributor

adamgit commented Apr 6, 2013

There's some good stuff in here - I noted some good bugfixes - but also some new bugs (including things that would break existing projects).

Unfortunately, I can't merge this.

Ideally, if you split this up into separate pull-requests, I can review pull-requests one-by-one, and some of those I think we could isntantly merge. Others will need some review / changes.

From a brief scan, here's some of the issues (but the changelist is so long I might have missed some others, sorry):

  1. This renames some of the W3C-named classes. All the DOM classes have official names. Unless we are forced to (polluted namespace - Apple has one class that they accidentally left in the global namespace), then we must not rename them. They were correct to start with.
  2. This adds some non-iOS files in the iOS project. e.g. "SVGKit_Mac.m" is now in SVGKit-iOS. Mac-specific classes must only appear in a Mac-specific Xcodeproject.
  3. There's a lot of stylistic changes which I understand are personal preference, but most of them appear to be less clear than the existing code (renaming e.g. variable "prefix" to "pre" - the former is clearer). Unless there's a good reason to rename them, they should remain as-is.
  4. #define removing NSLog is very dangerous - we can't do this! (although I've done it on projects in the past, that's not approprite for the default shipping version of the library)

adamgit closed this Apr 6, 2013

Contributor

cjhanson commented Apr 10, 2013

Thanks for looking through that. I actually intended to only request a pull of just one commit. For some reason github has put all of my commits into the request even those made well after I pressed the pull request button. (they are mostly just hacks so I could get things working in my specific situation).

I'll try to put together a clean version and make a new request for you taking into account your suggestions as well.

Contributor

adamgit commented May 11, 2013

FYI: MaddTheSane seems to be getting close to a version with good Mac OS X too:

MaddTheSane/SVGKit@master

Perhaps worth combining both your work?

Contributor

MaddTheSane commented May 11, 2013

The only thing really lacking in my port is the SVGKView and children. For the most part, the code uses calls that are also in OS X.

Edit: SVGKView and children for OS X are now in my SVGKit fork.

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