Converting examples to RxJava2 #5141
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Conflicts: CHANGELOG.md
|
I've encountered the following problem: It seems a similar problem with #4662. Since it is not related to this PR, I will only focus on examples which use RealmJava. |
|
@dalinaum Android Gradle Plugin |
|
It sometimes encounters the following error: I should dig into this tomorrow. |
Conflicts: realm/realm-library/build.gradle realm/realm-library/src/main/java/io/realm/BaseRealm.java realm/realm-library/src/main/java/io/realm/RealmList.java realm/realm-library/src/main/java/io/realm/RealmResults.java
|
After merging feature/rxjava2 into this branch again, I don't encounter native crashes yet. So I will focus on converting rxJavaExample first. |
Zhuinden
reviewed
Aug 31, 2017
examples/rxJavaExample/build.gradle
Outdated
| implementation 'io.reactivex.rxjava2:rxjava:2.1.0' | ||
| implementation 'com.squareup.retrofit2:retrofit:2.3.0' | ||
| implementation 'com.jakewharton.rxbinding2:rxbinding:2.0.0' | ||
| compile 'com.squareup.retrofit2:adapter-rxjava2:2.3.0' |
There was a problem hiding this comment.
why is some compile and other implementation?
Zhuinden
reviewed
Aug 31, 2017
| return realm.where(Person.class).findAllSorted("name").get(0); | ||
| } | ||
| }); | ||
|
|
||
| new Function<Realm, Person>() { |
Zhuinden
reviewed
Aug 31, 2017
| subscription = realm.where(Person.class).findAllAsync().asObservable() | ||
| .flatMap(new Func1<RealmResults<Person>, Observable<Person>>() { | ||
| disposable = realm.where(Person.class).findAllAsync().asFlowable() | ||
| .flatMap(new Function<RealmResults<Person>, Publisher<Person>>() { |
There was a problem hiding this comment.
You know, I've actually just realized that with Android build tools 3.0, lambda support is out of the box, even without Retrolambda.
I'll try to give a mention to @cmelchior to see what he thinks about converting these Functions and Predicates to lambda expressions instead? Although you still need to tell Gradle that you're actually Java 8 language compatible.
There was a problem hiding this comment.
Converting to Java 8 is fine with me 👍 Would improve the readability quite a lot for this.
Zhuinden
reviewed
Aug 31, 2017
| @Override | ||
| public Observable<RealmResults<Person>> call(TextViewTextChangeEvent event) { | ||
| .toFlowable( BackpressureStrategy.BUFFER) | ||
| .flatMap(new Function<TextViewTextChangeEvent, Publisher<RealmResults<Person>>>() { |
There was a problem hiding this comment.
Hm, shouldn't this be switchMap()?
Zhuinden
reviewed
Aug 31, 2017
| @@ -39,6 +39,7 @@ | |||
| testPersons.put("Donn", "donnfelker"); | |||
| testPersons.put("Nabil", "nhachicha"); | |||
| testPersons.put("Ron", null); | |||
| testPersons.put("Leonardo", "dalinaum"); | |||
There was a problem hiding this comment.
Welcome to test persons 😄
Zhuinden
reviewed
Sep 1, 2017
| }); | ||
| // We only want the list once it is loaded. | ||
| .filter(people -> people.isLoaded()) | ||
| .flatMap(people -> Flowable.fromIterable(people)) |
There was a problem hiding this comment.
I wonder if this should also be switchMap().
|
@beeender It seems fixed. I don't encounter the native crash yet. Thanks. |
|
Since I merged I will change this PR's target to |
|
There is a native crash during |
cmelchior
approved these changes
Sep 11, 2017
| } | ||
|
|
||
| @Override | ||
| public void onPause() { | ||
| subscriptions.unsubscribe(); | ||
| disposables.dispose(); |
There was a problem hiding this comment.
Most people seem to think that disposables.clear() is the better to use: https://blog.kaush.co/2017/06/21/rxjava-1-rxjava-2-disposing-subscriptions/
Same in other places
There was a problem hiding this comment.
If you do disposables = null afterwards and re-create it when it is null then dispose() also works well.
clear() means you can re-use it, but you don't have to if you don't want to
There was a problem hiding this comment.
I will try to reuse disposables.
There was a problem hiding this comment.
I dunno, I tend to just use dispose() and be like whatever, it really depends on if you want to re-use them. For example, in Fragments, it might be safer to use clear() unless you do re-create the composite disposable.
| }) | ||
| private Disposable testSubscribeOn() { | ||
| Disposable subscribeOn = realm.asFlowable() | ||
| .map(realm -> realm.where(Person.class).findAllSorted("name").get(0)) |
There was a problem hiding this comment.
Not sure why this was added?
There was a problem hiding this comment.
Well it was already there 😄
There was a problem hiding this comment.
I'll remove that map operator and there are still weird things in example such as a useless button and other subtle things.
There was a problem hiding this comment.
The intent of testSubscribeOn seems to show that if Realm was created on the UI thread and accessing it on Schedulers.io() will crash. So I think I would not change this.
|
Link to PR #5197 |
|
@cmelchior @Zhuinden Just updated this PR. |
cmelchior
approved these changes
Sep 12, 2017
There was a problem hiding this comment.
You also forgot a uncommented line in the JenkinsFile I think https://github.com/realm/realm-java/blob/master/Jenkinsfile#L54
|
Looks good to me 👍 |
beeender
approved these changes
Sep 12, 2017
GulajavaMinistudio
added a commit
to GulajavaMinistudio/realm-java
that referenced
this pull request
Sep 13, 2017
Converting examples to RxJava2 (realm#5141)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #4991
TODO: