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

RxJava support #304

Merged
merged 5 commits into from
Feb 6, 2017
Merged

RxJava support #304

merged 5 commits into from
Feb 6, 2017

Conversation

zugaldia
Copy link
Member

@zugaldia zugaldia commented Feb 3, 2017

WIP to bring back support for Rx as part of the libjava-services-rx module.

The first stab at it, extending the current MapboxDirections class, works but brings an inconsistent API. Unlike with the normal client where the Builder brings the MapboxDirections instance. Right now, we need to use them separately as we cannot use the build method:

MapboxDirectionsRx.Builder builder = new MapboxDirectionsRx.Builder()
  .setAccessToken(Utils.getMapboxAccessToken(this))
  .setCoordinates(positions)
  .setProfile(DirectionsCriteria.PROFILE_DRIVING)
  .setSteps(true)
  .setOverview(DirectionsCriteria.OVERVIEW_FULL);
MapboxDirectionsRx clientRx = new MapboxDirectionsRx(builder);
clientRx.getObservable()
  .subscribeOn(Schedulers.newThread())
  .observeOn(AndroidSchedulers.mainThread())
  .subscribe(new Action1<DirectionsResponse>() {
    @Override
    public void call(DirectionsResponse response) {
      DirectionsRoute currentRoute = response.getRoutes().get(0);
      Log.d(LOG_TAG, "Response code: " + response.getCode());
      Log.d(LOG_TAG, "Distance: " + currentRoute.getDistance());
    }
  });

This needs one more iteration.

Fixes #131.

cc: @mapbox/android for folks familiar with Rx willing to get some 👀.

@zugaldia
Copy link
Member Author

zugaldia commented Feb 4, 2017

Alright, nothing that couldn't be fixed bringing some generics to the Builder class (f6fb9f9). Now the API for Rx looks similar the non-Rx one:

MapboxDirectionsRx clientRx = new MapboxDirectionsRx.Builder()
  .setAccessToken(Utils.getMapboxAccessToken(this))
  .setCoordinates(positions)
  .setProfile(DirectionsCriteria.PROFILE_DRIVING)
  .setSteps(true)
  .setOverview(DirectionsCriteria.OVERVIEW_FULL)
  .build();
clientRx.getObservable()
  .subscribeOn(Schedulers.newThread())
  .observeOn(AndroidSchedulers.mainThread())
  .subscribe(new Action1<DirectionsResponse>() {
    @Override
    public void call(DirectionsResponse response) {
      DirectionsRoute currentRoute = response.getRoutes().get(0);
      Log.d(LOG_TAG, "Response code: " + response.getCode());
      Log.d(LOG_TAG, "Distance: " + currentRoute.getDistance());
    }
  });

(Working sample in DirectionsV5Activity.)

If this looks good to @mapbox/android we just need to bring in some test and adapt the same approach to the other clients (distance, geocoding, mapmatching, staticimage).

@cammace
Copy link
Contributor

cammace commented Feb 6, 2017

Would it make sense to also have a libandroid-rx module for our geocoding widget and other future widgets/UI elements?

@zugaldia
Copy link
Member Author

zugaldia commented Feb 6, 2017

Would it make sense to also have a libandroid-rx module for our geocoding widget and other future widgets/UI elements?

Yes, but let's wait to have some content before we create the module. For example, #4 should closed for now as it's unclear that the benefits of the Rx approach in the widget outbalance the weight of the dependency.

@cammace PR is ready, care to review please?

Copy link
Contributor

@cammace cammace left a comment

Choose a reason for hiding this comment

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

👍

@zugaldia zugaldia changed the title [WIP] RxJava support RxJava support Feb 6, 2017
@zugaldia zugaldia merged commit 4923ce6 into master Feb 6, 2017
@zugaldia zugaldia deleted the 131-rx branch February 6, 2017 22:21
@zugaldia zugaldia mentioned this pull request Feb 10, 2017
9 tasks
@zugaldia zugaldia mentioned this pull request Feb 22, 2017
9 tasks
@zugaldia zugaldia mentioned this pull request Mar 9, 2017
9 tasks
@zugaldia zugaldia mentioned this pull request Mar 17, 2017
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants