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

fix: allow triggering of events after a stream has reached the end #217

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

oscnord
Copy link
Contributor

@oscnord oscnord commented Dec 11, 2023

This PR fixes an issue where events wouldn't be emitted after the end state due to isNotReady being set to true even if the video element would trigger play/pause/seek etc.

@bwallberg
Copy link
Collaborator

@oscnord this is by design ( see https://github.com/Eyevinn/media-event-filter#limitations ), but is something we want so support #3.

We just have to test it very thoroughly so that the event flow works properly.

@oscnord
Copy link
Contributor Author

oscnord commented Dec 12, 2023

@oscnord this is by design ( see https://github.com/Eyevinn/media-event-filter#limitations ), but is something we want so support #3.

We just have to test it very thoroughly so that the event flow works properly.

Ah okay, I should have checked the open issues... 🤦‍♂️

@@ -114,7 +116,7 @@ export const getMediaEventFilter = ({
...initialState,
};

const isNotReady = (): boolean => state.loading;
const isNotReady = (): boolean => allowResumeAfterEnded ? state.loading : state.loading || state.ended;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this the wrong way around?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If allowResumeAfterEnded is true we want to allow triggering of events i.e only checking state.loading. If false we want to have the same behaviour as before.

Copy link
Contributor Author

@oscnord oscnord Dec 12, 2023

Choose a reason for hiding this comment

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

Or maybe I'm wrong(?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

if allowResumeAfterEnded ( or maybe allowPlaybackAfterEnded is a clearer name 🤔 ) is true we want the new behavior, if false ( default ) we want the old behavior.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a dumb dumb, misunderstood the code 🤦

Copy link
Collaborator

@bwallberg bwallberg left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@oscnord oscnord force-pushed the fix/seeking-in-ended-stream branch from 2961eb1 to 4d5c7f6 Compare December 12, 2023 12:32
@bwallberg bwallberg self-requested a review December 12, 2023 12:32
@oscnord oscnord force-pushed the fix/seeking-in-ended-stream branch from 4d5c7f6 to 38abf62 Compare December 12, 2023 13:18
Copy link
Collaborator

@bwallberg bwallberg left a comment

Choose a reason for hiding this comment

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

👍

@oscnord oscnord merged commit e5fe405 into main Dec 12, 2023
2 checks passed
@oscnord oscnord deleted the fix/seeking-in-ended-stream branch December 12, 2023 16:07
@oscnord oscnord linked an issue Dec 12, 2023 that may be closed by this pull request
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.

Allow "restarting" video after "ended"
2 participants