ChewieController #99

cbenhagen · 2019-01-16T10:10:49Z

This is an attempt in fixing #1 by implementing a ChewieController which is internally accessed by an InheritedWidget.

TODO:

Check if playback hack is still needed and if yes, add it back in
VideoPlayer tries to access disposed VideoController after we changed and disposed it in the ChewieController
Add data to be rendered on all the Widgets, probably as Listenables, such as ChangeNotifier or ValueNotifier.
Update changelog
Document how to migrate from Chewie < 1.0.0

fixes #1
fixes #23
fixes #49

cbenhagen · 2019-01-16T10:14:29Z

@brianegan I know it still has some rough edges but if you have time it would be awesome if you could have a quick look at the changes and tell me if this is the direction you'd like this to go.

lib/src/chewie_controller.dart

brianegan · 2019-01-16T13:45:54Z

Other than a couple small nitpicks, this is exactly the direction I was thinking of going. Really well done :D :D :D

brianegan · 2019-01-16T15:04:05Z

Two more things: As part of this PR, could you please add yourself to the list of authors in the pubspec.yaml? I'll also give you uploader rights for this repo so once this is ready to roll you can go ahead and publish it to pub if you'd like :)

cbenhagen · 2019-01-16T15:10:55Z

Done :)

brianegan · 2019-01-16T15:14:04Z

Cool, just added that email as an uploader as well. You should get a confirmation email to that address!

Only thing I'd ask: Please update the CHANGELOG as well when ya push new versions to let folks know what's changed. Other than that, not too much to publishing new versions!

Thanks again for all the contributions :) Seems like more and more folks are using this library, great to make it better.

cbenhagen · 2019-01-16T15:34:13Z

Sure! I will also not push bigger changes without discussing them first. And then we should always guide the users how to migrate. Thank you for that great package!

We can switch back to using ValueNotifier when we have a lot more values and it is more convenient to change and notify with one `value.copyWith(foo='bar')` instead of explicitly having to call notifyListeners(). Until then the code is just much cleaner without so much boiler plate.

cbenhagen · 2019-01-17T14:43:29Z

@brianegan, you once mentioned to "add data to be rendered on all the Widgets, probably as Listenables, such as ChangeNotifier or ValueNotifier". What do you think, should we add this to this release? If yes, what data did you have in mind?

More finegraned customisation of exisiting controls to be added later.

brianegan · 2019-01-17T16:38:21Z

Nope, this covers it -- the idea was to give something folks can listen to for data updates -- in this case, the ChewieController does the job, and Widgets can get the info they need from that controller (or the nested VideoController)!

Awesome work! I'll do a final review :)

cbenhagen · 2019-01-20T15:06:42Z

@brianegan I got more plans for Chewie like supporting multiple versions of a clip as well as playlists. I guess they can both be added without breaking the API again by adding named constructors and internally switching to always having a list(of lists) of VideoControllers but setting one as the current videoPlayerController. So unless you see any breaking changes ahead, you could do a final review of this PR so we can publish this.

brianegan

Just a few small details and a bug report! Looking great overall:)

Bug Report:

Run Example, Tap "Full Screen" Button, Tap android back button, tap "Full Screen" Button again -- broken state. We probably aren't handling the Android Back Button properly here, which can return a "null" value and might be throwing an error if that isn't checked.

lib/src/chewie_controller.dart

pubspec.yaml

brianegan · 2019-01-21T12:21:43Z

lib/src/material_controls.dart

+    chewieController = ChewieControllerProvider.of(context);
+    controller = chewieController.videoPlayerController;
+
+    _dispose();


Unlike initState, didChangeDependencies can run multiple times throughout the lifecycle of the Widget. Therefore, we may want to add some logic around this _dispose / initialize code: Only run that dispose and init IF the controller has changed.

Right, it was also reinitializing when the theme changed. fc1bc16 fixes that.

lib/src/chewie_controller.dart

And make the provider private

cbenhagen · 2019-01-23T11:27:34Z

Bug Report:

Run Example, Tap "Full Screen" Button, Tap android back button, tap "Full Screen" Button again -- broken state. We probably aren't handling the Android Back Button properly here, which can return a "null" value and might be throwing an error if that isn't checked.

Nice catch! Fixed in cd20458.

brianegan

Woohoo! Again, fantastic work 🎉 😄 👏

This Looks Good To Me. Feel free to merge and ship when you think it's ready!

cbenhagen · 2019-01-24T09:02:40Z

Just noticed that I need a gmail account to upload. Please add my personal mail as uploader: c.ben.hagen@gmail.com

brianegan · 2019-01-24T16:47:06Z

Oh dang -- just added that email address!

…

On Thu, Jan 24, 2019 at 10:02 AM Ben Hagen ***@***.***> wrote: Just noticed that I need a gmail account to upload. Please add my personal mail as uploader: ***@***.*** — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#99 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHujM1bgit4_3RdgqNd-YWg8gszyhbWks5vGXawgaJpZM4aCuU6> .

cbenhagen added 3 commits January 15, 2019 16:42

Remove new statement

990d839

Run dartfmt

ef4cfeb

ChewieController accessed by an InheritedWidget

5ee6608

cbenhagen requested a review from brianegan January 16, 2019 10:10

cbenhagen mentioned this pull request Jan 16, 2019

Allow customisation of the control widgets #100

Open

brianegan reviewed Jan 16, 2019

View reviewed changes

lib/src/chewie_controller.dart Outdated Show resolved Hide resolved

brianegan reviewed Jan 16, 2019

View reviewed changes

lib/src/chewie_controller.dart Outdated Show resolved Hide resolved

cbenhagen added 2 commits January 16, 2019 16:09

Add myself to the authors

5961f50

Remove comments

b578b47

Shorten API by adding getters

79abb08

cbenhagen added 5 commits January 17, 2019 13:34

Remove setter for videoPlayerController

79011c7

Push version to 1.0.0 -> breaking changes

40e8700

Pause and rewind video when switching

5b33793

Allow replacing the ChewieController

e9d84e0

cbenhagen force-pushed the ChewieController branch from a8a19f2 to bc12686 Compare January 17, 2019 14:15

Remove todo

4b96ceb

cbenhagen added 2 commits January 17, 2019 15:59

Pass through all methods of VideoPlayerController

f3fa335

Allow customising controls

aeac812

More finegraned customisation of exisiting controls to be added later.

cbenhagen added 3 commits January 20, 2019 14:03

Do not initialize twice

a16cabd

Sort constructor declarations before other members

f2e66aa

Add analysis options

bdb0278

cbenhagen changed the title ~~WIP: ChewieController~~ ChewieController Jan 20, 2019

Update README and CHANGELOG

b41d83d

cbenhagen force-pushed the ChewieController branch from bb0285a to b41d83d Compare January 20, 2019 15:53

This was referenced Jan 20, 2019

Video player throws error when put in ListView and scrolled past really quickly #21

Closed

Launch video in fullscreen through a custom button. #23

Closed

Fullscreen events #49

Closed

brianegan requested changes Jan 21, 2019

View reviewed changes

cbenhagen added 2 commits January 21, 2019 18:00

Version 0.9.0

333cb2f

Add documentation

bf58a0d

cbenhagen force-pushed the ChewieController branch from 75804f9 to bf58a0d Compare January 21, 2019 17:00

cbenhagen mentioned this pull request Jan 22, 2019

Update pubspec on Dart pub #101

Closed

Allow ChewieController.of

2bd06b7

And make the provider private

cbenhagen force-pushed the ChewieController branch from 6030ab9 to 2bd06b7 Compare January 22, 2019 11:37

cbenhagen added 3 commits January 23, 2019 11:49

Only reinitialize if ChewieController changed

fc1bc16

Properly set full screen state when exiting.

cd20458

Be consistent when naming methods and variables

c553696

brianegan approved these changes Jan 23, 2019

View reviewed changes

cbenhagen merged commit 58931ed into fluttercommunity:master Jan 24, 2019

cbenhagen deleted the ChewieController branch January 25, 2019 08:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ChewieController #99

ChewieController #99

cbenhagen commented Jan 16, 2019 •

edited

cbenhagen commented Jan 16, 2019

brianegan commented Jan 16, 2019

brianegan commented Jan 16, 2019 •

edited

cbenhagen commented Jan 16, 2019

brianegan commented Jan 16, 2019 •

edited

cbenhagen commented Jan 16, 2019

cbenhagen commented Jan 17, 2019

brianegan commented Jan 17, 2019 •

edited

cbenhagen commented Jan 20, 2019

brianegan left a comment •

edited

brianegan Jan 21, 2019

cbenhagen Jan 23, 2019

cbenhagen commented Jan 23, 2019

brianegan left a comment

cbenhagen commented Jan 24, 2019

brianegan commented Jan 24, 2019 via email

ChewieController #99

ChewieController #99

Conversation

cbenhagen commented Jan 16, 2019 • edited

cbenhagen commented Jan 16, 2019

brianegan commented Jan 16, 2019

brianegan commented Jan 16, 2019 • edited

cbenhagen commented Jan 16, 2019

brianegan commented Jan 16, 2019 • edited

cbenhagen commented Jan 16, 2019

cbenhagen commented Jan 17, 2019

brianegan commented Jan 17, 2019 • edited

cbenhagen commented Jan 20, 2019

brianegan left a comment • edited

Choose a reason for hiding this comment

brianegan Jan 21, 2019

Choose a reason for hiding this comment

cbenhagen Jan 23, 2019

Choose a reason for hiding this comment

cbenhagen commented Jan 23, 2019

brianegan left a comment

Choose a reason for hiding this comment

cbenhagen commented Jan 24, 2019

brianegan commented Jan 24, 2019 via email

cbenhagen commented Jan 16, 2019 •

edited

brianegan commented Jan 16, 2019 •

edited

brianegan commented Jan 16, 2019 •

edited

brianegan commented Jan 17, 2019 •

edited

brianegan left a comment •

edited