Skip to content

Conversation

@mz2
Copy link

@mz2 mz2 commented Mar 18, 2015

  • Adds an optional relative scale parameter to the image context handler.
  • Also fixes a compilation error in a scenario where source is being built without Foundation.h coming in via prefix header.

@cparnot
Copy link
Owner

cparnot commented Mar 20, 2015

iOS too?

@mz2
Copy link
Author

mz2 commented Mar 20, 2015

Damn it, no. One moment...

@mz2
Copy link
Author

mz2 commented Mar 20, 2015

Oh, wait, but actually the iOS implementation has already scale: as part of the method selector. I passed it as a value you can include with the context handler instead of a separate argument (as I treated it similarly as an optional value -- there's an implicit scale of 1.0).

@mz2
Copy link
Author

mz2 commented Mar 20, 2015

So technically the iOS and Mac implementations are inconsistent (on iOS there's a scale: parameter, on OSX not, and on OSX the scale is in this branch accepted as an optional context value where previously no scaling existed on OSX). I would propose an API change where scale is made indeed a value passed into the context handler instead of it being a named parameter to the method.

@cparnot
Copy link
Owner

cparnot commented Mar 21, 2015

So technically the iOS and Mac implementations are inconsistent

It's more subtle that this. On iOS, the scaling has to be done by your code based on the device scale. On OS X, the NSImage implementation takes care of it and you render in the block at scale 1 no matter what (NSImage takes care of scaling based on the screen). This scaling is only exposed for the purpose of tests. On OS X, the tests render the NSImage instance in various... NSImage with different sizes, to mimick different screens.

The "scale" you want to apply is different. It's not the screen scale, it's a custom scaling factor on top of that. Both implementations are consistent: no such factor exists at the moment. But it could be added. Also on iOS where it is not yet present in your branch ;-)

@mz2
Copy link
Author

mz2 commented Mar 21, 2015

Ok thanks for clarifying -- then I should indeed add this scale factor to iOS too. Is there an iOS target someplace public already, or shall I make one?

@cparnot
Copy link
Owner

cparnot commented Mar 21, 2015

Is there an iOS target someplace public already, or shall I make one?

Yes. And there are tests :-)

@mz2
Copy link
Author

mz2 commented Mar 21, 2015

zomg!

@cparnot cparnot mentioned this pull request Mar 26, 2015
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