-
-
Notifications
You must be signed in to change notification settings - Fork 28
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
Added Common Methods and Properties #4
Conversation
Thanks very much for the PR - I'll find some time soon to check it out and hopefully merge! |
@JPVenson Thanks for opening this PR! Using the code changes from this PR I was able to use it to workout the desired functionality I need to programmatically change the Source video url and then to start playing the next video in the list. What I essentially have is a main video player that will start with the default video for a Module. There are usually multiple videos per module in a Playlist. The user can start playing the next one in the list if they so choose. My use case is the following:
I have done some by performing the following behavior. Does this look like the correct call flow in order to stop the currently playing video, set the new source and begin to play the desired next video?
|
@chrislangston I have done essentially the same only with more "steps" in between but in essence your code should workout fine. Apart from proactivly calling StopPlayback before reloading the control your code looks ok |
Use Jquery if present for event listener
Thanks @JPVenson. Are there any other gotcha's that I should watch out for for example should we do any Dispose or unregister of Event Handlers before switching the source and reloading? |
hmm not really, not for this case. Not as i have observed it as all eventhandlers seem to be removed of when the object is removed from DOM. You should be careful to not try to start the playback while the pause promise is not yet completed as this will cancel the promise and will result in an error message. But this is just "for information" it does not seem to be effecting anything. if you are interested i can put my project on Github, I have wrapped the whole player to support my own controls. |
@JPVenson That would be fantastic if you can share your code! I would love to see all the advanced stuff you are doing. |
@chrislangston this is the repro: For Video related stuff i was doing, the most important classes are as follwing: |
@SQL-MisterMagoo I hope you're doing well. Any chance of getting these code changes merged in so we can pull the Nuget library instead of manually bringing in the various components? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for the PR - I'd really like to get this merged in, but I have concerns about the number of properties with setters that either will do nothing or could benefit from some validation.
I didn't flag them all up as it's late and I think a pattern is emerging that should be easy enough to follow.
One of the reasons why I haven't added any of this myself is the concern about getting it right and preventing support issues, so I'd really like it if you could remove properties and/or setters where they don't make sense or are not implemented yet and create some samples of the API usage.
I'd understand if you don't want to/can't do that - for the same reasons I'd hope you understand my reasons for wanting them.
set { SetValue(value); } | ||
} | ||
|
||
public object AudioTracks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it is wise to add properties that will just throw an exception - I think it would be a better experience to just no provide these - was there a reason in your mind for having them ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that like you in the first place but if the properties are there, and some day they are supported you could use them without recompiling the lib.
( object because of lazyness )
Yea same here. I will reiterrate over your mentions and make as much changes as you asked for, but I just recently suffered some hardware loss (GPU Waterblock literally Burned today) i dont know when I again will have a working pc for the changes to be made. |
@JPVenson Hey, hope you're doing great. This is a great PR, nice job! I stumbled upon this repo yesterday and I immediately thought these properties and methods were very much needed. I even posted an issue asking for them! I didn't know this PR existed. |
Hi @AradAral This is going to turn out to be a very important library for the Blazor eco-system. What I did was to pull the PR locally and I am using it as it to fit my needed in the meantime. Once the changes come in I'll replace with the latest and great code. However, this allows me to reload new videos using familiar C# method names which makes it amazing :-). The actual project that JP shared actual demonstrates a very advanced usage of this library. |
@AradAral Without promises, maybe Tuesday next week. |
Made GetValue and SetValue protected Fixed setters present on readonly properties
…avior Changed name of js object to be BlazoredVideo to avoid possible nameing conflics Enabled package generation on build
OK a bit of explaining required:
<script src="_content/Blazored.Video/blazoredVideo.js"></script>
Apart from that, I added comments for all properties according to: https://www.w3schools.com/tags/ref_av_dom.asp |
@JPVenson Good job. Thanks, man. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much for all your efforts, please don't feel the need to alter your PR based on the latest review, as I'm going to pull this down and test it out, and add some samples. I'll take care of any final tweaks while I do that.
There are some failing tests that I need to look at as well.
Hopefully we can get all this merged in fairly soon.
@@ -25,10 +25,13 @@ | |||
<PublishRepositoryUrl>true</PublishRepositoryUrl> | |||
<EmbedUntrackedSources>true</EmbedUntrackedSources> | |||
<PackageReleaseNotes>Initial Release</PackageReleaseNotes> | |||
<Version>1.1.0</Version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Versioning happens in the github workflow when we release, this is not needed/wanted in the source
@@ -25,10 +25,13 @@ | |||
<PublishRepositoryUrl>true</PublishRepositoryUrl> | |||
<EmbedUntrackedSources>true</EmbedUntrackedSources> | |||
<PackageReleaseNotes>Initial Release</PackageReleaseNotes> | |||
<Version>1.1.0</Version> | |||
<GeneratePackageOnBuild>true</GeneratePackageOnBuild> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Packaging happens in the github workflow, this is not needed
@@ -1,80 +1,22 @@ | |||
@* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this code is required for the component to work, the default result of this is more work for the end developer and a breaking change for existing projects.
While I can see that some devs might want the JS separate, I think it would be best if this was controlled by an optional parameter on the component, leaving the default behaviour as it is now, but enabling those devs who want to control the JS externally with that ability.
I would want something like this in BlazoredVideo.razor.cs
[Parameter] public bool UseExternalJavaScript { get; set; } = false;
and then in this file,
@if (!Configured && !UseExternalJavaScript)
{
...
}
It would then also be of benefit to raise a decent error if the developer has not included the JS required for this to run.
@SQL-MisterMagoo hey has been a while. Any progress here? |
Hi, yeah, sorry personal reasons, but I've been working on it again today and come across something I hadn't tested yet and don't know if I am doing something wrong.... I was trying to set the PlaybackRate, but cannot find a syntax that works - I've never seen a property defined as a Task and don't know how to use it to set a value - can you help? |
@SQL-MisterMagoo in my last pull request the property was of type double:
What do you mean with a property of type |
Hmm, ok so I have to look back and see what happened - thanks |
@SQL-MisterMagoo I reviewed your PR and with 846cc30 you changed all the types to be of |
Yeah, I just saw that myself - I think you can see why I took a break! I'll fix it 😱😱 |
I am finally able to look at this again - I have updated the tests to work with the new code and am in the process of updating the samples to use some of the new methods/properties. Hitting a few snags but hope to have it sorted out tomorrow. |
Any updates on this? |
any update? |
Hey all - really sorry for the loooong delay - It's been a rough couple of years and I just haven't had the time to do anything, but I am actively looking to merge this in now. Hopefully not too long. |
@SQL-MisterMagoo No worries we understand. I hope everything now works out for you! But if you need more help i can offer to help you out with the video repro. |
@SQL-MisterMagoo We all understand the day job commitments. I still greatly appreciate you and @JPVenson creating / enhancing this library. I've been using it non-stop without issues since Dec 2020 which is a strong testament to your creativity and contributions to the community. If I can be of assistance let me know. |
I have added (nearly) all methods and properties as described in
https://www.w3schools.com/tags/ref_av_dom.asp
to be called directly from an
BlazoredVideo
object.