Skip to content
This repository has been archived by the owner on Jan 13, 2022. It is now read-only.

Use drawViewHierarchyInRect for iOS > 7 for rendering UIImage from an UIView #19

Merged
merged 1 commit into from
Mar 18, 2014

Conversation

x2on
Copy link
Contributor

@x2on x2on commented Mar 13, 2014

On iOS 7 we could use drawViewHierarchyInRect:afterScreenUpdates: for rendering the current UIView.

With this change the rendered UIImage is exactly that what it's shown on device.
With the current implementation this is not so. For example UIPickerView is not correctly displayed and completely broken with iOS 7.1.

I implemented it with backward compatibility for iOS 6.x.

@dblock
Copy link
Contributor

dblock commented Mar 13, 2014

This breaks things for me in https://github.com/dblock/namapkit/tree/tiled-map-view, the screenshots don't look like reality any more, there're views being added that are missing.

@x2on
Copy link
Contributor Author

x2on commented Mar 13, 2014

Could you give me an example?
Yes some test are failing - because with this change the pngs should be more realistic.

@dblock
Copy link
Contributor

dblock commented Mar 13, 2014

Ok, I looked at this more carefully. There're some animations going on and now I get the animated version and it takes longer and so I get a snapshot that's mostly in the middle of an animation. You can see the changes in https://github.com/dblock/NAMapKit/compare/tiled-map-view...ios-snapshot-test-case-pr19?expand=1.

tl;dr +1 on this change, however it does not fix #20

@x2on
Copy link
Contributor Author

x2on commented Mar 13, 2014

So that means the changes in this pull request works as expected, or?

@dblock
Copy link
Contributor

dblock commented Mar 13, 2014

@x2on As far as I can see, yes, everything here works as expected (and actually better that what we have in HEAD).

@x2on
Copy link
Contributor Author

x2on commented Mar 17, 2014

I had this running a few days, and i think we need to check this pull request really if it works as expected and is fast enough.

@dblock
Copy link
Contributor

dblock commented Mar 17, 2014

cc: @dstnbrkr +1

@x2on
Copy link
Contributor Author

x2on commented Mar 17, 2014

Perhaps snapshotViewAfterScreenUpdates: could be used instead of drawViewHierarchyInRect:afterScreenUpdates:

@dstnbrkr
Copy link
Contributor

If I understand right, 'afterScreenUpdates' is the key change here. We're allowing any views that must animate into their correct positions to do so before snapshotting, right?

I haven't dug into our options yet - but is there an iOS 6 compatible way to allow the views to animate into place?

@x2on
Copy link
Contributor Author

x2on commented Mar 17, 2014

yes thats one change. The other is to use the native view rendering from iOS 7. With this change some native elements are rendered more correct than with the current implementation.

@x2on
Copy link
Contributor Author

x2on commented Mar 17, 2014

I don't see any native implementations on iOS 6.

- (UIImage *)_renderView:(UIView *)view
{
#ifdef __IPHONE_7_0
if ([view respondsToSelector:@selector(drawViewHierarchyInRect:afterScreenUpdates:)]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

respondsToSelector should be sufficient, do we need the macro?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you wan't to compile this code with iOS 6 SDK we need the macro.
The respondsToSelector call is only on runtime for checking if this feature is available.

Copy link
Contributor

Choose a reason for hiding this comment

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

True in the current form due to the direct call to drawViewHierarchyInRect:afterScreenUpdates: (as in that statement won't compile under iOS 6). We could convert that to a dynamic invocation and rely solely on the respondsToSelector check. Think it's good to avoid conditional compilation wherever we can.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll add: if the no macro / invocation version feels too unwieldy, then let's go with the macro approach but lose the respondsToSelector check. That way we're firmly relying on either the macro or the runtime check, but not both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you remove one of the two checks, the following can happen:

  • Remove respondToSelector check: If you compile with iOS 7 SDK and run with iOS 6 --> It will crash.
  • Remove Macro: If you compile with iOS 6 SDK --> It won't compile.

Copy link
Contributor

Choose a reason for hiding this comment

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

It will compile in the latter case if the invocation is dynamic (i.e. performSelector or invocation). WDYT - worth considering to simplify logic?

@dblock dblock mentioned this pull request Mar 17, 2014
dstnbrkr added a commit that referenced this pull request Mar 18, 2014
Use drawViewHierarchyInRect for iOS > 7 for rendering UIImage from an UIView
@dstnbrkr dstnbrkr merged commit 1a17524 into facebookarchive:master Mar 18, 2014
@dstnbrkr
Copy link
Contributor

Merged - don't want to block on the macro/invocation decision as this is a great improvement. Thanks for contributing this @x2on!

@dblock
Copy link
Contributor

dblock commented Mar 18, 2014

Related to this, we're now all upgraded to 7.1, however Travis-CI is not. Is there a way with this code to force the iOS 6 implementation at runtime?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants