Skip to content

Fix observeOn() invocation order. #67

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

Jump to bottom
Merged
merged 1 commit into from
Nov 24, 2014
Merged

Fix observeOn() invocation order. #67

merged 1 commit into from
Nov 24, 2014

Conversation

omo
Copy link
Contributor

@omo omo commented Nov 21, 2014

It should be called before lift() so that the operator can observe it on the main thread.

I tried to write a test but it didn't work, presumably because Robolectric doesn't have an event loop?

omo referenced this pull request Nov 21, 2014
@omo
bindActivity(): Move observeOn() rightside.
9d36905 
To make it conventional.
@dpsm
Copy link
Contributor

dpsm commented Nov 21, 2014

@omo can you post your test to the pull request as well so I can pull it down and help you with robolectric? It is important that we have tests that specify these kinds of behaviours as we decide on them.

Thanks!

@omo
Copy link
Contributor Author

omo commented Nov 21, 2014

@dpsm Well, I should've tried a bit harder. A quick googling told me that Robolectric has a way a way to pump the event loop.

Added tests. PTAL?

dpsm
dpsm reviewed Nov 21, 2014
View reviewed changes
rxandroid/src/test/java/rx/android/TestUtil.java Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't that the same as http://robolectric.org/javadoc/org/robolectric/Robolectric.html#runUiThreadTasksIncludingDelayedTasks()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, this is exactly what I wanted here. Thanks for the tip! Updated.

@JakeWharton
Copy link
Contributor

This needs a rebase now.

@omo
Copy link
Contributor Author

omo commented Nov 21, 2014

Done.

JakeWharton
JakeWharton reviewed Nov 21, 2014
View reviewed changes
rxandroid/src/main/java/rx/android/content/OperatorConditionalBinding.java Outdated
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 right? I thought it moved to rx.android.internal.

@omo
Fix observeOn() invocation order.
7be64f3 
It should be called before lift() so that the operator can observe it on the main thread.
This change also adds some test for other bindXx() variants to get better coverage.
@omo
Copy link
Contributor Author

omo commented Nov 21, 2014

Oops. I left some change uncommitted. Sorry about that. It should be fixed now.

@omo
Copy link
Contributor Author

omo commented Nov 24, 2014

Ping? This is a regression fix with test. Travis build is green :-)

mttkay added a commit that referenced this pull request Nov 24, 2014
@mttkay
Merge pull request #67 from omo/observeon-first
c22d0bb 
Fix observeOn() invocation order.
@mttkay mttkay merged commit c22d0bb into ReactiveX:0.x Nov 24, 2014
@mttkay
Copy link
Collaborator

mttkay commented Nov 24, 2014

LGTM 👍

@omo
Copy link
Contributor Author

omo commented Nov 24, 2014

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@omo @dpsm @JakeWharton @mttkay