RxSwiftCommunity / RxMediaPicker

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

Update API: Observable -> Single #10

Open AndrewSB opened 7 years ago

AndrewSB commented 7 years ago

In #9 we talked about updating to the new RxSwift Single stream.

I'll take a shot at this next time I'm using the library, otherwise anyone else is more than welcome to make a PR 😄

freak4pc commented 7 years ago

This will probably require some restructuring due to the nature of the way this was built :)

evgeny-sureev commented 6 years ago

I've converted library to Maybe, thus eliminated "cancelled" error. Also I've added ability to select files using UIDocumentPickerViewController.

freak4pc commented 6 years ago

@evgeny-sureev Is there a PR associated with this ? Or did you just suggest an idea here ?

evgeny-sureev commented 6 years ago

I can make PR by weekend, now all work is done in local copy.

freak4pc commented 6 years ago

I'm trying to think if Maybe is the correct behavior 🤔 It's not specific enough IMO

I'm thinking "if I would write this today, what would I do" and my initial thought is returning a Result-style enum with a Single:

typealias Image = UIImage
typealias Video = URL

protocol Pickable {}
extension Image: Pickable {}
extension Video: Pickable {}

enum PickerResult<Asset: Pickable> {
    case selected(Asset)
    case cancelled
}

And then accordingly return Observable from the methods, which would return either a Video or Image type, according to the request you made.

Would you mind not doing the PR for now until I give this some more thought? And also, what's your opinion on the proposed solution here @evgeny-sureev ?

evgeny-sureev commented 6 years ago

Single with PickerResult:

logoTapGesture.rx.event
    .flatMap { [unowned self] _ in
        return self.picker.takePhoto()
    }
    .map { result -> UIImage in
        switch result {
            case let .selected(asset):
                return asset.0 // return original image
            case .cancelled:
                // So how do we abort here? Throw something? Or use flatmap with Observable.empty()?
        }
    }
    .bind(to: logoImageView.rx.image)
    .disposed(by: disposeBag)

Maybe:

logoTapGesture.rx.event
    .flatMap { [unowned self] _ in
        return self.picker.takePhoto()
    }
    .map { $0.0 } // return original image
    .bind(to: logoImageView.rx.image)
    .disposed(by: disposeBag)

Also we returning two images from photo actions, so typealias for Image must be tuple: typealias Image = (UIImage, UIImage?). But tuples cannot be extended to declare protocol conformance.

freak4pc commented 6 years ago

@evgeny-sureev The point isn't necessarily aborting. It's so you can have some side-effects related to cancelation.

Maybe could work as well, but I think nullability isn't describing the fact the operation was cancelled, necessarily.

An error (the state of it today) isn't good as well IMO, same as throwing - since it terminates the sequence (and outer sequences, at that). Since Cancellation is an expected result (and not a terminal error), it shouldn't emit an error event.

Regarding handling the separate variants, it would be exactly like a regular Result type. There are many ways to handle a result type that aren't as convoluted as a full-blown switch. (like elements() and errors() from RxSwiftExt, for example)

I just think this needs some more though :)

evgeny-sureev commented 6 years ago

Right, with my approach you'll need to use something like amb(self.picker.takePhoto, Observable.just(defaultPicture)) for side effects on cancelation. Of course, if side effects relatively simple, they all could fit into subscribe(onCompleted:).

freak4pc commented 6 years ago

@evgeny-sureev amb in this situation will always emit the .just, as its the first to emit between the two streams - so this would still not solve what you're trying to do.

I still think the PickerResult approach better, but I'll think this more thoroughly throughout the weekend and put some notes here.