Skip to content

RxJava support#304

Merged
zugaldia merged 5 commits into
masterfrom
131-rx
Feb 6, 2017
Merged

RxJava support#304
zugaldia merged 5 commits into
masterfrom
131-rx

Conversation

@zugaldia

@zugaldia zugaldia commented Feb 3, 2017

Copy link
Copy Markdown
Member

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

zugaldia commented Feb 4, 2017

Copy link
Copy Markdown
Member Author

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

cammace commented Feb 6, 2017

Copy link
Copy Markdown

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

@zugaldia

zugaldia commented Feb 6, 2017

Copy link
Copy Markdown
Member Author

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?

@cammace cammace left a comment

Copy link
Copy Markdown

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