Skip to content

Conversation

@RuslanTsitser
Copy link

  • Added video payback control
  • Added video file support

Copy link
Owner

@olexale olexale left a comment

Choose a reason for hiding this comment

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

Could you please update one of the examples with the proposed functionality? We rely on these examples for manual smoke testing to confirm the plugin works correctly before releasing new updates.

switch call.method {
case "play":
VideoArkitPlugin.nodes[id]?.play()
VideoArkitPlugin.players[id]?.play()
Copy link
Owner

Choose a reason for hiding this comment

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

Wouldn't that be a breaking change for folks who use SKVideoNode?


/// Seek video to a specific time.
Future<void> seekTo(Duration duration) =>
_channel.invokeMethod<void>('seek', {'id': id, 'seconds': duration.inSeconds});
Copy link
Owner

Choose a reason for hiding this comment

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

I think using milliseconds would be a better fit, as it gives users finer control over the timing.

} else if let filePath = dict["filePath"] as? String {
let videoFileURL = URL(fileURLWithPath: filePath)
videoPlayer = AVPlayer(url: videoFileURL)
videoNode = SKVideoNode(avPlayer: videoPlayer!)
Copy link
Owner

Choose a reason for hiding this comment

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

Please avoid using force unwraps. If videoPlayer creation fails, call logPluginError instead.

String? filename,
String? url,
bool? autoplay = true,
String? filePath,
Copy link
Owner

Choose a reason for hiding this comment

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

For users, the distinction between filename and filePath might not be obvious. What benefits do we gain from using SKVideoNode(fileNamed: videoFilename)? Do you think it would be more consistent to always rely on SKVideoNode(avPlayer: videoPlayer) instead?

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

Successfully merging this pull request may close these issues.

2 participants