fix: stand down swipe-to-leave while a voice note is playing - #1668
fix: stand down swipe-to-leave while a voice note is playing#1668Chessing234 wants to merge 3 commits into
Conversation
A playing voice note overlays its waveform with a seek DragGesture(minimumDistance: 0). The DM sheet attaches swipe-right-to-leave to the same message list with .highPriorityGesture, and a high-priority ancestor drag starves descendant gestures -- so a scrub never reaches the seek, and one that drifts past horizontal > 80 / vertical < 60 ends the conversation mid-playback instead. While audio is audible the scrub is the likelier intent, so the leave gesture stands down for the duration. Leaving stays available from the sidebar. The thresholds move into PrivateChatSwipeToLeavePolicy so the decision is testable without a gesture recogniser, and the coordinator that already enforces single-playback exposes hasActivePlayback as the signal. Refs permissionlesstech#1658
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f61aa98c4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard PrivateChatSwipeToLeavePolicy.shouldLeave( | ||
| translation: value.translation, | ||
| isVoiceNotePlaying: VoiceNotePlaybackCoordinator.shared.hasActivePlayback |
There was a problem hiding this comment.
When the touch starts on an interactive waveform while playback is active, this still installs the same high-priority ancestor DragGesture; this guard only runs after that gesture has already won recognition over the waveform's DragGesture(minimumDistance: 0). In that scenario the scrub still never reaches WaveformView—the change only prevents the conversation from leaving—so the swipe gesture needs to be omitted or disabled while playback is active rather than returning here.
Useful? React with 👍 / 👎.
| var hasActivePlayback: Bool { | ||
| activeController != nil | ||
| } |
There was a problem hiding this comment.
A paused voice note still leaves activeController set: VoiceNotePlaybackController.pause() sets isPlaying = false and releases the audio session, but only stop()/finish deactivate the coordinator. Because this property equates holding the slot with audible playback, any qualifying private-chat swipe remains suppressed after the user pauses a note, until the row disappears or playback is stopped some other way.
Useful? React with 👍 / 👎.
pause() left activeController set, so the slot meant "a row was played at some point" rather than "audio is audible". Any reader of hasActivePlayback — the swipe-to-leave policy above all — kept seeing playback after the user paused, until the row disappeared or something called stop(). A paused note has nothing to yield to the next playback, so it releases the slot. hasActivePlayback becomes stored and published, so a view can react to it rather than sample it once.
The guard in onEnded prevented the conversation from being left, but the scrub was still lost: an armed .highPriorityGesture starves its descendants at recognition time, long before either gesture ends, so the waveform's seek never saw the drag. The gesture is now attached with `including: .subviews` while a note is audible, which yields recognition to the waveform. The onEnded guard stays as the second line of defence for a drag already in flight when playback starts.
|
both findings are right, and the first one is the important half — the onEnded guard stopped the conversation from being left but did nothing for the scrub, because an armed highPriorityGesture starves its descendants at recognition time. 2da25a9: the gesture is attached with ca15f66: pause() now releases the slot. it was leaving activeController set, so hasActivePlayback meant "a row was played at some point" and the gesture stayed suppressed after a pause until the row went away. a paused note has nothing to yield to the next playback, so the slot is the wrong thing for it to hold. hasActivePlayback is stored and published now, which is also what lets the view react rather than sample it once. verification, plainly: xcode is not on this machine any more (command line tools only), so swift-testing does not build and i could not run the suite locally this time — the app target does type-check with both changes. the added coordinator test covers pause releasing the slot; CI runs it. gesture arbitration itself is still not unit-testable, as noted in the PR body. |
1 participant
Refs #1658.
What happens
WaveformViewoverlays the bars with a seekDragGesture(minimumDistance: 0)(
WaveformView.swift:55), live while the note plays —VoiceNoteView.swift:71passes
isInteractive: playback.isPlaying. In a private chat that waveformrenders inside the message list, and the sheet attaches swipe-right-to-leave to
that same list with
.highPriorityGesture(ContentSheetViews.swift:575).A high-priority ancestor drag starves descendant gestures, so two things follow:
a scrub across the waveform never reaches the seek, and a scrub that happens to
travel past
horizontal > 80, vertical < 60ends the conversation while the noteis still playing.
The issue notes this is the third appearance of the same starvation: #1405 fixed
it as a cause (the mic press-and-hold was cancelled 3-10 ms after touch-down, and
the swipe moved off the whole sheet onto the message list), and #1402 hardened
against it (the reveal moved onto a
Button, because Button actions survivestarvation while descendant gestures do not). The comment at the attachment site
already records the mechanism.
Fix
While audio is audible the scrub is far likelier to be the reader's intent than
leaving, so the leave gesture stands down for the duration of playback.
Two supporting changes keep that decision honest rather than buried in a view:
PrivateChatSwipeToLeavePolicyholds the 80/60 thresholds and the playbackguard, so the decision is testable without a gesture recogniser.
VoiceNotePlaybackCoordinator— which already exists to enforcesingle-playback and already exposes an
internal init()"so tests can isolatetheir own exclusivity slot" — gains
hasActivePlayback. No new state, no newownership; it is the object that already knows.
Deliberate behaviour change, stated plainly: while a voice note plays, the
swipe gesture will not leave the conversation. Leaving stays available from the
sidebar. That is the trade this fix makes, and it is exactly the reported harm
("leaves the conversation while the note is playing") — but it is a change, not
a pure bug fix, so please push back if you'd rather the seek be rescued a
different way (e.g. moving the seek onto a control that survives starvation, the
#1402 route).
Test plan
swift build --build-tests— clean; no new warnings (the fourno calls to throwing functionswarnings inBLEAnnounceHandlerTestsandthe unhandled-resource warning are pre-existing on
main).swift test --skip-build --parallel— 2032 tests in 221 suites passed.Baseline on
mainbefore this change: 2018 tests in 219 suites, also green.The delta is exactly the 14 new tests in 2 new suites.
guard !isVoiceNotePlayingline fails exactly the two suppression testsand nothing else.
swift testcannot exerciseSwiftUI gesture arbitration, so "the seek now actually receives the drag"
is verified by reading
.highPriorityGesturesemantics and the two priorPRs, not by execution. A simulator UI test would be needed to prove it, and
I have not written one.
New coverage, 14 tests:
steep drags do not; vertical is judged on magnitude so a shallow upward drag
still leaves
stopped, and playback never invents a leave from a drag that never qualified
hasActivePlayback— idle is false, activate/deactivate flip it, a barereservedoes not count as playing (an async starter that never becomesaudible must not suppress the gesture), a foreign
deactivateis ignored, anda takeover pauses the previous holder while staying active