AliSoftware/Reusable

loadNibContent always loads nib named after base class

calmez opened this issue · 14 comments

calmez commented

Consider the following classes:

class MyCustomClass: UIView, NibOwnerLoadable {
    required init?(coder aDecoder: NSCoder) {
        super.init(coder: aDecoder)
        self.loadNibContent()
        // ... rest of init
    }
}

class MyCustomSubClass: MyCustomClass {
    required init?(coder aDecoder: NSCoder) {
        super.init(coder: aDecoder)
        // ... rest of init
    }
}

What happens now when instantiating MyCustomSubClass is that MyCustomClass's nib file will be loaded instead of MyCustomSubClass's one.
My guess is that this has something to do with the way the nib is loaded in Sources/View/NibOwnerLoadable.swift#L41. Self.nib somehow will be the the base class's nib file and not resolve to the actual class this is called on.

calmez commented

I guess I formulated the title of this issue a bit harsh. Maybe this behavior was intended like this by you @AliSoftware ?
I just found it more intuitive to use the name of the dynamic type rather than the name of the base class. The place where this happens is in NibOwnerLoadable's loadNibContent

Reusable/Sources/View/NibOwnerLoadable.swift

Line 42 in 80778cf

for case let view as UIView in Self.nib.instantiate(withOwner: self, options: nil) {

Essentially Self will always be the type that immediately declares conformance to the protocol (in my example that is MyCustomClass). Changing that to type(of: self) instead would give you the true dynamic type of the object and hence load the nib the the class' name.

@AliSoftware Do you think that is a reasonable change? If so, I can prepare a PR for you.

AliSoftware commented

Mmmh interesting. I'm wondering if we could instead make sure that dynamic dispatch takes place on nib property … but not sure it's entirely possible in our use case, so your suggestion makes sense I think, and I'd love a PR

calmez commented

Great. I'm not really sure that the dynamic dispatch can be achieved in the nib property since to the computed property this seems to be equivalent to being called from the base class itself. Essentially the problem will be something like calling a static method on the dynamic type vs. calling it on a fixed metatype (e.g. like here https://stackoverflow.com/questions/42260337/swift-calling-static-methods-typeof-self-vs-explicit-class-name)
I created a PR with that small change where we can discuss this further if needed.

AliSoftware commented

Closed by #96 🎉

rgkobashi commented

Hi,

First of all thank you very much for this library, it has been very useful for the projects I've been working on. I have one question though, I believe the PR that fixes this broke our app, we got some crashes when creating an IBOutlet from a subclass, in the example provided it would be MyCustomSubClass (fortunately we catch this before shipping).

My question is, when subclassing now the parent class needs to implement nib: UINib otherwise there will be a crash. Was this a bug or that has been the intentional behavior?

Thanks in advance 😄

AliSoftware commented

Thanks for reporting this.

I don't think this was intentional; but I'm unsure why the nib property would need to now always be implemented in the parent class explicitly if your IBOutlet points to a subclass and thus the PR fixing this issue is supposed to get the nib property from the subclass anyway 🤔

Could you maybe provide a simplified sample code so we can pinpoint what's happening?

rgkobashi commented

Hi @AliSoftware, thanks for your reply.
Perhaps I explained myself incorrectly, there was no need of an IBOutlet, I am having the same crash if I add a NibOwnerLoadable directly on a ViewController on a storyboard.

I created this simple project which has the same problem, the uncommitted change is what fixes the crash.
Let me know if I can help on something else.
Thanks

Btw, this is the crash:

*** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Could not load NIB in bundle: 'NSBundle <.../TestReusable.app> (loaded)' with name 'SubclassCustomTextField''
terminating with uncaught exception of type NSException
calmez commented
rgkobashi commented

Did you try commenting out the uncommitted changes?

//static var nib: UINib {
//    return UINib(nibName: "CustomTextField", bundle: Bundle(for: self))
//}

Doing this I can get the crash. Unless is the Xcode version I am using 12.0.1 (12A7300)

calmez commented
  • 👍1
calmez commented
rgkobashi commented

Ah ok got it, thank you very much for your reply.
Just wanted to make sure we are implementing it in the proper way and this is an expected behavior.
Unless @AliSoftware has something else to add 😄

AliSoftware commented

Thank you so much @calmez for looking into that and for giving such a detailed answer and thorough explanation 👍

I think this can be considered the intended behavior for this case indeed now that I get the setup.

We can't really support both cases at the same time (the case where you have a single XIB for parent and subclasses, vs the case where you have one XIB for each subclass) anyway and have to pick one over the other in our choice of implementation 😉

I think it could still be nice to mention something about this in the docs though (either in the README, or as doc comments in the code), with @calmez 's good explanation as a good start for the documentation copy 🙂

rgkobashi commented

Ah ok got it, understood.
Thank you very much guys!