Enhance SongPlayer with queue management features#1110
Enhance SongPlayer with queue management features#1110star620 wants to merge 2 commits intoUnrealMultiple:masterfrom
Conversation
There was a problem hiding this comment.
Hey - 我发现了 1 个问题,并给出了一些整体性的反馈:
EndSong(false)和PlayNext()都会修改Listening并调用MusicPlayer.ListeningCheck(),这可能导致重复的状态更新和检查;建议把这部分逻辑集中到一个地方,避免重复调用,并保持队列结束时的行为一致。SongQueue目前是一个可变的公共字段;将其改为私有,只能通过EnqueueSong/PlayNext(或只读属性)访问,可以防止外部代码以破坏自动播放逻辑的方式修改队列。
给 AI 代理的提示
请根据这次代码评审的意见进行修改:
## 整体意见
- `EndSong(false)` 和 `PlayNext()` 都会修改 `Listening` 并调用 `MusicPlayer.ListeningCheck()`,这可能导致重复的状态更新和检查;建议把这部分逻辑集中到一个地方,避免重复调用,并保持队列结束时的行为一致。
- `SongQueue` 目前是一个可变的公共字段;将其改为私有,只能通过 `EnqueueSong`/`PlayNext`(或只读属性)访问,可以防止外部代码以破坏自动播放逻辑的方式修改队列。
## 单独评论
### 评论 1
<location path="src/MusicPlayer/SongPlayer.cs" line_range="13" />
<code_context>
public bool Listening { get; private set; }
public TSPlayer Player { get; set; }
public PlaySongInfo? CurrentSong { get; private set; }
+ public Queue<PlaySongInfo> SongQueue = new();
public SongPlayer(TSPlayer ply)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** 通过方法或只读封装来暴露队列,而不是通过可变的公共字段。
将 `SongQueue` 暴露为公共且可变的字段,会让调用方在 `SongPlayer` 的控制之外执行 `Clear`/`Enqueue`/`Dequeue`,从而可能导致 `Listening`/`CurrentSong` 与实际队列状态不同步。更推荐使用 `private` 队列(或只读视图),并通过 `EnqueueSong`/`PlayNext` 风格的方法来完成所有修改,以保持不变式。
建议的实现:
```csharp
public bool Listening { get; private set; }
public TSPlayer Player { get; set; }
public PlaySongInfo? CurrentSong { get; private set; }
private readonly Queue<PlaySongInfo> _songQueue = new();
public IReadOnlyCollection<PlaySongInfo> SongQueue => _songQueue;
```
```csharp
public SongPlayer(TSPlayer ply)
{
this.Listening = false;
}
public void EnqueueSong(PlaySongInfo playSongInfo)
{
if (playSongInfo == null)
throw new ArgumentNullException(nameof(playSongInfo));
_songQueue.Enqueue(playSongInfo);
}
public bool TryDequeueNext(out PlaySongInfo? nextSong)
{
if (_songQueue.Count == 0)
{
nextSong = null;
return false;
}
nextSong = _songQueue.Dequeue();
return true;
}
```
1. 确保文件顶部包含 `using System;`,以便使用 `ArgumentNullException`。
2. 将现有任何对 `SongQueue.Enqueue/Dequeue/Clear` 的外部调用更新为使用 `EnqueueSong` 和 `TryDequeueNext`(如有需要,也可以添加 `ClearQueue()` 等辅助方法),以确保没有代码直接修改 `_songQueue`。
3. 如果存在依赖清空队列或查看下一首歌曲的逻辑,建议添加 `ClearQueue()` 和/或 `PeekNextSong()` 包装方法,而不是直接访问队列。
</issue_to_address>帮我变得更有用!请在每条评论上点击 👍 或 👎,我会根据这些反馈改进后续评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
EndSong(false)andPlayNext()both manipulateListeningand callMusicPlayer.ListeningCheck(), which can cause duplicate state updates and checks; consider centralizing this logic in one place to avoid double-calls and keep the end-of-queue behavior consistent.SongQueueis currently a public mutable field; making it private and only accessible viaEnqueueSong/PlayNext(or a read-only property) would help prevent external code from mutating the queue in ways that break the auto-play logic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- `EndSong(false)` and `PlayNext()` both manipulate `Listening` and call `MusicPlayer.ListeningCheck()`, which can cause duplicate state updates and checks; consider centralizing this logic in one place to avoid double-calls and keep the end-of-queue behavior consistent.
- `SongQueue` is currently a public mutable field; making it private and only accessible via `EnqueueSong`/`PlayNext` (or a read-only property) would help prevent external code from mutating the queue in ways that break the auto-play logic.
## Individual Comments
### Comment 1
<location path="src/MusicPlayer/SongPlayer.cs" line_range="13" />
<code_context>
public bool Listening { get; private set; }
public TSPlayer Player { get; set; }
public PlaySongInfo? CurrentSong { get; private set; }
+ public Queue<PlaySongInfo> SongQueue = new();
public SongPlayer(TSPlayer ply)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Expose the queue via methods or a read-only wrapper instead of a mutable public field.
Exposing `SongQueue` as a public, mutable field lets callers `Clear`/`Enqueue`/`Dequeue` outside `SongPlayer`’s control, which can desynchronize `Listening`/`CurrentSong` from the actual queue. Prefer a `private` queue (or a read-only view) and route all modifications through `EnqueueSong`/`PlayNext`-style methods to preserve invariants.
Suggested implementation:
```csharp
public bool Listening { get; private set; }
public TSPlayer Player { get; set; }
public PlaySongInfo? CurrentSong { get; private set; }
private readonly Queue<PlaySongInfo> _songQueue = new();
public IReadOnlyCollection<PlaySongInfo> SongQueue => _songQueue;
```
```csharp
public SongPlayer(TSPlayer ply)
{
this.Listening = false;
}
public void EnqueueSong(PlaySongInfo playSongInfo)
{
if (playSongInfo == null)
throw new ArgumentNullException(nameof(playSongInfo));
_songQueue.Enqueue(playSongInfo);
}
public bool TryDequeueNext(out PlaySongInfo? nextSong)
{
if (_songQueue.Count == 0)
{
nextSong = null;
return false;
}
nextSong = _songQueue.Dequeue();
return true;
}
```
1. Ensure `using System;` is present at the top of the file for `ArgumentNullException`.
2. Update any existing external usages of `SongQueue.Enqueue/Dequeue/Clear` to use `EnqueueSong` and `TryDequeueNext` (or additional helper methods like `ClearQueue()` if needed), so no code mutates `_songQueue` directly.
3. If there is logic that depends on clearing or peeking at the next song, consider adding `ClearQueue()` and/or `PeekNextSong()` wrapper methods instead of accessing the queue directly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| public bool Listening { get; private set; } | ||
| public TSPlayer Player { get; set; } | ||
| public PlaySongInfo? CurrentSong { get; private set; } | ||
| public Queue<PlaySongInfo> SongQueue = new(); |
There was a problem hiding this comment.
suggestion (bug_risk): 通过方法或只读封装来暴露队列,而不是通过可变的公共字段。
将 SongQueue 暴露为公共且可变的字段,会让调用方在 SongPlayer 的控制之外执行 Clear/Enqueue/Dequeue,从而可能导致 Listening/CurrentSong 与实际队列状态不同步。更推荐使用 private 队列(或只读视图),并通过 EnqueueSong/PlayNext 风格的方法来完成所有修改,以保持不变式。
建议的实现:
public bool Listening { get; private set; }
public TSPlayer Player { get; set; }
public PlaySongInfo? CurrentSong { get; private set; }
private readonly Queue<PlaySongInfo> _songQueue = new();
public IReadOnlyCollection<PlaySongInfo> SongQueue => _songQueue; public SongPlayer(TSPlayer ply)
{
this.Listening = false;
}
public void EnqueueSong(PlaySongInfo playSongInfo)
{
if (playSongInfo == null)
throw new ArgumentNullException(nameof(playSongInfo));
_songQueue.Enqueue(playSongInfo);
}
public bool TryDequeueNext(out PlaySongInfo? nextSong)
{
if (_songQueue.Count == 0)
{
nextSong = null;
return false;
}
nextSong = _songQueue.Dequeue();
return true;
}- 确保文件顶部包含
using System;,以便使用ArgumentNullException。 - 将现有任何对
SongQueue.Enqueue/Dequeue/Clear的外部调用更新为使用EnqueueSong和TryDequeueNext(如有需要,也可以添加ClearQueue()等辅助方法),以确保没有代码直接修改_songQueue。 - 如果存在依赖清空队列或查看下一首歌曲的逻辑,建议添加
ClearQueue()和/或PeekNextSong()包装方法,而不是直接访问队列。
Original comment in English
suggestion (bug_risk): Expose the queue via methods or a read-only wrapper instead of a mutable public field.
Exposing SongQueue as a public, mutable field lets callers Clear/Enqueue/Dequeue outside SongPlayer’s control, which can desynchronize Listening/CurrentSong from the actual queue. Prefer a private queue (or a read-only view) and route all modifications through EnqueueSong/PlayNext-style methods to preserve invariants.
Suggested implementation:
public bool Listening { get; private set; }
public TSPlayer Player { get; set; }
public PlaySongInfo? CurrentSong { get; private set; }
private readonly Queue<PlaySongInfo> _songQueue = new();
public IReadOnlyCollection<PlaySongInfo> SongQueue => _songQueue; public SongPlayer(TSPlayer ply)
{
this.Listening = false;
}
public void EnqueueSong(PlaySongInfo playSongInfo)
{
if (playSongInfo == null)
throw new ArgumentNullException(nameof(playSongInfo));
_songQueue.Enqueue(playSongInfo);
}
public bool TryDequeueNext(out PlaySongInfo? nextSong)
{
if (_songQueue.Count == 0)
{
nextSong = null;
return false;
}
nextSong = _songQueue.Dequeue();
return true;
}- Ensure
using System;is present at the top of the file forArgumentNullException. - Update any existing external usages of
SongQueue.Enqueue/Dequeue/Clearto useEnqueueSongandTryDequeueNext(or additional helper methods likeClearQueue()if needed), so no code mutates_songQueuedirectly. - If there is logic that depends on clearing or peeking at the next song, consider adding
ClearQueue()and/orPeekNextSong()wrapper methods instead of accessing the queue directly.
SongPlayer.cs:添加 Queue 队列字段,新增 EnqueueSong 、 PlayNext 方法,并修改 StartSong 和 EndSong 以支持队列自动播放逻辑。
添加插件
更新插件/修复BUG
其他
Summary by Sourcery
为音乐播放器添加歌曲队列管理,并将其与歌曲开始/结束流程集成。
New Features:
Enhancements:
Original summary in English
Summary by Sourcery
Add song queue management to the music player and integrate it with song start/end flow.
New Features:
Enhancements: