Skip to content
This repository has been archived by the owner on Sep 4, 2020. It is now read-only.

[ios] "the right way" to swizzle #427

Closed
athibaud opened this issue Dec 15, 2015 · 4 comments
Closed

[ios] "the right way" to swizzle #427

athibaud opened this issue Dec 15, 2015 · 4 comments

Comments

@athibaud
Copy link
Contributor

it seems using method_exchangeImplementations() can have some undesired implications and the right way to do method swizzling is to use method_setImplementation()

shouldn't run into the implications described in that article seeing the use of this plugin is more of a standalone application use rather than library use but still.
i could implement and pr this if needed.

cheers

@macdonst macdonst added this to the Release 1.5.0 milestone Dec 15, 2015
@macdonst
Copy link
Member

PR merged

@athibaud
Copy link
Contributor Author

i imagine you mean this pr #426 (thanks for merging :-)) but the code still uses method_exchangeImplementations() rather than method_setImplementation()

the pr i'm referring too in this issue does not yet exist :-)

@macdonst
Copy link
Member

@athibaud my bad

@macdonst macdonst reopened this Dec 18, 2015
@macdonst macdonst modified the milestones: Release 1.6.0, Release 1.5.0 Dec 18, 2015
@macdonst macdonst modified the milestones: Release 1.7.0, Release 1.6.0 Feb 29, 2016
devgeeks added a commit that referenced this issue Mar 1, 2016
This still isn't the "right way" linked in #427, but seems to be safe
and void of side effects due to the namespacing of all the methods.

It has been tested to work side by side with another plugin that also
swizzles AppDelegate's applicationDidBecomeActive.

1. wrapped swizzling in dispatch_once
2. first attempts class_addMethod / class_replaceMethod but if that
fails falls back to method_exchangeImplementations.
3. swizzled_init => pushPluginSwizzledInit (if other plugins attempt to
swizzle init and use the same name, there seems to be a collision)
4. added an observer for UIApplicationDidBecomeActiveNotification
instead of overriding applicationDidBecomeActive
5. applicationDidBecomeActive => pushPluginOnApplicationDidBecomeActive
(and it becomes a notification observer instead)

See http://nshipster.com/method-swizzling/ and "Avoid collisions" under
"Considerations".

This will need testng by someone that understands the plugin better than
I do, but it should not be changing the core functionality at all. The
pushPluginOnApplicationDidBecomeActive method *is* called when the
application becomes active, etc.

Goes a long way toward fixing #427 without regressing #447, etc.
@macdonst macdonst modified the milestones: Release 1.6.0, Release 1.7.0 Mar 1, 2016
macdonst pushed a commit that referenced this issue Mar 9, 2016
This still isn't the "right way" linked in #427, but seems to be safe
and void of side effects due to the namespacing of all the methods.

It has been tested to work side by side with another plugin that also
swizzles AppDelegate's applicationDidBecomeActive.

1. wrapped swizzling in dispatch_once
2. first attempts class_addMethod / class_replaceMethod but if that
fails falls back to method_exchangeImplementations.
3. swizzled_init => pushPluginSwizzledInit (if other plugins attempt to
swizzle init and use the same name, there seems to be a collision)
4. added an observer for UIApplicationDidBecomeActiveNotification
instead of overriding applicationDidBecomeActive
5. applicationDidBecomeActive => pushPluginOnApplicationDidBecomeActive
(and it becomes a notification observer instead)

See http://nshipster.com/method-swizzling/ and "Avoid collisions" under
"Considerations".

This will need testng by someone that understands the plugin better than
I do, but it should not be changing the core functionality at all. The
pushPluginOnApplicationDidBecomeActive method *is* called when the
application becomes active, etc.

Goes a long way toward fixing #427 without regressing #447, etc.
@macdonst macdonst removed the staging label Mar 9, 2016
@macdonst macdonst closed this as completed Mar 9, 2016
@lock
Copy link

lock bot commented Jun 5, 2018

This thread has been automatically locked.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

2 participants