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 🙂. |
|
@mseh1128 I did some more testing and found this issue where condensed mode stops working when its enabled with other modes at the same time. At the beginning, condensed mode is demonstrated working. When navigating 5 seconds back into a no-subtitle-region, the player automatically snaps back to the beginning of the ～♪ subtitle. But when auto repeat or auto pause is enabled with condensed mode at the same time, seeking back 5 seconds no longer snaps you back. Screen.Recording.2026-01-18.at.9.52.51.AM.movIt's probably too late but I wonder if this complexity can be dealt with by implementing multiple play modes as pure, reusable and testable logic rather than having to implement it twice in two different locations close to the UI. |
…eeking not resetting timestamp
I had previously disabled the Also, while testing I realized that seeking via keyboard shortcuts or the progress bar was broken due to some code I had removed before. I tried fixing this by using |
|
I agree that implementing multiple play modes as reusable, testable logic would've been better. Especially for testing purposes. That being said, there are a few significant differences in how multiple play modes are handled in the extension vs the web app. I haven't looked much at the testing framework here, but it would be nice to have testable logic for some of the different Play Mode interactions. |
| } | ||
|
|
||
| await seek(currentTime * 1000, clock, forwardToMedia); | ||
| // When forwardToMedia is false, the message came from the video element's seeked event, |
There was a problem hiding this comment.
This is the logic I had added to deal with seeks via the progress bar or keyboard shortcuts. This seems like the entrypoint for those seeks in Player.tsx, which is why I added it here. I'm not sure if this forwardToMedia check is always applicable though.
There was a problem hiding this comment.
Sorry. part of the code snippet got cut off. You can see it in my latest commit.
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.