remove play/pause toggle behavior #5
Conversation
NicholasNagy
left a comment
NicholasNagy
left a comment
There was a problem hiding this comment.
I just had a question regarding the scope of the expected changes
There was a problem hiding this comment.
What about in the dart layer, and on the Android side? Is the toggle removed there?
| throw StateError("The data source has not been initialized"); | ||
| } | ||
| return videoPlayerController!.value.isPlaying; | ||
| return videoPlayerController!.isPlaying; |
There was a problem hiding this comment.
Get the isPlaying value directly from the controller
| } | ||
|
|
||
| @override | ||
| Future<bool> getIsPlaying(int? textureId) { |
There was a problem hiding this comment.
Method channel for getting the isPlaying boolean from the native layer
| } | ||
| _updatePosition(newPosition, absolutePosition: newAbsolutePosition); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Moved play logic directly in the play method instead of calling the toggling (_applyPlayPause) method
| if (!_created || _isDisposed) { | ||
| return; | ||
| } | ||
| await _videoPlayerPlatform.pause(_textureId); |
There was a problem hiding this comment.
Moved pause logic directly in the pause method instead of calling the toggling (_applyPlayPause) method
NicholasNagy
left a comment
NicholasNagy
left a comment
There was a problem hiding this comment.
I had a question about the play method, otherwise, the changes look good.
| _timer = Timer.periodic( | ||
| const Duration(milliseconds: 500), | ||
| (Timer timer) async { | ||
| if (_isDisposed) { | ||
| return; | ||
| } | ||
| final Duration? newPosition = await position; | ||
| final DateTime? newAbsolutePosition = await absolutePosition; | ||
| // ignore: invariant_booleans | ||
| if (_isDisposed) { | ||
| return; | ||
| } | ||
| _updatePosition(newPosition, absolutePosition: newAbsolutePosition); | ||
| }, |
There was a problem hiding this comment.
I this code necessary? What is the position and absolute position for?
There was a problem hiding this comment.
I guess these will be useful when we add the skip ahead feature. (seekTo method)
position is for getting the position in the video
absolutePosition is for getting the position in the video stream
We can remove it for now, But maybe we will need it in future
There was a problem hiding this comment.
I'd rather remove it and add back when needed, as we can then design it in a way that makes most sense for us
There was a problem hiding this comment.
Cool, I'll remove it then
Changes included:
play will simply play the pause will simply pause