github / Rebel

Cocoa framework for improving AppKit
Other
1.13k stars 111 forks source link

Fix completion blocks being completely broken. #61

Closed jwilling closed 11 years ago

jwilling commented 11 years ago

I'm not exactly sure why the block was stored using associated objects in the first place, but regardless, it did not function, and the tests did not cover this issue.

Changed it to a much simpler implementation, which simply stores a copy of the block on the fake delegate.

cc @dannygreg

jspahrsummers commented 11 years ago

Can you please also add a unit test for this?

I'll leave this open until @dannygreg has had a chance to review it as well.

jwilling commented 11 years ago

:cherries: I'm at a loss for how to do a proper unit test for this. I'm sure there's a much better way than what I did.

jspahrsummers commented 11 years ago

:cherry_blossom:

jwilling commented 11 years ago

:apple:

dannygreg commented 11 years ago

The current implementation is working just fine for us… where were you seeing it fail?

The reason for the associated objects is that the completion block belongs to the popover not the delegate. Therefore storing it on the stub delegate doesn't really make much sense.

It also avoided the problem which you are running into in the current implementation that I didn't have to make the returning of the completion block at all complex, I just returned the associated object.

jwilling commented 11 years ago

@dannygreg That's odd that it worked for you. In my case I tested with a CAGroupAnimation, which I added onto a layer, naturally. By the time the animation completed, it turned out that the delegate always existed, but the completion block was always nil.

Correct me if I'm wrong on this, but since CAAnimations are copied before added onto layers, wouldn't the associated object point to the old animation, and therefore wouldn't be able to be referenced in the new, copied animation? Regardless, I'll try to see if I can come up with a simple test case where the old one would fail.

jwilling commented 11 years ago

Actually, the test I wrote in there fails with your implementation. So if you want an example there is one. :wink:

dannygreg commented 11 years ago

Meh… good enough for me!

:melon:

jwilling commented 11 years ago

:icecream: