Fixed certain issues with the Swift wrapper - #649
Merged
Conversation
…llation The scheduled reset timer used the target/selector form, which retains its target, so dismissing the controller mid-adjustment kept the crop view and both full-size image copies alive until the timer fired. It now uses the block form with a weak reference and is invalidated on dealloc. Touch and gesture cancellation (incoming calls, system alerts) also restart the reset timer instead of leaving the view stuck in its editing appearance.
The closure handlers captured the unwrapped delegate strongly, so the nominally weak delegate was retained for the controller's lifetime -- and in the common pattern where the delegate owns the controller, the resulting cycle leaked both, along with the full-resolution image. The closures now resolve the delegate through self at call time, which also stops a replaced delegate from continuing to receive callbacks.
- The bridging closures capture the delegate weakly, honouring the weak contract of the delegate property, and fall back to the default dismissal if it has been deallocated before a callback fires - Closures the host assigned directly are no longer cleared for selectors the delegate doesn't implement; the bridge tracks which slots it owns so that replacing the delegate tears down only its own - main's cropViewControllerDidTapDone bridge is folded into the same scheme. It gets no dismissal fallback, since it fires alongside the crop callbacks rather than instead of them, so those still own the dismissal
`CropViewController.swift` had no automated coverage at all, so the closure bridge it maintains over the Objective-C delegate -- the part of this library with the most intricate lifetime rules, and the source of two of the bugs fixed in 3.1.2 -- was only ever verified by hand. Rather than introduce a second test target and a second CI scheme, the existing TOCropViewControllerTests bundle now also compiles CropViewController.swift and a Swift test class, which is how both example apps consume the wrapper: sources compiled in directly, with the Objective-C layer exposed through a bridging header. `fastlane test` picks the new cases up with no change to the workflow. Eleven cases, covering: per-selector installation of the bridging closures; tear-down when the delegate is replaced or cleared; survival of closures the host assigned directly; the weak delegate not being retained; tolerance of a delegate that has been deallocated; forwarding of the ~20 wrapped properties to the underlying controller; and the child view controller relationship. Reverting the delegate-bridge fix from the previous commit fails three assertions across two of them, so they do bind to the behaviour they cover. One thing the exercise turned up but which isn't fixed here: TOCropView.h forward-declares TOCropOverlayView rather than importing it, and Swift drops properties whose class is incomplete, so `cropView.gridOverlayView` is invisible to Swift wherever these headers are consumed without a module. Importing it from the public header breaks the framework target's module verifier, because that target publishes no headers at all -- untangling that is its own change, so the test bridging header imports it locally for now.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.