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

Update VideoPlayer widget when the attached controller changes #769

Merged
merged 2 commits into from
Sep 11, 2018

Conversation

nichtverstehen
Copy link
Contributor

This fixes a bug introduced in #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.

@nichtverstehen
Copy link
Contributor Author

Also added tests for both conditions.

Copy link
Contributor

@sigurdm sigurdm left a comment

Choose a reason for hiding this comment

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

This needs a version bump/CHANGELOG update.

This change seems to be breaking the interface (though for the better).
Is it possible to first make a non-breaking fix that is version compatible?

@nichtverstehen
Copy link
Contributor Author

You mean, keep _isDisposed, package, timer members writable?

@sigurdm
Copy link
Contributor

sigurdm commented Sep 7, 2018

Yes.
You can also just mark this as a breaking update - I just worry that people with a dependency on ^0.6 would end up with 0.6.5 that is broken.

@nichtverstehen
Copy link
Contributor Author

Updated the changes.

There are two commits: the first one fixes the bug, the second one changes the interface and introduces the test. I put a minor version bump in the bugfix change (to 0.6.6) and bumped the major version number to 0.7.0 in the second CL (the test + the breaking API change aiding with the test).

What do you think?

…hanges.

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.
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).
@nichtverstehen
Copy link
Contributor Author

nichtverstehen commented Sep 7, 2018

(Updated the CHANGELOG too.)

@sigurdm
Copy link
Contributor

sigurdm commented Sep 7, 2018

The commits LGTM, but I can only land a squashed commit, so it has to be two PRs :(.

@sigurdm
Copy link
Contributor

sigurdm commented Sep 7, 2018

I am going on weekend now. So if you need to land this urgent you need to find another reviewer.

@cbenhagen
Copy link
Contributor

The currently published video_player is broken. It would be nice if this could be merged as soon as possible. @sigurdm are you back from the weekend?

@sigurdm sigurdm merged commit 8178bf1 into flutter:master Sep 11, 2018
@sigurdm
Copy link
Contributor

sigurdm commented Sep 11, 2018

I published a new version on pub

@nichtverstehen
Copy link
Contributor Author

Sorry, it was a long weekend in Zurich, so didn't get to split the PR. Thank you for taking care of this, Sigurd!

@sigurdm
Copy link
Contributor

sigurdm commented Sep 11, 2018

I haven't made the split, just published the full thing. Let's hope that's good enough.

andreidiaconu pushed a commit to andreidiaconu/plugins that referenced this pull request Feb 17, 2019
…er#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).
andreidiaconu added a commit to andreidiaconu/plugins that referenced this pull request Feb 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants