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.

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

Proposal/rehash: Use async Messaging #416

Closed
wants to merge 21 commits into from
Closed
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
4 changes: 2 additions & 2 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ android:
components:
- tools
- platform-tools
- android-26
- build-tools-26.0.2
- android-28
- build-tools-28.0.0

# as per http://blog.travis-ci.com/2014-12-17-faster-builds-with-container-based-infrastructure/
sudo: false
Expand Down
9 changes: 5 additions & 4 deletions build.gradle
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
subprojects {
allprojects {
buildscript {
repositories {
mavenCentral()
jcenter()
google()
}
dependencies {
classpath 'com.android.tools.build:gradle:3.0.1'
classpath 'com.android.tools.build:gradle:3.1.3'
classpath 'com.github.dcendents:android-maven-gradle-plugin:2.0'
classpath 'com.jfrog.bintray.gradle:gradle-bintray-plugin:1.8.0'
classpath 'org.jfrog.buildinfo:build-info-extractor-gradle:4.5.2'
Expand All @@ -15,12 +15,13 @@ subprojects {

repositories {
mavenCentral()
google()
}
}

ext {
minSdkVersion = 9
compileSdkVersion = 26
buildToolsVersion = '26.0.2'
compileSdkVersion = 28
buildToolsVersion = '28.0.0'
sourceCompatibility = JavaVersion.VERSION_1_7
}
2 changes: 1 addition & 1 deletion gradle/wrapper/gradle-wrapper.properties
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@ distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-4.1-all.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-4.8.1-all.zip
Original file line number Diff line number Diff line change
Expand Up @@ -13,20 +13,21 @@
*/
package io.reactivex.android.schedulers;

import android.annotation.TargetApi;
import android.os.Build;
import android.os.Handler;
import android.os.Looper;

import java.util.concurrent.Callable;

import io.reactivex.Scheduler;
import io.reactivex.android.plugins.RxAndroidPlugins;
import java.lang.reflect.InvocationTargetException;
import java.util.concurrent.Callable;

/** Android-specific Schedulers. */
public final class AndroidSchedulers {

private static final class MainHolder {

static final Scheduler DEFAULT = new HandlerScheduler(new Handler(Looper.getMainLooper()));
static final Scheduler DEFAULT
= new HandlerScheduler(createAsyncHandler(Looper.getMainLooper()));
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it's going to require a major version update. The implications to the entirety of the world which uses RxAndroid are scary and a silent change in a minor release doesn't seem appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but a major change would require breaking with keeping the same major version as rxjava :|.

Either that or maybe do an extended preview release and ask for feedback?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we overload from(Looper) with a version that takes a boolean for async just like the Handler constructor no matter what.

As to getting it into mainThread(), you can use the RxAndroidPlugins to install an init callback for its creation which you can then delegate to from(Looper.getMainLooper(), true).

I think that's the minimum-viable API to at least get this into a release now for people to start testing it. And from there we can determine whether or not it's even possible to make it the default behind mainThread() in the next major version–or sometime in the future–and whether or not we need to expose a vsyncMainThread() or similar accessor to a singleton instance for code which truly needs the old behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That works too! For some reason I had it in my head that you were against having support for both. Can update. Should I add a brief mention/example in the readme with it or save that for changelog?

}

private static final Scheduler MAIN_THREAD = RxAndroidPlugins.initMainThreadScheduler(
Expand All @@ -47,6 +48,28 @@ public static Scheduler from(Looper looper) {
return new HandlerScheduler(new Handler(looper));
}

@TargetApi(Build.VERSION_CODES.P)
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree! But lint fails without it despite the if-check :|

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you try just using 28 as a literal? I prefer avoiding the codenames (which are thankfully almost dead) and letters (which we should be trying to kill next).

Copy link
Contributor Author

@ZacSweers ZacSweers Jun 28, 2018

Choose a reason for hiding this comment

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

Huh, yeah using just 28 worked. c9e2bbf

RIP dessert club

private static Handler createAsyncHandler(Looper looper) {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.P) {
return Handler.createAsync(looper);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

else not required after return

Copy link
Contributor Author

Choose a reason for hiding this comment

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

try {
//noinspection JavaReflectionMemberAccess
return Handler.class
.getConstructor(Looper.class, Handler.Callback.class, boolean.class)
.newInstance(looper, null, true);
} catch (IllegalAccessException e) {
return new Handler(looper);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just make these all empty and hoist a single return outside.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch 551b7b4

} catch (InstantiationException e) {
return new Handler(looper);
} catch (NoSuchMethodException e) {
return new Handler(looper);
} catch (InvocationTargetException e) {
return new Handler(looper);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should recycle the Message to be a good citizen!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}
}

private AndroidSchedulers() {
throw new AssertionError("No instances.");
}
Expand Down
2 changes: 1 addition & 1 deletion sample-app/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ android {

defaultConfig {
minSdkVersion 15
targetSdkVersion 23
targetSdkVersion 28
versionCode 1
versionName '1.0'
}
Expand Down