🐛 End the wait when a document picker sheet is swiped away - #643
Open
anggrayudi wants to merge 1 commit into
Open
🐛 End the wait when a document picker sheet is swiped away#643anggrayudi wants to merge 1 commit into
anggrayudi wants to merge 1 commit into
Conversation
UIDocumentPickerViewController is presented as a sheet, and a swipe dismissal does not always reach documentPickerWasCancelled — the reporter of vinceglb#138 sees it when the swipe starts before the presentation animation has finished. The caller waits on a suspendCancellableCoroutine, so the picker never returns and the tap looks like it did nothing. PHPicker already guards this with a UIAdaptivePresentationControllerDelegate. Do the same for both document picker paths, but on the existing delegate rather than a second object: a dismissal often reports through both protocols, and resuming a continuation twice throws. One finishOnce() guard on one object makes that impossible. Setting presentationController?.delegate is a no-op where there is no presentation controller, so nothing changes for styles that have none. Closes vinceglb#138 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Closes #138. Follows up on my comment there.
The gap
PHPickerViewControllergets aUIAdaptivePresentationControllerDelegateso a swipe dismissal resumes the caller. The twoUIDocumentPickerViewControllerpaths never got one:.delegatepresentationController.delegateUIDocumentPickerViewController(save)UIImagePickerController(camera)UIDocumentPickerViewController(open)PHPickerViewControllerSo when
documentPickerWasCancelleddoes not fire — which @derynia reports happens when the swipe starts before the presentation animation finishes — thesuspendCancellableCoroutinewaits forever and the tap looks dead.The change
Rather than a second delegate object mirroring
PhPickerDismissDelegate,DocumentPickerDelegatenow implements both protocols behind onefinishOnce()guard.That is deliberate. A dismissal frequently reports through both
documentPickerWasCancelledandpresentationControllerDidDismiss, and the call sites resume a continuation that accepts exactly one answer — a second resume throwsIllegalStateException. One object with one guard makes double-resume impossible. (For what it's worth, the PHPicker pair has its guard only onPhPickerDelegate, not onPhPickerDismissDelegate.)presentationController?.delegate = …is a no-op where there is no presentation controller, so styles without one behave exactly as before.The camera path is left alone — it is full-screen and not what #138 describes.
Verification, and its limits
Tested: three new tests in
filekit-dialogs/src/iosTestcovering the delegate contract — swipe-dismiss with no cancel callback reports cancelled; cancel followed by swipe-dismiss reports cancelled once; a pick followed by swipe-dismiss reports only the pick. 45 tests pass oniosSimulatorArm64Test,compileKotlinIosArm64clean. DisablingfinishOnce()makes two of the three fail, so the guard is doing real work.Not tested — please read before merging: I could not reproduce the original bug or observe the fix working on a simulator. Driving a mid-animation swipe needs synthetic UI events, and the environment I work in cannot grant the Accessibility permission that requires; two attempts got as far as a running sample app and no further. So this fix is argued from the delegate contract, the pattern already in this repo, and the reporter's description — not from watching the symptom disappear.
You said in October 2024 that you could reproduce it. If you can run that same reproduction against this branch, that is the check I was unable to do.
Why it may matter more than a missing callback
Field report, cause unconfirmed: in an app I work on the iOS file picker intermittently stopped opening at all, and stayed broken until the app was killed. UIKit ignores
presentViewController()while the presenter already has apresentedViewController, and that presenter is the root view controller — so one picker left presented breaks every later one. We patched the symptom by clearing stale presentations before launching, and never identified the trigger. If a mid-animation swipe can leave the controller presented while no delegate fires, this issue would explain it. I cannot prove that link.🤖 Generated with Claude Code