Allow multiple enabled playback modes #791
Conversation
…b.com/mseh1128/asbplayer into allow-multiple-enabled-playback-modes
|
Hey thanks for doing this. I've been pretty busy but will try to read this on the weekend. |
|
Btw I was testing this some more today, and realized I forgot about Auto Pause at start of subtitles. I'm working on making this work w/ Repeat mode. |
|
@mseh1128 On the first pass my only comment is that there seems to be a lot of duplicated code related to modifying the play mode set. Maybe this should be its own collection class? "Resolve conflicts" is the only part that is different depending on the context, but it should still be possible to extract that out from all the duplicated lines. |
…b.com/mseh1128/asbplayer into allow-multiple-enabled-playback-modes
|
Thanks for the review. I refactored most of the playMode related code into play-mode-manager.ts. Also, I was able to get "auto pause at start" working with Repeat and Condensed modes surprisingly easily. |
killergerbah
left a comment
killergerbah
left a comment
There was a problem hiding this comment.
Hey thanks for doing that refactor. Did a second pass.
This ensures the repeat seek behavior works regardless of how playback is triggered (keyboard shortcut, UI button click, etc.), not just when the play() method is called directly.
…calls with togglePlayMode
|
Sorry for the delay. I fixed the issues from the previous code review. Thanks 🙂. |
common/app/components/Player.tsx
Outdated
| } | ||
| }, | ||
| [playMode, clock, mediaAdapter, videoFileUrl, settings.autoPausePreference, seek] | ||
| [playModes, clock, mediaAdapter, videoFileUrl, settings.autoPausePreference, seek, subtitleCollection, resetPendingAutoRepeatTargetTimestamp] |
There was a problem hiding this comment.
Might be due to the merge but this line breaks for me because it's above where subtitleCollection is being initialized
There was a problem hiding this comment.
Yep, nice catch. subtitleCollection was initialized further down in the merge. I moved it back up so should work now.
extension/src/services/binding.ts
Outdated
| private _seekDuration = 3; | ||
| private _speedChangeStep = 0.1; | ||
| private _pendingAutoRepeatTargetTimestamp = 0; | ||
| private _lastKnownTime = 0; |
There was a problem hiding this comment.
The value of this field doesn't seem to be used anywhere. Did you mean to store only _pendingAutoRepeatTargetTimestamp instead? I think _pendingAutoRepeatTargetTimestamp is not properly being reset sometimes. For example, it's pretty easy to trigger a bug where auto-repeat is enabled, but seeking to the next subtitle still retains the repeat timestamp for the previous one.
There was a problem hiding this comment.
Looks like I removed the resetting of the timestamp in seekedListener() by accident. I added it back and tested so should work now.
2 participants
Implementation for #735
Took forever, but was able to get multiple playback modes working at the same time. In particular, handling the interactions for "Repeat+Autopause" and "Autopause+Condensed" was tricky and required some significant (and hopefully not too hacky) changes to Player.tsx/binding.ts.