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

@nstudio/nativescript-exoplayer renaming and documentation updates #68

Open
brianrclow opened this issue Nov 8, 2021 · 16 comments
Open

Comments

@brianrclow
Copy link

Over the past few months I have noticed some confusion around using video with nativescript. The main issue stems from exoplayer and videoplayer both being used but not clear which should be used or why. The idea would be to potential renaming @nstudio/nativescript-exoplayer using the word "video" but being clear that it is more robust and requires the exo libraries with it.

It would be nice to also include some sort of video section in the UI docs or including it in the plugins section of the official nativescript website. This would ideally solve some confusion around if nativescript supports video and which plugin would be best for your project.

I believe it would be helpful if it is more clear that nativescript can support video and does, as well as give some information around which plugin might be better for the project someone is currently working on. This might also create more interest in PRs for the plugins as they will be more visible.

@brianrclow
Copy link
Author

I'm still very interested in finding a good way to inform the community through the docs about the different video players(as well as improve them so new community members can get started with video quickly). When I started my {N} journey, I was very confused as to which player to use. I tested out random marketplace players and several other that were in the community.

The goal:
Solve confusion around what video player to use for what level of technical need and where to find the plugins. For new comers and for devs that have been around {N} for a bit it can still be confusing.

What I understand now:
The @nstudio/nativescript-exoplayer and @nstudio/nativescript-videoplayer are the two best video solutions.(If there is a better solution for NativeScript I'm all ears!) Both plugins use Apple AV Player for iOS but different players for Android. For Android, @nstudio/nativescript-videoplayer is the "more basic" version as the Android MediaPlayer doesn't have many features. @nstudio/nativescript-exoplayer uses Google ExoPlayer which is the "more advanced" version with much more features and is better for streaming video.

I get the reason for having both plugins, so that devs that just need a slimmed down video player use @nstudio/nativescript-videoplayer. Then the devs that need more features and are fine with the added packages that come with exoplayer, use @nstudio/nativescript-exoplayer.

A possible solution:
I still think there is some value in allowing the choice between the 2 different Android players, having 2 plugins makes sense. Wether they belong under the @NativeScript or the @nstudio umbrella idk what would be best. The biggest area of clarity could come from the naming and documenting of the plugins. I wouldn't know how to accomplish this idea entirely but I am putting it out here to get feedback and discussion going.

Create a @nativescript/basic-videoplayer (Apple AVPlayer and Android MediaPlayer) and a @nativescript/advanced-videoplayer (Apple AVPlayer and Google ExoPlayer). Add links in the plugin section of the {N} docs like other plugins have. Then in each repo, add somewhat concise wording to the readme describing what the main features/benefits of each. This could also be partially documented in a blog post or in the {N} docs.

I believe doing something like this would create more community awareness, allow people trying out NativeScript to see how easy it could be to get video going quickly and likely lead to better plugins overall. If this type of idea is unwanted or there are reasons why it shouldn't happen I'd be interested to hear those as well. I'm interested in finding a good solution to move forward with and looking for other's opinions too!

I'm not sure who to chat with so I'm tagging @nstudio @NathanWalker

@NathanWalker
Copy link
Member

Great thoughts here @brianrclow thanks for outlining this. I think the naming could be:
Basic: @nativescript/video
Advanced: @nativescript/video-advanced

The advanced naming probably makes sense because the API would also be more advanced and thorough which would likely prove the value of having both since not all cases would need the more advanced player.

@brianrclow
Copy link
Author

Thanks @NathanWalker Yea, I think naming like that would make more sense. What would be the first steps in creating a timeline to accomplish something like this? I would be interested in helping out with documentation and figuring out how to add a few more features to the plugins.

@NathanWalker
Copy link
Member

Big help! You could fork https://github.com/NativeScript/plugins
Run ‘npm run setup’ then:
npm run add

video

npm run add

video-advanced

and begin moving the plugins in there - once readmes are updated in there we have nightly scripts that would add them to the docs

@brianrclow
Copy link
Author

Okay great, thanks for the direction. I'll get this started in an upcoming sprint, need to balance paid work and OSS.

@brianrclow
Copy link
Author

@NathanWalker I've forked, created the new packages under the @NativeScript scope and started adding in the readme's. Creating a new plugin in that monorepo is pretty slick and clean. Before I get too far, I wanted to ask you some questions:

  1. What would be the best way to transfer the actual plugin code over? Should I literally be copy and pasting?
  2. Would there be a way to still list the contributors if I just bring the code over? I still want to give credit where credit is due. I know that some packages list contributors at the bottom and others do not. Not sure what the best direction to go is.
  3. I also am curious about the backwards compatibility with {N} 6 or 7 and how/if this process could still support the older {N} versions?

Here is where I am currently working on it: https://github.com/brianrclow/plugins/tree/video-advanced

I'm going to continue to add to the readme's and try to understand some of the code better so I can make a smoother transition. Any advice is appreciated, thanks!

@brianrclow
Copy link
Author

Also - I've noticed that all of the commits are using the conventional commit pattern, is there a place where Nativescript has the guidelines around that for community members to understand how to best commit their code? I don't want to do it wrong and mess up things. 😅😅

@NathanWalker
Copy link
Member

  1. What would be the best way to transfer the actual plugin code over? Should I literally be copy and pasting?

Your fork looks like great direction - yes, you can copy/paste into that new package, @nativescript/video-advanced in your workspace fork.

@NathanWalker
Copy link
Member

2. Would there be a way to still list the contributors if I just bring the code over? I still want to give credit where credit is due. I know that some packages list contributors at the bottom and others do not. Not sure what the best direction to go is.

Best way is for us to mention in the README here for the exoplayer the proper credit and keep the commits here for history. That way it's clear where the credit is due and how to find it. The new package can also link and mention the original for proper history investigations.

@NathanWalker
Copy link
Member

NathanWalker commented May 25, 2022

3. I also am curious about the backwards compatibility with {N} 6 or 7 and how/if this process could still support the older {N} versions?

The plugin workspace fork you have is only for {N} 7 and above; all plugin workspaces support 7 and above only.

@NathanWalker
Copy link
Member

NathanWalker commented May 25, 2022

Also - I've noticed that all of the commits are using the conventional commit pattern, is there a place where Nativescript has the guidelines around that for community members to understand how to best commit their code? I don't want to do it wrong and mess up things.

Great question, we try to follow this format: https://www.conventionalcommits.org/en/v1.0.0/
It's a great standard used across broad open source projects. You don't have to adhere precisely all the time (as a contributor) because when maintainers merge pull requests, squash & merge option is often used which gives the maintainer the opportunity to reword the commit (squashing all commits from the pull request into one) into an acceptable format for main branch cleanliness.

@farfromrefug
Copy link

@NathanWalker @brianrclow about the naming / split of the 2 plugins . wouldn't it be better to make exoplayer an "extension" to the other plugin?
the idea being behind that is to have same base code same api...
the extension would depend on the base plugin and simply override/subclass the android part (would be possible for iOS too).
just my thought

@rigor789
Copy link
Member

I'll leave my thoughts here as well:

Most of the time, simply crediting the prior art/work in the repo/readme is enough (though do always check the license, and if necessary include a copy of it).

Preserving git history is possible, I have done it before, however it requires quite a bit of git trickery, and it's not perfect. An example where I did this:

https://github.com/NativeScript/nativescript-cli/tree/master/packages/doctor
pulled in from:
https://github.com/NativeScript/nativescript-doctor

The upside is that all the prior commits are preserved, however merge commits now point to incorrect pull requests, since #1 etc now points to the PR#1 in the CLI repo and not the doctor repo. That's something I wasn't able to fix, since rebasing doesn't work on merge commits as far as I could tell from various sources.

Is it worth the trouble? Maybe? Not really...

About organizing and splitting, I'd agree with @farfromrefug's take that it might make sense to have a base plugin with a nice api, and make the underlying player implementation swappable with a single call in main.ts or something simple like that.

@jcassidyav
Copy link

jcassidyav commented May 25, 2022

I was considering this recently and I think the license point is important, and depending on the licence does appear to place specific requirements on the copy. for example the license governing exoplayer here is apache 2 and explicitly states ( among other things) in section 4 "(a) You must give any other recipients of the Work or Derivative Works a copy of this License".

My reading of this is the contents of https://raw.githubusercontent.com/nstudio/nativescript-plugins/main/LICENSE must be brought across to the new repo, and furthermore must end up in the npm package. Also there are instructions for indicating what files are changed etc. so there are requirements placed on any copy by the license.

The reason I was thinking about it was that I was considering similar to this, bringing https://github.com/EddyVerbruggen/nativescript-secure-storage into NativeScript plugins and rewriting the android portion to use EncryptedSharedPreferences. ( it is MIT license, which is simpler but also requires the original license travels with the code ).

I find this not very straight forward, but it would seem to me to be important to understand ( which I probably do not fully )

@NathanWalker
Copy link
Member

@brianrclow looks like this escalated quickly 🤣 ... some considerations here, great input from everyone.

@brianrclow
Copy link
Author

brianrclow commented May 27, 2022

Thank you all for the feedback!

@NathanWalker I will get started on bringing over exoplayer to @nativescript/video-advanced and I'll add the contributors to the readme. Support for 7 and above is great, I should probably make that clear and link back to the other repo as well. I can start to use the conventional commit pattern (we already loosely use it at work, so it is familiar).

@farfromrefug I'm not against making the exoplayer an "extension" to the other plugin like you say. I haven't seen this done before though and I wouldn't know how to accomplish that. There is some value in only maintaining "1 iOS side" instead of duplicating that in both the "video" and "video-advanced" versions. How might you suggest going about the extension process? Is there another plugin that follows this I could check out?

@rigor789 Preserving history sounds like it would be a bit of a pain and I'm not sure if I'm up for it. Thanks for sending those links though. I would still want to follow the licenses and give credit to prior work done. As for the plugin extension idea/implementation. Do you have any example of one currently with a similar setup? I think that idea would simplify things further, which is my goal. Having a single plugin that has the capability of changing the Android native implementation sounds like a better solution than 2 plugins that have the same iOS native implementation.

@jcassidyav Good call on the license. It's something I wanted to be careful about. The nativescript-videoplayer license is MIT and the nativescript-exoplayer license is Apache License 2.0. It would appear that the MIT license is easier to deal like you say. I would need to look into what happens if I would bring the 2 plugins together into a single source of "video plugin" truth for NativeScript. Not sure which license to use in that case. Definitely not straight forward though.

I'm very interested in this project, both because of an app I have in production that utilizes video and for the greater community moving forward. I think making video clear to use and adding important features will be a good improvement to the developer experience with NativeScript, especially for newer developers coming from a web background.

I'll definitely need to lean on some community members/maintainers for this as time goes on as I've never written or assisted with a plugin like this before!

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

5 participants