2.x: Add lazy loading of default main Scheduler #338

peter-tackage · 2016-10-05T22:59:24Z

Initial proposal to address #332

I've essentially used the same implementation as in RxJava: ReactiveX/RxJava#4585.

For the sake of common implementation, I've used RxJava's internal(?) classes ObjectHelper and ExceptionHelper. Happy to consider alternatives if we don't want this.

JakeWharton · 2016-10-06T18:53:38Z

rxandroid/src/main/java/io/reactivex/android/plugins/RxAndroidPlugins.java

 import io.reactivex.functions.Function;
+import io.reactivex.internal.functions.ObjectHelper;
+import io.reactivex.internal.util.ExceptionHelper;


Remove both of these. We cannot depend on internal classes.

JakeWharton · 2016-10-06T18:53:49Z

rxandroid/src/main/java/io/reactivex/android/plugins/RxAndroidPlugins.java

- public static Scheduler initMainThreadScheduler(Scheduler scheduler) {
- Function<Scheduler, Scheduler> f = onInitMainThreadHandler;
+ public static Scheduler initMainThreadScheduler(Callable<Scheduler> scheduler) {
+ ObjectHelper.requireNonNull(scheduler, "Scheduler Callable can't be null");


Just do a null check yourself

Also change error message to "scheduler == null"

JakeWharton · 2016-10-06T18:53:56Z

rxandroid/src/main/java/io/reactivex/android/plugins/RxAndroidPlugins.java

 }

 public static void setMainThreadSchedulerHandler(Function<Scheduler, Scheduler> handler) {
 onMainThreadHandler = handler;
 }

 public static Scheduler onMainThreadScheduler(Scheduler scheduler) {
+ ObjectHelper.requireNonNull(scheduler, "Scheduler can't be null");


JakeWharton · 2016-10-06T18:54:37Z

rxandroid/src/main/java/io/reactivex/android/plugins/RxAndroidPlugins.java

+ try {
+ return ObjectHelper.requireNonNull(s.call(), "Scheduler Callable result can't be null");
+ } catch (Throwable ex) {
+ throw ExceptionHelper.wrapOrThrow(ex);


Inline this implementation.

How about I change it back to Exceptions.propagate which is functionally the same and uses a public RxJava API?

JakeWharton · 2016-10-06T18:54:44Z

rxandroid/src/main/java/io/reactivex/android/plugins/RxAndroidPlugins.java

@@ -64,6 +62,26 @@ public static void reset() {
 setMainThreadSchedulerHandler(null);
 }

+ static Scheduler callRequireNonNull(Callable<Scheduler> s) {
+ try {
+ return ObjectHelper.requireNonNull(s.call(), "Scheduler Callable result can't be null");


Just do null check yourself

I think "returned null" instead of "result can't be null" is more clear

JakeWharton · 2016-10-06T18:57:58Z

rxandroid/src/test/java/io/reactivex/android/plugins/RxAndroidPluginsTest.java

+ };
+
+ // unsafe default Scheduler Callable should not be evaluated when overriding
+ try {


This whole try/catch isn't needed. The plugins are reset before and after each test.

JakeWharton · 2016-10-06T18:58:10Z

rxandroid/src/test/java/io/reactivex/android/plugins/RxAndroidPluginsTest.java

+ // fail when Callable is null
+ try {
+ RxAndroidPlugins.initMainThreadScheduler(null);
+ fail("Should have thrown NullPointerException");


Remove message from fail().

JakeWharton · 2016-10-06T18:58:28Z

rxandroid/src/test/java/io/reactivex/android/plugins/RxAndroidPluginsTest.java

+
+ @Test
+ public void overrideInitMainSchedulerThrowsWhenSchedulerCallableIsNull() {
+ // fail when Callable is null


Comment is redundant with test name

JakeWharton · 2016-10-06T18:58:43Z

rxandroid/src/test/java/io/reactivex/android/plugins/RxAndroidPluginsTest.java

+ }
+ };
+
+ // fail when Callable result is null


Comment is redundant with test name.

JakeWharton · 2016-10-06T18:58:52Z

rxandroid/src/test/java/io/reactivex/android/plugins/RxAndroidPluginsTest.java

+ // fail when Callable result is null
+ try {
+ RxAndroidPlugins.initMainThreadScheduler(nullResultCallable);
+ fail("Should have thrown NullPointerException");


remove message

JakeWharton

This looks great. Just a few really tiny style comments.

JakeWharton · 2016-10-07T20:38:23Z

rxandroid/src/main/java/io/reactivex/android/plugins/RxAndroidPlugins.java

- if (f == null) {
- return scheduler;
+ public static Scheduler initMainThreadScheduler(Callable<Scheduler> scheduler) {
+ if(scheduler == null) {


nit: space after if

Is there a project style settings file (IntelliJ/AS format) available? I've been manually formatting which can lead to missing stuff like this.

JakeWharton · 2016-10-07T20:38:51Z

rxandroid/src/main/java/io/reactivex/android/plugins/RxAndroidPlugins.java

+ static Scheduler callRequireNonNull(Callable<Scheduler> s) {
+ try {
+ Scheduler scheduler = s.call();
+ if(scheduler == null) {


nit: space after if

JakeWharton · 2016-10-07T20:40:27Z

rxandroid/src/test/java/io/reactivex/android/plugins/RxAndroidPluginsTest.java

+ RxAndroidPlugins
+ .setInitMainThreadSchedulerHandler(new Function<Callable<Scheduler>, Scheduler>() {
+ @Override public Scheduler apply(Callable<Scheduler> scheduler) {
+ throw new RuntimeException();


should be AssertionError for better semantics

JakeWharton · 2016-10-07T20:40:38Z

rxandroid/src/test/java/io/reactivex/android/plugins/RxAndroidPluginsTest.java

+ public void defaultMainThreadSchedulerIsInitializedLazily() {
+ final Function<Callable<Scheduler>, Scheduler> safeOverride =
+ new Function<Callable<Scheduler>, Scheduler>() {
+ @Override public Scheduler apply(Callable<Scheduler> __) {


__ -> scheduler

JakeWharton · 2016-10-07T20:41:16Z

rxandroid/src/test/java/io/reactivex/android/plugins/RxAndroidPluginsTest.java

+ return new EmptyScheduler();
+ }
+ };
+ final Callable<Scheduler> unsafeDefault = new Callable<Scheduler>() {


nit: doesn't need to be final

JakeWharton · 2016-10-07T20:41:22Z

rxandroid/src/test/java/io/reactivex/android/plugins/RxAndroidPluginsTest.java

+
+ @Test
+ public void defaultMainThreadSchedulerIsInitializedLazily() {
+ final Function<Callable<Scheduler>, Scheduler> safeOverride =


nit: doesn't need to be final

JakeWharton · 2016-10-07T21:09:50Z

Sadly, no. I just use our Square style except with the (terrible) 4 space
indent.

On Fri, Oct 7, 2016 at 5:04 PM Peter Tackage [email protected]
wrote:

@peter-tackage commented on this pull request.

In
rxandroid/src/main/java/io/reactivex/android/plugins/RxAndroidPlugins.java
#338:
     onInitMainThreadHandler = handler;
 }
public static Scheduler initMainThreadScheduler(Scheduler scheduler) {
   Function<Scheduler, Scheduler> f = onInitMainThreadHandler;
   if (f == null) {
       return scheduler;
public static Scheduler initMainThreadScheduler(Callable scheduler) {
   if(scheduler == null) {
Is there a project style settings file (IntelliJ/AS format) available?
I've been manually formatting which can lead to missing stuff like this.

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#338, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEEEScumvpcDq6hsOjtPt4mVndnvunmks5qxrPNgaJpZM4KPYKd
.

JakeWharton · 2016-10-07T21:11:52Z

Er, that was a reply to your inline comment. Guess it doesn't work well from email.

peter-tackage · 2016-10-07T22:03:52Z

OK, I think I've covered each of your comments. Thanks for the review.

JakeWharton · 2016-10-07T23:49:57Z

Thanks!

Add lazy loading of default main Scheduler

d18cc6a

peter-tackage changed the title ~~Add lazy loading of default main Scheduler~~ 2.x: Add lazy loading of default main Scheduler Oct 5, 2016

Peter Tackage added 3 commits October 6, 2016 01:04

Tidy formatting

0810d07

More tidy formatting

1857cf6

Remove unnecessary dependency

a710149

JakeWharton reviewed Oct 6, 2016

View reviewed changes

Peter Tackage added 2 commits October 7, 2016 20:50

Address some review comments

ee5d34b

Formatting

9e3d911

JakeWharton approved these changes Oct 7, 2016

View reviewed changes

Peter Tackage added 2 commits October 7, 2016 23:08

Tidying and some address some review comments

9521b90

Formatting nit

9367319

Use AssertionError for consistency

495d8c4

Don't use __ parameter

cb8265f

JakeWharton merged commit 79d0991 into ReactiveX:2.x Oct 7, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2.x: Add lazy loading of default main Scheduler #338

2.x: Add lazy loading of default main Scheduler #338

peter-tackage commented Oct 5, 2016 •

edited

Loading

JakeWharton Oct 6, 2016

JakeWharton Oct 6, 2016

JakeWharton Oct 6, 2016

JakeWharton Oct 6, 2016

JakeWharton Oct 6, 2016

peter-tackage Oct 7, 2016

JakeWharton Oct 7, 2016

JakeWharton Oct 6, 2016

JakeWharton Oct 6, 2016

JakeWharton Oct 6, 2016

JakeWharton Oct 6, 2016

JakeWharton Oct 6, 2016 •

edited

Loading

JakeWharton Oct 6, 2016

JakeWharton Oct 6, 2016

JakeWharton left a comment

JakeWharton Oct 7, 2016

peter-tackage Oct 7, 2016

JakeWharton Oct 7, 2016

JakeWharton Oct 7, 2016

JakeWharton Oct 7, 2016

JakeWharton Oct 7, 2016

JakeWharton Oct 7, 2016

JakeWharton commented Oct 7, 2016

@peter-tackage commented on this pull request.

JakeWharton commented Oct 7, 2016

peter-tackage commented Oct 7, 2016

JakeWharton commented Oct 7, 2016

2.x: Add lazy loading of default main Scheduler #338

2.x: Add lazy loading of default main Scheduler #338

Conversation

peter-tackage commented Oct 5, 2016 • edited Loading

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

Choose a reason for hiding this comment

JakeWharton Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeWharton 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

JakeWharton commented Oct 7, 2016

@peter-tackage commented on this pull request.

JakeWharton commented Oct 7, 2016

peter-tackage commented Oct 7, 2016

JakeWharton commented Oct 7, 2016

peter-tackage commented Oct 5, 2016 •

edited

Loading

JakeWharton Oct 6, 2016 •

edited

Loading