Fix preview tracks playing after their owning overlay has hidden #27898
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RFC. Closes ppy#27883. The idea here is that `PopOut()` is called _when the hide is requested_, so once an overlay trigger would hide, the overlay would `StopAnyPlaying()`, but because of async load things, the actual track would start playing after that but before the overlay has fully hidden. (That last part is significant because after the overlay has fully hidden, schedules save the day.) Due to the loose coupling between `PreviewTrackManager` and `IPreviewTrackOwner` there's really no easy way to handle this locally to the usages of the preview tracks. Heck, `PreviewTrackManager` doesn't really know which preview track owner is to be considered _present_ at any time, it just kinda works on vibes based on DI until the owner tells all of its preview tracks to stop. This solution causes the preview tracks to stop a little bit later but maybe that's fine? Just trying to not overthink the issue is all. No tests because this is going to suck to test automatically while it is pretty easy to test manually (got it in a few tries on master). The issue also mentions that the track can sometimes resume playing after the overlay is pulled up again, but I don't see that as a problem necessarily, and even if it was, it's not going to be that easy to address due to the aforementioned loose coupling - to fix that, play buttons would have to know who is the current preview track owner and cancel schedules upon determining that their preview track owner has gone away.
peppy
reviewed
Apr 16, 2024
| // base call is responsible for stopping preview tracks. | ||
| // delay it until the fade has concluded to ensure that nothing inside the overlay has triggered | ||
| // another preview track playback in the meantime, leaving an "orphaned" preview playing. | ||
| .OnComplete(_ => base.PopOut()); |
There was a problem hiding this comment.
I don't have a huge amount of faith in OnComplete but let's see how this goes.
peppy
approved these changes
Apr 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
RFC. Closes #27883.
The idea here is that
PopOut()is called when the hide is requested, so once an overlay hide would trigger, the overlay wouldStopAnyPlaying(), but because of async load things, the actual track would start playing after that but before the overlay has fully hidden. (That last part is significant because after the overlay has fully hidden, schedules save the day.)Due to the loose coupling between
PreviewTrackManagerandIPreviewTrackOwnerthere's really no easy way to handle this locally to the usages of the preview tracks. Heck,PreviewTrackManagerdoesn't really know which preview track owner is to be considered present at any time, it just kinda works on vibes based on DI until the owner tells all of its preview tracks to stop.This solution causes the preview tracks to stop a little bit later but maybe that's fine? Just trying to not overthink the issue is all.
No tests because this is going to suck to test automatically while it is pretty easy to test manually (got it in a few tries on master).
The issue also mentions that the track can sometimes resume playing after the overlay is pulled up again, but I don't see that as a problem necessarily, and even if it was, it's not going to be that easy to address due to the aforementioned loose coupling - to fix that, play buttons would have to know who is the current preview track owner and cancel schedules upon determining that their preview track owner has gone away.