Skip to content

Improve UIScreen Capture() support for complex views - #1235

Merged
rolfbjarne merged 2 commits into
dotnet:masterfrom
christianhelle:uiscreen-capture-fix
Nov 24, 2016
Merged

Improve UIScreen Capture() support for complex views#1235
rolfbjarne merged 2 commits into
dotnet:masterfrom
christianhelle:uiscreen-capture-fix

Conversation

@christianhelle

Copy link
Copy Markdown
Member

The current UIScreen Capture() method doesn't support complex views with multiple CA Layers (for example iCarousel). The changes here are in use in our apps in the AppStore with over half a million daily active users and no one has ever reported a bug regarding it. I kept the original code for backward compatibility as drawViewHierarchyInRect:afterScreenUpdates: was introduced in iOS 7.0. My changes were based from https://developer.apple.com/library/content/qa/qa1817/_index.html

Screenshot using original Capture() implementation:
Alt text

Screenshot using the changes in this pull request:
Alt text

The images above are from an app with a screen using iCarousel with 8 items containing both images and texts. I used the UIScreen Capture() method to capture this screen before opening another screen where I show it as the background image.

I unfortunately had to crop out the rest of the image due to corporate reasons, but the images provided above are enough to illustrate the problem. I can provide a video of the problem if needed as long as this video is not shared with the general public

@dnfclas

dnfclas commented Nov 23, 2016

Copy link
Copy Markdown

Hi @christianhelle, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.

TTYL, DNFBOT;

@monojenkins

Copy link
Copy Markdown
Contributor

Hello! I'm the build bot for the Mono project.

I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done.

Contributors can ignore this message.

@rolfbjarne

Copy link
Copy Markdown
Member

approve

@rolfbjarne rolfbjarne left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this looks good, except for a minor style issue: we use a space before the opening parenthesis in a method call. Could you please fix this (you've also changed it in the existing code you've re-indented due to the additional if clause)?

Also I don't think there are any backwards compatibility issues, and the new code is clearly rendering a better screenshot according to the provided screenshots.

@christianhelle

Copy link
Copy Markdown
Member Author

Thanks for the input @rolfbjarne, I'll fix the styling issue you pointed out.

I'm pretty sure that the method drawViewHierarchyInRect:afterScreenUpdates: was introduced in iOS 7.0 since it states that in the documentation page. If I didn't include the original implementation then calling Capture() would fail on older iOS versions.

I'm fine with replacing the old implementation but I wanted to tread carefully since this is my first time contributing to the product I thought I should consider backwards compatibility

@spouliot spouliot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 (modulo Rolf's comments)
Thanks for your contribution!

@monojenkins

Copy link
Copy Markdown
Contributor

Build failure

@christianhelle

Copy link
Copy Markdown
Member Author

I updated my changes based on the comments by @rolfbjarne

@monojenkins

Copy link
Copy Markdown
Contributor

Build failure

@rolfbjarne

Copy link
Copy Markdown
Member

Test failures are unrelated (watchOS/System test failures filed as bug #47167).

@rolfbjarne
rolfbjarne merged commit cf07825 into dotnet:master Nov 24, 2016
@rolfbjarne

Copy link
Copy Markdown
Member

Thanks!

@mandel-macaque

Copy link
Copy Markdown
Contributor

Looking at this PR, we should also test that the UIView.Capture added in commit 7c51c3b

@rolfbjarne what do you think?
@christianhelle do you have an example I could use for testing?

@mandel-macaque

Copy link
Copy Markdown
Contributor

/cc @spouliot I forgot you ;)

@christianhelle
christianhelle deleted the uiscreen-capture-fix branch December 5, 2016 13:42
spouliot pushed a commit that referenced this pull request Dec 7, 2016
Use DrawViewHierarchy to capture a snapshot of the current screen
spouliot pushed a commit that referenced this pull request Dec 7, 2016
Use DrawViewHierarchy to capture a snapshot of the current screen
@dalexsoto dalexsoto added the community Community contribution ❤ label May 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community Community contribution ❤

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants