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 addUIBlock to allow third party to resolve a view #8217

Closed
wants to merge 3 commits into from

Conversation

gre
Copy link
Contributor

@gre gre commented Jun 18, 2016

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.


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

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 the view!
  }
});

...and we can finally implement that generic view screenshot in a third party library... -> this is a gist implementing it! <-

NB: this gist will turn into a PR for @jsierles' react-native-view-snapshot library, when this gets into React Native.

… by tag

Java Android usage example:

UIManagerModule uiManager = reactContext.getNativeModule(UIManagerModule.class);
uiManager.addUIBlock(new UIBlock() {
  public void execute (NativeViewHierarchyManager nvhm) {
    View view = nvhm.resolveView(tag);
    ...do something with view...
  }
});
@ghost
Copy link

ghost commented Jun 18, 2016

By analyzing the blame information on this pull request, we identified @dmmiller and @korDen to be potential reviewers.

@ghost ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jun 18, 2016
@gre
Copy link
Contributor Author

gre commented Jun 18, 2016

@mkonicek @kmagiera here is finally my PR :) cheers

@christopherdro
Copy link
Contributor

👍 I still vote to make the view snapshot part of core since the API for iOS has already been added.

@gre
Copy link
Contributor Author

gre commented Jun 19, 2016

@christopherdro I understand the idea behind not making react-native too monolithic though.
Having snapshot in RN means one more thing to maintain and it's time consuming for the RN team where they should focus on core features. I like react-native to try to stay as minimalist as possible so they focus on "just solve React abstraction on top of Native views".
Making it possible to have third party native libraries is IMO the most important part here. Third party libraries will be easier to patch & improve (rather than having to send PR to react-native every time) AND it allows more alternative implementations, bringing better overall quality (more distributed development to the community!).

@jsierles
Copy link
Contributor

@mkonicek Does FB use the snapshot code internally? If not, I volunteer to extract the missing features out to the module once this is in.

@christopherdro
Copy link
Contributor

christopherdro commented Jun 19, 2016

@gre I understand that approach and it makes sense to me with functionalities that do not exist in core for any of the platforms. However, I have to disagree with that if the functionality already exists for one of the platforms within core.

Unless the plan is to remove ac12f98 which I doubt is going to happen.

@gre
Copy link
Contributor Author

gre commented Jun 19, 2016

@christopherdro yup agreed. APIs should eventually converge to implement same features everywhere.

@mkonicek
Copy link
Contributor

mkonicek commented Jun 20, 2016

Assigning to @lexs to review. Would you mind taking a look please?

@@ -473,4 +473,8 @@ public EventDispatcher getEventDispatcher() {
public void sendAccessibilityEvent(int tag, int eventType) {
mUIImplementation.sendAccessibilityEvent(tag, eventType);
}

public void addUIBlock (UIBlock block) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add documentation here? Maybe an example of how you'd expect it to be used?

@astreet
Copy link
Contributor

astreet commented Jun 21, 2016

Code lgtm, can you just add that documentation? I agree we should have this implemented in a single way across platforms, which would hopefully mean implementing it as a separate module in iOS as well at some point.

@gre
Copy link
Contributor Author

gre commented Jun 21, 2016

@astreet I've added the doc with the description (it's the same of the iOS implementation, turns out it still make sense for the Android implementation :) ) with an Android example.

Thanks

@astreet
Copy link
Contributor

astreet commented Jun 22, 2016

Great, thanks!

@facebook-github-bot shipit

@ghost ghost added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Jun 22, 2016
@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to Phabricator to review.

@ghost ghost closed this in af28de5 Jun 22, 2016
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
Copy link
Contributor Author

gre commented Aug 25, 2016

@astreet @christopherdro @nicklockwood I've published third party library https://github.com/gre/react-native-view-shot that re-implement part of the takeSnapshot iOS implementation and also have the Android implementation.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants