Update VideoPlayer widget when the attached controller changes (flutter#769)

* Update textureId in VideoPlayer widget when the attached controller changes.

This fixes a bug introduced in flutter#767: if a State is attached to a new widget it never switched to the new controller.

Also this change makes sure to remove listeners during deinitialization.

* Add tests for conditions identified in flutter/flutter#21483.

This is a breaking change since it reduces the public interface of VideoPlayerController to make it easier to fake it in the test (and hide unnecessary implementation details).

Author: nichtverstehen

packages/video_player/CHANGELOG.md

@@ -1,3 +1,12 @@
+## 0.7.0
+
+* Add a unit test for controller and texture changes. This is a breaking change since the interface
+  had to be cleaned up to facilitate faking.
+
+## 0.6.6
+
+* Fix the condition where the player doesn't update when attached controller is changed.
+
 ## 0.6.5
 
 * Eliminate race conditions around initialization: now initialization events are queued and guaranteed

packages/video_player/lib/video_player.dart

@@ -148,9 +148,9 @@ class VideoPlayerController extends ValueNotifier<VideoPlayerValue> {
   /// is constructed with.
   final DataSourceType dataSourceType;
 
-  String package;
-  Timer timer;
-  bool isDisposed = false;
+  final String package;
+  Timer _timer;
+  bool _isDisposed = false;
   Completer<void> _creatingCompleter;
   StreamSubscription<dynamic> _eventSubscription;
   _VideoAppLifeCycleObserver _lifeCycleObserver;
@@ -171,6 +171,7 @@ class VideoPlayerController extends ValueNotifier<VideoPlayerValue> {
   /// null.
   VideoPlayerController.network(this.dataSource)
       : dataSourceType = DataSourceType.network,
+        package = null,
         super(new VideoPlayerValue(duration: null));
 
   /// Constructs a [VideoPlayerController] playing a video from a file.
@@ -180,8 +181,12 @@ class VideoPlayerController extends ValueNotifier<VideoPlayerValue> {
   VideoPlayerController.file(File file)
       : dataSource = 'file://${file.path}',
         dataSourceType = DataSourceType.file,
+        package = null,
         super(new VideoPlayerValue(duration: null));
 
+  // Visible for testing.
+  int get textureId => _textureId;
+
   Future<void> initialize() async {
     _lifeCycleObserver = new _VideoAppLifeCycleObserver(this);
     _lifeCycleObserver.initialize();
@@ -231,7 +236,7 @@ class VideoPlayerController extends ValueNotifier<VideoPlayerValue> {
           break;
         case 'completed':
           value = value.copyWith(isPlaying: false);
-          timer?.cancel();
+          _timer?.cancel();
           break;
         case 'bufferingUpdate':
           final List<dynamic> values = map['values'];
@@ -251,7 +256,7 @@ class VideoPlayerController extends ValueNotifier<VideoPlayerValue> {
     void errorListener(Object obj) {
       final PlatformException e = obj;
       value = new VideoPlayerValue.erroneous(e.message);
-      timer?.cancel();
+      _timer?.cancel();
     }
 
     _eventSubscription = _eventChannelFor(_textureId)
@@ -268,9 +273,9 @@ class VideoPlayerController extends ValueNotifier<VideoPlayerValue> {
   Future<void> dispose() async {
     if (_creatingCompleter != null) {
       await _creatingCompleter.future;
-      if (!isDisposed) {
-        isDisposed = true;
-        timer?.cancel();
+      if (!_isDisposed) {
+        _isDisposed = true;
+        _timer?.cancel();
         await _eventSubscription?.cancel();
         await _channel.invokeMethod(
           'dispose',
@@ -279,7 +284,7 @@ class VideoPlayerController extends ValueNotifier<VideoPlayerValue> {
       }
       _lifeCycleObserver.dispose();
     }
-    isDisposed = true;
+    _isDisposed = true;
     super.dispose();
   }
 
@@ -299,7 +304,7 @@ class VideoPlayerController extends ValueNotifier<VideoPlayerValue> {
   }
 
   Future<void> _applyLooping() async {
-    if (!value.initialized || isDisposed) {
+    if (!value.initialized || _isDisposed) {
       return;
     }
     _channel.invokeMethod(
@@ -309,29 +314,29 @@ class VideoPlayerController extends ValueNotifier<VideoPlayerValue> {
   }
 
   Future<void> _applyPlayPause() async {
-    if (!value.initialized || isDisposed) {
+    if (!value.initialized || _isDisposed) {
       return;
     }
     if (value.isPlaying) {
       await _channel.invokeMethod(
         'play',
         <String, dynamic>{'textureId': _textureId},
       );
-      timer = new Timer.periodic(
+      _timer = new Timer.periodic(
         const Duration(milliseconds: 500),
         (Timer timer) async {
-          if (isDisposed) {
+          if (_isDisposed) {
             return;
           }
           final Duration newPosition = await position;
-          if (isDisposed) {
+          if (_isDisposed) {
             return;
           }
           value = value.copyWith(position: newPosition);
         },
       );
     } else {
-      timer?.cancel();
+      _timer?.cancel();
       await _channel.invokeMethod(
         'pause',
         <String, dynamic>{'textureId': _textureId},
@@ -340,7 +345,7 @@ class VideoPlayerController extends ValueNotifier<VideoPlayerValue> {
   }
 
   Future<void> _applyVolume() async {
-    if (!value.initialized || isDisposed) {
+    if (!value.initialized || _isDisposed) {
       return;
     }
     await _channel.invokeMethod(
@@ -351,7 +356,7 @@ class VideoPlayerController extends ValueNotifier<VideoPlayerValue> {
 
   /// The position in the current video.
   Future<Duration> get position async {
-    if (isDisposed) {
+    if (_isDisposed) {
       return null;
     }
     return new Duration(
@@ -363,7 +368,7 @@ class VideoPlayerController extends ValueNotifier<VideoPlayerValue> {
   }
 
   Future<void> seekTo(Duration moment) async {
-    if (isDisposed) {
+    if (_isDisposed) {
       return;
     }
     if (moment > value.duration) {
@@ -430,29 +435,48 @@ class VideoPlayer extends StatefulWidget {
 }
 
 class _VideoPlayerState extends State<VideoPlayer> {
-  int textureId;
+  VoidCallback _listener;
+  int _textureId;
+
+  _VideoPlayerState() {
+    _listener = () {
+      final int newTextureId = widget.controller.textureId;
+      if (newTextureId != _textureId) {
+        setState(() {
+          _textureId = newTextureId;
+        });
+      }
+    };
+  }
 
   @override
   void initState() {
     super.initState();
-    textureId = widget.controller._textureId;
+    _textureId = widget.controller.textureId;
     // Need to listen for initialization events since the actual texture ID
     // becomes available after asynchronous initialization finishes.
-    widget.controller.addListener(() {
-      final int newTextureId = widget.controller._textureId;
-      if (newTextureId != textureId) {
-        setState(() {
-          textureId = newTextureId;
-        });
-      }
-    });
+    widget.controller.addListener(_listener);
+  }
+
+  @override
+  void didUpdateWidget(VideoPlayer oldWidget) {
+    super.didUpdateWidget(oldWidget);
+    oldWidget.controller.removeListener(_listener);
+    _textureId = widget.controller.textureId;
+    widget.controller.addListener(_listener);
+  }
+
+  @override
+  void deactivate() {
+    super.deactivate();
+    widget.controller.removeListener(_listener);
   }
 
   @override
   Widget build(BuildContext context) {
-    return textureId == null
+    return _textureId == null
         ? new Container()
-        : new Texture(textureId: textureId);
+        : new Texture(textureId: _textureId);
   }
 }
 

packages/video_player/pubspec.yaml

@@ -2,7 +2,7 @@ name: video_player
 description: Flutter plugin for displaying inline video with other Flutter
   widgets on Android and iOS.
 author: Flutter Team <flutter-dev@googlegroups.com>
-version: 0.6.5
+version: 0.7.0
 homepage: https://github.com/flutter/plugins/tree/master/packages/video_player
 
 flutter:
@@ -16,6 +16,10 @@ dependencies:
   flutter:
     sdk: flutter
 
+dev_dependencies:
+  flutter_test:
+    sdk: flutter
+
 environment:
   sdk: ">=2.0.0-dev.28.0 <3.0.0"
   flutter: ">=0.2.5 <2.0.0"

packages/video_player/test/video_player_test.dart

@@ -0,0 +1,76 @@
+import 'dart:async';
+import 'package:flutter/foundation.dart';
+import 'package:flutter/widgets.dart';
+import 'package:video_player/video_player.dart';
+import 'package:flutter_test/flutter_test.dart';
+
+class FakeController extends ValueNotifier<VideoPlayerValue>
+    implements VideoPlayerController {
+  FakeController() : super(new VideoPlayerValue(duration: null));
+
+  @override
+  Future<void> dispose() async {
+    super.dispose();
+  }
+
+  @override
+  int textureId;
+
+  @override
+  String get dataSource => '';
+  @override
+  DataSourceType get dataSourceType => DataSourceType.file;
+  @override
+  String get package => null;
+  @override
+  Future<Duration> get position async => value.position;
+
+  @override
+  Future<void> seekTo(Duration moment) async {}
+  @override
+  Future<void> setVolume(double volume) async {}
+  @override
+  Future<void> initialize() async {}
+  @override
+  Future<void> pause() async {}
+  @override
+  Future<void> play() async {}
+  @override
+  Future<void> setLooping(bool looping) async {}
+}
+
+void main() {
+  testWidgets('update texture', (WidgetTester tester) async {
+    final FakeController controller = FakeController();
+    await tester.pumpWidget(VideoPlayer(controller));
+    expect(find.byType(Texture), findsNothing);
+
+    controller.textureId = 123;
+    controller.value = controller.value.copyWith(
+      duration: new Duration(milliseconds: 100),
+    );
+
+    await tester.pump();
+    expect(find.byType(Texture), findsOneWidget);
+  });
+
+  testWidgets('update controller', (WidgetTester tester) async {
+    final FakeController controller1 = FakeController();
+    controller1.textureId = 101;
+    await tester.pumpWidget(VideoPlayer(controller1));
+    expect(
+        find.byWidgetPredicate(
+          (Widget widget) => widget is Texture && widget.textureId == 101,
+        ),
+        findsOneWidget);
+
+    final FakeController controller2 = FakeController();
+    controller2.textureId = 102;
+    await tester.pumpWidget(VideoPlayer(controller2));
+    expect(
+        find.byWidgetPredicate(
+          (Widget widget) => widget is Texture && widget.textureId == 102,
+        ),
+        findsOneWidget);
+  });
+}