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

Mutual exclusive delegate methods #1087

Closed
5 of 17 tasks
djmadcat opened this issue Jan 31, 2017 · 3 comments
Closed
5 of 17 tasks

Mutual exclusive delegate methods #1087

djmadcat opened this issue Jan 31, 2017 · 3 comments

Comments

@djmadcat
Copy link

djmadcat commented Jan 31, 2017

Short description of the issue:

When a delegate implements a new version of the method, the old version is not called.
So when we use func methodInvoked(_ selector: Selector) -> Observable<[Any]> method to intercept older method, it does not invoked.

Expected outcome:

Any method must be able to be intercepted.

What actually happens:

For example:
UITextFieldDelegate has two versions for didEndEditing method:

  • old (iOS 2) func textFieldDidEndEditing(_ textField: UITextField)
  • new (iOS 10) func textFieldDidEndEditing(_ textField: UITextField, reason: UITextFieldDidEndEditingReason)

What happens:

  • User creates Observable by calling delegate.methodInvoked(#selector(UITextFieldDelegate.textFieldDidEndEditing(_:)))
  • iOS (10!) asks delegate for textFieldDidEndEditing:reason: support by calling respondsToSelector:.
  • _RXDelegateProxy collects all UITextFieldDelegate methods and finds it.
  • iOS invokes textFieldDidEndEditing:reason: with forwardInvocation:.
  • textFieldDidEndEditing: not being called.
  • "forwardDelegate" methods also not being called.

Self contained code example that reproduces the issue:

class RxTextFieldDelegateProxy
    : DelegateProxy
    , DelegateProxyType
    , UITextFieldDelegate {

    /// For more information take a look at `DelegateProxyType`.
    class func setCurrentDelegate(_ delegate: AnyObject?, toObject object: AnyObject) {
        let o: UITextField = object as! UITextField
        o.delegate = delegate.map { $0 as! UITextFieldDelegate }
    }

    /// For more information take a look at `DelegateProxyType`.
    class func currentDelegateFor(_ object: AnyObject) -> AnyObject? {
        let o: UITextField = object as! UITextField
        return o.delegate
    }
}

extension UITextField {
    public func createRxDelegateProxy() -> RxTextFieldDelegateProxy {
        return RxTextFieldDelegateProxy(parentObject: self)
    }
}

extension Reactive where Base: UITextField {
    public var delegate: DelegateProxy {
        return RxTextFieldDelegateProxy.proxyForObject(self.base)
    }

    public func setDelegate(_ delegate: UITextFieldDelegate) -> Disposable {
        return RxTextFieldDelegateProxy.installForwardDelegate(delegate, retainDelegate: false, onProxyForObject: self.base)
    }

    public var didBeginEditing: ControlEvent<()> {
        return ControlEvent<()>(events: self.delegate.methodInvoked(#selector(UITextFieldDelegate.textFieldDidBeginEditing(_:)))
            .map { _ in () })
    }

    public var didEndEditing: ControlEvent<()> {
        return ControlEvent<()>(events: self.delegate.methodInvoked(#selector(UITextFieldDelegate.textFieldDidEndEditing(_:)))
            .map { _ in () })
    }
}

class SomeViewController: UIViewController, UITextFieldDelegate {
    @IBOutlet weak var textField: UITextField!
    @IBOutlet weak var hideButton: UIButton!

    var disposeBag = DisposeBag()

    override func viewDidLoad() {
        super.viewDidLoad()

        self.textField.rx.didBeginEditing
            .subscribe(onNext: { _ in
                print("didBeginEditing being called")
            }).addDisposableTo(self.disposeBag)
        self.textField.rx.didEndEditing
            .subscribe(onNext: { _ in
                print("didEndEditing not being called")
            }).addDisposableTo(self.disposeBag)
        self.hideButton.rx.tap
            .subscribe(onNext: { [weak self] _ in
                guard let `self` = self else {
                    return
                }
                self.textField.endEditing(true)
            }).addDisposableTo(self.disposeBag)

        self.textField.delegate.rx.setDelegate(self)
            .addDisposableTo(self.disposeBag)
    }

    func textFieldDidBeginEditing(_ textField: UITextField) {
        print("didBeginEditing being called")
    }

    func textFieldDidEndEditing(_ textField: UITextField) {
        print("didEndEditing not being called")
    }
}

RxSwift/RxCocoa/RxBlocking/RxTest version/commit

3.1.0

Platform/Environment

  • iOS (tested on 10.2 iPad device and iPad simulator)
  • macOS
  • tvOS
  • watchOS
  • playgrounds

How easy is to reproduce? (chances of successful reproduce after running the self contained code)

  • easy, 100% repro
  • sometimes, 10%-100%
  • hard, 2% - 10%
  • extremely hard, %0 - 2%

Xcode version:

Version 8.2.1 (8C1002)

Installation method:

  • CocoaPods (1.1.1)
  • Carthage
  • Git submodules

I have multiple versions of Xcode installed:
(so we can know if this is a potential cause of your issue)

  • yes. Xcode 7.3.1, Xcode 8.0, Xcode 8.1
  • no

Level of RxSwift knowledge:
(this is so we can understand your level of knowledge
and formulate the response in an appropriate manner)

  • just starting
  • I have a small code base
  • I have a significant code base
@kzaher
Copy link
Member

kzaher commented Feb 2, 2017

Hi @djmadcat ,

So if I understood you correctly, you are saying that didBeginEditing should be implemented as:

let oldEvent = self.delegate.methodInvoked(#selector(UITextFieldDelegate.textFieldDidBeginEditing(_:)))
            .map { _ in () }
let newEvent = self.delegate.methodInvoked(#selector(UITextFieldDelegate.textFieldDidBeginEditing(_:reason:)))
            .map { _ in () }
let events = Observable.of(oldEvent, newEvent).merge()
return ControlEvent<()>(events: events)

... and the same for didEndEditing?

@djmadcat
Copy link
Author

djmadcat commented Feb 3, 2017

This won't work on Deployment Target iOS versions prior to 10=(

This will:

public var didEndEditing: ControlEvent<()> {
    if #available(iOS 10.0, *) {
       return ControlEvent<()>(events: self.delegate.methodInvoked(#selector(UITextFieldDelegate.textFieldDidEndEditing(_:reason:)))
            .map { _ in () })
    } else {
        return ControlEvent<()>(events: self.delegate.methodInvoked(#selector(UITextFieldDelegate.textFieldDidEndEditing(_:)))
            .map { _ in () })
    }
}

But this solution is incompatible with iOS updates: application can't handle addition of new delegate method in new iOS version properly. It only knows about methods in SDK.

I think there are two major problems:

  • _RXDelegateProxy responds to any protocol method even if all delegates don't respond to it
  • we must not modify system behavior for delegates making exceptional cases

So I see there two solutions:

  • modify _RXDelegateProxy, so it will respond to implemented methods only, but can slow down delegate methods handling
  • add "caution" section in documentation for these cases

@kzaher
Copy link
Member

kzaher commented Feb 5, 2017

Hi @djmadcat ,

But this solution is incompatible with iOS updates: application can't handle addition of new delegate method in new iOS version properly. It only knows about methods in SDK.

I don't it's realistic to expect for this library to be bulletproof on major os updates.

I think there are two major problems:
_RXDelegateProxy responds to any protocol method even if all delegates don't respond to it

The reason why this is the case is because delegate setter caches all selectors delegate responds to. We could theoretically solve this by resetting delegate each time number of observers goes from 0 to 1 or from 1 to 0 for a specific selector.

The reason why this strategy wasn't chosen was because it seemed to me that setting delegate multiple times fast could cause glitches (especially for UI{Table,Collection}Views) or resetting delegate while scroll view is decelerating.

we must not modify system behavior for delegates making exceptional cases

Don't understand this one.

So I see there two solutions:
modify _RXDelegateProxy, so it will respond to implemented methods only, but can slow down delegate methods handling

I'm not sure this would be a good idea, but maybe it is. I would need to test it more thoroughly.

add "caution" section in documentation for these cases

I'm ok with documenting this behavior for now. Feel free to create a PR.

We can investigate for 4.0 would resetting delegate proxies in fast succession cause any issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants