diff --git a/ReactAndroid/src/main/java/com/facebook/react/flat/AbstractClippingDrawCommand.java b/ReactAndroid/src/main/java/com/facebook/react/flat/AbstractClippingDrawCommand.java deleted file mode 100644 index 5e9dfba8b..000000000 --- a/ReactAndroid/src/main/java/com/facebook/react/flat/AbstractClippingDrawCommand.java +++ /dev/null @@ -1,67 +0,0 @@ -/** - * Copyright (c) 2015-present, Facebook, Inc. - * All rights reserved. - * - * This source code is licensed under the BSD-style license found in the - * LICENSE file in the root directory of this source tree. An additional grant - * of patent rights can be found in the PATENTS file in the same directory. - */ - -package com.facebook.react.flat; - -import android.graphics.Canvas; - -/* package */ abstract class AbstractClippingDrawCommand implements DrawCommand { - - protected boolean mNeedsClipping; - private float mClipLeft; - private float mClipTop; - private float mClipRight; - private float mClipBottom; - - public final boolean clipBoundsMatch( - float clipLeft, - float clipTop, - float clipRight, - float clipBottom) { - return mClipLeft == clipLeft && mClipTop == clipTop - && mClipRight == clipRight && mClipBottom == clipBottom; - } - - public final void setClipBounds( - float clipLeft, - float clipTop, - float clipRight, - float clipBottom) { - mClipLeft = clipLeft; - mClipTop = clipTop; - mClipRight = clipRight; - mClipBottom = clipBottom; - mNeedsClipping = mClipLeft != Float.NEGATIVE_INFINITY; - } - - public final float getClipLeft() { - return mClipLeft; - } - - public final float getClipTop() { - return mClipTop; - } - - public final float getClipRight() { - return mClipRight; - } - - public final float getClipBottom() { - return mClipBottom; - } - - protected final void applyClipping(Canvas canvas) { - // We put this check here to not clip when we have the default [-infinity, infinity] bounds, - // since clipRect in those cases is essentially no-op anyway. This is needed to fix a bug that - // shows up during screenshot testing. Note that checking one side is enough, since if one side - // is infinite, all sides will be infinite, since we only set infinite for all sides at the - // same time - conversely, if one side is finite, all sides will be finite. - canvas.clipRect(mClipLeft, mClipTop, mClipRight, mClipBottom); - } -} diff --git a/ReactAndroid/src/main/java/com/facebook/react/flat/AbstractDrawCommand.java b/ReactAndroid/src/main/java/com/facebook/react/flat/AbstractDrawCommand.java index c8729923c..e803b8d34 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/flat/AbstractDrawCommand.java +++ b/ReactAndroid/src/main/java/com/facebook/react/flat/AbstractDrawCommand.java @@ -19,8 +19,7 @@ import android.graphics.Color; * The idea is to be able to reuse unmodified objects when we build up DrawCommands before we ship * them to UI thread, but we can only do that if DrawCommands are immutable. */ -/* package */ abstract class AbstractDrawCommand extends AbstractClippingDrawCommand - implements Cloneable { +/* package */ abstract class AbstractDrawCommand implements DrawCommand, Cloneable { private float mLeft; private float mTop; @@ -28,10 +27,66 @@ import android.graphics.Color; private float mBottom; private boolean mFrozen; + protected boolean mNeedsClipping; + private float mClipLeft; + private float mClipTop; + private float mClipRight; + private float mClipBottom; + + public final boolean clipBoundsMatch( + float clipLeft, + float clipTop, + float clipRight, + float clipBottom) { + return mClipLeft == clipLeft && mClipTop == clipTop + && mClipRight == clipRight && mClipBottom == clipBottom; + } + + public final void setClipBounds( + float clipLeft, + float clipTop, + float clipRight, + float clipBottom) { + mClipLeft = clipLeft; + mClipTop = clipTop; + mClipRight = clipRight; + mClipBottom = clipBottom; + // We put this check here to not clip when we have the default [-infinity, infinity] bounds, + // since clipRect in those cases is essentially no-op anyway. This is needed to fix a bug that + // shows up during screenshot testing. Note that checking one side is enough, since if one side + // is infinite, all sides will be infinite, since we only set infinite for all sides at the + // same time - conversely, if one side is finite, all sides will be finite. + mNeedsClipping = mClipLeft != Float.NEGATIVE_INFINITY; + } + + public final float getClipLeft() { + return mClipLeft; + } + + public final float getClipTop() { + return mClipTop; + } + + public final float getClipRight() { + return mClipRight; + } + + public final float getClipBottom() { + return mClipBottom; + } + + protected final void applyClipping(Canvas canvas) { + canvas.clipRect(mClipLeft, mClipTop, mClipRight, mClipBottom); + } + + /** + * Don't override this unless you need to do custom clipping in a draw command. Otherwise just + * override onPreDraw and onDraw. + */ @Override - public final void draw(FlatViewGroup parent, Canvas canvas) { + public void draw(FlatViewGroup parent, Canvas canvas) { onPreDraw(parent, canvas); - if (shouldClip()) { + if (mNeedsClipping && shouldClip()) { canvas.save(Canvas.CLIP_SAVE_FLAG); applyClipping(canvas); onDraw(canvas); @@ -67,8 +122,11 @@ import android.graphics.Color; /** * Updates boundaries of the AbstractDrawCommand and freezes it. * Will return a frozen copy if the current AbstractDrawCommand cannot be mutated. + * + * This should not be called on a DrawView, as the DrawView is modified on UI thread. Use + * DrawView.collectDrawView instead to avoid race conditions. */ - public final AbstractDrawCommand updateBoundsAndFreeze( + public AbstractDrawCommand updateBoundsAndFreeze( float left, float top, float right, diff --git a/ReactAndroid/src/main/java/com/facebook/react/flat/ClippingDrawCommandManager.java b/ReactAndroid/src/main/java/com/facebook/react/flat/ClippingDrawCommandManager.java index 2fbea716a..f847115f1 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/flat/ClippingDrawCommandManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/flat/ClippingDrawCommandManager.java @@ -75,25 +75,57 @@ import com.facebook.react.views.view.ReactClippingViewGroupHelper; public void mountViews(ViewResolver viewResolver, int[] viewsToAdd, int[] viewsToDetach) { for (int viewToAdd : viewsToAdd) { if (viewToAdd > 0) { + // This view was not previously attached to this parent. View view = viewResolver.getView(viewToAdd); ensureViewHasNoParent(view); - mFlatViewGroup.addViewInLayout(view); - } else { - View view = viewResolver.getView(-viewToAdd); - ensureViewHasNoParent(view); - DrawView drawView = Assertions.assertNotNull(mDrawViewMap.get(-viewToAdd)); - if (!drawView.mPreviouslyDrawn) { - // The DrawView has not been drawn before, which means the bounds changed and triggered - // a new DrawView when it was collected from the shadow node. We have a view with the - // same id temporarily detached, but we no longer know the bounds. - unclip(drawView.reactTag); - mFlatViewGroup.attachViewToParent(view); - } else if (!isClipped(drawView.reactTag)) { - // The DrawView has been drawn before, and is not clipped. Attach it, and it will get - // removed if we update the clipping rect. - mFlatViewGroup.attachViewToParent(view); + DrawView drawView = Assertions.assertNotNull(mDrawViewMap.get(viewToAdd)); + drawView.mWasMounted = true; + if (animating(view) || withinBounds(drawView)) { + // View should be drawn. This view can't currently be clipped because it wasn't + // previously attached to this parent. + mFlatViewGroup.addViewInLayout(view); + } else { + clip(drawView.reactTag, view); + } + } else { + // This view was previously attached, and just temporarily detached. + DrawView drawView = Assertions.assertNotNull(mDrawViewMap.get(-viewToAdd)); + View view = viewResolver.getView(drawView.reactTag); + ensureViewHasNoParent(view); + if (drawView.mWasMounted) { + // The DrawView has been mounted before. + if (!isClipped(drawView.reactTag)) { + // The DrawView is not clipped. Attach it. + mFlatViewGroup.attachViewToParent(view); + } + // else The DrawView has been previously mounted and is clipped, so don't attach it. + } else { + // We are mounting it, so lets get this part out of the way. + drawView.mWasMounted = true; + // The DrawView has not been mounted before, which means the bounds changed and triggered + // a new DrawView when it was collected from the shadow node. We have a view with the + // same id temporarily detached, but its bounds have changed. + if (animating(view) || withinBounds(drawView)) { + // View should be drawn. + if (isClipped(drawView.reactTag)) { + // View was clipped, so add it. + mFlatViewGroup.addViewInLayout(view); + unclip(drawView.reactTag); + } else { + // View was just temporarily removed, so attach it. We already know it isn't clipped, + // so no need to unclip it. + mFlatViewGroup.attachViewToParent(view); + } + } else { + // View should be clipped. + if (!isClipped(drawView.reactTag)) { + // View was onscreen. + mFlatViewGroup.removeDetachedView(view); + clip(drawView.reactTag, view); + } + // else view is already clipped and not within bounds. + } } - // The DrawView has been previously drawn and is clipped, so don't attach it. } } @@ -133,6 +165,15 @@ import com.facebook.react.views.view.ReactClippingViewGroupHelper; } } + // Return true if a DrawView is currently onscreen. + boolean withinBounds(DrawView drawView) { + return mClippingRect.intersects( + Math.round(drawView.getLeft()), + Math.round(drawView.getTop()), + Math.round(drawView.getRight()), + Math.round(drawView.getBottom())); + } + @Override public boolean updateClippingRect() { ReactClippingViewGroupHelper.calculateClippingRect(mFlatViewGroup, mClippingRect); @@ -155,7 +196,7 @@ import com.facebook.react.views.view.ReactClippingViewGroupHelper; // Now off the screen. Don't invalidate in this case, as the canvas should not be // redrawn unless new elements are coming onscreen. clip(drawView.reactTag, view); - mFlatViewGroup.detachView(--index); + mFlatViewGroup.removeViewsInLayout(--index, 1); } } else { // Clipped, invisible. We obviously aren't animating here, as if we were then we would not @@ -163,7 +204,7 @@ import com.facebook.react.views.view.ReactClippingViewGroupHelper; if (withinBounds(view)) { // Now on the screen. Invalidate as we have a new element to draw. unclip(drawView.reactTag); - mFlatViewGroup.attachViewToParent(view, index++); + mFlatViewGroup.addViewInLayout(view, index++); needsInvalidate = true; } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/flat/DrawImageWithPipeline.java b/ReactAndroid/src/main/java/com/facebook/react/flat/DrawImageWithPipeline.java index aae8c6e30..a8a0b84e0 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/flat/DrawImageWithPipeline.java +++ b/ReactAndroid/src/main/java/com/facebook/react/flat/DrawImageWithPipeline.java @@ -120,10 +120,11 @@ import com.facebook.react.views.imagehelper.MultiSourceHelper.MultiSourceResult; mFadeDuration = fadeDuration; } + /** + * If the bitmap isn't loaded by the first draw, fade it in, otherwise don't fade. + */ @Override protected void onPreDraw(FlatViewGroup parent, Canvas canvas) { - super.onPreDraw(parent, canvas); - Bitmap bitmap = Assertions.assumeNotNull(mRequestHelper).getBitmap(); if (bitmap == null) { mFirstDrawTime = 0; @@ -167,7 +168,7 @@ import com.facebook.react.views.imagehelper.MultiSourceHelper.MultiSourceResult; PAINT.setShader(mBitmapShader); canvas.drawPath(getPathForRoundedBitmap(), PAINT); } - + bitmap = null; drawBorders(canvas); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/flat/DrawView.java b/ReactAndroid/src/main/java/com/facebook/react/flat/DrawView.java index 530c19478..ec88ca215 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/flat/DrawView.java +++ b/ReactAndroid/src/main/java/com/facebook/react/flat/DrawView.java @@ -11,41 +11,51 @@ package com.facebook.react.flat; import android.graphics.Canvas; -/* package */ final class DrawView extends AbstractClippingDrawCommand { +/* package */ final class DrawView extends AbstractDrawCommand { /* package */ final int reactTag; - // Indicates if the DrawView is frozen. If it is frozen then any setting of the clip bounds - // should create a new DrawView. - private boolean mFrozen; - // Indicates whether this DrawView has been previously drawn. If it has been drawn, then we know - // that the bounds haven't changed, as a bounds change would trigger a new DrawView, which will - // set this to false for the new DrawView. Leaving this as package for direct access, but this - // should only be set from draw in DrawView, to avoid race conditions. - /* package */ boolean mPreviouslyDrawn; + // Indicates whether this DrawView has been previously mounted to a clipping FlatViewGroup. This + // lets us know that the bounds haven't changed, as a bounds change would trigger a new DrawView, + // which will set this to false for the new DrawView. This is safe, despite the dual access with + // FlatViewGroup, because the FlatViewGroup copy is only ever modified by the FlatViewGroup. + // Changing how this boolean is used should be handled with caution, as race conditions are the + // quickest way to create unreproducible super bugs. + /* package */ boolean mWasMounted; public DrawView(int reactTag) { this.reactTag = reactTag; } + /** + * Similar to updateBoundsAndFreeze, but thread safe as the mounting flag is modified on the UI + * thread. + * + * @return A DrawView with the passed bounds and clipping bounds. If we can use the same + * DrawView, it will just be this, otherwise it will be a frozen copy. + */ public DrawView collectDrawView( + float left, + float top, + float right, + float bottom, float clipLeft, float clipTop, float clipRight, float clipBottom) { - if (mFrozen) { - return clipBoundsMatch(clipLeft, clipTop, clipRight, clipBottom) ? - this : - new DrawView(reactTag).collectDrawView(clipLeft, clipTop, clipRight, clipBottom); - } else { - mFrozen = true; - setClipBounds(clipLeft, clipTop, clipRight, clipBottom); - return this; + DrawView drawView = (DrawView) + updateBoundsAndFreeze(left, top, right, bottom, clipLeft, clipTop, clipRight, clipBottom); + if (drawView != this) { + // It is very important that we unset this, as our spec is that newly created DrawViews are + // handled differently by the FlatViewGroup. This is needed because updateBoundsAndFreeze + // uses .clone(), so we maintain the previous state. + drawView.mWasMounted = false; } + return drawView; } @Override public void draw(FlatViewGroup parent, Canvas canvas) { - mPreviouslyDrawn = true; + onPreDraw(parent, canvas); if (mNeedsClipping) { canvas.save(Canvas.CLIP_SAVE_FLAG); applyClipping(canvas); @@ -56,6 +66,11 @@ import android.graphics.Canvas; } } + @Override + protected void onDraw(Canvas canvas) { + // no op as we override draw. + } + @Override public void debugDraw(FlatViewGroup parent, Canvas canvas) { parent.debugDrawNextChild(canvas); diff --git a/ReactAndroid/src/main/java/com/facebook/react/flat/FlatShadowNode.java b/ReactAndroid/src/main/java/com/facebook/react/flat/FlatShadowNode.java index 28cc4fa45..09a1641ff 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/flat/FlatShadowNode.java +++ b/ReactAndroid/src/main/java/com/facebook/react/flat/FlatShadowNode.java @@ -468,17 +468,32 @@ import com.facebook.react.views.view.ReactClippingViewGroupHelper; } } - /* package */ final DrawView collectDrawView(float left, float top, float right, float bottom) { + /* package */ final DrawView collectDrawView( + float left, + float top, + float right, + float bottom, + float clipLeft, + float clipTop, + float clipRight, + float clipBottom) { Assertions.assumeNotNull(mDrawView); if (mDrawView.reactTag == 0) { // This is the first time we have collected this DrawView, but we have to create a new - // DrawView anyway, as reactTag is final. - mDrawView = new DrawView(getReactTag()).collectDrawView(left, top, right, bottom); - } else { - // We have collected the DrawView before, so the react tag is correct, but we may need a new - // copy with updated bounds. If the bounds match, the same view is returned. - mDrawView = mDrawView.collectDrawView(left, top, right, bottom); + // DrawView anyway, as reactTag is final, and our DrawView instance is the static copy. + mDrawView = new DrawView(getReactTag()); } + // We have the correct react tag, but we may need a new copy with updated bounds. If the bounds + // match or were never set, the same view is returned. + mDrawView = mDrawView.collectDrawView( + left, + top, + right, + bottom, + clipLeft, + clipTop, + clipRight, + clipBottom); return mDrawView; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/flat/FlatViewGroup.java b/ReactAndroid/src/main/java/com/facebook/react/flat/FlatViewGroup.java index 731afc44e..d5db547bb 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/flat/FlatViewGroup.java +++ b/ReactAndroid/src/main/java/com/facebook/react/flat/FlatViewGroup.java @@ -401,11 +401,7 @@ import com.facebook.react.views.view.ReactClippingViewGroup; @Override protected void onDetachedFromWindow() { if (!mIsAttached) { - // Hack. Our current behaviour of add then immediately remove if a view is clipped pretty - // much guarantees that we kill network requests for images in feed. We have a fix, but are - // going to add it in master and patch this in in the meantime. - return; - // throw new RuntimeException("Double detach"); + throw new RuntimeException("Double detach"); } mIsAttached = false; @@ -713,12 +709,12 @@ import com.facebook.react.views.view.ReactClippingViewGroup; addViewInLayout(view, -1, ensureLayoutParams(view.getLayoutParams()), true); } - /* package */ void attachViewToParent(View view) { - attachViewToParent(view, -1, ensureLayoutParams(view.getLayoutParams())); + /* package */ void addViewInLayout(View view, int index) { + addViewInLayout(view, index, ensureLayoutParams(view.getLayoutParams()), true); } - /* package */ void attachViewToParent(View view, int index) { - attachViewToParent(view, index, ensureLayoutParams(view.getLayoutParams())); + /* package */ void attachViewToParent(View view) { + attachViewToParent(view, -1, ensureLayoutParams(view.getLayoutParams())); } private void processLayoutRequest() { @@ -756,6 +752,8 @@ import com.facebook.react.views.view.ReactClippingViewGroup; int right = Integer.MIN_VALUE; int bottom = Integer.MIN_VALUE; for (int i = 0; i < childCount; i++) { + // This is technically a dupe, since the DrawView has its bounds, but leaving in to handle if + // the View is animating or rebelling against the DrawView bounds for some reason. View child = getChildAt(i); left = Math.min(left, child.getLeft()); top = Math.min(top, child.getTop()); diff --git a/ReactAndroid/src/main/java/com/facebook/react/flat/StateBuilder.java b/ReactAndroid/src/main/java/com/facebook/react/flat/StateBuilder.java index 6cd0761f9..a966e6694 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/flat/StateBuilder.java +++ b/ReactAndroid/src/main/java/com/facebook/react/flat/StateBuilder.java @@ -536,6 +536,10 @@ import com.facebook.react.uimanager.events.EventDispatcher; addNativeChild(node); if (!parentIsAndroidView) { mDrawCommands.add(node.collectDrawView( + left, + top, + right, + bottom, parentClipLeft, parentClipTop, parentClipRight, @@ -544,8 +548,8 @@ import com.facebook.react.uimanager.events.EventDispatcher; updated = collectStateForMountableNode( node, - left - left, - top - top, + 0, // left - left + 0, // top - top right - left, bottom - top, parentClipLeft - left,