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

Coexistence with KVO. Yes, it works!! #115

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

doggy
Copy link

@doggy doggy commented Mar 3, 2017

KVO works if observers are created after your calls aspect_hookSelector: It most likely will crash the other way around. Still looking for workarounds here - any help appreciated.

Hi, @steipete

I loved aspects, and I really want it can be used by everyone in every projects. But I think the KVO crash is one of limitation which block the way.

So, I have spent many times(and in many ways) to making it works, and hopefully, I got a solution:

  • call original selector with the original _cmd

That means we need re-swizzle the originalSelector we saved priviously to class-isa and set invocation.selector with it also before calling [invocation invoke];

All things works like a charm.

KVO Coexistence with different aspects hook mode:

Aspects mode Hook after KVO KVO after Hook
Class hook Works Works
Instance hook Works with one limit Works

All tests above added/updated to the testCase target.

I can provide more detail about why this way has been choosen with this PR if you're interested.

One limitation:

  • Instance Hook after KVO

    Doing instance hook on a KVO'ed class will trigger an in-place swizzle, then, the forwardInvocation: will be applied to the isa-swizzled subclass created by KVO. Aspects will works until the isa-swizzled subclass destoryed once all observer has been removed. As our usual practice, removes all observers in dealloc method. This is the same logic as RAC and it's acceptable I think.

Many thanks for all your work on this project!

@doggy
Copy link
Author

doggy commented Mar 3, 2017

The version number should be bumped to v1.4.3 I think.

@doggy doggy mentioned this pull request Mar 3, 2017
@steipete
Copy link
Owner

steipete commented Mar 7, 2017

This looks great! Working on a v2 is on my TODO for a while now, the day-to-day work to manage our PDF SDK PSPDFKit takes a lot of work but we also actively use Aspects there so it's not going anywhere. I'll see that I find time on the weekend for that.

Thanks a lot for working on that KVO problem - that's a tough one!

doggy added 2 commits November 1, 2017 01:02
…oks`, further more, aspects' forwardInvocation has been skipped while iterating the super class.
@TozyZuo
Copy link

TozyZuo commented Nov 15, 2017

Hi @doggy
I found a new issue about coexistence with KVO.
code like this

id token = [object aspect_hookSelector...]
[object addObserver...] KVO
[token remove]

This will crash.
Because
[className stringByReplacingOccurrencesOfString:AspectsSubclassSuffix withString:@""]
is a Non-existent KVO class.

How could we keep our aspect-hooded class and clean up the object?

@doggy
Copy link
Author

doggy commented Nov 21, 2017

@TozyZuo Yeah, you're right!!

Some code for testing the class is needed after trimming aspects' suffix on that class.
I can handle it for several days, I think.

Thanks for the reporting..
Text to me with DingTalk @振坤 if you want.

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

Successfully merging this pull request may close these issues.

3 participants