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

AndroidScheduler.java #317

Closed

Conversation

prabirshrestha
Copy link
Contributor

Initial spike for AndroidScheduler.java.

Been using this for quite some time. But I don't like the concept using calling observer.observeOn(AndroidScheduler.getInstance()) as it sort of puts Android specific stuffs. Should RxJava support some concept of Schedulers.mainScheduler()? We can then set the default main schedulers using Schedulers.setMainScheduler(AndroidScheduler.getInstance()) when app starts.

@cloudbees-pull-request-builder

RxJava-pull-requests #195 FAILURE
Looks like there's a problem with this pull request

@benjchristensen
Copy link
Member

Thanks @prabirshrestha for the submission. Support for Android is something several people will be happy about getting.

Since I don't work with Android I'd like to pull @mustafasezgin into this conversation as he's using RxJava on Android at SoundCloud. Mustafa, can you review, contribute and/or answer questions?

Mustafa gave a presentation that hinted at the use of Schedulers for Android: http://backstage.soundcloud.com/2013/08/responsive-android-applications-with-sane-code/

android-schedulers-soundcloud

I don't like the idea of global settings like Schedulers.setMainScheduler, as libraries would be stepping on each other.

I think something such as the following is a good approach, similar to the Schedulers factory class in Rx core:

AndroidScheduler.MAIN_SCHEDULER
... or ...
AndroidScheduler.mainScheduler()

@prabirshrestha
Copy link
Contributor Author

AndroidScheduler.mainScheduler() definitely sounds better then AndroidScheduler.getInstance().

I named the method as getInstance so that the AndroidScheduler is similar to the SwingScheduler which uses getInstance().

I would also need help on writing the gradle build scripts.(Android SDK should also be installed in the CI server)

@benjchristensen
Copy link
Member

Android dependencies should be in a repo such as Maven Central so that someone does not need to configure their environment correctly to build.

Is this what it needs?

http://search.maven.org/#artifactdetails%7Ccom.android.tools%7Csdk-common%7C22.1.3%7Cjar

The gradle file will be similar to this: https://github.com/Netflix/RxJava/blob/master/rxjava-contrib/rxjava-swing/build.gradle

... but you will add this dependency:

compile 'com.android.tools:sdk-common:22.1.3'

Also add your module to this file: https://github.com/Netflix/RxJava/blob/master/settings.gradle

For example, place this on the 2nd to last line before rxjava-swing:

'rxjava-contrib:rxjava-android', \

@muzzah
Copy link

muzzah commented Aug 14, 2013

@benjchristensen happy to help out, @mttkay (from the SoundCloud Android team) will also be valuable to the discussion.

We use the first method outlined by @benjchristensen to set a the main thread scheduler on an observable. The main thread scheduler is similar to the one submitted by @prabirshrestha with the second method being implemented using the Handler.postDelayed() method. For consistency with the core package I think the static method approach for the scheduler is more suited in this instance.

@mttkay wrote some helpful components related to fragments which we were also looking to opensource. Maybe we should look to getting that out sooner. @mttkay thoughts?

Also you should only need to add the main android sdk dependency http://search.maven.org/#search%7Cgav%7C1%7Cg%3A%22com.google.android%22%20AND%20a%3A%22android%22 using the provided maven scope.

@benjchristensen
Copy link
Member

Thanks @mustafasezgin for getting involved. Nice to know you and @mttkay work together.

Yes, we only need the SDK as a provided not compile dependency. So like this in the build.gradle file:

provided 'com.google.android:android:4.1.1.4'


@Override
public <T> Subscription schedule(T state, Func2<Scheduler, T, Subscription> action, long dueTime, TimeUnit unit) {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why return null? Is this work in progress? - Maybe throw a NotSupportedException while this is not implemented?

@jmhofer
Copy link
Contributor

jmhofer commented Aug 14, 2013

Awesome. I was playing around with Android and RxJava already, too. But I guess for me, it'll be a libgdx integration mostly.

@mttkay
Copy link
Contributor

mttkay commented Aug 14, 2013

Hey guys, indeed we have been using a more complete scheduler for a while, one that has an implementation for delayed messages too.

My suggestion is to rename AndroidScheduler to AndroidSchedulers (plural) since there may be more schedulers to be implemented for Android down the road (like scheduling via AsyncTasks). So we should have:

AndroidSchedulers.mainThread()

I'm also missing tests and build integration with this PR. It seems like it's a WIP, maybe we should combine our efforts around this?

@mttkay
Copy link
Contributor

mttkay commented Aug 14, 2013

Have a look at the above PR. I've pulled the scheduler component out of our production app code and into rxjava-contrib.

The build should pass, the Scala adaptor seems to be causing trouble though? Seems to neither build on the public CI nor on my local machine. Test execution time is slightly up due to the addition of Robolectric, which goes through a start-up procedure to initialize a fake Android application environment for testing.

@prabirshrestha
Copy link
Contributor Author

closing in favor of #318

jihoonson pushed a commit to jihoonson/RxJava that referenced this pull request Mar 6, 2020
* Add response predicate to retry sync and async for enhancement ReactiveX#259

* ReactiveX#317 skipping Java Error type from onError Rx retry transformer

* ReactiveX#317 adding proper comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants