Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Android] Implement UIManager.takeSnapshot #6431

Closed
wants to merge 4 commits into from

Conversation

gre
Copy link
Contributor

@gre gre commented Mar 12, 2016

Implement the takeSnapshot feature for Android.

It works almost like the iOS implementation. it also supports "webm". There might be some tiny difference in Android snapshotting behavior. For instance, the UIExplorer doesn't seem to show a "recursive" screenshot like in iOS. Otherwise everything works fine.

Had to change the internal bridge API from UIManager.takeSnapshot(view/*String or int*/, options) to UIManager.takeSnapshot(view/*String*/, tag/*int*/, options) because RN Android don't allow generic id type like in iOS implementation.

@gre
Copy link
Contributor Author

gre commented Mar 12, 2016

cc @mkonicek @nicklockwood

@@ -33,29 +33,56 @@ var ScreenshotExample = React.createClass({

render() {
return (
<View>
<View style={style.root} ref={ref => this._view = ref}>

Choose a reason for hiding this comment

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

no-return-assign: Arrow function should not return assignment.

Choose a reason for hiding this comment

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

property _view Property not found in React component

@gre
Copy link
Contributor Author

gre commented Mar 13, 2016

the android implementation now support width/height in points and resize the snapshot

@facebook-github-bot
Copy link
Contributor

@gre updated the pull request.

@facebook-github-bot
Copy link
Contributor

@gre updated the pull request.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@gre updated the pull request.

View view = mNativeViewHierarchyManager.resolveView(mTag);
FileOutputStream fileOutputStream = new FileOutputStream(destFile);
snapshot.captureViewToFileOutputStream(view, fileOutputStream);
fileOutputStream.close();
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be in a finally section, check out some tutorials or just look for "outputstream" across the RN codebase to see examples. Found e.g. http://www.quepublishing.com/articles/article.aspx?p=26067

@mkonicek
Copy link
Contributor

This is a huge PR and I'm not sure we should include and maintain this functionality by default. Would it make sense to release it as a separate module?

@gre
Copy link
Contributor Author

gre commented Mar 15, 2016

@mkonicek separate module you still mean inside RN right?

@gre
Copy link
Contributor Author

gre commented Mar 15, 2016

I mean, since the iOS implementation have it built-in & it's part of UIManager https://github.com/facebook/react-native/blob/4b4455f8278d8d728d69d84804bbe8fed9b3d972/Libraries/Utilities/UIManager.js, I thought it was making sense to also have it in Android. ( also just for context, I've made it here after this discussion https://twitter.com/matzatorski/status/707628299258863617 )

@gre
Copy link
Contributor Author

gre commented Mar 18, 2016

I've addressed comments

@facebook-github-bot
Copy link
Contributor

@gre updated the pull request.

@mkonicek
Copy link
Contributor

I'm not sure making it part of the UIManager was the best decision actually.

But I still feel like this is a lot of code to understand and maintain for the RN community (we'll get issues and pull requests) and would be nice if you could add it to a project using e.g. npm install react-native-android-snapshot.

@javache What do you think?

If we want to add it to React Native itself it would be nice to extract all the code to Snapshot.java so it lives in one place and delegate to it from the UIManagerModule and UIViewOperationQueue.

@gre
Copy link
Contributor Author

gre commented Mar 22, 2016

So maybe this should go outside of RN then? I'm fine with that but decision need to be consistent between iOS and Android version IMO.

Otherwise, BTW we can't implement it outside of RN at the moment because there is no way to execute something on a arbitrary view (by its tag), something equivalent to iOS' addUIBlock would be interesting (not only for this feature)

@mkonicek
Copy link
Contributor

mkonicek commented Apr 1, 2016

Sorry for the delay, I was on vacation. I think this is a lot of code to maintain and not sure how many people need this feature yet. I'd prefer it released separately, not sure why it was added for iOS by default.

@kmagiera What do you think? Does having a way to execute something on a arbitrary view (by its tag) (something equivalent to iOS' addUIBlock) sound good? It could help extensibility, to make it possible to implement features like this as standalone modules.

@mkonicek mkonicek assigned kmagiera and unassigned kmagiera Apr 1, 2016
@jsierles
Copy link
Contributor

jsierles commented Apr 4, 2016

A lot of people have been asking for android support. I'm happy to maintain this feature as an external module. So I think the biggest priority should be the 'arbitrary view' behavior if this isn't going to be merged.

@mkonicek
Copy link
Contributor

mkonicek commented Apr 7, 2016

@jsierles OK that sounds good, could you send a PR and cc @kmagiera please?

@jsierles
Copy link
Contributor

jsierles commented Apr 8, 2016

I have no idea how to implement this in Android, @gre do you know?

@gre
Copy link
Contributor Author

gre commented Apr 8, 2016

ok I can take a look when I find time for this :)

@ghost
Copy link

ghost commented Apr 17, 2016

@nicklockwood would you mind taking a look at this pull request? It's been a while since the last commit was reviewed.

@christopherdro
Copy link
Contributor

@mkonicek I'm requesting that we give this another look and agree with @gre that things should be kept consistent across both platforms.

I find this to be a very useful feature and makes this feature available on both platform.

Thoughts?

@niksinfinite10
Copy link

When is this happening ?
It has been quite a long this pull request has been created and waiting.

@jsierles
Copy link
Contributor

Since this feature doesn't seem in high demand, I think it's time to close this out in favor of an exernal module, and another PR for the addUIBlock feature on Android to support it. Any objections?

@gre
Copy link
Contributor Author

gre commented Jun 17, 2016

yeah I think we will do this addUIBlock feature and delegate the implementation on your react-native-view-snapshot .

I'll try to work on this this weekend.

@gre
Copy link
Contributor Author

gre commented Jun 18, 2016

@jsierles @niksinfinite10 @christopherdro please see #8217

ghost pushed a commit that referenced this pull request Jun 22, 2016
Summary:
This PR follows the work started in #6431 but instead of implementing the snapshot for Android, will just allow that feature to be implemented by a third party library.

The missing feature is the ability to resolve a View for a given tag integer. which is now possible with a `addUIBlock` on the `UIManagerModule` method where you give a `UIBlock` instance (a new class) that implements a `execute(NativeViewHierarchyManager)` function.

This is already possible in iOS API. a third party can use the `addUIBlock` too.
I have kept the name `addUIBlock` so it's the same as in the [iOS' API](https://github.com/facebook/react-native/search?q=addUIBlock&type=Code&utf8=%E2%9C%93).

 ---

With this PR a third party lib can now do...

```java
UIManagerModule uiManager = reactContext.getNativeModule(UIManagerModule.class);
uiManager.addUIBlock(new UIBlock() {
  public void execute (NativeViewHierarchyManager nvhm) {
    View view = nvhm.resolveView(tag);
    ...do something with view... like... screenshot t
Closes #8217

Differential Revision: D3469311

Pulled By: astreet

fbshipit-source-id: bb56ecc7e8936299337af47ca8114875ee1fd2b0
bubblesunyum pushed a commit to iodine/react-native that referenced this pull request Aug 23, 2016
Summary:
This PR follows the work started in facebook#6431 but instead of implementing the snapshot for Android, will just allow that feature to be implemented by a third party library.

The missing feature is the ability to resolve a View for a given tag integer. which is now possible with a `addUIBlock` on the `UIManagerModule` method where you give a `UIBlock` instance (a new class) that implements a `execute(NativeViewHierarchyManager)` function.

This is already possible in iOS API. a third party can use the `addUIBlock` too.
I have kept the name `addUIBlock` so it's the same as in the [iOS' API](https://github.com/facebook/react-native/search?q=addUIBlock&type=Code&utf8=%E2%9C%93).

 ---

With this PR a third party lib can now do...

```java
UIManagerModule uiManager = reactContext.getNativeModule(UIManagerModule.class);
uiManager.addUIBlock(new UIBlock() {
  public void execute (NativeViewHierarchyManager nvhm) {
    View view = nvhm.resolveView(tag);
    ...do something with view... like... screenshot t
Closes facebook#8217

Differential Revision: D3469311

Pulled By: astreet

fbshipit-source-id: bb56ecc7e8936299337af47ca8114875ee1fd2b0
mpretty-cyro pushed a commit to HomePass/react-native that referenced this pull request Aug 25, 2016
Summary:
This PR follows the work started in facebook#6431 but instead of implementing the snapshot for Android, will just allow that feature to be implemented by a third party library.

The missing feature is the ability to resolve a View for a given tag integer. which is now possible with a `addUIBlock` on the `UIManagerModule` method where you give a `UIBlock` instance (a new class) that implements a `execute(NativeViewHierarchyManager)` function.

This is already possible in iOS API. a third party can use the `addUIBlock` too.
I have kept the name `addUIBlock` so it's the same as in the [iOS' API](https://github.com/facebook/react-native/search?q=addUIBlock&type=Code&utf8=%E2%9C%93).

 ---

With this PR a third party lib can now do...

```java
UIManagerModule uiManager = reactContext.getNativeModule(UIManagerModule.class);
uiManager.addUIBlock(new UIBlock() {
  public void execute (NativeViewHierarchyManager nvhm) {
    View view = nvhm.resolveView(tag);
    ...do something with view... like... screenshot t
Closes facebook#8217

Differential Revision: D3469311

Pulled By: astreet

fbshipit-source-id: bb56ecc7e8936299337af47ca8114875ee1fd2b0
@gre gre mentioned this pull request Sep 12, 2016
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.

9 participants