nui icon indicating copy to clipboard operation
nui copied to clipboard

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

Open yjkogan opened this issue 10 years ago • 8 comments

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.

yjkogan avatar Oct 08 '14 18:10 yjkogan

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 avatar Nov 19 '15 21:11 timbodeit

@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.

timbodeit avatar Nov 19 '15 21:11 timbodeit

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

yjkogan avatar Nov 23 '15 03:11 yjkogan

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".

timbodeit avatar Nov 23 '15 03:11 timbodeit

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

yjkogan avatar Nov 23 '15 03:11 yjkogan

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:

  • this call goes to override_didMoveToWindow in UILabel+NUI.m
  • which calls didMoveToWindow on UIKit/UILabel (see my second comment)
  • which should call super didMoveToWindow
  • which goes to override_didMoveToWindow in UIView+NUI.m
  • which calls didMoveToWindow on UIKit/UIView or any other swizzled implementations

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.

timbodeit avatar Nov 23 '15 04:11 timbodeit

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!

yjkogan avatar Nov 24 '15 00:11 yjkogan

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.

tonyrzhang avatar Nov 30 '15 19:11 tonyrzhang