diff --git a/lib/widgets/lightbox.dart b/lib/widgets/lightbox.dart index ca79da71a1..807fc71ad2 100644 --- a/lib/widgets/lightbox.dart +++ b/lib/widgets/lightbox.dart @@ -248,6 +248,31 @@ class _ImageLightboxPageState extends State<_ImageLightboxPage> { } } +class VideoDurationLabel extends StatelessWidget { + const VideoDurationLabel(this.duration, { + super.key, + this.semanticsLabel, + }); + + final Duration duration; + final String? semanticsLabel; + + @visibleForTesting + static String formatDuration(Duration value) { + final hours = value.inHours.toString().padLeft(2, '0'); + final minutes = value.inMinutes.remainder(60).toString().padLeft(2, '0'); + final seconds = value.inSeconds.remainder(60).toString().padLeft(2, '0'); + return '${hours == '00' ? '' : '$hours:'}$minutes:$seconds'; + } + + @override + Widget build(BuildContext context) { + return Text(formatDuration(duration), + semanticsLabel: semanticsLabel, + style: const TextStyle(color: Colors.white)); + } +} + class _VideoPositionSliderControl extends StatefulWidget { final VideoPlayerController controller; @@ -277,27 +302,7 @@ class _VideoPositionSliderControlState extends State<_VideoPositionSliderControl } void _handleVideoControllerUpdate() { - setState(() { - // After 'controller.seekTo' is called in 'Slider.onChangeEnd' the - // position indicator switches back to the actual controller's position - // but since the call 'seekTo' completes before the actual controller - // updates are notified, the position indicator that switches to controller's - // position can show the older position before the call to 'seekTo' for a - // single frame, resulting in a glichty UX. - // - // To avoid that, we delay the position indicator switch from '_sliderValue' to - // happen when we are notified of the controller update. - if (_isSliderDragging && _sliderValue == widget.controller.value.position) { - _isSliderDragging = false; - } - }); - } - - static String _formatDuration(Duration value) { - final hours = value.inHours.toString().padLeft(2, '0'); - final minutes = value.inMinutes.remainder(60).toString().padLeft(2, '0'); - final seconds = value.inSeconds.remainder(60).toString().padLeft(2, '0'); - return '${hours == '00' ? '' : '$hours:'}$minutes:$seconds'; + setState(() {}); } @override @@ -307,8 +312,8 @@ class _VideoPositionSliderControlState extends State<_VideoPositionSliderControl : widget.controller.value.position; return Row(children: [ - Text(_formatDuration(currentPosition), - style: const TextStyle(color: Colors.white)), + VideoDurationLabel(currentPosition, + semanticsLabel: "Current position"), Expanded( child: Slider( value: currentPosition.inMilliseconds.toDouble(), @@ -325,20 +330,20 @@ class _VideoPositionSliderControlState extends State<_VideoPositionSliderControl _sliderValue = Duration(milliseconds: value.toInt()); }); }, - onChangeEnd: (value) { + onChangeEnd: (value) async { final durationValue = Duration(milliseconds: value.toInt()); - setState(() { - _sliderValue = durationValue; - }); - widget.controller.seekTo(durationValue); - - // The toggling back of '_isSliderDragging' is omitted here intentionally, - // see '_handleVideoControllerUpdates'. + await widget.controller.seekTo(durationValue); + if (mounted) { + setState(() { + _sliderValue = durationValue; + _isSliderDragging = false; + }); + } }, ), ), - Text(_formatDuration(widget.controller.value.duration), - style: const TextStyle(color: Colors.white)), + VideoDurationLabel(widget.controller.value.duration, + semanticsLabel: "Video duration"), ]); } } diff --git a/test/widgets/lightbox_test.dart b/test/widgets/lightbox_test.dart index e314c2fd71..acbe5fe9e9 100644 --- a/test/widgets/lightbox_test.dart +++ b/test/widgets/lightbox_test.dart @@ -1,36 +1,92 @@ import 'dart:async'; import 'package:checks/checks.dart'; +import 'package:clock/clock.dart'; +import 'package:flutter/services.dart'; import 'package:flutter_gen/gen_l10n/zulip_localizations.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:flutter/material.dart'; import 'package:plugin_platform_interface/plugin_platform_interface.dart'; import 'package:video_player_platform_interface/video_player_platform_interface.dart'; import 'package:video_player/video_player.dart'; +import 'package:zulip/model/localizations.dart'; import 'package:zulip/widgets/lightbox.dart'; import 'package:zulip/widgets/store.dart'; import '../example_data.dart' as eg; import '../model/binding.dart'; +import 'dialog_checks.dart'; + +const kTestVideoUrl = "https://a/video.mp4"; +const kTestUnsupportedVideoUrl = "https://a/unsupported.mp4"; +const kTestVideoDuration = Duration(seconds: 10); class FakeVideoPlayerPlatform extends Fake with MockPlatformInterfaceMixin implements VideoPlayerPlatform { - static const int _textureId = 0xffffffff; - - static StreamController _streamController = StreamController(); - static bool initialized = false; - static bool isPlaying = false; + static final FakeVideoPlayerPlatform instance = FakeVideoPlayerPlatform(); static void registerWith() { - VideoPlayerPlatform.instance = FakeVideoPlayerPlatform(); + VideoPlayerPlatform.instance = instance; + } + + static const int _kTextureId = 0xffffffff; + + StreamController _streamController = StreamController(); + bool _hasError = false; + Duration _lastSetPosition = Duration.zero; + Stopwatch? _stopwatch; + + List get callLog => _callLog; + final List _callLog = []; + + bool get initialized => _initialized; + bool _initialized = false; + + bool get isCompleted => _isCompleted; + bool _isCompleted = false; + + bool get isPlaying => _stopwatch?.isRunning ?? false; + + Duration get position { + assert(_stopwatch != null); + final pos = _stopwatch!.elapsed + _lastSetPosition; + return pos >= kTestVideoDuration ? kTestVideoDuration : pos; } - static void reset() { + void reset() { _streamController.close(); _streamController = StreamController(); - initialized = false; - isPlaying = false; + _hasError = false; + _lastSetPosition = Duration.zero; + _stopwatch?.stop(); + _stopwatch?.reset(); + _callLog.clear(); + _initialized = false; + _isCompleted = false; + } + + // This helper function explicitly dispatches events that are + // automatically dispatched by the platform video player in + // a real implementation: + // https://github.com/flutter/packages/blob/260102b64c0fac9c66b7574035421fa6c09f5f89/packages/video_player/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayer.java#L189 + void pumpEvents() { + if (position >= kTestVideoDuration) { + if (!_isCompleted) { + _isCompleted = true; + _streamController.add(VideoEvent( + eventType: VideoEventType.completed, + )); + } + + if (isPlaying) { + _stopwatch?.stop(); + _streamController.add(VideoEvent( + eventType: VideoEventType.isPlayingStateUpdate, + isPlaying: false, + )); + } + } } @override @@ -38,40 +94,55 @@ class FakeVideoPlayerPlatform extends Fake @override Future dispose(int textureId) async { + if (_hasError) { + assert(!initialized); + assert(textureId == VideoPlayerController.kUninitializedTextureId); + return; + } + assert(initialized); - assert(textureId == _textureId); - initialized = false; + assert(textureId == _kTextureId); } @override Future create(DataSource dataSource) async { assert(!initialized); - initialized = true; + if (dataSource.uri == kTestUnsupportedVideoUrl) { + _hasError = true; + _streamController.addError( + PlatformException( + code: "VideoError", + message: "Failed to load video: Cannot Open")); + return null; + } + + _stopwatch = clock.stopwatch(); + _initialized = true; _streamController.add(VideoEvent( eventType: VideoEventType.initialized, - duration: const Duration(seconds: 1), - size: const Size(0, 0), + duration: kTestVideoDuration, + size: const Size(100, 100), rotationCorrection: 0, )); - return _textureId; + return _kTextureId; } @override Stream videoEventsFor(int textureId) { - assert(textureId == _textureId); + assert(textureId == _kTextureId); return _streamController.stream; } @override Future setLooping(int textureId, bool looping) async { - assert(textureId == _textureId); + assert(textureId == _kTextureId); assert(!looping); } @override Future play(int textureId) async { - assert(textureId == _textureId); - isPlaying = true; + assert(textureId == _kTextureId); + _stopwatch?.start(); _streamController.add(VideoEvent( eventType: VideoEventType.isPlayingStateUpdate, isPlaying: true, @@ -80,8 +151,8 @@ class FakeVideoPlayerPlatform extends Fake @override Future pause(int textureId) async { - assert(textureId == _textureId); - isPlaying = false; + assert(textureId == _kTextureId); + _stopwatch?.stop(); _streamController.add(VideoEvent( eventType: VideoEventType.isPlayingStateUpdate, isPlaying: false, @@ -90,17 +161,32 @@ class FakeVideoPlayerPlatform extends Fake @override Future setVolume(int textureId, double volume) async { - assert(textureId == _textureId); + assert(textureId == _kTextureId); + } + + @override + Future seekTo(int textureId, Duration pos) async { + _callLog.add('seekTo'); + assert(textureId == _kTextureId); + + _lastSetPosition = pos >= kTestVideoDuration ? kTestVideoDuration : pos; + _stopwatch?.reset(); } @override Future setPlaybackSpeed(int textureId, double speed) async { - assert(textureId == _textureId); + assert(textureId == _kTextureId); + } + + @override + Future getPosition(int textureId) async { + assert(textureId == _kTextureId); + return position; } @override Widget buildView(int textureId) { - assert(textureId == _textureId); + assert(textureId == _kTextureId); return const SizedBox(width: 100, height: 100); } } @@ -108,15 +194,81 @@ class FakeVideoPlayerPlatform extends Fake void main() { TestZulipBinding.ensureInitialized(); + group('VideoDurationLabel', () { + const cases = [ + (Duration(milliseconds: 1), '00:00', '1ms'), + (Duration(milliseconds: 900), '00:00', '900ms'), + (Duration(milliseconds: 1000), '00:01', '1000ms'), + (Duration(seconds: 59), '00:59', '59s'), + (Duration(seconds: 60), '01:00', '60s'), + (Duration(minutes: 59), '59:00', '59m'), + (Duration(minutes: 60), '01:00:00', '60m'), + (Duration(hours: 23), '23:00:00', '23h'), + (Duration(hours: 24), '24:00:00', '24h'), + (Duration(hours: 25), '25:00:00', '25h'), + (Duration(hours: 100), '100:00:00', '100h'), + ]; + + for (final (duration, expected, title) in cases) { + testWidgets('with $title shows $expected', (tester) async { + await tester.pumpWidget(MaterialApp(home: VideoDurationLabel(duration))); + final text = tester.widget(find.byType(Text)); + check(text.data) + ..equals(VideoDurationLabel.formatDuration(duration)) + ..equals(expected); + }); + } + }); + group("VideoLightboxPage", () { FakeVideoPlayerPlatform.registerWith(); + final platform = FakeVideoPlayerPlatform.instance; + + /// Find the position shown by the slider, and check the label agrees. + Duration findSliderPosition(WidgetTester tester) { + final sliderValue = tester.widget(find.byType(Slider)).value; + final result = Duration(milliseconds: sliderValue.toInt()); + check(tester.widget( + find.descendant(of: find.bySemanticsLabel('Current position'), + matching: find.byType(RichText))).text.toPlainText()) + .equals(VideoDurationLabel.formatDuration(result)); + return result; + } + + /// Check the slider and label show position [slider], + /// and the actual position of the video controller is [video]. + void checkPositions(WidgetTester tester, { + required Duration slider, + required Duration video, + }) { + check(findSliderPosition(tester)).equals(slider); + check(platform.position).equals(video); + } + + /// Like [checkPositions], but expressed in units of [kTestVideoDuration]. + void checkPositionsRelative(WidgetTester tester, { + required double slider, + required double video, + }) { + checkPositions(tester, + slider: kTestVideoDuration * slider, + video: kTestVideoDuration * video); + } + + (Offset, Offset) calculateSliderDimensions(WidgetTester tester) { + const padding = 24.0; + final rect = tester.getRect(find.byType(Slider)); + final trackStartPos = rect.centerLeft + const Offset(padding, 0); + final trackLength = Offset(rect.width - padding - padding, 0); + return (trackStartPos, trackLength); + } Future setupPage(WidgetTester tester, { required Uri videoSrc, }) async { addTearDown(testBinding.reset); await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); - addTearDown(FakeVideoPlayerPlatform.reset); + addTearDown(platform.reset); await tester.pumpWidget(GlobalStoreWidget(child: MaterialApp( localizationsDelegates: ZulipLocalizations.localizationsDelegates, @@ -127,30 +279,149 @@ void main() { routeEntranceAnimation: kAlwaysCompleteAnimation, message: eg.streamMessage(), src: videoSrc))))); - await tester.pumpAndSettle(); + await tester.pump(); // global store + await tester.pump(); // per-account store + await tester.pump(); // video controller initialization } testWidgets('shows a VideoPlayer, and video is playing', (tester) async { - await setupPage(tester, videoSrc: Uri.parse('https://a/b.mp4')); + await setupPage(tester, videoSrc: Uri.parse(kTestVideoUrl)); - check(FakeVideoPlayerPlatform.initialized).isTrue(); - check(FakeVideoPlayerPlatform.isPlaying).isTrue(); + check(platform.initialized).isTrue(); + check(platform.isPlaying).isTrue(); await tester.ensureVisible(find.byType(VideoPlayer)); }); testWidgets('toggles between play and pause', (tester) async { - await setupPage(tester, videoSrc: Uri.parse('https://a/b.mp4')); - check(FakeVideoPlayerPlatform.isPlaying).isTrue(); + await setupPage(tester, videoSrc: Uri.parse(kTestVideoUrl)); + check(platform.isPlaying).isTrue(); await tester.tap(find.byIcon(Icons.pause_circle_rounded)); - check(FakeVideoPlayerPlatform.isPlaying).isFalse(); + check(platform.isPlaying).isFalse(); // re-render to update player controls await tester.pump(); await tester.tap(find.byIcon(Icons.play_circle_rounded)); - check(FakeVideoPlayerPlatform.isPlaying).isTrue(); + check(platform.isPlaying).isTrue(); + }); + + testWidgets('unsupported video shows an error dialog', (tester) async { + await setupPage(tester, videoSrc: Uri.parse(kTestUnsupportedVideoUrl)); + final zulipLocalizations = GlobalLocalizations.zulipLocalizations; + await tester.tap(find.byWidget(checkErrorDialog(tester, + expectedTitle: zulipLocalizations.errorDialogTitle, + expectedMessage: zulipLocalizations.errorVideoPlayerFailed))); + }); + + testWidgets('video advances over time and stops playing when it ends', (tester) async { + await setupPage(tester, videoSrc: Uri.parse(kTestVideoUrl)); + check(platform.isPlaying).isTrue(); + + await tester.pump(kTestVideoDuration * 0.5); + platform.pumpEvents(); + checkPositionsRelative(tester, slider: 0.5, video: 0.5); + check(platform.isCompleted).isFalse(); + check(platform.isPlaying).isTrue(); + + // Near the end of the video. + await tester.pump((kTestVideoDuration * 0.5) - const Duration(milliseconds: 500)); + platform.pumpEvents(); + checkPositions(tester, + slider: kTestVideoDuration - const Duration(milliseconds: 500), + video: kTestVideoDuration - const Duration(milliseconds: 500)); + check(platform.isCompleted).isFalse(); + check(platform.isPlaying).isTrue(); + + // At exactly the end of the video. + await tester.pump(const Duration(milliseconds: 500)); + platform.pumpEvents(); + checkPositionsRelative(tester, slider: 1.0, video: 1.0); + check(platform.isCompleted).isTrue(); // completed + check(platform.isPlaying).isFalse(); // stopped playing + + // After the video ended. + await tester.pump(const Duration(milliseconds: 500)); + platform.pumpEvents(); + checkPositionsRelative(tester, slider: 1.0, video: 1.0); + check(platform.isCompleted).isTrue(); + check(platform.isPlaying).isFalse(); + }); + + testWidgets('ensure \'seekTo\' is called only once', (tester) async { + await setupPage(tester, videoSrc: Uri.parse(kTestVideoUrl)); + check(platform.isPlaying).isTrue(); + + final (trackStartPos, trackLength) = calculateSliderDimensions(tester); + + // Verify the actually displayed current position at each + // gesture increments. + final gesture = await tester.startGesture(trackStartPos); + await tester.pump(); + checkPositionsRelative(tester, slider: 0.0, video: 0.0); + + await gesture.moveBy(trackLength * 0.2); + await tester.pump(); + checkPositionsRelative(tester, slider: 0.2, video: 0.0); + + await gesture.moveBy(trackLength * 0.4); + await tester.pump(); + checkPositionsRelative(tester, slider: 0.6, video: 0.0); + + await gesture.moveBy(trackLength * -0.2); + await tester.pump(); + checkPositionsRelative(tester, slider: 0.4, video: 0.0); + + await gesture.up(); + await tester.pump(); + checkPositionsRelative(tester, slider: 0.4, video: 0.4); + + // Verify seekTo is called only once. + check(platform.callLog.where((v) => v == 'seekTo').length).equals(1); + }); + + testWidgets('ensure slider doesn\'t flicker right after it is moved', (tester) async { + // Regression test for a potential bug that we successfully avoided + // but is described in the comment quoted here: + // https://github.com/zulip/zulip-flutter/pull/587#discussion_r1596190776 + await setupPage(tester, videoSrc: Uri.parse(kTestVideoUrl)); + check(platform.isPlaying).isTrue(); + + final (trackStartPos, trackLength) = calculateSliderDimensions(tester); + + final gesture = await tester.startGesture(trackStartPos); + await tester.pump(); + checkPositionsRelative(tester, slider: 0.0, video: 0.0); + + await gesture.moveBy(trackLength * 0.5); + await tester.pump(); + checkPositionsRelative(tester, slider: 0.5, video: 0.0); + + await gesture.up(); + await tester.pump(); + checkPositionsRelative(tester, slider: 0.5, video: 0.5); + + // The video_player plugin only reports a new position every 500ms, alas: + // https://github.com/zulip/zulip-flutter/pull/694#discussion_r1635506000 + const videoPlayerPollIntervalMs = 500; + const frameTimeMs = 10; // 100fps + const maxIterations = 1 + videoPlayerPollIntervalMs ~/ frameTimeMs; + + // The slider may stay in place for several frames. + // But find when it first moves again… + int iterations = 0; + Duration position = findSliderPosition(tester); + final basePosition = position; + while (true) { + if (++iterations > maxIterations) break; + await tester.pump(const Duration(milliseconds: frameTimeMs)); + position = findSliderPosition(tester); + if (position != basePosition) break; + } + // … and check the movement is forward, and corresponds to the video. + check(position).isGreaterThan(basePosition); + check(platform.position).equals(position); }); }); }