Repository navigation
Conversation
Summary of ChangesHello @kazukokawagawa, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求的核心目标是通过将文件下载和音乐元数据写入功能迁移到 Rust 原生模块来显著提升应用程序的性能和可靠性。这一重构将原本由 Node.js 处理的 I/O 密集型任务转移到更高效的 Rust 代码中，从而优化了文件操作的效率和稳定性。同时，还对前端界面进行了小幅调整，以适应新的封面显示设置。 Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
这次重构将下载和元数据写入的核心逻辑从 TypeScript 迁移到了 Rust 原生模块，这是一个很棒的改进，可以带来显著的性能提升和更好的并发控制。Rust 代码的实现质量很高，特别是在异步处理和取消逻辑方面。
我发现了一些可以改进的地方：
- TypeScript 代码中存在一个重复的文件扩展名。
- 一些用于调试的
console.log语句应该被移除。 - 取消下载的错误处理机制可以更健壮一些。
- Rust 代码中有几处可以改进的地方，包括使用更安全的错误处理代替
.unwrap()，以及使用结构化日志代替println!。
总的来说，这是一个很棒的 PR，在解决上述问题后，代码质量会更高。
| } else { | ||
| let tag_type = tagged_file.primary_tag_type(); | ||
| tagged_file.insert_tag(Tag::new(tag_type)); | ||
| tagged_file.primary_tag_mut().unwrap() |
There was a problem hiding this comment.
使用 .unwrap() 可能会在前面的操作返回 None 时导致程序崩溃 (panic)。虽然在调用 insert_tag 之后 primary_tag_mut() 预期会返回 Some，但显式处理 Option 会更安全，可以防止潜在的崩溃。建议使用 if let 或 match 语句进行更健壮的错误处理。
if let Some(tag) = tagged_file.primary_tag_mut() {
tag
} else {
// 如果 insert_tag 按预期工作，这个路径理论上不会被走到。
// 但返回错误比 panic 更安全。
return Err(Error::from_reason("Failed to create a new tag"));
}| "aiff", | ||
| "aif", | ||
| "aifc", | ||
| "aifc", |
There was a problem hiding this comment.
文件扩展名 aifc 在 musicExtensions 数组中重复了。这可能是复制粘贴错误，应该修正以避免冗余处理。
| console.log("[Download] Preparing metadata for:", songData.name); | ||
| console.log("[Download] songData cover info:", { | ||
| cover: songData.cover, | ||
| coverSize: songData.coverSize, | ||
| downloadCover | ||
| }); | ||
|
|
||
| const coverUrl = (downloadCover && (songData.coverSize?.l || songData.cover)) ? (songData.coverSize?.l || songData.cover) : undefined; | ||
|
|
||
| metadata = { | ||
| title: songData.name || "未知曲目", | ||
| artist: artist, | ||
| album: (typeof songData.album === "string" ? songData.album : songData.album?.name) || "未知专辑", | ||
| coverUrl: coverUrl, | ||
| lyric: (downloadLyric && lyric) ? lyric : undefined, | ||
| description: songData.alia || "", | ||
| }; | ||
| console.log("[Download] Resolved cover URL:", coverUrl); |
There was a problem hiding this comment.
这些 console.log 语句看起来是用于调试的。虽然在开发过程中很有用，但在合并到主分支之前，应该将它们移除或替换为正式的日志记录（例如 ipcLog），以避免在生产环境中输出不必要的控制台信息。
| if (error.message && error.message.includes("cancelled")) { | ||
| return { status: "cancelled", message: "下载已取消" }; | ||
| } |
There was a problem hiding this comment.
通过检查错误消息字符串（.includes("cancelled")）来处理下载取消操作是不够健壮的。如果将来原生模块的错误消息发生变化，这个逻辑就会失效。更稳妥的方法是让原生模块为取消操作返回一个特定的错误码或结构化错误，这样 TypeScript 这边就能进行更可靠的错误处理。
| println!("Start downloading: {} -> {}", url, file_path); | ||
|
|
||
| // Check cancellation | ||
| if token.is_cancelled() { | ||
| return Err(Error::from_reason("Download cancelled")); | ||
| } | ||
|
|
||
| let client = reqwest::Client::builder() | ||
| .user_agent("SPlayer/1.0") | ||
| .build() | ||
| .map_err(|e| Error::from_reason(e.to_string()))?; | ||
|
|
||
| let response = client.get(&url) | ||
| .send() | ||
| .await | ||
| .map_err(|e| Error::from_reason(e.to_string()))?; | ||
|
|
||
| let total_size = response.content_length().unwrap_or(0); | ||
|
|
||
| // Create file | ||
| let mut file = tokio::fs::File::create(&file_path) | ||
| .await | ||
| .map_err(|e| Error::from_reason(e.to_string()))?; | ||
|
|
||
| let mut stream = response.bytes_stream(); | ||
| let mut downloaded: u64 = 0; | ||
| let mut last_progress_time = std::time::Instant::now(); | ||
| let mut last_percent = 0.0; | ||
|
|
||
| while let Some(item) = tokio::select! { | ||
| _ = token.cancelled() => None, | ||
| item = stream.next() => item, | ||
| } { | ||
| let chunk = item.map_err(|e| Error::from_reason(e.to_string()))?; | ||
| file.write_all(&chunk).await.map_err(|e| Error::from_reason(e.to_string()))?; | ||
| downloaded += chunk.len() as u64; | ||
|
|
||
| if total_size > 0 { | ||
| let percent = downloaded as f64 / total_size as f64; | ||
| let now = std::time::Instant::now(); | ||
|
|
||
| // Throttle: update if > 1% change or > 1 second passed, or complete | ||
| if percent - last_percent >= 0.01 || now.duration_since(last_progress_time).as_millis() > 500 || percent >= 1.0 { | ||
| let json = format!( | ||
| "{{\"percent\": {:.4}, \"transferredBytes\": {}, \"totalBytes\": {}}}", | ||
| percent, downloaded, total_size | ||
| ); | ||
| on_progress.call(Ok(json), ThreadsafeFunctionCallMode::NonBlocking); | ||
| last_progress_time = now; | ||
| last_percent = percent; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if token.is_cancelled() { | ||
| println!("Download cancelled: {}", file_path); | ||
| drop(file); | ||
| let _ = tokio::fs::remove_file(&file_path).await; | ||
| return Err(Error::from_reason("Download cancelled")); | ||
| } | ||
|
|
||
| file.flush() | ||
| .await | ||
| .map_err(|e| Error::from_reason(e.to_string()))?; | ||
| drop(file); // Close file so we can reopen it for metadata | ||
| println!("Download complete: {}", file_path); | ||
|
|
||
| // Metadata | ||
| if let Some(meta) = metadata { | ||
| println!("Processing metadata for: {}", meta.title); | ||
| // Download cover | ||
| let cover_data = if let Some(cover_url) = &meta.cover_url { | ||
| if !cover_url.is_empty() { | ||
| println!("Downloading cover: {}", cover_url); | ||
| match client.get(cover_url).send().await { | ||
| Ok(resp) => { | ||
| if resp.status().is_success() { | ||
| match resp.bytes().await { | ||
| Ok(b) => { | ||
| println!("Cover downloaded, size: {}", b.len()); | ||
| Some(b) | ||
| } | ||
| Err(e) => { | ||
| println!("Failed to read cover bytes: {}", e); | ||
| None | ||
| } | ||
| } | ||
| } else { | ||
| println!("Cover download failed with status: {}", resp.status()); | ||
| None | ||
| } | ||
| } | ||
| Err(e) => { | ||
| println!("Failed to download cover: {}", e); | ||
| None | ||
| } | ||
| } | ||
| } else { | ||
| println!("Cover URL is empty string"); | ||
| None | ||
| } | ||
| } else { | ||
| println!("No cover URL provided in metadata"); | ||
| None | ||
| }; | ||
|
|
||
| // Write tags using lofty | ||
| let path_clone = file_path.clone(); | ||
|
|
||
| tokio::task::spawn_blocking(move || { | ||
| write_metadata(&path_clone, meta, cover_data) | ||
| .map_err(|e| Error::from_reason(e.to_string())) | ||
| }) | ||
| .await | ||
| .map_err(|e| Error::from_reason(e.to_string()))??; | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| fn write_metadata(path: &str, meta: SongMetadata, cover_data: Option<bytes::Bytes>) -> Result<()> { | ||
| println!("Writing metadata to: {}", path); | ||
| let path_obj = Path::new(path); | ||
| let mut tagged_file = Probe::open(path_obj) | ||
| .map_err(|e| Error::from_reason(format!("Failed to open file for tagging: {}", e)))? | ||
| .read() | ||
| .map_err(|e| Error::from_reason(format!("Failed to read tags: {}", e)))?; | ||
|
|
||
| let tag = match tagged_file.primary_tag_mut() { | ||
| Some(primary_tag) => primary_tag, | ||
| None => { | ||
| if let Some(first_tag) = tagged_file.first_tag_mut() { | ||
| first_tag | ||
| } else { | ||
| let tag_type = tagged_file.primary_tag_type(); | ||
| tagged_file.insert_tag(Tag::new(tag_type)); | ||
| tagged_file.primary_tag_mut().unwrap() | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| tag.set_title(meta.title.clone()); | ||
| tag.set_artist(meta.artist.clone()); | ||
| tag.set_album(meta.album.clone()); | ||
|
|
||
| if let Some(desc) = meta.description.clone() { | ||
| tag.set_comment(desc); | ||
| } | ||
|
|
||
| if let Some(lyric) = meta.lyric.clone() { | ||
| tag.insert_text(ItemKey::Lyrics, lyric); | ||
| } | ||
|
|
||
| if let Some(data) = cover_data { | ||
| println!("Embedding cover art..."); | ||
| let mime_type = if data.starts_with(&[0xFF, 0xD8, 0xFF]) { | ||
| MimeType::Jpeg | ||
| } else if data.starts_with(&[0x89, 0x50, 0x4E, 0x47]) { | ||
| MimeType::Png | ||
| } else { | ||
| MimeType::Jpeg | ||
| }; | ||
|
|
||
| let picture = Picture::new_unchecked( | ||
| PictureType::CoverFront, | ||
| Some(mime_type), | ||
| None, | ||
| data.to_vec(), | ||
| ); | ||
| tag.push_picture(picture); | ||
| } | ||
|
|
||
| tagged_file | ||
| .save_to_path(path_obj, WriteOptions::default()) | ||
| .map_err(|e| Error::from_reason(format!("Failed to save tags: {}", e)))?; | ||
|
|
||
| println!("Metadata written successfully"); | ||
| Ok(()) |
There was a problem hiding this comment.
代码在下载和写入元数据的过程中使用了 println! 进行日志记录。虽然这对于调试很方便，但在生产环境中可能会有问题，因为它直接写入 stdout，并且缺乏日志级别和结构。建议使用像 tracing 这样的结构化日志框架。这可以提供可配置的日志级别、格式和输出，使生产环境中的调试更加容易。
|
/gemini review |
There was a problem hiding this comment.
Code Review
这次重构工作非常出色，将核心的下载逻辑从 TypeScript 迁移到了 Rust，带来了显著的性能提升和更好的多线程支持。新的 Rust 实现考虑了多线程下载、动态分块以及完善的元数据处理，代码质量很高。
除了核心重构，本次提交还包含了一些有用的功能增强和 Bug 修复，例如：
- 更精确的歌词时间戳解析。
- 支持合并翻译和音译歌词到主歌词文件。
- 新增了下载线程数和歌词动画的设置选项。
整体来看，这是一次高质量的提交。我只发现了一些小问题，并在代码中提出了具体的修改建议。
| pub fn write_music_metadata( | ||
| file_path: String, | ||
| metadata: SongMetadata, | ||
| cover_path: Option<String>, | ||
| ) -> Result<()> { | ||
| let cover_data = if let Some(path) = cover_path { | ||
| match std::fs::read(&path) { | ||
| Ok(bytes) => Some(bytes::Bytes::from(bytes)), | ||
| Err(_) => None, | ||
| } | ||
| } else { | ||
| None | ||
| }; | ||
| write_metadata(&file_path, metadata, cover_data) | ||
| } |
There was a problem hiding this comment.
write_music_metadata 函数是同步的，但它内部执行了阻塞式的文件 I/O 操作（例如 std::fs::read 和 lofty 的文件读写）。虽然 napi-rs 会将同步函数放在 libuv 工作线程上执行，不会阻塞主事件循环，但更好的实践是将 I/O 密集型函数显式声明为 async，以保持代码库风格的一致性，并能更好地利用 tokio 的异步能力。
建议将 write_music_metadata 函数修改为 async 函数。这样可以：
- 使用
tokio::fs::read异步读取封面文件。 - 将同步的
lofty操作包裹在tokio::task::spawn_blocking中，明确地将其放在阻塞线程池中执行，避免意外阻塞异步运行时。
注意：采纳此建议后，需要在 TypeScript 调用侧（electron/main/ipc/ipc-file.ts 的 set-music-metadata 处理器中）相应地使用 await 来调用此函数：
await tools.writeMusicMetadata(songPath, meta, coverPath);pub async fn write_music_metadata(
file_path: String,
metadata: SongMetadata,
cover_path: Option<String>,
) -> Result<()> {
let cover_data = if let Some(path) = cover_path {
match tokio::fs::read(&path).await {
Ok(bytes) => Some(bytes::Bytes::from(bytes)),
Err(_) => None,
}
} else {
None
};
tokio::task::spawn_blocking(move || {
write_metadata(&file_path, metadata, cover_data)
})
.await
.map_err(|e| Error::from_reason(e.to_string()))??;
Ok(())
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
这次重构非常出色，将核心的下载逻辑从 TypeScript 迁移到了原生的 Rust 模块。这是一个巨大的进步，显著提升了性能并引入了多线程下载等新功能。
主要亮点包括：
- 性能提升：通过使用 Rust 和多线程，下载速度和元数据处理效率应该会有很大提升。
- 代码质量：Rust 代码结构清晰，遵循了异步编程和错误处理的最佳实践。例如，在下载时对
HEAD和Range请求的健壮处理，以及在写入元数据时使用spawn_blocking避免阻塞事件循环。 - 功能增强：
- 实现了多线程下载，并提供了可配置的线程数。
- 增强了歌词处理，现在能够将翻译和音译歌词合并到主歌词文件中，并支持生成 TTML 格式。
- UI 改进：添加了新的设置项（如下载线程数、歌词切换动画）和一些小的 UI 优化，这些都很好地与新功能集成。
我发现了一些小问题，主要是关于代码清理的，已经在具体的 review comments 中提出。总的来说，这是一次高质量的重构，做得很好！
| "aiff", | ||
| "aif", | ||
| "aifc", | ||
| "aifc", |
There was a problem hiding this comment.
aifc 这个文件扩展名在这里重复了。建议移除多余的一行，保持数组整洁。
| // Log for debugging cover issue | ||
| console.log("[Download] Preparing metadata for:", songData.name); | ||
| console.log("[Download] songData cover info:", { | ||
| cover: songData.cover, | ||
| coverSize: songData.coverSize, | ||
| downloadCover | ||
| }); |
There was a problem hiding this comment.
这几行 console.log 看起来是用于调试的。为了保持生产环境日志的整洁，建议在合并前将它们移除。
2 participants
No description provided.