Implement Generics parameter in DelegateProxy #1379

tarunon · 2017-08-15T08:51:53Z

Hello there,
This proposal is first implementation of #1287, added generics parameter on DelegateProxy.

Motivation

I wish avoid to use force casting when implement DelegateProxy subclasses.

// as is
class func setCurrentDelegate(_ delegate: AnyObject?, toObject object: AnyObject) {
  let scrollView: UIScrollView = castOrFatalError(object)
  scrollView.delegate = castOptionalOrFatalError(delegate)
}

// to be
override class func setCurrentDelegate(_ delegate: UIScrollViewDelegate?, toObject object: ParentObject) {
  object.delegate = delegate
}

Every DelegateProxy subclass had same force casting implementation, I have thought can move these force casting into DelegateProxy core implementation, and use Generics parameter instead.

Changes

DelegateProxy

The new DelegateProxy have 2 generics parameter that ParentObject and Delegate.
Made a workaround protocol DelegateProxyBase for protocol extension conformance.

DelegateProxyType

Moved some of method that affected by generics parameter to DelegateProxyBase.

DelegateProxyFactory

Added static func sharedFactory(for proxyType: DelegateProxy.Type) -> DelegateProxyFactory because generics class cannot define static stored property.
Use DelegateProxySubclass.prepareForDelegateProxy instead of DelegateProxyFactory.extended.

RxPullRequestBot · 2017-08-15T08:53:35Z

	3 Warnings
⚠️	Please target PRs to `develop` branch
⚠️	No CHANGELOG changes made
⚠️	Big PR

Generated by 🚫 Danger

tarunon · 2017-08-15T09:03:07Z

RxCocoa/Common/DelegateProxyType.swift

-            assert(Self.currentDelegateFor(object) === proxy)
-            assert(proxy.forwardToDelegate() === currentDelegate)
+        let currentDelegate = self.currentDelegateFor(object)
+        let delegateProxy = unsafeDowncast(proxy, to: self)


I used unsafeDowncast instead of common force casting.
Let's think about UITableView.
We can get instance of DelegateProxy using RxScrollViewDelegateProxy.proxyForObject(scrollView) or RxTableViewDelegateProxy.proxyForObject(tableView) and if scrollView and tableView are same object, then DelegateProxy instances should be same object.
In this case, proxyForObject should cast two types that has different generics parameter.
Common force casting is failed, but unsafeDowncast is succeed.

I just want to understand, what did you force cast to here? To what type?

For example, in RxTableViewDelegateProxy case,

(tableView as UITableView).rx.delegate // DelegateProxy<UITableView, UIScrollViewDelegate> (tableView as UIScrollView).rx.delegate // DelegateProxy<UIScrollView, UIScrollViewDelegate>

In this case, actual type is RxTableViewDelegateProxy<UITableView> because it is checked on runtime in DelegateProxyFactory.
And in this case, I force cast from RxTableViewDelegateProxy<UITableView> to RxScrollViewDelegateProxy<UIScrollView>.

I think it is not good now...

Yeah, I think this is the biggest problem right now.

Could I make commit that avoid unsafeDowncast way?
I think it will be better than current implemetation.

We definitely need to figure this thing out, otherwise I'm not sure we can merge this :)

tarunon · 2017-08-15T09:13:05Z

RxCocoa/iOS/Proxies/RxScrollViewDelegateProxy.swift

-    , UIScrollViewDelegate
-    , DelegateProxyType {
+open class RxScrollViewDelegateProxy<P: UIScrollView>
+    : DelegateProxy<P, UIScrollViewDelegate>


I think the Delegate should fix in a type declared DelegateProxyType.
Delegate affect DelegateProxy.installForwardDelegate implementation, and of course we can call DelegateProxy.installForwardDelegate from upper casted parent object.

// e.g. let tableView = UITableView() RxScrollViewDelegateProxy.installForwardDelegate(myScrollViewDelegate, retainDelegate: false, onProxyForObject: tableView) tableView.rx.delegate.forwardToDelegate() // -> It is myScrollViewDelegate.

We know same pattern in UIKit, it seems strange.

let tableView = UITableView() (tableView as UIScrollView).delegate = myScrollViewDelegate tableView.delegate // -> myScrollViewDelegate, but it behave as UITableViewDelegate

Hi @tarunon ,

I'm sorry but I don't think I understand what did you want to say here 😅 ? Is there some problem?

I think it is no problem here.
I mean it is difficult making RxTableViewDelegateProxy<UITableView, UITableViewDelegate>.
I'm sorry confused you.

kzaher

There is really a bunch of code here. It will take some time for me to go through this more thoroughly.

This is my initial feedback.

I have a general impression that this is a step in good direction, but we'll need to simplify this PR and make API a bit clearer to use.

kzaher · 2017-08-17T21:00:26Z

RxCocoa/Common/DelegateProxy.swift

@@ -21,18 +21,20 @@ var dataSourceAssociatedTag: UnsafeRawPointer = UnsafeRawPointer(UnsafeMutablePo
 /// Base class for `DelegateProxyType` protocol.
 ///
 /// This implementation is not thread safe and can be used only from one thread (Main thread).
-open class DelegateProxy : _RXDelegateProxy {
-
+open class DelegateProxy<P: AnyObject, D: NSObjectProtocol>: _RXDelegateProxy, DelegateProxyBase {


Why DelegateProxyBase. Maybe DelegateProxyProtocol is more idiomatic to Swift?

kzaher · 2017-08-17T21:06:37Z

RxCocoa/Common/DelegateProxy.swift

+    ///
+    /// - parameter object: Object that has delegate property.
+    /// - returns: Value of delegate property.
+    open class func currentDelegateFor(_ object: ParentObject) -> Delegate? {


Is there some reason we are conforming to DelegateProxy to DelegateProxyBase. It seems to me we aren't getting anything from that and using abstract methods in Swift is usually avoided.

Yes, we use them in a couple of places, but we don't need here IMHO.

I'm actually not sure we need both DelegateProxyBase and DelegateProxyType. Is there some reason we have two?

I can merge it, thanks.

It seems to me that we don't need this method defined here.

kzaher · 2017-08-17T21:07:32Z

RxCocoa/Common/DelegateProxy.swift

@@ -224,8 +253,8 @@ open class DelegateProxy : _RXDelegateProxy {
    /// through `self`.
    ///
    /// - returns: Value of reference if set or nil.
-    open func forwardToDelegate() -> AnyObject? {
-        return self._forwardToDelegate
+    open func forwardToDelegate() -> D? {


Using associated type is probably more readable: Delegate.

kzaher · 2017-08-17T21:27:17Z

RxCocoa/Common/DelegateProxyFactory.swift

+     - parameter extends: Extend DelegateProxyFactory if needs. See 'DelegateProxyType'.
+     - returns: DelegateProxyFactory shared instance.
+     */
+    public static func sharedFactory<DelegateProxy: DelegateProxyBase & DelegateProxyType>(for proxyType: DelegateProxy.Type, extends: (() -> Void)? = nil) -> DelegateProxyFactory {


I think we should rename this. The name, intended usage and implementation are confusing to me.

kzaher · 2017-08-17T21:29:03Z

RxCocoa/Common/DelegateProxyType.swift

+    /// When make 'RxXXXDelegateProxy' subclass, call 'RxXXXDelegateProxySubclass.prepareForFactory()' 1 time, or use it in DelegateProxyFactory
+    /// 'RxXXXDelegateProxy' can have one subclass implementation per concrete ParentObject type.
+    /// Should call it from concrete DelegateProxy type, not generic.
+    public static func prepareForFactory() {


Maybe just call it register?

kzaher · 2017-08-17T21:33:09Z

RxCocoa/Common/DelegateProxyFactory.swift

+        MainScheduler.ensureExecutingOnScheduler()
+        if let factory = _sharedFactories[ObjectIdentifier(DelegateProxy.Delegate.self)] {
+            return factory
+        }


This looks weird to me, if factory already exists, extends is ignored.

I think we should rename extends to knownImplementations, make it a static method on DelegateProxyType and create a default implementation that has void implementation.

This API doesn't prevent anyone from calling sharedFactory twice with different extends parameter, which is not how this API show work IMHO.

kzaher · 2017-08-17T21:47:26Z

RxCocoa/Common/DelegateProxyFactory.swift

+
+    /**
+     Shared instance of DelegateProxyFactory, if isn't exist shared instance, make DelegateProxyFactory instance for proxy type and extends.
+     DlegateProxyFactory have a shared instance per Delegate type.


Typo DlegateProxyFactory.

tarunon · 2017-08-19T01:35:12Z

Thank you, @kzaher
I will improve this PR.

I have a discussion.
I think we can avoid using generics ParentObject in DelegateProxy subclass (e.g. RxTableViewDelegateProxy), I thought it for a few days after making this PR.
The biggest reason is swift not support covariant feature in generics parameter.
I used some of workaround like unsafeDownCast, but I feel it's not good now.
Actually, I've made some of confusing API.
I think next example is more clear than current implementation.

// Root DelegateProxy implementation
class RxScrollViewDelegateProxy
    : DelegateProxy<UIScrollView, UIScrollViewDelegate>, ... { ... }

// Subclass of Root DelegateProxy
class RxTableViewDelegateProxy: RxScrollViewDelegateProxy {
   require init(parentObject: UIScrollView) {
      self.tableView = castOrFatalError(parentObject)
      super.init(parentObject: parentObject)
   }
}

// Extends Root DelegateProxy's factory
RxTableViewDelegateProxy.register(for: UITableView)

…s `extends`

kzaher · 2017-08-27T18:16:50Z

RxCocoa/Common/DelegateProxyFactory.swift

-                RxTableViewDelegateProxy(parentObject: parentObject)
+        static var factory: DelegateProxyFactory {
+            return DelegateProxyFactory.sharedFactory(for: RxScrollViewDelegateProxy<UIScrollView>.self) {
+                RxTableViewDelegateProxy<UITableView>.register()


It think we've changed this part.

kzaher · 2017-08-27T18:18:59Z

RxCocoa/Common/DelegateProxyType.swift

    static var factory: DelegateProxyFactory { get }

+    /// Creates new proxy for target object.
+    /// Should not call this function directory, use 'DelegateProxy.proxyForObject'
+    static func createProxy(for object: AnyObject) -> AnyObject


I think we can remove this from DelgateProxyType since protocol extension is already implementing it depending on factory.

Shouldn't this now have the following signature?
static func createProxy(for object: ParentObject) -> Self

If I use static func createProxy(for object: ParentObject) -> Self instead,
I can see compile error like Method 'createProxy(for:)' in non-final class 'RxScrollViewDelegate'.

kzaher · 2017-08-27T18:22:18Z

RxCocoa/Common/DelegateProxyType.swift

+    /// Store DelegateProxy subclass to factory.
+    /// When make 'RxXXXDelegateProxy' subclass, call 'RxXXXDelegateProxySubclass.register()' 1 time.
+    /// 'RxXXXDelegateProxy' can have one subclass implementation per concrete ParentObject type.
+    static func register()


It seems to me that register could be also removed from this interface.

kzaher · 2017-08-27T18:24:23Z

RxCocoa/Common/DelegateProxyType.swift

    ///         }
    ///
    ///         public var text: ControlProperty<String> {
    ///             let source: Observable<String> = self.delegate.observe(#selector(UISearchBarDelegate.searchBar(_:textDidChange:)))
    ///             ...
    ///         }
    ///     }
-    public static func proxyForObject(_ object: AnyObject) -> Self {
+    public static func proxyForObject(_ object: ParentObject) -> Self {


We should make this proxy(for:)

kzaher · 2017-08-27T18:24:40Z

RxCocoa/Common/DelegateProxyType.swift

-    ///         public var delegate: DelegateProxy {
-    ///            return RxSearchBarDelegateProxy.proxyForObject(base)
+    ///         public var delegate: DelegateProxy<Base, UISearchBarDelegate> {
+    ///            return RxSearchBarDelegateProxy<Base>.proxyForObject(base)


I think that we should make this RxSearchBarDelegateProxy<Base>.proxy(for: base)

kzaher · 2017-08-27T18:32:21Z

RxCocoa/Common/DelegateProxyType.swift

-            assert(Self.currentDelegateFor(object) === proxy)
-            assert(proxy.forwardToDelegate() === currentDelegate)
+        let currentDelegate = self.currentDelegateFor(object)
+        let delegateProxy = unsafeDowncast(proxy, to: self)


I just want to understand, what did you force cast to here? To what type?

kzaher · 2017-08-27T18:35:02Z

RxCocoa/Common/DelegateProxyType.swift

    /// - parameter object: Object that has delegate property.
    /// - returns: Value of delegate property.
-    static func currentDelegateFor(_ object: AnyObject) -> AnyObject?
+    static func currentDelegateFor(_ object: ParentObject) -> Delegate?


I believe this should now be func currentDelegate(for object: ParentObject) -> Delegate?

kzaher · 2017-08-27T18:35:25Z

RxCocoa/Common/DelegateProxyType.swift

    /// Returns assigned proxy for object.
    ///
    /// - parameter object: Object that can have assigned delegate proxy.
    /// - returns: Assigned delegate proxy or `nil` if no delegate proxy is assigned.
-    static func assignedProxyFor(_ object: AnyObject) -> AnyObject?
-
+    static func assignedProxyFor(_ object: ParentObject) -> Delegate?


I believe this should now be static func assignedProxy(for object: ParentObject) -> Delegate?

I agree it, let's add it to Deprecated.swift also.

kzaher · 2017-08-27T18:38:21Z

RxCocoa/Common/DelegateProxyType.swift

    static var factory: DelegateProxyFactory { get }

+    /// Creates new proxy for target object.
+    /// Should not call this function directory, use 'DelegateProxy.proxyForObject'
+    static func createProxy(for object: AnyObject) -> AnyObject


Shouldn't this now have the following signature?
static func createProxy(for object: ParentObject) -> Self

kzaher · 2017-08-27T18:44:28Z

RxCocoa/iOS/Proxies/RxScrollViewDelegateProxy.swift

-    , UIScrollViewDelegate
-    , DelegateProxyType {
+open class RxScrollViewDelegateProxy<P: UIScrollView>
+    : DelegateProxy<P, UIScrollViewDelegate>


Hi @tarunon ,

I'm sorry but I don't think I understand what did you want to say here 😅 ? Is there some problem?

kzaher · 2017-08-27T18:49:32Z

RxCocoa/Common/DelegateProxyType.swift

+    static func createProxy(for object: AnyObject) -> AnyObject
+
+    /// Store DelegateProxy subclass to factory.
+    /// When make 'RxXXXDelegateProxy' subclass, call 'RxXXXDelegateProxySubclass.register()' 1 time.


Maybe we can use Rx*DelegateProxy or some other wildcard instead of 'RxXXXDelegateProxy. Seems a bit nicer 😄 .

I think the same would apply to other parts of docs.

rename `assignedProxyFor(_:)` -> `assignedProxy(for:)` rename `currentDelegateFor(_:)` -> `currentDelegate(for:)` rename `proxyForObject(_:)` -> `proxy(for:)`

…feature/type_safe_delegateproxy

tarunon · 2017-08-29T04:16:17Z

RxCocoa/Common/DelegateProxyType.swift

-            assert(Self.currentDelegateFor(object) === proxy)
-            assert(proxy.forwardToDelegate() === currentDelegate)
+        let currentDelegate = self.currentDelegate(for: object)
+        let delegateProxy = unsafeDowncast(proxy, to: self)


The comment thread was closed by github, but still problem is here.

This is to solve cast between RxTableViewDelegateProxy<UITableView> and RxScrollViewDelegateProxy<UIScrollView>.
But I think it's better that quit using it.
Remove ParentObject type generics parameter from RxScrollViewDelegateProxy, then we don't need to use unsafeDowncast anymore.

…feature/type_safe_delegateproxy

kzaher · 2017-09-10T18:39:52Z

Hi @tarunon ,

are you still working on this? I think that we first need to figure out how does UI{Table,Scroll}View case work with this and what is the type of delegate proxy for those 2 classes.

tarunon · 2017-09-12T01:06:55Z

Hello @kzaher ,

I'm still considering about what is the best way.
It is my UI{Table,Scroll}View understanding,

Let's consider 3 cases when we use DelegateProxy in UI{Table,Scroll}View

Run time	Compile time
`UIScrollView`	`UIScrollView`
`UITableView`	`UIScrollView`
`UITableView`	`UITableView`

We declare DelegateProxy type based ParentObject type (Compile time), and we generate DelegateProxy instance uses ParentObject(Run time).
It will be...

ParentObject	DelegateProxy
`UIScrollView`	`RxScrollViewDelegateProxy`
`UITableView`	`RxTableViewDelegateProxy`

In master repo, RxTableViewDelegateProxy is subclass of RxScrollViewDelegateProxy, so there are no problem, but in this pr, RxTableViewDelegateProxy<UITableView> is not subclass of RxScrollViewDelegateProxy<UIScrollView>.
I use unsafeDowncast and it pass compile time and run time, but it's bad practice, ~~maybe it is swift compiler bug(I think swift compiler don't check generics parameter when use unsafeDowncast).~~ not bug, but not good 😥

I suggest avoid using unsafeDowncast, and I wish make 1 more commit that remove unsafeDowncast.
May I make commit?

kzaher · 2017-09-12T22:07:40Z

Hi @tarunon , we definitely have to do this properly. Swift compiler doesn't support covariance as far as I can tell.

So I'm assuming RxTableViewDelegateProxy won't have generic arguments and will just inherit from RxScrollViewDelegateProxy<UIScrollView>?

I think extension mechanism will also need to be modified a bit since it relies on DelegateProxyType .

tarunon · 2017-09-13T09:36:47Z

So I'm assuming RxTableViewDelegateProxy won't have generic arguments and will just inherit from RxScrollViewDelegateProxy?

I think so too, and it is better that implement class RxScrollViewDelegateProxy: DelegateProxy<UIScrollView, UIScrollViewDelegate>.
Let me create one more commit, plz wait few hours. 🙏

…y subclasses subclass.

kzaher · 2017-09-13T18:29:32Z

@tarunon do you want me to take a look now?

tarunon · 2017-09-13T18:37:36Z

@kzaher Looks like test is going now, but maybe it will pass.
Current code is good for me, please check when you have time 🙇

kzaher · 2017-09-13T19:54:31Z

Hi @tarunon ,

I think we could maybe improve this code. I'll try to prototype something on top of 679961e and show you if I succeed.

kzaher · 2017-09-17T11:10:29Z

Hi @tarunon ,

I've merged your changes and simplified them a bit so we only need two castOrFatalError and have smaller public interface. You can take a look in swift4.0 branch, mostly at d1bc89b.

tarunon · 2017-09-18T09:03:27Z

Hi @kzaher ,

I checked your refactoring. I like it, DelegateProxyFactory now be private, great!
Thank you XD

Implement Generics parameter for DelegateProxy

af188fb

tarunon commented Aug 15, 2017

View reviewed changes

kzaher requested changes Aug 17, 2017

View reviewed changes

tarunon added 3 commits August 19, 2017 10:46

refactoring: renaming some of type, functions, fix typo.

508c68f

using static func knownImplementations instead of optional argument…

098c156

…s `extends`

Merge DelegateProxyProtocol and DelegateProxyType

496a3d2

kzaher reviewed Aug 27, 2017

View reviewed changes

kzaher requested changes Aug 27, 2017

View reviewed changes

tarunon added 3 commits August 29, 2017 12:41

refactoring

ea10503

rename `assignedProxyFor(_:)` -> `assignedProxy(for:)` rename `currentDelegateFor(_:)` -> `currentDelegate(for:)` rename `proxyForObject(_:)` -> `proxy(for:)`

Merge branch 'swift4.0' of https://github.com/ReactiveX/RxSwift into …

c21fbad

…feature/type_safe_delegateproxy

forgot fix comment

679961e

tarunon commented Aug 29, 2017

View reviewed changes

Merge branch 'swift4.0' of https://github.com/ReactiveX/RxSwift into …

c4bfcd3

…feature/type_safe_delegateproxy

tarunon added 2 commits September 14, 2017 00:12

Avoid use unsafeDowncast. Remove generics parameter from DelegateProx…

16f6c3a

…y subclasses subclass.

fix comments

fc26b0d

kzaher merged commit fc26b0d into ReactiveX:swift4.0 Sep 16, 2017

sergdort mentioned this pull request Sep 20, 2017

[Feature Request] simplify DelegateProxy implementation #1415

Closed

5 tasks

tarunon deleted the feature/type_safe_delegateproxy branch October 4, 2017 05:07

Implement Generics parameter in DelegateProxy #1379

Implement Generics parameter in DelegateProxy #1379

Conversation

tarunon commented Aug 15, 2017

Motivation

Changes

DelegateProxy

DelegateProxyType

DelegateProxyFactory

RxPullRequestBot commented Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarunon Aug 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzaher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarunon commented Aug 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzaher commented Sep 10, 2017

tarunon commented Sep 12, 2017 • edited Loading

kzaher commented Sep 12, 2017

tarunon commented Sep 13, 2017 • edited Loading

kzaher commented Sep 13, 2017

tarunon commented Sep 13, 2017

kzaher commented Sep 13, 2017

kzaher commented Sep 17, 2017

tarunon commented Sep 18, 2017

RxPullRequestBot commented Aug 15, 2017 •

edited

Loading

tarunon Aug 27, 2017 •

edited

Loading

tarunon commented Aug 19, 2017 •

edited

Loading

tarunon commented Sep 12, 2017 •

edited

Loading

tarunon commented Sep 13, 2017 •

edited

Loading