check location settings always throws StatusException if resolution is required.

[Stonepaw]

When using the LocationSettings.check() methods to handle the resolution myself I am always getting a StatusException instead of getting the LocationSettingsResult. This is due to SingleResultCallBack throwing a StatusException if the result status is not a success. However, the LocationSettingsResult will not have a success status if a resolution is required.

[patloew]

After thinking about it, I think this should stay as is. StatusException is always provided in onError() when isSuccess() is false. In case of LocationSettingsResult, the request was not successful when a resolution is required. You can however get the Status from the StatusException and then call startResolutionForResult().

[Oyashirox]

Same remark here, but I don't think it's a good idea to let it be this way. This forces us to add a lot of boilerplate code to get to the same result. And I also think it is misleading: We're expecting a LocationSettingsResult that is present and available, but the lib chooses not to transmit it and that restrain our possibilities for no valid reasons.

Here's my workaround :

mLocationProvider.settings().check(settingsRequest)
                .map(new Function<LocationSettingsResult, Status>() {
                    @Override
                    public Status apply(@NonNull LocationSettingsResult locationSettingsResult) throws Exception {
                        return locationSettingsResult.getStatus();
                    }
                })
                .onErrorReturn(new Function<Throwable, Status>() {
                    @Override
                    public Status apply(@NonNull Throwable throwable) throws Exception {
                        StatusException statusException = (StatusException) throwable;
                        return statusException.getStatus();
                    }
                })
                .flatMapObservable(new Function<Status, Observable<Boolean>>() {
                    @Override
                    public Observable<Boolean> apply(@NonNull Status status) throws Exception {
                        if (status.getStatusCode() == LocationSettingsStatusCodes.RESOLUTION_REQUIRED) {
                             ...
                        }
                    }
                });

[patloew]

I still think that this is OK. I would not do a map/onErrorReturn, but instead directly handle the resolution in the onError callback on subscribing. What do you think?

rxLocation.settings().check(locationRequest)
    .subscribe(result -> {}, throwable -> {
        if(throwable instanceof StatusException) {
            Status status = ((StatusException)throwable).getStatus();
            ...
        }
    });

Also, if you need a boolean anyway, why don't you use checkAndHandleResolution()?

[Oyashirox]

I do not subscribe to this observable directly, it is composed with others. This method implements an interface, so this specific implementation should not leak further.

I do not use checkAndHandleResolution() for two reasons : I'm currently moving this code from RxJava 1 (with Android-ReactiveLocation) in baby steps, and also because I have a custom behavior for resolution.

This is not a big workaround, but I was not expecting the call to throw an error in this case, because the object is already retrieved, and I don't think the library should decide for me if this is an error or not. Moreover, that's more code for you and less freedom for the user. What's your reasoning about this ?

[patloew]

The reasoning is that the rest of the library also handles unsuccessful requests in the same way. As soon as isSuccess() is false on a Result, a StatusException is provided in onError. It would be strange to make an exception for this case, in my opinion. Maybe it would be better to make check() return a Completable, so that it is clear that there is only a success and an error case (with the Status for resolution in the StatusException)?

[Oyashirox]

Well I'm not fond of this general behavior. If the method returns a Completable, then it should be called enforce instead of check, otherwise it is still missleading. We would also lose access to LocationSettingsResultor even Status, so if another field is of interest, we could not get it.

[patloew]

In 1.0.2 I added a StatusExceptionResumeNextTransformer, which can be used to get the original result back, if there was a StatusException with a resolution. Use it like this:

 rxLocation.settings().check(locationRequest)
     .compose(StatusExceptionResumeNextTransformer.forSingle())