Fixed and simplified rounded arrow code.

sammcewan / WYPopoverController

WYPopoverController is for the presentation of content in popover on iPhone / iPad devices. Very customizable.

Other

253 stars 74 forks source link

Fixed and simplified rounded arrow code. #30

Closed rsanchezsaez closed 9 years ago

rsanchezsaez commented 9 years ago

Fixes issue #28. The problem was related to drawing the rounded arrow points in the incorrect order.

Also:

Simplified arrow drawing code: no longer creating needless UIBezierPaths, using the single outerPathRefinstead.
Made all rounded arrows equal (up/down arrows were different than left/right ones)
Generalised target point calculation for rounded and straight arrows.
Renamed adjustsTintColor to dimsBackgroundViewsTintColor.
Changed usesRoundedArrow and dimsBackgroundViewsTintColor from NSNumber to BOOL. We avoid packing/unpacking between NSNumber and BOOL when setting this.
Updated README.md.

maciekish commented 9 years ago

I cherry picked these into my fork but it crashes on this line https://github.com/obviousengineering/WYPopoverController/blob/06a877d2336f6ff838a48fc65c44e9e236e54bf4/WYPopoverController/WYPopoverController.m#L1730

The error is:

* Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '* Illegal property type, c for appearance setter, _installAppearanceSwizzlesForSetter:'

I could have merged it wrong but it seemed like a very straight forward merge... Any ideas?

Here is the debugger view of aTheme

alt text

and appearance

alt text

rsanchezsaez commented 9 years ago

Hmm, it doesn't crash for me.

Can you push your cherry-picked code to a temporary branch on your fork? A sample of the user code that makes it crash would help as well.

If you are using a WYPopoverTheme, did you replace the @NO / @YES in your theme by NO / YES?

maciekish commented 9 years ago

Yes i replaced them, wouldn't build otherwise :) I reverted my changes for now because Im busy with something else. Will look into this again soon though.

rsanchezsaez commented 9 years ago

Keep me posted, I'm really intrigued and want to fix any problem that there may be.

I don't use a custom WYPopoverTheme, but this code works for me:

WYPopoverBackgroundView *popoverAppearance = [WYPopoverBackgroundView appearance];
popoverAppearance.usesRoundedArrow = NO;

rsanchezsaez commented 9 years ago

Another thought I had, on which iOS version did you experience the crash? Maybe it's an iOS 7 (or earlier) thing.

maciekish commented 9 years ago

I think it was on iOS 7 actually.

rsanchezsaez commented 9 years ago

Indeed, it's actually an iOS 7 and earlier issue. The problem is that UIAppearanceContainer doesn't officially support BOOLs (although it seems Apple added undocumented BOOL support on iOS 8).

I'm going to add a new commit to the pull request fixing this in a moment.

rsanchezsaez commented 9 years ago

Done. It should work now. Thanks for spotting this @maciekish! :-)

maciekish commented 9 years ago

Ill try it in a bit, thanks :)

maciekish commented 9 years ago

Works a treat, thanks again.

maciekish commented 9 years ago

@sammcewan Any chance we can have this merged please?

sammcewan commented 9 years ago

@maciekish Sincere apologies. I've been AWOL for a little. Back on the job.