search

jcoleman / JCNotificationBannerPresenter

A library for generic presentation of "banners" (e.g. to present a push notification) from anywhere inside an iOS app.
MIT License
198 stars 46 forks source link

TapHandler Block execution fails on SDK 5.0 #2

Closed amitpdev closed 12 years ago

amitpdev commented 12 years ago

When compiled with SDK 5.0, the following exception occurs once banner is displayed:

amitpdev commented 12 years ago

I was able to solve this issue, it was bad practice on my side. I'm using JC in a non-ARC project. Xcode forced me to include a more specific property declaration on: @property (nonatomic) JCNotificationBannerTapHandlingBlock tapHandler;

So I modified it to: @property (nonatomic, strong) JCNotificationBannerTapHandlingBlock tapHandler;

All was okay and working under Xcode 4.5.1 on my ML desktop. but running same project under Xcode 4.2 SL laptop crashed.

Anyway it turns out that 'copy' is the right way to go: @property (nonatomic, copy) JCNotificationBannerTapHandlingBlock tapHandler;

So its a good note for any of you who runs JC in non-ARC project.

wiki ? :)

jcoleman commented 12 years ago

Does this happen in the simulator as well? I haven't tested on anything below 5.1; I'm currently downloading the 5.0 simulator as I don't currently have any devices running anything below 5.1.

Edit: I didn't see your newest post until after I clicked comment.

jcoleman commented 12 years ago

Are you using the latest code? My copy has:

@property (nonatomic, strong) JCNotificationBannerTapHandlingBlock tapHandler;

rather than

@property (nonatomic) JCNotificationBannerTapHandlingBlock tapHandler;

Assuming you use the first form, I believe it should work without "copy" as long as you have ARC enabled on the files from this library (I mention this in the README.) Have you enabled ARC on the JC* files? If not, you'll have memory leaks in your project as well as bugs like this one.

I'm updating the README to link to instructions for enabling ARC per file with the following text:

You can enabled ARC per-file by adding the -fobjc-arc flag, as described on a common StackOverflow question.

amitpdev commented 12 years ago

Hi James,

Yes, i've enabled -fobjc-arc on all .m JC files. All projects settings are okay. The crash most probably happens due to 'strong' property. Not why it compiling with Xcode 4.5.1 is bulletproof to that issue.

I really recommend you modify your code to 'copy'. ( when I forked, it was just (nonatomic) )

see this thread for example: http://stackoverflow.com/questions/12061756/blocks-and-arc-copy-or-crash-with-release-build-caused-by-optimization-level

Also, I was looking on other "famos" projects and they all seem to use 'copy': https://github.com/tonymillion/Reachability

On Tue, Oct 30, 2012 at 4:41 PM, James Coleman notifications@github.comwrote:

Are you using the latest code? My copy has:

@property (nonatomic, strong) JCNotificationBannerTapHandlingBlock tapHandler;

rather than

@property (nonatomic) JCNotificationBannerTapHandlingBlock tapHandler;

Assuming you use the first form, I believe it should work without "copy" as long as you have ARC enabled on the files from this library (I mention this in the README.) Have you enabled ARC on the JC* files? If not, you'll have memory leaks in your project as well as bugs like this one.

I'm updating the README to link to instructions for enabling ARC per file with the following text:

You can enabled ARC per-file by adding the -fobjc-arc flag, as described on a common StackOverflow questionhttp://stackoverflow.com/questions/6646052/how-can-i-disable-arc-for-a-single-file-in-a-project .

— Reply to this email directly or view it on GitHubhttps://github.com/jcoleman/JCNotificationBannerPresenter/issues/2#issuecomment-9912966.

jcoleman commented 12 years ago

Interesting, if you read further in the thread on SO it appears that strong should work. It appears to be a compiler bug (http://stackoverflow.com/a/12063130/1114761) which is probably why it works on the newer versions of Xcode.

Edit: That is, strong should copy the block if necessary. Apparently there was a known issue with a previous version of the compiler that didn't do this correctly.

amitpdev commented 12 years ago

If that is the case, would you consider support for earlier Xcode versions on this project ? I'm willing to test on each release. Please make the 'copy' change so I can stop using my fork and sync directly from master.

jcoleman commented 12 years ago

I've updated the code to use copy property assignment semantics. Unfortunately the Apple/Clang ARC docs aren't incredibly clear what should/shouldn't be required. It seems that strong should work as long as it's a property typed as a block (not at id as in the case of NSMutableArray#addObject:), but given the apparent compiler bugs I'll just leave it as copy for now.