From f02924b9a55aaa98cdb7ad33096a98eba83323d6 Mon Sep 17 00:00:00 2001 From: edde746 <86283021+edde746@users.noreply.github.com> Date: Wed, 29 Jul 2026 04:36:18 +0200 Subject: [PATCH] fix(player): require a fresh double tap for every mobile skip-zone seek The skip badge doubled as an armed state: while it was up, any single tap in the same-direction zone seeked again. The badge is also raised by keyboard, D-pad, media-transport and live seeks, so one remote press armed one-tap seeking on the touch surface with no double tap at all. It stayed armed for 1200 ms plus the fade and renewed on every tap, leaving the side zones - 35% of the width each, over 70% of the height - unable to raise the chrome. Pair taps off the pending single-tap timer rather than differencing DateTime.now(). The window is then one deadline that a clock adjustment cannot stretch, suppressing touch taps disarms a half-finished pair, and _lastSkipTapTime belongs to the desktop double-click paths alone. Consecutive completed skips still accumulate into one running badge total. --- .../video_controls/parts/playback_input.dart | 54 ++- .../video_controls/video_controls.dart | 8 +- ...video_controls_mobile_skip_zones_test.dart | 393 ++++++++++++++++++ 3 files changed, 424 insertions(+), 31 deletions(-) create mode 100644 test/widgets/video_controls_mobile_skip_zones_test.dart diff --git a/lib/widgets/video_controls/parts/playback_input.dart b/lib/widgets/video_controls/parts/playback_input.dart index 41ae8a3a..b419757c 100644 --- a/lib/widgets/video_controls/parts/playback_input.dart +++ b/lib/widgets/video_controls/parts/playback_input.dart @@ -559,45 +559,41 @@ extension _PlexVideoControlsPlaybackInputMethods on _PlexVideoControlsState { _lastSkipTapTime = now; } - /// Handle tap in skip zone with custom double-tap detection + /// Handle a tap in a skip zone. Every skip costs a fresh same-direction + /// double tap; a lone tap toggles the chrome. + /// + /// The badge left over from the previous skip is a readout, not an armed + /// state — it says nothing about what the next tap does. + /// + /// The pending single-tap timer *is* the pairing window: while it is live the + /// tap that started it is still unresolved, so a same-direction tap pairs with + /// it. One deadline instead of a timer plus a `DateTime.now()` difference that + /// a clock adjustment could stretch or collapse — and it leaves + /// [_lastSkipTapTime] to the desktop double-click paths alone. Suppressing + /// touch taps cancels the timer, which correctly disarms a half-finished pair. void _handleTapInSkipZone({required bool isForward}) { if (_isTouchTapSuppressed) return; - // Cancel any pending single-tap action + final pairsWithPendingTap = (_singleTapTimer?.isActive ?? false) && _lastSkipTapWasForward == isForward; + + // Either way the pending tap is resolved now: paired below, or replaced by + // this one as the start of a new pair. _singleTapTimer?.cancel(); _singleTapTimer = null; - // While the skip readout is visible, every tap in the same-direction zone - // stacks another skip immediately — repeat skips cost one tap, not a - // fresh double-tap. A tap in the opposite zone falls through to pairing. - if (_showDoubleTapFeedback && _lastDoubleTapWasForward == isForward) { + if (pairsWithPendingTap) { _handleDoubleTapSkip(isForward: isForward); return; } - final now = DateTime.now(); - final isDoubleTap = - _lastSkipTapTime != null && - now.difference(_lastSkipTapTime!) < kDoubleTapTimeout && - _lastSkipTapWasForward == isForward; + _lastSkipTapWasForward = isForward; - // Skip ONLY on detected double-tap (no single-tap-to-add behavior) - if (isDoubleTap) { - _lastSkipTapTime = null; - _handleDoubleTapSkip(isForward: isForward); - } else { - // First tap - record timestamp and start timer for single-tap action - _lastSkipTapTime = now; - _lastSkipTapWasForward = isForward; - - // If no second tap within the double-tap window, treat as single tap - // to toggle controls - _singleTapTimer = Timer(kDoubleTapTimeout, () { - if (mounted) { - _toggleControls(); - } - }); - } + // No partner within the window, and this resolves as a lone tap. + _singleTapTimer = Timer(kDoubleTapTimeout, () { + if (mounted) { + _toggleControls(); + } + }); } Size _sizeOf(BuildContext context) { @@ -613,7 +609,7 @@ extension _PlexVideoControlsPlaybackInputMethods on _PlexVideoControlsState { _showSkipFeedback(isForward: isForward); } - /// Handle a skip-zone double tap (and every stacked tap that follows it). + /// Handle a completed skip-zone double tap. void _handleDoubleTapSkip({required bool isForward}) { if (!widget.canControl) return; diff --git a/lib/widgets/video_controls/video_controls.dart b/lib/widgets/video_controls/video_controls.dart index ed1dbb9a..d89c37d2 100644 --- a/lib/widgets/video_controls/video_controls.dart +++ b/lib/widgets/video_controls/video_controls.dart @@ -659,11 +659,15 @@ class _PlexVideoControlsState extends State bool _lastDoubleTapWasForward = true; Timer? _feedbackTimer; int _accumulatedSkipSeconds = 0; // Stacking skip: total skip during active feedback - // Custom tap detection state (more reliable than Flutter's onDoubleTap) + // Desktop double-click detection (more reliable than Flutter's onDoubleTap). + // The mobile skip zones do not use this; they pair off _singleTapTimer. DateTime? _lastSkipTapTime; + // Direction of the skip-zone tap _singleTapTimer is currently counting down. bool _lastSkipTapWasForward = true; Timer? _feedbackHideTimer; // Removes the skip readout after its fade-out completes - Timer? _singleTapTimer; // Timer for delayed single-tap action (toggle controls) + // Deferred lone-tap action for the skip zones, and the pairing window itself: + // while it is active the tap that started it can still become a double tap. + Timer? _singleTapTimer; final TwoFingerDoubleTapTracker _twoFingerDoubleTapTracker = TwoFingerDoubleTapTracker(); final MobileEdgeAdjustmentTracker _edgeAdjustmentTracker = MobileEdgeAdjustmentTracker(); final DeviceAdjustmentService _deviceAdjustmentService = DeviceAdjustmentService.instance; diff --git a/test/widgets/video_controls_mobile_skip_zones_test.dart b/test/widgets/video_controls_mobile_skip_zones_test.dart new file mode 100644 index 00000000..ac9ab822 --- /dev/null +++ b/test/widgets/video_controls_mobile_skip_zones_test.dart @@ -0,0 +1,393 @@ +import 'package:drift/native.dart'; +import 'package:flutter/gestures.dart' show kDoubleTapTimeout; +import 'package:flutter/material.dart'; +import 'package:flutter/services.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:intl/date_symbol_data_local.dart'; +import 'package:provider/provider.dart'; + +import 'package:plezy/database/app_database.dart'; +import 'package:plezy/i18n/strings.g.dart'; +import 'package:plezy/mpv/mpv.dart'; +import 'package:plezy/providers/playback_state_provider.dart'; +import 'package:plezy/services/settings_service.dart'; +import 'package:plezy/services/video_volume_controller.dart'; +import 'package:plezy/utils/platform_detector.dart'; +import 'package:plezy/watch_together/providers/watch_together_provider.dart'; +import 'package:plezy/widgets/video_controls/player_chrome_controller.dart'; +import 'package:plezy/widgets/video_controls/video_controls.dart'; +import 'package:plezy/widgets/video_controls/widgets/double_tap_feedback.dart'; +import 'package:plezy/widgets/video_controls/widgets/player_toast_indicator.dart'; + +import '../test_helpers/media_items.dart'; +import '../test_helpers/prefs.dart'; +import '../test_helpers/theme.dart'; + +/// A skip in the mobile skip zones costs one full same-direction double tap. +/// +/// An earlier revision let the leftover skip badge stand in for an armed state, +/// so every later lone tap seeked: the side zones — nearly half the picture — +/// could not raise the chrome for as long as the badge stayed up, and a badge +/// raised by a keyboard or remote seek armed one-tap seeking on the touch +/// surface with no double tap at all. +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + late _RecordingPlayer player; + late PlayerChromeController chrome; + late PlayerToastController toast; + late VideoVolumeController volume; + late PlaybackStateProvider playbackState; + late WatchTogetherProvider watchTogether; + late AppDatabase database; + + setUp(() async { + LocaleSettings.setLocaleSync(AppLocale.en); + await initializeDateFormatting('en'); + resetSharedPreferencesForTest(); + SettingsService.resetForTesting(); + final settings = await SettingsService.getInstance(); + await settings.write(SettingsService.seekTimeSmall, 10); + + // Phone layout: the skip zones only exist when PlatformDetector.isMobile. + TvDetectionService.debugSetAppleTVOverride(false); + PlatformDetector.debugSetIsDesktopOSOverride(false); + + database = AppDatabase.forTesting(NativeDatabase.memory()); + player = _RecordingPlayer(); + chrome = PlayerChromeController(); + toast = PlayerToastController(); + volume = VideoVolumeController(player: player, settings: settings, initialVolume: 100); + playbackState = PlaybackStateProvider(); + watchTogether = WatchTogetherProvider(); + }); + + tearDown(() async { + TvDetectionService.debugSetAppleTVOverride(null); + PlatformDetector.debugSetIsDesktopOSOverride(null); + volume.dispose(); + playbackState.dispose(); + watchTogether.dispose(); + chrome.dispose(); + toast.dispose(); + await database.close(); + }); + + // Derived from the laid-out player rather than hard-coded, so the cases + // survive a change of test surface. mobileSkipZoneDimensions: each side zone + // is 35% of the width, excluding the top and bottom 15% of the height. + const surface = Size(800, 600); + + Offset forwardZoneOf(WidgetTester tester) { + final rect = tester.getRect(find.byType(PlexVideoControls)); + return Offset(rect.right - rect.width * 0.1, rect.center.dy); + } + + Offset backwardZoneOf(WidgetTester tester) { + final rect = tester.getRect(find.byType(PlexVideoControls)); + return Offset(rect.left + rect.width * 0.1, rect.center.dy); + } + + Offset neutralZoneOf(WidgetTester tester) => tester.getRect(find.byType(PlexVideoControls)).center; + + Future pumpControls(WidgetTester tester) async { + await tester.pumpWidget( + MultiProvider( + providers: [ + Provider.value(value: database), + ChangeNotifierProvider.value(value: playbackState), + ChangeNotifierProvider.value(value: watchTogether), + ], + child: MaterialApp( + theme: ThemeData(platform: TargetPlatform.android, extensions: const [testMonoTokens]), + home: Scaffold( + body: SizedBox( + width: surface.width, + height: surface.height, + child: PlexVideoControls( + player: player, + volumeController: volume, + metadata: testMediaItem(id: 'mobile-skip-zones'), + toastController: toast, + chromeController: chrome, + canNavigateMediaItems: false, + ), + ), + ), + ), + ), + ); + await tester.pump(); + // Every case starts from hidden chrome — the state the skip zones own. + chrome.hide(); + chrome.markControlsHidden(); + await tester.pump(); + expect(chrome.controlsVisible, isFalse); + } + + /// Two taps inside [kDoubleTapTimeout], which pair into one skip. + /// + /// Pairing runs off `_singleTapTimer`, a fake-clock timer, so the pumped + /// durations here are the real contract and not decoration — see the + /// pairing-window cases below. + Future doubleTap(WidgetTester tester, Offset zone) async { + await tester.tapAt(zone); + await tester.pump(const Duration(milliseconds: 50)); + await tester.tapAt(zone); + await tester.pump(); + } + + /// One tap, then past [kDoubleTapTimeout] so the deferred lone-tap action + /// fires. + Future loneTap(WidgetTester tester, Offset zone) async { + await tester.tapAt(zone); + await tester.pump(const Duration(milliseconds: 400)); + } + + Future settleFeedback(WidgetTester tester) async { + chrome.cancelAutoHide(); + toast.hide(); + await tester.pumpWidget(const SizedBox.shrink()); + } + + testWidgets('a double tap in the forward zone skips once', (tester) async { + await pumpControls(tester); + + await doubleTap(tester, forwardZoneOf(tester)); + + expect(player.seeks, [const Duration(minutes: 10, seconds: 10)]); + expect(find.text('10s'), findsOneWidget); + expect(chrome.controlsVisible, isFalse, reason: 'skipping must not raise the chrome'); + + await settleFeedback(tester); + }); + + testWidgets('a double tap in the backward zone rewinds once', (tester) async { + await pumpControls(tester); + + await doubleTap(tester, backwardZoneOf(tester)); + + expect(player.seeks, [const Duration(minutes: 9, seconds: 50)]); + expect(find.text('10s'), findsOneWidget); + + await settleFeedback(tester); + }); + + testWidgets('a lone tap after a skip toggles the chrome instead of skipping again', (tester) async { + await pumpControls(tester); + + await doubleTap(tester, forwardZoneOf(tester)); + expect(player.seeks.length, 1); + // The badge is still up. It is a readout, not an armed state. + expect(find.byType(DoubleTapFeedback), findsOneWidget); + + await loneTap(tester, forwardZoneOf(tester)); + + expect(player.seeks.length, 1, reason: 'a single tap must never seek'); + expect(chrome.controlsVisible, isTrue, reason: 'a single tap in a skip zone toggles the chrome'); + + await settleFeedback(tester); + }); + + testWidgets('a second tap just inside the pairing window skips', (tester) async { + await pumpControls(tester); + + await tester.tapAt(forwardZoneOf(tester)); + await tester.pump(kDoubleTapTimeout - const Duration(milliseconds: 1)); + await tester.tapAt(forwardZoneOf(tester)); + await tester.pump(); + + expect(player.seeks, [const Duration(minutes: 10, seconds: 10)]); + expect(chrome.controlsVisible, isFalse); + + await settleFeedback(tester); + }); + + testWidgets('a second tap just past the pairing window does not skip', (tester) async { + await pumpControls(tester); + + await tester.tapAt(forwardZoneOf(tester)); + await tester.pump(kDoubleTapTimeout + const Duration(milliseconds: 1)); + // The window closed, so the first tap already resolved as a lone tap. + expect(chrome.controlsVisible, isTrue); + + await tester.tapAt(forwardZoneOf(tester)); + await tester.pump(); + + expect(player.seeks, isEmpty, reason: 'two taps a window apart are two lone taps, not a skip'); + + await settleFeedback(tester); + }); + + testWidgets('an uninterrupted tap stream pairs into one skip per two taps', (tester) async { + await pumpControls(tester); + + // Six taps, nothing between them. Pairs must not overlap: taps 1+2, 3+4 and + // 5+6 each buy one skip, and no tap is left over to toggle the chrome. + final skipped = []; + for (var i = 0; i < 6; i++) { + final before = player.seeks.length; + await tester.tapAt(forwardZoneOf(tester)); + await tester.pump(); + skipped.add(player.seeks.length > before); + } + + expect(skipped, [false, true, false, true, false, true]); + expect(player.seeks, [ + const Duration(minutes: 10, seconds: 10), + const Duration(minutes: 10, seconds: 20), + const Duration(minutes: 10, seconds: 30), + ]); + expect(find.text('30s'), findsOneWidget, reason: 'consecutive skips accumulate into one readout'); + + // The last tap completed a pair, so nothing is pending to raise the chrome. + await tester.pump(const Duration(milliseconds: 400)); + expect(chrome.controlsVisible, isFalse); + + await settleFeedback(tester); + }); + + testWidgets('an odd tap left over by a tap stream toggles the chrome', (tester) async { + await pumpControls(tester); + + for (var i = 0; i < 5; i++) { + await tester.tapAt(forwardZoneOf(tester)); + await tester.pump(); + } + + expect(player.seeks.length, 2, reason: 'five taps buy two skips'); + expect(chrome.controlsVisible, isFalse, reason: 'the fifth tap is still waiting for a partner'); + + await tester.pump(const Duration(milliseconds: 400)); + expect(chrome.controlsVisible, isTrue, reason: 'the unpaired tap resolves as a lone tap'); + expect(player.seeks.length, 2); + + await settleFeedback(tester); + }); + + testWidgets('a keyboard seek does not arm one-tap seeking on the touch surface', (tester) async { + await pumpControls(tester); + + await tester.sendKeyDownEvent(LogicalKeyboardKey.arrowRight); + await tester.pump(); + await tester.sendKeyUpEvent(LogicalKeyboardKey.arrowRight); + await tester.pump(); + expect(player.seeks.length, 1); + expect(find.byType(DoubleTapFeedback), findsOneWidget); + + await loneTap(tester, forwardZoneOf(tester)); + + expect(player.seeks.length, 1, reason: 'the badge a keyboard seek raised must not make a lone tap seek'); + + await settleFeedback(tester); + }); + + testWidgets('a media-key seek does not arm one-tap seeking either', (tester) async { + await pumpControls(tester); + + await tester.sendKeyDownEvent(LogicalKeyboardKey.mediaFastForward); + await tester.pump(); + await tester.sendKeyUpEvent(LogicalKeyboardKey.mediaFastForward); + await tester.pump(); + expect(find.byType(DoubleTapFeedback), findsOneWidget, reason: 'the media key raised the readout'); + final seeksBefore = player.seeks.length; + + await loneTap(tester, forwardZoneOf(tester)); + + expect(player.seeks.length, seeksBefore, reason: 'a lone tap is not a skip'); + + await settleFeedback(tester); + }); + + testWidgets('a lone tap in the opposite zone does not skip', (tester) async { + await pumpControls(tester); + + await doubleTap(tester, forwardZoneOf(tester)); + expect(player.seeks.length, 1); + + await loneTap(tester, backwardZoneOf(tester)); + + expect(player.seeks.length, 1); + + await settleFeedback(tester); + }); + + testWidgets('taps split across the two zones never pair into a skip', (tester) async { + await pumpControls(tester); + + await tester.tapAt(forwardZoneOf(tester)); + await tester.pump(const Duration(milliseconds: 50)); + await tester.tapAt(backwardZoneOf(tester)); + await tester.pump(const Duration(milliseconds: 400)); + + expect(player.seeks, isEmpty, reason: 'both halves of a double tap must land in one direction'); + + await settleFeedback(tester); + }); + + testWidgets('a lone tap outside the skip zones toggles the chrome', (tester) async { + await pumpControls(tester); + + await loneTap(tester, neutralZoneOf(tester)); + + expect(player.seeks, isEmpty); + expect(chrome.controlsVisible, isTrue); + + await settleFeedback(tester); + }); +} + +/// Minimal [Player] recording seek targets against a fixed 45-minute item. +class _RecordingPlayer implements Player { + final List seeks = []; + + bool _playing = true; + Duration _position = const Duration(minutes: 10); + + @override + String get playerType => 'mpv'; + + @override + PlayerState get state => + PlayerState(playing: _playing, position: _position, duration: const Duration(minutes: 45), seekable: true); + + @override + PlayerStreams get streams => PlayerStreams( + playing: const Stream.empty(), + completed: const Stream.empty(), + buffering: const Stream.empty(), + position: const Stream.empty(), + duration: const Stream.empty(), + seekable: const Stream.empty(), + buffer: const Stream.empty(), + volume: const Stream.empty(), + rate: const Stream.empty(), + tracks: const Stream.empty(), + track: const Stream.empty(), + log: const Stream.empty(), + error: const Stream.empty(), + audioDevice: const Stream.empty(), + audioDevices: const Stream>.empty(), + bufferRanges: const Stream>.empty(), + playbackRestart: const Stream.empty(), + backendSwitched: const Stream.empty(), + ); + + @override + Future seek(Duration position) async { + seeks.add(position); + _position = position; + } + + @override + Future play() async => _playing = true; + + @override + Future pause() async => _playing = false; + + @override + Future playOrPause() async => _playing = !_playing; + + @override + dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); +}