Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove url requirement and checking #11

Open
artths opened this issue Jan 17, 2020 · 6 comments
Open

Remove url requirement and checking #11

artths opened this issue Jan 17, 2020 · 6 comments
Assignees

Comments

@artths
Copy link
Contributor

artths commented Jan 17, 2020

I want to be able create blank video player view first, put it in the view tree and then update it with the url when I have one. Like that:

// create widget  
Video _videoPlayer = Video();

...

// put in some widget tree
child: AspectRatio(
                aspectRatio: 16 / 9,
                child: _videoPlayer,
              ),

...

// update with url and title
 _videoPlayer = Video(
              autoPlay: true,
              url: link,
              title: 'Title',
              subtitle: 'Subtitle',
            );

ATM it's not possible, because if there is no URL, it's null or not valid player instance would not be created (simple UIView in native part and Container() in dart part). Even if I update player widget with the url later it still will not be playing because there is no player instance created and it won't be created because _setupPlayer() is called in initState() only.

I propose to make this parameter optional and to completely remove all checks for its validity. Just create a player instance without a link so we can add it later. Moreover, in its current form, you have a chance to add a link when creating a player, which in your opinion will be valid, but the player will consider it not valid and you will no longer have the opportunity to create a player. All subsequent new url requests will be ignored.

@KhuramKhalid KhuramKhalid self-assigned this Jan 17, 2020
@KhuramKhalid
Copy link
Member

If we change the _onMediaChanged() method in lib/video.dart to following then this issue could be fixed.

void _onMediaChanged() {
    if (widget.url != null) {
      if (_methodChannel == null) {
        _setupPlayer();
      } else {
        _methodChannel.invokeMethod("onMediaChanged", {
          "autoPlay": widget.autoPlay,
          "url": widget.url,
          "title": widget.title,
          "subtitle": widget.subtitle,
          "isLiveStream": widget.isLiveStream,
        });
      }
    }
  }

I tested this by first passing null as URL which resulted in a plan Container() and then when I pass an actual URL and rebuild, it creates the player. Let me know your thoughts on this.

@artths
Copy link
Contributor Author

artths commented Jan 17, 2020

Yes, it works if passing null, but will enter useless state if we supply some broken link to it:

  Video _videoPlayer = Video(
    url: "http://some_site.com/broken_link.mp4",
  );

Native view will not be created because of the isPlayable check but native channel will.

@KhuramKhalid
Copy link
Member

That's true. The change you mentioned in #10 will fix this. Moving init code out of view() in native iOS. Then media change can call init again if player was never initialised. Would it be possible for you to create a PR for this? Otherwise I'll try working on this later.

@artths
Copy link
Contributor Author

artths commented Jan 18, 2020

Also this check should be removed from didUpdateWidget

    if (widget.url == null || widget.url.isEmpty) {
      _disposePlatformView();
    }

Because it will crash player when I just want to pause it:

      _videoPlayer = VideoPlayout(
        desiredState: PlayerState.STOPPED,
      );

Want you do some PlayerController so we could control the video state without needing of recreate widget?

I will do PR when find all the errors.

@artths
Copy link
Contributor Author

artths commented Apr 11, 2020

Sorry, was working on another project.
I created PR with the changes you propose in _onMediaChanged() and also in native part, because it crashes if I supply null url since last commit.

@KhuramKhalid
Copy link
Member

Thanks for the PR. I've pushed a new version out which includes this. See v1.0.35

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants