tombenner / nui

Style iOS apps with a stylesheet, similar to CSS
MIT License
3.76k stars 461 forks source link

Compatibility Issue: Implementation of `didMoveToWindow` swizzling doesn't work with other frameworks #275

Open yjkogan opened 10 years ago

yjkogan commented 10 years ago

Is there any reason why you implemented didMoveToWindow for UILabel (and the other classes) instead of just letting them all drop down to UIView's swizzled implementation? That would allow the swizzle chain to stay intact. The other way to do it would be to call [super didMoveToWindow] in UILabel's override_didMoveToWindow]. I've tested both and they seem to work.

timbodeit commented 8 years ago

Interesting observation. I personally can't think of a reason why this would be needed.

@tombenner can you comment on this?

@yjkogan A pr that cleans this up a bit would be very welcome.

timbodeit commented 8 years ago

@yjkogan Note however that super is in fact called already. Consider the following "implementation".

// vanilla UILabel
- (void)didMoveToWindow {
    /* Maybe something else, maybe not */
    [super didMoveToWindow];
}
// UILabel+NUI
- (void)override_didMoveToWindow
{
    if (!self.isNUIApplied) {
        [self applyNUI];
    }
    [self override_didMoveToWindow];
}

Method swizzeling swaps the two implementations:

- (void)override_didMoveToWindow
{
    /* Maybe something else, maybe not */
    [super didMoveToWindow];
}
- (void)didMoveToWindow 
{
    if (!self.isNUIApplied) {
        [self applyNUI];
    }
    [self override_didMoveToWindow];
}

The [self override_didMoveToWindow]; call first calls the original implementation, which will call the super classes implementation.

yjkogan commented 8 years ago

Hey @timbodeit, can you clarify what you mean by "why this would be needed?" Do you mean the swizzle chain or the implementation of the method?

I don't remember much about this particular issue, since opened the issue so long ago. I believe what I found was that the SDK I was working on didn't work with NUI installed because NUI wasn't calling back up the swizzle chain. As you correctly point out, calling [self override_didMoveToWindow] will call the original implementation, and if you do it that way then other SDKs can swizzle the same method (either before or after) and both SDKs will get a chance to intercept the call before it hits the original implementation. I believe that something in this SDK was skipping straight to the original implementation, bypassing anyone else who had swizzled the method

timbodeit commented 8 years ago

What I mean is: You are correct in your observation. I do not see a reason why one would need to swizzle every class individually instead of letting them "drop down". Essentially answering your first sentence with "I didn't implement it, but I can't see such a reason".

yjkogan commented 8 years ago

Got it, sounds good! Sorry to say but I know I wont have time to open a PR into this repo :(

timbodeit commented 8 years ago

About the swizzle chain: While I agree that only swizzling UIView would be the cleaner way, the chain seems to me like it is intact.

When calling didMoveToWindow on UILabel:

I could only imagine that one of UIKit's classes doesn't make the super call. As in: Apple Engineer thinking: UIView doesn't do anything of relevance anyways, no need for me to call it.

If this is still relevant to you, I'd be happy to investigate further if you could provide an example project alongside actual and expected behavior.

yjkogan commented 8 years ago

Yeah I can't remember the exact details as it was a while ago. @tonyrzhang still works on the project though so he may be able to help? Miss you @tonyrzhang!

tonyrzhang commented 8 years ago

Aww thanks @yjkogan Miss you too!

Hey @timbodeit, It's definitely been a while since I looked at this as well, but IIRC it had to do with the way NUI was implementing its swizzling. We had a customer that had the Optimizely SDK as well as the NUI SDK and when both were initialized, the NUI swizzling didn't seem to play nice with ours. To remedy this I believe we ended up helping them make some adjustments to the NUI code base for their app. I don't have a copy or PR of it anymore but essentially we changed the NUI swizzling to follow this paradigm: https://blog.newrelic.com/2014/04/16/right-way-to-swizzle/

And things worked after that. LMK if you have any other questions.