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

Feature/video queue #23

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Conversation

JPVenson
Copy link

@JPVenson JPVenson commented Jan 3, 2023

Related to #22

  • Added VideoQueue component to queue multiple video sources to run after each other
  • Fixed some code indentions
  • Fixed NREX when an otherwise empty event is requested. In that case the state object will be serialized as null and the video property cannot be set on it
  • Added Example in Example project
  • Added Readme Example

@SQL-MisterMagoo
Copy link
Collaborator

Thanks for this interesting contribution - I'll hopefully get time to review it within the next couple of days.

Added additional check that no prior event is overwritten
Copy link
Collaborator

@SQL-MisterMagoo SQL-MisterMagoo left a comment

Choose a reason for hiding this comment

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

There are a few items I would like you to review - I am open to different opinions / corrections to my comments, so feel free to defend your position 😄

}

/// <summary>
/// Specifies the delay between source changes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer this to be a TimeSpan to avoid confusion about units.

Copy link
Author

Choose a reason for hiding this comment

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

Timespan Binding is a bit of a hassel in Blazor. This is the reason why I made it a int-ms delay variable. Its the easiest way to bind it from my perspective. Sure a Timespan would be more declarative and i could add a proxy property with a timespan but best would be to leave it with an int property

src/Blazored.Video/VideoQueue.cs Outdated Show resolved Hide resolved
src/Blazored.Video/VideoQueue.cs Outdated Show resolved Hide resolved
}

#pragma warning disable BL0005
BlazoredVideo.EndedEvent = new EventCallback<VideoState>(this, OnPlaybackEnded);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you consider adding a method to BlazoredVideo e.g. SubscribeToEvent to handle this instead of disabling the warning?

Does this need to be an EventCallback? It seems like you are calling StateHasChanged() whenever the CurrentItem changes, so this feels like unnecessary overhead.

Copy link
Author

Choose a reason for hiding this comment

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

I did consider it however to "not just" get rid of the warning i would have needed to build a general cs event based handling, which was out-of-scope of this PR. Not necessary needs to be an EventCallback no. Would you recommand using the Ended property instead?

Copy link
Author

Choose a reason for hiding this comment

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

In the end it is not much different to what you would do in razor file but without the autogenerated code. I think it is missleading to raise that warning when a property is in use by another component.

[CascadingParameter]
public BlazoredVideo BlazoredVideo { get; set; }

public IList<VideoItem> VideoItems { get; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to be using the component VideoItem as a data class, which makes me uncomfortable - Blazor is in control of the lifetime of components, and taking these references and storing them in another component feels suspect.

Instead of passing in VideoItems like this

VideoQueue.AddVideoItem(this);

I would prefer something like

VideoData = new(this);
VideoQueue.AddVideoItem(VideoData)

Where VideoData is a DTO class carrying the values we need in VideoQueue

Copy link
Author

Choose a reason for hiding this comment

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

I added some DTOs to be used as an intermediary. However I am not sure how everything will behave when Source or type is bound and does update. I will have to evaluate that later.

src/Blazored.Video/VideoQueue.cs Outdated Show resolved Hide resolved
src/Blazored.Video/VideoQueue.cs Show resolved Hide resolved
/// <summary>
/// Will repeat the current loaded video forever.
/// </summary>
Once,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find this name misleading - once should mean once, not forever.

samples/SharedRCL/Pages/QueueItems.razor Outdated Show resolved Hide resolved
samples/SharedRCL/Pages/QueueItems.razor Outdated Show resolved Hide resolved
@JPVenson
Copy link
Author

JPVenson commented Jan 5, 2023

Defend your position

image

Sory i could not resist ...

@JPVenson
Copy link
Author

maxresdefault-2566076843

@chrislangston
Copy link

@SQL-MisterMagoo @JPVenson Pushing this over the finish line would be a great addition to this amazing library. I recently upgraded my solution from .net 5 to .net 8 and the control is still going strong! I appreciate all time and hardwork dedicated to this library.

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.

3 participants