Convert RxJava1 to RxJava2 #4992

cmelchior · 2017-07-19T11:03:24Z

Fixes #4095
Part of #4991

Convert the existing API's to RxJava 2 types:

asObservable() -> asFlowable() ... Since Realm is a hot stream, dropping notifications and only using the latest is fine. The naming in the public API reflects this.
RealmQuery is exposed as Single(). Note this is still not used in our public API, but the primary use case would move the query to a background scheduler, run the query, and convert all types using copyFromRealm(). Single()` fits that nicely. Also, I don't see any use cases where you would be interested in listening to changes to the query construction.

Conflicts: CHANGELOG.md realm/realm-library/build.gradle realm/realm-library/src/main/java/io/realm/DynamicRealm.java

Conflicts: CHANGELOG.md

Zhuinden · 2017-07-19T13:00:40Z

realm/realm-library/src/main/java/io/realm/rx/RealmObservableFactory.java

-                subscriber.add(Subscriptions.create(new Action0() {
+
+                // Cleanup when stream is disposed
+                emitter.setDisposable(Disposables.fromRunnable(new Runnable() {


Well at least if I know right then RxJava2 solves the issue that addChangeListener actually had to be called after subscriber.add()

I'm not entirely sure what you are referring to?

#4095 figured it may as well be fixed if this stuff is being modified 😄

curiousily · 2017-07-19T15:28:08Z

realm/realm-library/src/main/java/io/realm/rx/RealmObservableFactory.java

        @Override
        protected StrongReferenceCounter<RealmModel> initialValue() {
            return new StrongReferenceCounter<RealmModel>();
        }
    };

+    private static final BackpressureStrategy BACK_PRESSURE_STRATEGY = BackpressureStrategy.LATEST;


Is this the only sensible back pressure strategy that Realm can support?

It is the one that makes the most sense. The UI generally doesn't care what changed, just that something changed, so yes.

Do you have any concerns about this?

@curiousily it is the only backpressure strategy that makes sense for RealmResults. It will always show its latest value, why enqueue events of the same thing?

Thanks for the clarification! Eagerly waiting to try out Realm with RxJava2. Will write back if some use case comes to mind 👍

alexrwegener · 2017-07-20T13:53:23Z

realm/realm-library/src/main/java/io/realm/rx/RealmObservableFactory.java

-                subscriber.add(Subscriptions.create(new Action0() {
+
+                // Cleanup when stream is disposed
+                emitter.setDisposable(Disposables.fromRunnable(new Runnable() {


Will automatically closing the local realm instance when disposing cause problems for longer running subscriptions on the same thread? Say you have a longer running Flowable and a shorter running Flowable using the same Realm instance and both are subscribed to. The shorter running Flowable is disposed which closes the realm instance. Would the longer running flowable no longer get updates at that point?

They won't use the same instance because each Realm.getInstance() call is reference counted and returns a "new local instance".

Missed that before in RealmCache. Thanks!

Zhuinden · 2017-07-20T13:57:17Z

The only thing that nags me a bit inside is that there is a rxjava3-preview which will eventually supercede RxJava2, and Flowable adheres to the reactive-streams specification that would mean you could just use org.reactivestreams.Publisher instead of RxJava2 specifically, which would be more future-proof.

But then RealmObservableFactory would need to have its own provided rxjava2 which would mean it'd move out to its own library just like realm-android-adapters did.

cmelchior · 2017-07-20T14:12:35Z

Publisher only has the subscribe method which is really limited :( Also we thought the same thing when we used rx.Observable as the interface for the original RxObsevableFactory, and then it changed.

Personally, I would rather have really great integration with the "most common" Rx library, and then extensions for any other (RxJava1, RxJava3, ). The RxRealm extension approach should work for anything and with Kotlin you even add them as extension methods for the fluent API.

Zhuinden · 2017-07-20T14:17:39Z

I never realized the default Publisher and Subscriber specification is so stupid that you get the Subscription not as a return value (like with subscribeWith()) but when you override onSubscribe() in the Subscriber? Yeah, that won't do. Let us be thankful that Flowable hides this. Realm adhering reactive-streams spec would be more trouble than worth. Nevermind me then 😄

Conflicts: realm/realm-library/src/main/java/io/realm/Realm.java

cmelchior · 2017-08-07T21:16:29Z

Ready for review @realm/java

bmeike

LGTM. So, we are supporting backpressure? I think it would be great if you would give a lightning talk, to the team about how that is going to work...

Zhuinden · 2017-08-11T18:37:49Z

@bmeike Flowable.LATEST to the best of my knowledge means that if there was an event emission, then only the latest is kept and sent to the subscribers.

Which makes sense because RealmResults when a notification is sent, only the latest is needed (every other event would also be the latest).

zaki50

+1 with minor comment

zaki50 · 2017-08-12T08:31:29Z

Jenkinsfile

@@ -51,7 +51,7 @@ try {
                }
              } finally {
                storeJunitResults 'realm/realm-annotations-processor/build/test-results/test/TEST-*.xml'
-                storeJunitResults 'examples/unitTestExample/build/test-results/**/TEST-*.xml'
+                // storeJunitResults 'examples/unitTestExample/build/test-results/**/TEST-*.xml'


Needs FIXME comment?

nhachicha · 2017-08-12T14:31:25Z

realm/realm-library/src/main/java/io/realm/rx/RxObservableFactory.java

@@ -41,7 +41,7 @@
     * @param realm {@link Realm} to listen to changes for.
     * @return Rx observable that emit all updates to the Realm.
     */
-    Observable<Realm> from(Realm realm);
+    Flowable<Realm> from(Realm realm);


should we change the Java doc for all these methods to Flowable instead of Observable ?

Creates a {@code Flowable} for a ...
@see http://reactivex.io/RxJava/2.x/javadoc/io/reactivex/Flowable.html

nhachicha · 2017-08-12T15:14:43Z

realm/realm-library/src/main/java/io/realm/rx/RealmObservableFactory.java

-                subscriber.add(Subscriptions.create(new Action0() {
+
+                // Cleanup when stream is disposed
+                emitter.setDisposable(Disposables.fromRunnable(new Runnable() {


why not use setCancellation ?

emitter.setCancellation(()-> { observableRealm.removeChangeListener(listener); observableRealm.close(); });

I think they kinda do the same thing and setDisposable works just fine.

After reading ReactiveX/RxJava#4812 setDisposable seemed like the right choice, but if anyone has any reason for why setCancellable is better, I'll be happy to change it.

I'm pretty sure it's dispose() for Realm's Observables.

nhachicha · 2017-08-12T15:22:24Z

realm/realm-library/src/androidTest/java/io/realm/RxJavaTests.java

            }
        });

-        subscription.unsubscribe();
+        subscription.dispose();
        assertTrue(dynamicRealm.isClosed());
    }

    @Test
    @UiThreadTest
    public void realmResults_closeInDoOnUnsubscribe() {


update test name to realmResults_closeInDoOnCancel same for all methods using *DoOnUnsubscribe

It seems to me that doOnUnsubscribe is more accurate, perhaps doOnDispose? From what I can tell unsubscribe is the general word used for telling a stream that you no longer want it around, you can do that by either calling dispose, not cancel. Also, subscribing returns a Disposable, not a Cancellable

nhachicha · 2017-08-12T15:25:57Z

realm/realm-library/src/androidTest/java/io/realm/RxJavaTests.java

            @Override
-            public void call(RealmResults<AllTypes> allTypes) {
+            public void accept(RealmResults<AllTypes> ignored) throws Exception {
            }


instead of doing nothing we can assert that the realm is still open or checking the refcount == 1 ?

* Add changelog * Move Pair into public API (#5081) * Convert RxJava1 to RxJava2 (#4992) * Apply rxjava2 to unitTestExample * Update Jenkinsfile to store junit result of unitTestExample. * Convert newsreaderExample to RxJava2. * RxJava2: Add support for changeset observables (#5089) * Update rxJavaExample and improve some formatting. * Fix unit testing. * Fix build.gradle. * Update build.gradle to fix lint errors. * Improve some formatting. * Improve formatting. * Update style of RxJavaExample. * Add missing colors.xml file. * Update styles and colors. * Rename some disposables. * PR feedback. * flatMap -> switchMap and improve formatting. * Improve formatting. * Converting newsreaserExample to lambda. * Improve formatting. * Improve formatting. * Apply RxJava2 to unit testing. * Improve formatting. * Apply Lambda to rxJavaExample. * Improve formatting. * PR feedback * Re-enable RxJava2 examples * CompositeDisposable.dispose() -> CompositeDisposable.clear() * PR feedback.

cmelchior added 4 commits June 8, 2017 13:02

Convert existing API to RxJava 2

e413747

Merge branch 'master-4.0' into cm/convert-rxjava1-to-rxjava2

39de17e

Conflicts: CHANGELOG.md realm/realm-library/build.gradle realm/realm-library/src/main/java/io/realm/DynamicRealm.java

Merge branch 'feature/rxjava2' into cm/convert-rxjava1-to-rxjava2

14394db

Conflicts: CHANGELOG.md

Mention changes to RxObservableFactory

54573bf

cmelchior added the S:In Progress label Jul 19, 2017

cmelchior self-assigned this Jul 19, 2017

Fix changelog

8521e56

Zhuinden mentioned this pull request Jul 19, 2017

Feature: RxJava2 Support #4991

Merged

6 tasks

Zhuinden reviewed Jul 19, 2017

View reviewed changes

curiousily reviewed Jul 19, 2017

View reviewed changes

alexrwegener reviewed Jul 20, 2017

View reviewed changes

Disable examples for now

20ff060

Disable unit test example

fb9ba18

cmelchior force-pushed the cm/convert-rxjava1-to-rxjava2 branch from 3023ab8 to 5b367d4 Compare August 2, 2017 09:39

cmelchior force-pushed the feature/rxjava2 branch from 27106b0 to dc0fe54 Compare August 2, 2017 09:43

Merge branch 'feature/rxjava2' into cm/convert-rxjava1-to-rxjava2

f119d6d

Conflicts: realm/realm-library/src/main/java/io/realm/Realm.java

cmelchior force-pushed the cm/convert-rxjava1-to-rxjava2 branch from 5b367d4 to f119d6d Compare August 2, 2017 09:44

cmelchior added 3 commits August 7, 2017 21:58

Disable collecting unitTest test results for now.

852353f

Merge branch 'feature/rxjava2' into cm/convert-rxjava1-to-rxjava2

0129a3b

Update test dependency

72fe063

cmelchior added S:Review and removed S:In Progress labels Aug 7, 2017

bmunkholm removed the S:Review label Aug 8, 2017

Merge branch 'feature/rxjava2' into cm/convert-rxjava1-to-rxjava2

e4738f7

cmelchior mentioned this pull request Aug 10, 2017

RxJava2: Add support for changeset observables #5089

Merged

1 task

bmeike approved these changes Aug 11, 2017

View reviewed changes

zaki50 approved these changes Aug 12, 2017

View reviewed changes

nhachicha reviewed Aug 12, 2017

View reviewed changes

PR feedback

16354eb

nhachicha approved these changes Aug 13, 2017

View reviewed changes

cmelchior merged commit f2f6e62 into feature/rxjava2 Aug 13, 2017

GulajavaMinistudio mentioned this pull request Sep 13, 2017

Converting examples to RxJava2 (#5141) GulajavaMinistudio/realm-java#104

Merged

cmelchior deleted the cm/convert-rxjava1-to-rxjava2 branch November 20, 2017 12:10

github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert RxJava1 to RxJava2 #4992

Convert RxJava1 to RxJava2 #4992

cmelchior commented Jul 19, 2017 •

edited

Zhuinden Jul 19, 2017 •

edited

cmelchior Jul 19, 2017

Zhuinden Jul 19, 2017 •

edited

curiousily Jul 19, 2017

cmelchior Jul 19, 2017

Zhuinden Jul 19, 2017

curiousily Jul 19, 2017

alexrwegener Jul 20, 2017

Zhuinden Jul 20, 2017 •

edited

alexrwegener Jul 20, 2017

Zhuinden commented Jul 20, 2017 •

edited

cmelchior commented Jul 20, 2017

Zhuinden commented Jul 20, 2017 •

edited

cmelchior commented Aug 7, 2017

bmeike left a comment

Zhuinden commented Aug 11, 2017

zaki50 left a comment

zaki50 Aug 12, 2017

nhachicha Aug 12, 2017

nhachicha Aug 12, 2017

Zhuinden Aug 13, 2017

cmelchior Aug 13, 2017

Zhuinden Aug 13, 2017

nhachicha Aug 12, 2017

cmelchior Aug 13, 2017

nhachicha Aug 12, 2017

Convert RxJava1 to RxJava2 #4992

Convert RxJava1 to RxJava2 #4992

Conversation

cmelchior commented Jul 19, 2017 • edited

Zhuinden Jul 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zhuinden Jul 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zhuinden Jul 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zhuinden commented Jul 20, 2017 • edited

cmelchior commented Jul 20, 2017

Zhuinden commented Jul 20, 2017 • edited

cmelchior commented Aug 7, 2017

bmeike left a comment

Choose a reason for hiding this comment

Zhuinden commented Aug 11, 2017

zaki50 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmelchior commented Jul 19, 2017 •

edited

Zhuinden Jul 19, 2017 •

edited

Zhuinden Jul 19, 2017 •

edited

Zhuinden Jul 20, 2017 •

edited

Zhuinden commented Jul 20, 2017 •

edited

Zhuinden commented Jul 20, 2017 •

edited