Bump version to 1.0.9 and add song queue commands#1112
Closed
star620 wants to merge 1 commit intoUnrealMultiple:masterfrom
Closed
Bump version to 1.0.9 and add song queue commands#1112star620 wants to merge 1 commit intoUnrealMultiple:masterfrom
star620 wants to merge 1 commit intoUnrealMultiple:masterfrom
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 我发现了 3 个问题，并且给出了一些宏观层面的反馈：
- 在
SongQueueCommand中，你多次调用args.Parameters[0].ToLower()；可以将其先赋值给一个本地变量subcommand来简化逻辑、提高可读性，并避免重复计算。 - 在列出队列（
songqueue list）时，目前只展示了每条记录的音符数量；建议同时展示歌曲名称或某种标识符，让队列对用户来说更有信息量。 - 在
SongQueueCommand的global分支中，同一个PlaySongInfo（以及performer）实例会被加入到所有玩家的队列中；如果这些类型是可变的，或者会记录播放状态，这种共享实例的做法可能会导致一些隐蔽的 bug，因此建议为每个玩家分别创建新的PlaySongInfo（以及/或者 performer）实例。
给 AI 代理的提示
Please address the comments from this code review:
## Overall Comments
- 在 `SongQueueCommand` 中，你多次调用 `args.Parameters[0].ToLower()`；可以将其先赋值给一个本地变量 `subcommand` 来简化逻辑、提高可读性，并避免重复计算。
- 在列出队列（`songqueue list`）时，目前只展示了每条记录的音符数量；建议同时展示歌曲名称或某种标识符，让队列对用户来说更有信息量。
- 在 `SongQueueCommand` 的 `global` 分支中，同一个 `PlaySongInfo`（以及 `performer`）实例会被加入到所有玩家的队列中；如果这些类型是可变的，或者会记录播放状态，这种共享实例的做法可能会导致一些隐蔽的 bug，因此建议为每个玩家分别创建新的 `PlaySongInfo`（以及/或者 performer）实例。
## Individual Comments
### Comment 1
<location path="src/MusicPlayer/MusicPlayer.cs" line_range="237-242" />
<code_context>
+ private void SongQueueCommand(CommandArgs args)
+ {
+ var songPlayer = SongPlayers[args.Player.Index];
+ if (songPlayer is null) return;
+
+ if (args.Parameters.Count == 0 || args.Parameters[0].ToLower() == "list")
</code_context>
<issue_to_address>
**suggestion:** 当没有 SongPlayer 时，建议向用户提供反馈，而不是静默返回。
当玩家在其 `SongPlayer` 尚未创建之前运行 `/songqueue` 时，命令会直接结束且没有任何输出，这对用户来说会比较困惑。建议发送一个简短的信息/错误提示，说明点歌系统尚未初始化，或者提示他们需要先开始播放一首歌。
```suggestion
private void SongQueueCommand(CommandArgs args)
{
var songPlayer = SongPlayers[args.Player.Index];
if (songPlayer is null)
{
args.Player.SendErrorMessage(GetString("歌曲系统尚未初始化，请先播放一首歌后再使用此命令。"));
return;
}
if (args.Parameters.Count == 0 || args.Parameters[0].ToLower() == "list")
```
</issue_to_address>
### Comment 2
<location path="src/MusicPlayer/MusicPlayer.cs" line_range="334-348" />
<code_context>
+ var searchTerm = args.Parameters[0];
+ var inst = args.Parameters.ElementAtOrDefault(1);
+
+ if (!TryResolveSong(searchTerm, out var fp, out var sn, out var em))
+ {
+ args.Player.SendErrorMessage(em);
+ return;
+ }
+
+ var nts = NoteFileParser.Read(fp!, out var tmp);
+ var perf = VirtualPerformer.GetPerformer(inst);
+
+ var newSong = new PlaySongInfo(nts, tmp, perf);
</code_context>
<issue_to_address>
**suggestion:** 在这个命令处理器中，过短且缺乏语义的变量名会降低可读性。
在这一段代码中，像 `fp`、`sn`、`em`、`nts`、`tmp` 和 `perf` 这样的名字会让逻辑更难理解。请使用更清晰的命名（例如 `filePath`、`songName`、`errorMessage`、`notes`、`tempo`、`performer`），以提高这个本就比较复杂的处理函数的可读性。
```suggestion
var searchTerm = args.Parameters[0];
var inst = args.Parameters.ElementAtOrDefault(1);
if (!TryResolveSong(searchTerm, out var filePath, out var songName, out var errorMessage))
{
args.Player.SendErrorMessage(errorMessage);
return;
}
var notes = NoteFileParser.Read(filePath!, out var tempo);
var performer = VirtualPerformer.GetPerformer(inst);
var newSong = new PlaySongInfo(notes, tempo, performer);
songPlayer.EnqueueSong(newSong);
args.Player.SendSuccessMessage(GetString("已将 {0} 加入点歌队列 (当前队列: {1}首)", songName, songPlayer.SongQueue.Count));
```
</issue_to_address>
### Comment 3
<location path="src/MusicPlayer/MusicPlayer.cs" line_range="268-277" />
<code_context>
+ return;
+ }
+
+ if (args.Parameters[0].ToLower() == "play")
+ {
+ if (songPlayer.Listening)
+ {
+ args.Player.SendInfoMessage(GetString("已经在播放中..."));
+ return;
+ }
+ if (songPlayer.SongQueue.Count == 0)
+ {
+ args.Player.SendErrorMessage(GetString("队列为空，无法播放"));
+ return;
+ }
+ songPlayer.PlayNext();
+ return;
+ }
+
+ if (args.Parameters[0].ToLower() == "skip")
+ {
+ if (!songPlayer.Listening)
+ {
+ args.Player.SendErrorMessage(GetString("当前没有正在播放的歌曲"));
+ return;
+ }
+ songPlayer.EndSong(false);
+ args.Player.SendSuccessMessage(GetString("已跳过当前歌曲"));
+ return;
+ }
+
+ if (args.Parameters[0].ToLower() == "global")
+ {
+ if (args.Parameters.Count < 2)
</code_context>
<issue_to_address>
**suggestion:** 可以通过先统一规范子命令字符串，然后使用 switch 或查找表来简化子命令处理逻辑。
当前方法在为每个子命令做判断时，会多次重复调用 `args.Parameters[0].ToLower()`。可以考虑先将其赋值给一个变量（例如 `var sub = args.Parameters[0].ToLowerInvariant();`），然后通过 `switch` 或查找表分发逻辑。这样可以减少重复，并让后续新增子命令时更简单、更不易出错。
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续的 Review。
Original comment in English
Hey - I've found 3 issues, and left some high level feedback:
- In
SongQueueCommand, you repeatedly callargs.Parameters[0].ToLower(), which can be simplified by assigning this once to a localsubcommandvariable to improve readability and avoid redundant work. - When listing the queue (
songqueue list), you only display the note count for each entry; consider including the song name or some identifier to make the queue more informative for users. - In the
globalbranch ofSongQueueCommand, the samePlaySongInfo(andperformer) instance is enqueued for all players; if these types are mutable or track playback state, this shared instance could cause subtle bugs, so consider creating a newPlaySongInfo(and/or performer) per player.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `SongQueueCommand`, you repeatedly call `args.Parameters[0].ToLower()`, which can be simplified by assigning this once to a local `subcommand` variable to improve readability and avoid redundant work.
- When listing the queue (`songqueue list`), you only display the note count for each entry; consider including the song name or some identifier to make the queue more informative for users.
- In the `global` branch of `SongQueueCommand`, the same `PlaySongInfo` (and `performer`) instance is enqueued for all players; if these types are mutable or track playback state, this shared instance could cause subtle bugs, so consider creating a new `PlaySongInfo` (and/or performer) per player.
## Individual Comments
### Comment 1
<location path="src/MusicPlayer/MusicPlayer.cs" line_range="237-242" />
<code_context>
+ private void SongQueueCommand(CommandArgs args)
+ {
+ var songPlayer = SongPlayers[args.Player.Index];
+ if (songPlayer is null) return;
+
+ if (args.Parameters.Count == 0 || args.Parameters[0].ToLower() == "list")
</code_context>
<issue_to_address>
**suggestion:** Consider providing user feedback when there is no SongPlayer instead of returning silently.
When a player runs `/songqueue` before their `SongPlayer` exists, the command exits with no output, which is confusing for users. Consider sending a short info/error message indicating that the song system isn’t initialized yet or that they need to start playing a song first.
```suggestion
private void SongQueueCommand(CommandArgs args)
{
var songPlayer = SongPlayers[args.Player.Index];
if (songPlayer is null)
{
args.Player.SendErrorMessage(GetString("歌曲系统尚未初始化，请先播放一首歌后再使用此命令。"));
return;
}
if (args.Parameters.Count == 0 || args.Parameters[0].ToLower() == "list")
```
</issue_to_address>
### Comment 2
<location path="src/MusicPlayer/MusicPlayer.cs" line_range="334-348" />
<code_context>
+ var searchTerm = args.Parameters[0];
+ var inst = args.Parameters.ElementAtOrDefault(1);
+
+ if (!TryResolveSong(searchTerm, out var fp, out var sn, out var em))
+ {
+ args.Player.SendErrorMessage(em);
+ return;
+ }
+
+ var nts = NoteFileParser.Read(fp!, out var tmp);
+ var perf = VirtualPerformer.GetPerformer(inst);
+
+ var newSong = new PlaySongInfo(nts, tmp, perf);
</code_context>
<issue_to_address>
**suggestion:** Short, non-descriptive variable names reduce readability in this command handler.
In this block, names like `fp`, `sn`, `em`, `nts`, `tmp`, and `perf` make the logic harder to follow. Please use clearer names (e.g., `filePath`, `songName`, `errorMessage`, `notes`, `tempo`, `performer`) to improve readability in an already complex handler.
```suggestion
var searchTerm = args.Parameters[0];
var inst = args.Parameters.ElementAtOrDefault(1);
if (!TryResolveSong(searchTerm, out var filePath, out var songName, out var errorMessage))
{
args.Player.SendErrorMessage(errorMessage);
return;
}
var notes = NoteFileParser.Read(filePath!, out var tempo);
var performer = VirtualPerformer.GetPerformer(inst);
var newSong = new PlaySongInfo(notes, tempo, performer);
songPlayer.EnqueueSong(newSong);
args.Player.SendSuccessMessage(GetString("已将 {0} 加入点歌队列 (当前队列: {1}首)", songName, songPlayer.SongQueue.Count));
```
</issue_to_address>
### Comment 3
<location path="src/MusicPlayer/MusicPlayer.cs" line_range="268-277" />
<code_context>
+ return;
+ }
+
+ if (args.Parameters[0].ToLower() == "play")
+ {
+ if (songPlayer.Listening)
+ {
+ args.Player.SendInfoMessage(GetString("已经在播放中..."));
+ return;
+ }
+ if (songPlayer.SongQueue.Count == 0)
+ {
+ args.Player.SendErrorMessage(GetString("队列为空，无法播放"));
+ return;
+ }
+ songPlayer.PlayNext();
+ return;
+ }
+
+ if (args.Parameters[0].ToLower() == "skip")
+ {
+ if (!songPlayer.Listening)
+ {
+ args.Player.SendErrorMessage(GetString("当前没有正在播放的歌曲"));
+ return;
+ }
+ songPlayer.EndSong(false);
+ args.Player.SendSuccessMessage(GetString("已跳过当前歌曲"));
+ return;
+ }
+
+ if (args.Parameters[0].ToLower() == "global")
+ {
+ if (args.Parameters.Count < 2)
</code_context>
<issue_to_address>
**suggestion:** The subcommand handling could be simplified by normalizing the subcommand once and using a switch or lookup.
This method repeats `args.Parameters[0].ToLower()` across multiple `if` checks for each subcommand. Consider assigning it once (e.g. `var sub = args.Parameters[0].ToLowerInvariant();`) and using a `switch` or lookup to dispatch. This will cut duplication and keep future subcommand additions simpler and less error-prone.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+237
to
+242
| private void SongQueueCommand(CommandArgs args) | ||
| { | ||
| var songPlayer = SongPlayers[args.Player.Index]; | ||
| if (songPlayer is null) return; | ||
|
|
||
| if (args.Parameters.Count == 0 || args.Parameters[0].ToLower() == "list") |
Contributor
There was a problem hiding this comment.
suggestion: 当没有 SongPlayer 时，建议向用户提供反馈，而不是静默返回。
当玩家在其 SongPlayer 尚未创建之前运行 /songqueue 时，命令会直接结束且没有任何输出，这对用户来说会比较困惑。建议发送一个简短的信息/错误提示，说明点歌系统尚未初始化，或者提示他们需要先开始播放一首歌。
Suggested change
| private void SongQueueCommand(CommandArgs args) | |
| { | |
| var songPlayer = SongPlayers[args.Player.Index]; | |
| if (songPlayer is null) return; | |
| if (args.Parameters.Count == 0 || args.Parameters[0].ToLower() == "list") | |
| private void SongQueueCommand(CommandArgs args) | |
| { | |
| var songPlayer = SongPlayers[args.Player.Index]; | |
| if (songPlayer is null) | |
| { | |
| args.Player.SendErrorMessage(GetString("歌曲系统尚未初始化，请先播放一首歌后再使用此命令。")); | |
| return; | |
| } | |
| if (args.Parameters.Count == 0 || args.Parameters[0].ToLower() == "list") |
Original comment in English
suggestion: Consider providing user feedback when there is no SongPlayer instead of returning silently.
When a player runs /songqueue before their SongPlayer exists, the command exits with no output, which is confusing for users. Consider sending a short info/error message indicating that the song system isn’t initialized yet or that they need to start playing a song first.
Suggested change
| private void SongQueueCommand(CommandArgs args) | |
| { | |
| var songPlayer = SongPlayers[args.Player.Index]; | |
| if (songPlayer is null) return; | |
| if (args.Parameters.Count == 0 || args.Parameters[0].ToLower() == "list") | |
| private void SongQueueCommand(CommandArgs args) | |
| { | |
| var songPlayer = SongPlayers[args.Player.Index]; | |
| if (songPlayer is null) | |
| { | |
| args.Player.SendErrorMessage(GetString("歌曲系统尚未初始化，请先播放一首歌后再使用此命令。")); | |
| return; | |
| } | |
| if (args.Parameters.Count == 0 || args.Parameters[0].ToLower() == "list") |
Comment on lines
+334
to
+348
| var searchTerm = args.Parameters[0]; | ||
| var inst = args.Parameters.ElementAtOrDefault(1); | ||
|
|
||
| if (!TryResolveSong(searchTerm, out var fp, out var sn, out var em)) | ||
| { | ||
| args.Player.SendErrorMessage(em); | ||
| return; | ||
| } | ||
|
|
||
| var nts = NoteFileParser.Read(fp!, out var tmp); | ||
| var perf = VirtualPerformer.GetPerformer(inst); | ||
|
|
||
| var newSong = new PlaySongInfo(nts, tmp, perf); | ||
| songPlayer.EnqueueSong(newSong); | ||
| args.Player.SendSuccessMessage(GetString("已将 {0} 加入点歌队列 (当前队列: {1}首)", sn, songPlayer.SongQueue.Count)); |
Contributor
There was a problem hiding this comment.
suggestion: 在这个命令处理器中，过短且缺乏语义的变量名会降低可读性。
在这一段代码中，像 fp、sn、em、nts、tmp 和 perf 这样的名字会让逻辑更难理解。请使用更清晰的命名（例如 filePath、songName、errorMessage、notes、tempo、performer），以提高这个本就比较复杂的处理函数的可读性。
Suggested change
| var searchTerm = args.Parameters[0]; | |
| var inst = args.Parameters.ElementAtOrDefault(1); | |
| if (!TryResolveSong(searchTerm, out var fp, out var sn, out var em)) | |
| { | |
| args.Player.SendErrorMessage(em); | |
| return; | |
| } | |
| var nts = NoteFileParser.Read(fp!, out var tmp); | |
| var perf = VirtualPerformer.GetPerformer(inst); | |
| var newSong = new PlaySongInfo(nts, tmp, perf); | |
| songPlayer.EnqueueSong(newSong); | |
| args.Player.SendSuccessMessage(GetString("已将 {0} 加入点歌队列 (当前队列: {1}首)", sn, songPlayer.SongQueue.Count)); | |
| var searchTerm = args.Parameters[0]; | |
| var inst = args.Parameters.ElementAtOrDefault(1); | |
| if (!TryResolveSong(searchTerm, out var filePath, out var songName, out var errorMessage)) | |
| { | |
| args.Player.SendErrorMessage(errorMessage); | |
| return; | |
| } | |
| var notes = NoteFileParser.Read(filePath!, out var tempo); | |
| var performer = VirtualPerformer.GetPerformer(inst); | |
| var newSong = new PlaySongInfo(notes, tempo, performer); | |
| songPlayer.EnqueueSong(newSong); | |
| args.Player.SendSuccessMessage(GetString("已将 {0} 加入点歌队列 (当前队列: {1}首)", songName, songPlayer.SongQueue.Count)); |
Original comment in English
suggestion: Short, non-descriptive variable names reduce readability in this command handler.
In this block, names like fp, sn, em, nts, tmp, and perf make the logic harder to follow. Please use clearer names (e.g., filePath, songName, errorMessage, notes, tempo, performer) to improve readability in an already complex handler.
Suggested change
| var searchTerm = args.Parameters[0]; | |
| var inst = args.Parameters.ElementAtOrDefault(1); | |
| if (!TryResolveSong(searchTerm, out var fp, out var sn, out var em)) | |
| { | |
| args.Player.SendErrorMessage(em); | |
| return; | |
| } | |
| var nts = NoteFileParser.Read(fp!, out var tmp); | |
| var perf = VirtualPerformer.GetPerformer(inst); | |
| var newSong = new PlaySongInfo(nts, tmp, perf); | |
| songPlayer.EnqueueSong(newSong); | |
| args.Player.SendSuccessMessage(GetString("已将 {0} 加入点歌队列 (当前队列: {1}首)", sn, songPlayer.SongQueue.Count)); | |
| var searchTerm = args.Parameters[0]; | |
| var inst = args.Parameters.ElementAtOrDefault(1); | |
| if (!TryResolveSong(searchTerm, out var filePath, out var songName, out var errorMessage)) | |
| { | |
| args.Player.SendErrorMessage(errorMessage); | |
| return; | |
| } | |
| var notes = NoteFileParser.Read(filePath!, out var tempo); | |
| var performer = VirtualPerformer.GetPerformer(inst); | |
| var newSong = new PlaySongInfo(notes, tempo, performer); | |
| songPlayer.EnqueueSong(newSong); | |
| args.Player.SendSuccessMessage(GetString("已将 {0} 加入点歌队列 (当前队列: {1}首)", songName, songPlayer.SongQueue.Count)); |
Comment on lines
+268
to
+277
| if (args.Parameters[0].ToLower() == "play") | ||
| { | ||
| if (songPlayer.Listening) | ||
| { | ||
| args.Player.SendInfoMessage(GetString("已经在播放中...")); | ||
| return; | ||
| } | ||
| if (songPlayer.SongQueue.Count == 0) | ||
| { | ||
| args.Player.SendErrorMessage(GetString("队列为空，无法播放")); |
Contributor
There was a problem hiding this comment.
suggestion: 可以通过先统一规范子命令字符串，然后使用 switch 或查找表来简化子命令处理逻辑。
当前方法在为每个子命令做判断时，会多次重复调用 args.Parameters[0].ToLower()。可以考虑先将其赋值给一个变量（例如 var sub = args.Parameters[0].ToLowerInvariant();），然后通过 switch 或查找表分发逻辑。这样可以减少重复，并让后续新增子命令时更简单、更不易出错。
Original comment in English
suggestion: The subcommand handling could be simplified by normalizing the subcommand once and using a switch or lookup.
This method repeats args.Parameters[0].ToLower() across multiple if checks for each subcommand. Consider assigning it once (e.g. var sub = args.Parameters[0].ToLowerInvariant();) and using a switch or lookup to dispatch. This will cut duplication and keep future subcommand additions simpler and less error-prone.
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
2 participants
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.
添加插件
更新插件/修复BUG
其他
Summary by Sourcery
提升 MusicPlayer 插件版本，并通过一个新指令新增对按玩家及全局歌曲队列的管理支持。
新功能：
/songqueue指令，用于管理每位玩家的歌曲队列，包括列出、清空、开始播放、跳过以及添加歌曲。增强内容：
VirtualPerformer，无需显式创建绑定到玩家的表演者实例。Original summary in English
Summary by Sourcery
Bump the MusicPlayer plugin version and add support for managing per-player and global song queues via a new command.
New Features:
Enhancements: