Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

Add video player buffering status #583

Merged
merged 5 commits into from
May 25, 2018

Conversation

skybur
Copy link
Contributor

@skybur skybur commented May 24, 2018

I have added property to know whether video player is currently buffering or not. I need this to show CircularProgressIndicator if the video is currently buffering. I haven't tested for the IOS because I don't have the devices so if anyone can test it, please do.

Copy link
Contributor

@mravn-google mravn-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! Please add a CHANGELOG.md entry and bump the version in pubspec.yaml.

case 'bufferingStart':
value = value.copyWith(
isBuffering: true,
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Here and below: we use trailing commas only when needed. And this statement easily fits on a single line:

value = value.copyWith(isBuffering: true);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah sorry, force of habit. Will remove.

} else if (context == playbackBufferFullContext) {
if (_eventSink != nil) {
_eventSink(@{@"event" : @"bufferingEnd"});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't quite work. Running the example app on my iOS device, the circular progress indicator is activated, but then stays active throughout playback.

I guess if the video fits in the buffer, the playbackBufferFull property never changes. So we probably have to fire the bufferingEnd event also on playbackLikelyToKeepUp.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, gonna add that.

@mravn-google mravn-google self-assigned this May 24, 2018
Copy link
Contributor

@mravn-google mravn-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mravn-google mravn-google merged commit a98cbf8 into flutter:master May 25, 2018
@mravn-google
Copy link
Contributor

Published, thanks again!

@skybur
Copy link
Contributor Author

skybur commented May 25, 2018

Thanks for reviewing 😁

@skybur skybur deleted the buffering-status branch May 25, 2018 07:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
3 participants