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

Let onVisibilityChanged() be called on children (mainly fixes Image issues) #2478

Merged

Conversation

danielgindi
Copy link
Contributor

Does any other open PR do the same thing?

Nope.

What issue is this PR fixing?

This is related to the following issues:
#1870 #2105 #2048

The REAL problem with displaying images inside the markers in Android, is not a bug in Fresco as claimed in some issues. It's that Fresco depends on the onVisibilityChanged(...) event to be called. And that's a protected method. And is propagated by dispatchVisibilityChanged(...) to the whole subview hierarchy. dispatchVisibilityChanged is protected too. And it depends on mAttachInfo etc.
So fiddling with Reflection is not an option as it counts on too many private stuff to not change in Android's core.

The solution I propose here is having a semi-invisible ViewGroup and add the markers' views to it, so it is properly "visible".
I'm calling it semi-invisible because setting visibility to INVISIBLE/GONE will again prevent the subviews from getting visibility events. What I did is set it to zero size, offscreen, clipping, and 0 opacity. So basically doing everything to avoid it trying to draw, but letting the events pass through.

For me, this fixes the issue - together with #2477

How did you test this PR?

Real life project with animatable Markers.

@rborn
Copy link
Collaborator

rborn commented Sep 9, 2018

LGTM @alvelig 🐽

@alvelig
Copy link
Contributor

alvelig commented Sep 9, 2018

Nice catch. I'll need to analyze that thoroughly, but overall LGTM.

@alvelig alvelig merged commit 9ae75a7 into react-native-maps:master Sep 9, 2018
@rborn
Copy link
Collaborator

rborn commented Sep 11, 2018

@danielgindi @alvelig can you investigate #2478 (comment) please?

@danielgindi
Copy link
Contributor Author

Yes, it’s an addition for RN 0.57, I just noticed that it’s a new feature. The other clipping code should do it’s best to avoid unnecessary drawing.

Is there a way to enclose that specific line for > 0.57 only?

@rborn
Copy link
Collaborator

rborn commented Sep 11, 2018

@danielgindi
Copy link
Contributor Author

@rborn I mean putting attacherGroup.setOverflow(ViewProps.HIDDEN); in a block like if (ReactNativeVersion.VERSION >= 0.57) { that Android Studio will respect. It works with SDK versions obviously because it's something supported out of the box for Android SDKs, but I'm not sure how the line you referred to will assist in this

In the worst case we could just omit that line. I took it as a precaution in case RN takes over the clipping stuff, so worst case it will be slower when rendering to the bitmap.

@rborn
Copy link
Collaborator

rborn commented Sep 11, 2018

my java-fu sucks, @alvelig might be able to help maybe

@danielgindi
Copy link
Contributor Author

So we should wait for his feedback before doing a release...

In anyway - the current situation is that 0.57 is the first to come with overflow: visible support in Android. It's implementation consists of setClipChildren(false); and a custom canvas.clipRect(new RectF(... based on the background of a view.
overflow: visible is on by default.
setClipChildren should be okay for now as the default is set in the constructor, but clipRect isn't.
The bigger issue might be in future versions - where I'm assuming they will change that implementation significantly, as current implementation does not support touches outside the view when overflow: visible.
My sources: facebook/react-native@b81c8b5

@danielgindi
Copy link
Contributor Author

Okay so I just made the same mistake as the guys over there:
facebook/react-native@f090840

Android's clipping affects children's overflow, not the parent's.
So this makes the setClipBounds call unnecessary, and setOverflow even more necessary performance wise - especially when they will have a more complete implementation of it.

@alvelig
Copy link
Contributor

alvelig commented Sep 11, 2018

@danielgindi just make it a string until we move definitely to 0.57

    attacherGroup.setOverflow("hidden");

See this change: facebook/react-native@cfce6ee#diff-b26c3186930f561d80acdc8f0dde5dec

@danielgindi
Copy link
Contributor Author

Oh yeah. I misread their errors here, thought that setOverflow is the one that's missing.
Could you do it as an additional commit or should I PR?

@alvelig
Copy link
Contributor

alvelig commented Sep 11, 2018

Fixed in 710dca4

@danielgindi danielgindi deleted the feature/onVisibilityChanged branch September 11, 2018 19:05
ryanbourneuk pushed a commit to 3sidedcube/react-native-maps that referenced this pull request Dec 10, 2018
* upstream/master: (28 commits)
  Calculate bounding box from region (react-native-maps#2615)
  [iOS GoogleMap] Fix animateCamera (react-native-maps#2608)
  Fix type definition error (react-native-maps#2607)
  [Android] Fix app crash in Android if building found but cannot getActiveLevelIndex (react-native-maps#2598)
  Provide a camera system (react-native-maps#2563)
  Get visible map bounding box (react-native-maps#2571)
  [0.22.1] Release (react-native-maps#2574)
  Move dev only deps to devDependencies. (react-native-maps#2548)
  Specify how to use Google Maps (react-native-maps#2550)
  r2507: remove marker: Attempt to invoke virtual method 'void com.google.android.gms.maps.model.setIcon(com.google.android.gms.maps.model.BitmapDescription)' on a null object reference #: remove marker: Attempt to invoke virtual method 'void com.google.android.gms.maps.model.setIcon(com.google.android.gms.maps.model.BitmapDescription)' on a null object reference (react-native-maps#2555)
  update to clarify cacheEnabled is apple maps only
  [0.22.0] Release (react-native-maps#2535)
  Fix for “The specified child already has a parent”
  Improve installation docs (react-native-maps#2541)
  fix fitToSuppliedMarkers function (react-native-maps#2524)
  Performance improvements for tracksViewChanges (react-native-maps#2487)
  fix spelling mistakes
  Added flag to make sure that there has an Observer of view.
  hotfix PR react-native-maps#2478
  Fix a peer dependencies warning
  ...
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.

5 participants