Skip to content

Commit

Permalink
Fix RefreshControl race condition
Browse files Browse the repository at this point in the history
Summary:
Improved version of #7317.

`setRefreshing` and `setProgressViewOffset` needs to be called after the view has been layed out. Instead of using `post` to do that we update the `refreshing` and `progressViewOffset` values in the first call to `onLayout`.

I also noticed that `progressViewOffset` default value wasn't exactly the same as when not calling `setProgressViewOffset` at all. Tweaked the values to match android defaults.

**Test plan (required)**
Make sure the integration test passes,
In UIExplorer: test RefreshControl with `refreshing = true` initially, test `progressViewOffset`.
Closes #7683

Differential Revision: D3334426

fbshipit-source-id: ddd63a5e9a6afe2b8b7fe6a25e875a40f4e888c6
  • Loading branch information
janicduplessis authored and Facebook Github Bot 9 committed May 23, 2016
1 parent 69bf0bd commit bb5aede
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,63 @@

import com.facebook.react.bridge.ReactContext;
import com.facebook.react.uimanager.events.NativeGestureUtil;
import com.facebook.react.uimanager.PixelUtil;

/**
* Basic extension of {@link SwipeRefreshLayout} with ReactNative-specific functionality.
*/
public class ReactSwipeRefreshLayout extends SwipeRefreshLayout {

private static final float DEFAULT_CIRCLE_TARGET = 64;

private boolean mDidLayout = false;

private boolean mRefreshing = false;
private float mProgressViewOffset = 0;


public ReactSwipeRefreshLayout(ReactContext reactContext) {
super(reactContext);
}

@Override
public void setRefreshing(boolean refreshing) {
mRefreshing = refreshing;

// `setRefreshing` must be called after the initial layout otherwise it
// doesn't work when mounting the component with `refreshing = true`.
// Known Android issue: https://code.google.com/p/android/issues/detail?id=77712
if (mDidLayout) {
super.setRefreshing(refreshing);
}
}

public void setProgressViewOffset(float offset) {
mProgressViewOffset = offset;

// The view must be measured before calling `getProgressCircleDiameter` so
// don't do it before the initial layout.
if (mDidLayout) {
int diameter = getProgressCircleDiameter();
int start = Math.round(PixelUtil.toPixelFromDIP(offset)) - diameter;
int end = Math.round(PixelUtil.toPixelFromDIP(offset + DEFAULT_CIRCLE_TARGET) - diameter);
setProgressViewOffset(false, start, end);
}
}

@Override
public void onLayout(boolean changed, int left, int top, int right, int bottom) {
super.onLayout(changed, left, top, right, bottom);

if (!mDidLayout) {
mDidLayout = true;

// Update values that must be set after initial layout.
setProgressViewOffset(mProgressViewOffset);
setRefreshing(mRefreshing);
}
}

@Override
public boolean onInterceptTouchEvent(MotionEvent ev) {
if (super.onInterceptTouchEvent(ev)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import com.facebook.react.bridge.ReadableArray;
import com.facebook.react.common.MapBuilder;
import com.facebook.react.common.SystemClock;
import com.facebook.react.uimanager.PixelUtil;
import com.facebook.react.uimanager.ThemedReactContext;
import com.facebook.react.uimanager.UIManagerModule;
import com.facebook.react.uimanager.ViewGroupManager;
Expand All @@ -33,8 +32,6 @@
*/
public class SwipeRefreshLayoutManager extends ViewGroupManager<ReactSwipeRefreshLayout> {

public static final float REFRESH_TRIGGER_DISTANCE = 48;

@Override
protected ReactSwipeRefreshLayout createViewInstance(ThemedReactContext reactContext) {
return new ReactSwipeRefreshLayout(reactContext);
Expand Down Expand Up @@ -74,30 +71,13 @@ public void setSize(ReactSwipeRefreshLayout view, int size) {
}

@ReactProp(name = "refreshing")
public void setRefreshing(final ReactSwipeRefreshLayout view, final boolean refreshing) {
// Use `post` otherwise the control won't start refreshing if refreshing is true when
// the component gets mounted.
view.post(new Runnable() {
@Override
public void run() {
view.setRefreshing(refreshing);
}
});
public void setRefreshing(ReactSwipeRefreshLayout view, boolean refreshing) {
view.setRefreshing(refreshing);
}

@ReactProp(name = "progressViewOffset", defaultFloat = 0)
public void setProgressViewOffset(final ReactSwipeRefreshLayout view, final float offset) {
// Use `post` to get progress circle diameter properly
// Otherwise returns 0
view.post(new Runnable() {
@Override
public void run() {
int diameter = view.getProgressCircleDiameter();
int start = Math.round(PixelUtil.toPixelFromDIP(offset)) - diameter;
int end = Math.round(PixelUtil.toPixelFromDIP(offset + REFRESH_TRIGGER_DISTANCE));
view.setProgressViewOffset(false, start, end);
}
});
view.setProgressViewOffset(offset);
}

@Override
Expand Down

1 comment on commit bb5aede

@zhuyifan2013
Copy link

@zhuyifan2013 zhuyifan2013 commented on bb5aede Aug 20, 2017

Choose a reason for hiding this comment

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

This PR causes the first refresh ignores the progressViewOffset , it is so bad @janicduplessis

Please sign in to comment.