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.

Sign up for GitHub

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

Fix RefreshControl race condition #7317

Closed
wants to merge 2 commits into from
Closed

Fix RefreshControl race condition #7317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0d61933
Fix RefreshControl race condition
janicduplessis May 1, 2016
9f87e2d
Fix integration test
janicduplessis May 16, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions ...tAndroid/src/main/java/com/facebook/react/views/swiperefresh/ReactSwipeRefreshLayout.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,25 @@
*/
public class ReactSwipeRefreshLayout extends SwipeRefreshLayout {

private boolean mRefreshing = false;

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

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

Choose a reason for hiding this comment

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

Is this field useful?
It is read only once in the new Runnable below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the problem is when setRefreshing gets called twice in the same bridge transaction the order the runnables get executed is bad, see the example in the PR desc, adding the field makes sure is uses the latest value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see thanks, that makes sense.

// Use `post` otherwise the control won't start refreshing if refreshing is true when
// the component gets mounted.
post(new Runnable() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it is good or bad but we have ReactSwipeRefreshLayout that posts a new Runnable with setRefreshing call while all the other Runnables are invoked from SwipeRefreshLayoutManager.
Does not look consistent and makes me confused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that other runnable was added pretty recently and the same bug could happen to that value too.

I'll have another look at this tonight see if I can make this cleaner and make it consistent.

@Override
public void run() {
ReactSwipeRefreshLayout.super.setRefreshing(mRefreshing);
}
});
}

@Override
public boolean onInterceptTouchEvent(MotionEvent ev) {
if (super.onInterceptTouchEvent(ev)) {
Expand Down
11 changes: 2 additions & 9 deletions ...ndroid/src/main/java/com/facebook/react/views/swiperefresh/SwipeRefreshLayoutManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,15 +74,8 @@ 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)
Expand Down