RxSwiftCommunity / RxMediaPicker

A reactive wrapper built around UIImagePickerController.
MIT License
180 stars 62 forks source link

Cancel emits an error instead of a next event #15

Closed adamsmaka closed 7 years ago

adamsmaka commented 7 years ago

I'm not sure if is it good to send an Error event when use cancels during picking an image. Even more, my app crash when I press cancel, what is the reason?

fatal error: Binding error to variable: canceled: file /Users/adamsmaka/Dropbox/Projects/TakeActionInspire/Pods/RxCocoa/RxCocoa/RxCocoa.swift, line 154

screen shot 2017-05-26 at 1 43 16 pm
freak4pc commented 7 years ago

Hey @adamsmaka

  1. I can't reproduce this on the demo project. Did you do anything specific here with code you could share? Also, is this simulator or device ?

  2. I don't think there's a problem with sending an error for a cancel event, there's no "Result" in this case. But i'm open to suggestions on changing if the community feels it shouldn't be this way.

adamsmaka commented 7 years ago

Oh yes it was my mistake. I did something like this, and in Error case, it crashed the app. newImageObs .subscribeOn(MainScheduler.instance) .bind(to: myImage) .addDisposableTo(self.bag)

I don't think that canceling action should emit error :) I would rather expect it when a camera is not available or something like that.

vhbit commented 7 years ago

@adamsmaka bind is not Error-compatible. You have to take care that errors are handled before binding, for example by doing

newImageObs.subscribeOn(MainScheduler.instance)
           .catchErrorJustReturns(nil)
           .bind(to: goalImage)
           .disposed(by: self.bag)
adamsmaka commented 7 years ago

thank you, im still newbie with RxSwift 👍

freak4pc commented 7 years ago

@adamsmaka Two things

  1. I renamed the issue to reflect your current question
  2. If there's a consensus of changing this (a few more members) to a next event we can do that. To be honest I think an error event makes sense (you can't have any more emissions after the user cancels the dialog which makes sense to me)
vhbit commented 7 years ago

changing this (a few more members) to a next event we can do that

.completed maybe, definitely not .next, but that may be a bit inconvenient as now it'll be unclear (without additional actions) why it was completed :smile:

I'd say this is misusage and issue can be closed.

freak4pc commented 7 years ago

I prefer Error as an event since completed happens after selecting an asset and not exclusively for cancel so it's hard to target that specific event.

Moving to Single later on will make this more important in that case, since it will only have a single success event.

I think I'm gonna close this for the time being but if a good reason to change this will come up, we can rediscuss.

Thanks for the feedback @vhbit !