Add OnCompleted event to PlaySongInfo class#1111
Open
star620 wants to merge 2 commits intoUnrealMultiple:masterfrom
Open
Add OnCompleted event to PlaySongInfo class#1111star620 wants to merge 2 commits intoUnrealMultiple:masterfrom
star620 wants to merge 2 commits intoUnrealMultiple:masterfrom
Conversation
Contributor
There was a problem hiding this comment.
Hey - 我在这里给出了一些整体性的反馈:
- 将完成行为从直接调用
EndSong()改为触发OnCompleted,意味着如果没有任何处理程序订阅该事件,就不会发生任何事;请确认所有现有调用方现在都订阅了这个事件,或者考虑一个合理的默认行为,以保持之前的语义。 - 目前的写法中,在
noteIndex == Notes.Count之后,每一次后续的Update调用都会触发OnCompleted;如果该事件预期是每首歌只触发一次,请添加保护(例如在调用前检查PlayCompleted),或者在第一次触发后取消订阅处理程序。
给 AI Agent 的提示词
请根据本次代码审查中的评论进行修改:
## 总体评论
- 将完成行为从直接调用 `EndSong()` 改为触发 `OnCompleted`,意味着如果没有任何处理程序订阅该事件,就不会发生任何事;请确认所有现有调用方现在都订阅了这个事件,或者考虑一个合理的默认行为,以保持之前的语义。
- 目前的写法中,在 `noteIndex == Notes.Count` 之后,每一次后续的 `Update` 调用都会触发 `OnCompleted`;如果该事件预期是每首歌只触发一次,请添加保护(例如在调用前检查 `PlayCompleted`),或者在第一次触发后取消订阅处理程序。帮我变得更有用!请对每条评论点 👍 或 👎,我会根据你的反馈改进后续的评审。
Original comment in English
Hey - I've left some high level feedback:
- Changing the completion behavior from directly calling
EndSong()to firingOnCompletedmeans nothing will happen if no handler is attached; confirm all existing callers now subscribe to this event or consider a sensible default behavior to preserve the previous semantics. - As written,
OnCompletedwill be invoked on every subsequentUpdatecall afternoteIndex == Notes.Count; if this event is intended to fire only once per song, add a guard (e.g., checkPlayCompletedbefore invoking) or unsubscribe handlers after the first invocation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Changing the completion behavior from directly calling `EndSong()` to firing `OnCompleted` means nothing will happen if no handler is attached; confirm all existing callers now subscribe to this event or consider a sensible default behavior to preserve the previous semantics.
- As written, `OnCompleted` will be invoked on every subsequent `Update` call after `noteIndex == Notes.Count`; if this event is intended to fire only once per song, add a guard (e.g., check `PlayCompleted` before invoking) or unsubscribe handlers after the first invocation.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or 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
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.
添加播放完成事件 OnCompleted
添加插件
更新插件/修复BUG
其他
Summary by Sourcery
在
PlaySongInfo流程中添加一个完成事件,并将歌曲结束时的处理交由订阅者负责,而不是直接结束歌曲。新功能:
PlaySongInfo上引入OnCompleted事件,用于在播放结束时通知监听者。优化:
EndSong,将PlaySongInfo与MusicPlayer解耦。Original summary in English
Summary by Sourcery
Add a completion event to the PlaySongInfo flow and delegate end-of-song handling to subscribers instead of directly ending the song.
New Features:
Enhancements: