Repository navigation
feat(chat): hold to speed up playback and leave it with an edge swipe - #263
TiaraBasori wants to merge 2 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Automated PR reviewHead: Review unavailable: Incomplete model answer (length); manual review required. Remaining gates
No PR code runs with the model key or write token. Free-only routing; provider/rate-limit failures never authorize a merge. Existing review findings must be resolved, not hidden by a new recommendation. |
iebb
left a comment
iebb
left a comment
There was a problem hiding this comment.
The new normal-path widget tests are useful and the feature is a genuine improvement, but the asynchronous speed-restore boundary still needs a regression before approval. _applyPlaybackSpeed awaits controller.setPlaybackSpeed then writes _speed whenever mounted; _stopHeldSpeed launches a separate unawaited restore. There is no generation/owner check rejecting an older held-rate completion after release, cancellation or a newer rate request. The fake platform in video_player_hold_speed_dismiss_test.dart completes setPlaybackSpeed immediately, so these tests do not exercise that ordering.
Please use deferred native-platform completions to test release/cancel before the held-rate Future completes, and disposal or another speed change while requests are pending. Verify both the final native rate and the displayed/persisted base rate remain the pre-hold/current intended value with no late stale write. Also cover multi-pointer cancellation of the edge-dismiss gesture; its listener currently finishes/cancels on any pointer-up/cancel rather than checking the tracked pointer. Add owner/generation/pointer guards if those production-path tests expose the race. #257 overlaps this player implementation, so the intended combined result must receive a fresh full Quality run. This is source-traced risk and a coverage request, not a claim a new reproduction was run.
41daab0 to
70d4d41
Compare
|
Addressed at head Two guards on the production path, plus the two regressions that exercise them. Every rate request is stamped with The edge swipe now tracks its pointer.
Mutation checks, quoted: removing the generation guard from Regarding the overlap: #257 has been force-pushed to its own head with its own three widget cases, and this branch's |
There was a problem hiding this comment.
Re-reviewed 70d4d41. The tracked-pointer fix and late Dart-state generation guard address important parts of the earlier review. [P2] Native rate ordering remains unprotected: _applyHeldSpeedRate issues controller.setPlaybackSpeed before its generation check (3725-3732), while release issues a separate unawaited restore (3751-3770). Nothing waits for an earlier held request to settle before the final native restore. Completion order restore(1x), then old held(2x) can leave the native player at 2x while the badge/base says 1x. The new fake appends playbackSpeeds before its barrier and never records an applied native rate after completion; its regression completes the held barrier first, then restore, so it cannot prove the reverse order claimed in the body. Serialize/reconcile native writes against the newest desired rate and test both completion orders, cancel, a newer ordinary-speed request/new hold, and disposal, asserting a fake native rate updated AFTER each barrier as well as displayed/base state. Also verify interaction with #257's revised surface-pan layer on the final combined result. This is a static causal trace of the current implementation, not a new local app reproduction. Keep this genuine feature open for revision.
Integration note after #257 landed at master 2306d2a: a read-only merge-tree check confirms conflicts in lib/chat/video_player_view.dart and all eight generated locale JSON files. Preserve #257's original-press measurement, displaced-tap fallback and deliberate-pan behavior when reconciling the held-speed replacement. If held seeking is intentionally replaced, adapt its tests to the equivalent release/cancel/disposal speed-restoration guarantees instead of dropping that safety coverage; then run the full combined suite. Regenerate the shared JSON from the reviewed canonical XML. No conflicting code was resolved speculatively.
|
Addressed at head [P2] Native rate ordering is now serialized against the newest desired rate. There is one place that writes to the controller: The fake now records the native rate after the barrier.
Mutation check, quoted: reverting release to an independent unawaited #257's surface-pan layer is preserved verbatim. A line-set comparison against The locale conflicts were regenerated from the canonical XML ( Combined-result gates on the merged tree: format |
TiaraBasori
left a comment
TiaraBasori
left a comment
There was a problem hiding this comment.
Incremental fix on the newest rebase head caf03d6d (parent 876cc39a, already rebased onto master 3c317d0c which carries #257). Two deltas:
1. One serialization point, one tail. _pumpHeldSpeedWrites() remains the only caller of setPlaybackSpeed (start, release, cancel, the chrome's speed step and _SerializedVideoActions all only move _heldSpeedDesiredRate), but the previous head's pump had a duplicated re-arm tail after its finally: both tails ran the same if (!_heldSpeedDisposed && mounted && _heldSpeedDesiredRate != null) unawaited(_pumpHeldSpeedWrites()), so after the in-flight flag cleared the second tail could re-issue a rate the newest desired rate had already replaced and re-settle it on the native side. Now exactly one tail remains and it re-reads _heldSpeedDesiredRate after the flag clears, so both completion orders (restore-first and held-first), cancel, a newer rate request and dispose all converge on the newest desired rate. lib/chat/video_player_view.dart:3901.
2. The hold is pointer-owned. The viewport-wide pinch listener ended the hold on ANY pointer up/cancel (video-pinch-zoom-listener), so a second finger lifting over the player restored the rate while the first finger was still holding — the same tracked-pointer gap you flagged on edge-dismiss, on the hold path. The listener now routes through _stopHeldSpeedForPointer, which ends the hold only for the pointer the long-press started on (_heldSpeedPointer, the oldest touch still down — matching PrimaryPointerGestureRecognizer's primary pointer, since LongPressStartDetails carries no pointer id). _stopHeldSpeed/_cancelHeldSpeed clear it; a non-touch hold keeps the old unconditional end.
Fake recording point, unchanged and verified. nativeRates is appended only after the per-call barrier releases (setPlaybackSpeed awaits the completer then records), so an assertion on it reads the rate the native side actually settled at, not the issue order.
Regressions (test/video_player_hold_speed_dismiss_test.dart, 15 pass, driven by deferred completions): both completion orders (held first then restore → [2.0, 1.0]; restore settles first then the held one → releasing the 1x barrier first applies nothing until the 2x settles, then [2.0, 1.0]), a cancelled press restores the base rate behind a pending held request, a newer speed request supersedes a pending held request ([2.0, 1.25], 1x never written), a second hold during a pending restore ends at its own base ([2.0, 1.0, 2.0, 1.0]), a held rate still pending at disposal writes nothing after unmount, plus the two new multi-pointer cases: a second finger lifting does not end the first hold (badge stays, no 1.0 queued, the owner's release restores) and a second finger cancelling does not abandon the tracked edge swipe.
Mutation evidence: reverting the pinch listener to the unconditional _stopHeldSpeed() fails a second finger lifting does not end the first hold (the 2x badge disappears the moment the second finger lifts); reverting release to an independent unawaited setPlaybackSpeed(1x) still fails the restore barrier opens before the held one with Expected: <1.0> Actual: <2.0>. Both reverted, suite green (flutter analyze clean on the touched file, dart format --set-exit-if-changed clean). PR body updated for the new head, counts and mutations.
2774ec3 to
a01accc
Compare
iebb
left a comment
iebb
left a comment
There was a problem hiding this comment.
Reviewed exact head a01accc9b55869131704cc68509de36413ef5f10. Changes requested before publication.
The single write pump is a real improvement, but [P2] ordinary playback ticks can still corrupt the remembered base rate. In lib/chat/video_player_view.dart:1524–1531, _heldSpeedBaseRate is updated whenever _heldSpeedActive is false. Release at :3827–3835 clears that flag while a held/restore native write may still be pending. A position/buffering tick reporting transient 2x can then overwrite the original 1x base; re-hold at :3796–3805 uses that corrupted remembered base and can request 4x and later restore the wrong rate.
Gate tick-derived base updates while any held, desired-rate or write-in-flight work is outstanding, rather than just while the badge is active. Extend the deferred-platform tests to inject ordinary ticks between release and held-write settlement, and while restore is pending; re-hold and assert the final native rate and displayed/remembered base return to the original rate.
I explicitly withdraw the old duplicate-write-pump/native-ordering and unconditional second-finger-release claims: single-tail serialization and pointer-owned release address those earlier mechanisms. The remaining tick/base race is distinct and present at this head. Existing tests that park only the rate calls do not establish safety against intervening ordinary controller notifications.
Resolve the reported current-master conflicts without dropping the merged surface-pan behavior or release/cancel/disposal coverage, and regenerate localization from canonical XML. This is a source-derived interleaving risk, not a claim that a new runtime reproduction was executed.
No approval is granted. Current-base merged-result Quality is not yet fresh in the reviewed evidence; before publication require fresh ordinary PR Quality on this exact head merged with live master, applicable platform checks, resolved findings and genuine exact-head owner approval. No incoming PR code was executed for this review.
a01accc to
43890a3
Compare
|
Addressed the [P2] tick/base race at new head Tick-derived base updates are now gated on all held work being outstanding. The old gate in Regression ( Gates at this head with CI's toolchain ( No conflicts with #257's surface-pan layer at this head — the rebase kept master's layer untouched; only the tick gate changed. |
iebb
left a comment
iebb
left a comment
There was a problem hiding this comment.
Re-reviewed updated head 43890a3770fe47730e05be7b7a0854c652e27406 against current master 257cd48398b60f92febd31cb37b02e5676fbba4e.
The earlier tick/base-rate source finding is addressed: _onTick now preserves the remembered rate while a hold is active, a native rate write is outstanding, or a desired rate is pending. I withdraw that P2 finding at this head. The added fake-platform buffering event drives the production controller listener while the held write is parked, which is a more faithful boundary than inspecting only the write queue.
Validation still blocks publication. A static three-way merge with live master reports conflicts in all eight generated assets/l10n/*.json files; there is no current-head merged-result Quality run. Resolve those by regenerating from the reviewed merged canonical XML, preserving current master keys, and run the updated focused reproduction plus full Quality in GitHub Actions. Retain coverage for a tick while restore is pending and re-hold during that interval; the new case emits its tick before the held write settles and re-holds only after restoration has finished, so it does not itself prove those additional interleavings.
The change remains a useful compliant/unchanged playback improvement. This comment acknowledges the corrected source rather than carrying forward a fixed defect; it does not claim a newly executed runtime reproduction or grant approval before the remaining gates pass. The separate master app-icon/supporter test failures also remain relevant to any current-base merge. No incoming PR code ran on the maintainer's computer.
iebb
left a comment
iebb
left a comment
There was a problem hiding this comment.
Reviewed the current error path at exact head 43890a3770fe47730e05be7b7a0854c652e27406 after repairing master. The previous transient-tick finding remains resolved.
[P2] A failed native rate write strands the pending restore. lib/chat/video_player_view.dart:3930-3935 catches any setPlaybackSpeed failure while still mounted, calls _stopHeldSpeed(), then returns. _stopHeldSpeed queues the base restore and calls the pump while _heldSpeedWriteInFlight is still true, so that call immediately returns. The finally clears the in-flight flag, but the return skips the tail at :3955-3959. No writer is left to process or clear the desired restore; later ticks/chrome continue treating that stale desired rate as outstanding. A mounted native failure is not necessarily a disposed route.
Define a bounded failure/restore policy and cover a rejected held write and rejected restore through the fake platform/controller boundary. Assert the native request sequence, cleared pending state, correct remembered/displayed rate after later callbacks, no retry loop, and no post-disposal writes. I have not executed this PR locally or claimed a device failure; the stranded-writer path follows directly from the current control flow. This is more useful coverage than adding further cosmetic or type-only assertions.
The eight generated localization conflicts still need regeneration from merged canonical XML before a fresh current-master Quality run. Keep this genuine playback improvement open for revision; the corrected tick/pointer handling is acknowledged.
Rebased onto master's surface-pan layer: the hold-to-seek rule master added is replaced by the hold-to-speed hold, while its tap/pan gesture rules stay untouched. Native rate writes serialize through one pump that always lands the newest desired rate, the hold tracks its owning pointer so a second finger cannot end it, and a rightward swipe from the left edge dismisses the player. The tap-toggle suite keeps master's tap guarantees and now asserts the speed hold in their place.
Releasing a hold cleared the badge flag while the held write and the restore were still travelling on the wire, so an ordinary buffering or position tick in that gap read the controller's transient 2x and overwrote the remembered base — a re-hold then doubled the corrupted base and the restore landed the wrong rate. The tick gate now treats a hold, an in-flight write or a pending desired rate as held work outstanding and only lets the controller's reported rate become the truth once nothing is left. Regression: a tick reporting the transient rate between release and settlement cannot poison the remembered base injects a buffering tick between release and the held write's settlement and asserts the base survives, the final native rate restores, and a re-hold doubles the untouched base. Fails on the previous head with base 2.0.
43890a3 to
4235783
Compare
|
Rebased onto current master |
|
New master-integration note for unchanged head The previously reviewed native-write failure path at |
Change
Holding a video played while the player stepped the position forward or backward by ten seconds, and the only way out of a fullscreen video was the close button. Both are now gestures on the playback surface:
setPlaybackSpeedand keyboard speed cycling cannot overwrite it mid-hold, and the periodic controller listener no longer records the held rate as the base rate._close()exit the close button uses. The band claims movement only once it travels 18pt and is clearly horizontal (|dx| > 1.2·|dy|), so brightness and volume drags that start in the same band still reach the vertical gesture, and progress seek, video navigation and pinch zoom keep working everywhere else. An abandoned swipe snaps back and is suppressed from finishing as a tap on the package's surface recognizer or as a seek.Affected platforms: Android and iOS (the touch gesture path in
videoPlaybackSurfaceUsesPanGestures, unchanged in scope). Desktop keeps package-owned pointer behaviour.A vertical (downward) dismissal was considered and not added: the brightness and volume gestures already own every vertical drag on the playback surface, and saving the gesture everywhere for one extra exit was not worth losing them. The edge swipe plus the existing close button cover the same need.
Telegram API Terms classification
Follow the contribution and agentic review rules. Select exactly one classification and explain it below.
This is an agentic source-code review classification. Every store version must remain compliant with applicable Telegram API Terms; the guarded-source exception does not apply to store distribution or review.
allowTelegramTosViolations(I_DONT_FUCKING_CARE_ABOUT_TOS), disabled in official builds, and available only in an explicitly enabled self-build.Reason and affected Telegram behavior:
Both changes are local playback presentation only. No TDLib request is added, removed or reordered, nothing is sent to Telegram, and no account state (read status, online, typing, self-destruct, read receipts) is touched. Hold-to-speed and the edge-swipe exit only change how this device renders and navigates a video the user already opened.
Basic checks (every PR)
Check each item after verification. If a check is inapplicable, explain why in the evidence below before checking it.
dart format --output=none --set-exit-if-changed lib test integration_test test_driver.flutter analyze.flutter test --concurrency=4.Commands, results, platform/build/UI evidence, and reasons for inapplicable checks:
Toolchain: Flutter 3.44.2 (
/tmp/flutter-3.44.2). This head is rebased onto current master3c317d0c(which already contains #257's surface-pan layer) and carries0e46ba40(serialized rate writes), the876cc39amaster merge, andcaf03d6d(the re-review fix: single-tail pump plus pointer-owned hold).Tests, run serially with the CI SDK (
--no-pub -j 1, one file per invocation), all on the merged tree:How the native rate is ordered. Every rate write goes through one place:
_heldSpeedDesiredRate(the newest rate anyone wants) plus_pumpHeldSpeedWrites()(lib/chat/video_player_view.dart:3901), whichawaitscontroller.setPlaybackSpeed, re-reads the desired rate and repeats while it keeps moving. Start, release, cancel, a chrome speed step and dispose only move the desired rate, so a held 2x that is in flight when release asks for 1x forces the 1x write after the 2x settles, whichever order the platform completes them in. The Dart side (_speed, the badge,VideoPlayerView.debugLastBaseSpeed) follows the same pump, and dispose issues nothing after unmount. On this head the pump also has exactly one re-arm tail: the previous head's duplicated tail could re-issue a rate the newest desired rate had already replaced once the in-flight flag cleared, so the second completion re-settled a stale rate.test/video_player_hold_speed_dismiss_test.dartrecords the rate the fake platform actually settled at, after the per-call barrier is released (nativeRates), so the assertions are about the native result rather than the order requests were issued:a held rate that completes first then the restore— held 2x settles, restore issued →nativeRates == [2.0, 1.0].the restore barrier opens before the held one— the 1x barrier is released first and applies nothing until the 2x settles, then 1x →[2.0, 1.0]; this is the reverse order the previous head could not prove and the one that used to leave the player at 2x.a cancelled press restores the pre-press rate— pointer cancel while the held write is pending → native ends at 1.0.a newer speed request supersedes a pending restore— chrome 1.25 while the held 2x and the restore are queued → native[2.0, 1.25]; 1x is never written.a second hold during a pending restore ends at its own base— the second hold measures 1.0 as its base (not the controller's 2x) →[2.0, 1.0, 2.0, 1.0].a held rate still pending at disposal writes nothing after unmount— the pending completion lands after teardown and nothing is written.a second finger lifting does not end the first hold— the hold is pointer-owned (_heldSpeedPointer); a second touch lifting or being system-cancelled over the player leaves the badge up and queues no restore, and the owning finger's release is what restores 1x.a second finger cancelling does not abandon the tracked edge swipe— the swipe drag keeps its offset when a different pointer is cancelled, and the tracked pointer still closes the player.Each case also asserts the displayed state:
debugVideoPlayerBaseSpeedand the speed menu label (Playback Speed 1x/2x).Interaction with #257. Master's surface-pan layer is preserved verbatim; a line-set comparison of
_playbackGestureEngageDistance = kTouchSlop * 1.75(:168),videoPlaybackPanEngages(:189),videoSurfacePanReplacesTap(:195), the original-press origin (:3358), the displaced-tap fallback (:3745) and_start/_update/_finish/_cancelSurfacePanGestureagainstorigin/mastershows 0 missing lines. Only the held-seek remnants this change replaces are gone (_heldSeekDwell,videoHeldSeekFirstStepAt,videoHeldSeekStepInterval,videoHeldSeekSteps, 0 references remain), and the equivalent safety coverage now lives intest/video_player_tap_toggle_test.dart(slow tap leaves no rate, a hold speeds up and restores on release, cancel/dispose restore, and a static contract that both ends route through the pump). One consequence worth naming: the old test that pinnedvideoHeldSeekFirstStepAtabove the platform drag slop cannot survive held-seek's removal, so that dwell-policy coupling is gone; the behaviour it protected (a press that outlives long-press never leaves a held rate) is covered behaviourally instead.Mutation checks, quoted: reverting release to an independent unawaited
controller.setPlaybackSpeed(1x)failsthe restore barrier opens before the held onewithExpected: <1.0> Actual: <2.0>— the native player left at 2x while the badge and base say 1x, exactly the trace in the review. ReplacingvideoHeldSpeedRatewith the current rate fails the hold case withFound 0 widgets with text "2x"; making the edge commit distance unreachable fails the close case withExpected: <1> Actual: <0>. On this head, reverting the pinch listener to the unconditional_stopHeldSpeed()on any pointer up failsa second finger lifting does not end the first hold(2xbadge gone after the second finger lifted). All reverted, suite green.Localization and generated files: one new key
videoPlayerHoldPlaybackSpeed("Hold for faster playback", plus zh-Hans-CN/zh-Hant-TW/ja-JP/ko-KR/de-DE/fr-FR/es-ES) added to all 8translations/strings/<locale>/strings.xmlandlib/l10n/app_localizations.dart; the merged tree'sassets/l10nwere regenerated from the canonical XML.python3 translations/tools/check.py(8 locales x 3935 keys, OK),python3 translations/tools/gen_assets.py --check(9 locale assets are up to date) andpython3 tool/check_l10n_strings.py(no visible UI literals outside lib/l10n) all pass.Build/platform paths: no native code, build file or registration changed. The gesture path only runs where
videoPlaybackSurfaceUsesPanGesturesalready enabled touch pan gestures (Android/iOS fullscreen); desktop keeps package-owned behaviour. Widget tests drive the realVideoPlayerViewagainst a fakeVideoPlayerPlatform; no new rendered surface outside the player is introduced.Line counts (
git diff --numstat origin/master...HEAD, headcaf03d6d):Section 1.4 exception checks
Complete every item for an exception. For a compliant/unchanged PR, write
Not applicablehere.Not applicable
--dart-define=I_DONT_FUCKING_CARE_ABOUT_TOS=false, the feature is absent and ordinary Telegram behavior remains intact.--dart-define=I_DONT_FUCKING_CARE_ABOUT_TOS=true, the feature works through the guarded entry points in a self-build; I included relevant test and build/UI evidence.falseand have no option that enables it. All store versions remain compliant with applicable Telegram API Terms.Guard locations and evidence for each flag variant:
Not applicable
Agentic reviewer confirmation
Agentic code reviewer only: record this confirmation in the PR review or check these boxes before approval.