store plain lyrics text in id3 tags #27
Conversation
Summary of ChangesHello @RetricSu, 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! This pull request significantly enhances the application's lyrics management by introducing a robust caching mechanism that utilizes ID3 tags within audio files. Upon successful retrieval, lyrics are now automatically embedded into the track's metadata. Consequently, the application will first attempt to load lyrics from these embedded tags, thereby minimizing reliance on external API services and providing a more responsive and offline-capable user experience. Highlights
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
This pull request introduces a great feature for caching lyrics in ID3 tags to avoid repeated API calls. The implementation is solid, but I've identified a couple of issues and areas for improvement. The main concern is a bug where empty lyrics could be written to and read from the cache, which would prevent fetching actual lyrics and result in a poor user experience. I've provided suggestions to fix this by adding checks for empty lyrics. Additionally, I've included a few recommendations to refactor the code for better readability and to adhere to common Rust idioms.
| if let Some(lyrics_frame) = tag.lyrics().next() { | ||
| let text = lyrics_frame.text.clone(); | ||
| tracing::debug!("📖 Found lyrics in ID3 tag ({} characters)", text.len()); | ||
|
|
||
| // Create a basic Lyrics struct from the ID3 lyrics | ||
| // We don't have all the metadata, so we'll create a minimal structure | ||
| Some(Lyrics { | ||
| id: 0, // Not applicable for cached lyrics | ||
| name: "Cached Lyrics".to_string(), | ||
| track_name: title.to_string(), | ||
| artist_name: artist.to_string(), | ||
| album_name: None, | ||
| duration: None, | ||
| instrumental: false, | ||
| plain_lyrics: Some(text), | ||
| synced_lyrics: None, | ||
| lines: Vec::new(), // We could potentially store synced lyrics differently | ||
| }) | ||
| } else { | ||
| tracing::debug!("📖 No lyrics found in ID3 tag"); | ||
| None | ||
| } |
There was a problem hiding this comment.
There's a potential issue here where lyrics text from an ID3 tag could be empty or only contain whitespace. If this happens, the function will return a Lyrics object with empty plain_lyrics. This can lead to showing an empty lyrics panel to the user and preventing a fetch for actual lyrics from the API.
To fix this, you should check if the lyrics text is empty after reading it from the tag. If it is, you should return None, treating it the same as if no lyrics were found.
if let Some(lyrics_frame) = tag.lyrics().next() {
let text = lyrics_frame.text.clone();
if text.trim().is_empty() {
tracing::debug!("📖 Found empty lyrics in ID3 tag, ignoring.");
None
} else {
tracing::debug!("📖 Found lyrics in ID3 tag ({} characters)", text.len());
// Create a basic Lyrics struct from the ID3 lyrics
// We don't have all the metadata, so we'll create a minimal structure
Some(Lyrics {
id: 0, // Not applicable for cached lyrics
name: "Cached Lyrics".to_string(),
track_name: title.to_string(),
artist_name: artist.to_string(),
album_name: None,
duration: None,
instrumental: false,
plain_lyrics: Some(text),
synced_lyrics: None,
lines: Vec::new(), // We could potentially store synced lyrics differently
})
}
} else {
tracing::debug!("📖 No lyrics found in ID3 tag");
None
}| let lyrics_text = if let Some(plain) = &lyrics.plain_lyrics { | ||
| plain.clone() | ||
| } else if let Some(synced) = &lyrics.synced_lyrics { | ||
| // For synced lyrics, we could store them as-is, but for now just store the plain version | ||
| // In a more advanced implementation, we could store synced lyrics in a custom frame | ||
| synced.clone() | ||
| } else { | ||
| tracing::warn!("📝 No lyrics content to write to file"); | ||
| return Ok(()); // Nothing to write | ||
| }; |
There was a problem hiding this comment.
This logic for selecting lyrics to write to the file can be simplified. More importantly, it doesn't prevent writing empty or whitespace-only lyrics to the file, which can cause issues when reading them back from the cache.
I suggest refactoring this to be more concise and to add a check to ensure we don't write empty lyrics.
| let lyrics_text = if let Some(plain) = &lyrics.plain_lyrics { | |
| plain.clone() | |
| } else if let Some(synced) = &lyrics.synced_lyrics { | |
| // For synced lyrics, we could store them as-is, but for now just store the plain version | |
| // In a more advanced implementation, we could store synced lyrics in a custom frame | |
| synced.clone() | |
| } else { | |
| tracing::warn!("📝 No lyrics content to write to file"); | |
| return Ok(()); // Nothing to write | |
| }; | |
| let lyrics_text = match lyrics.plain_lyrics.as_ref().or(lyrics.synced_lyrics.as_ref()) { | |
| Some(text) if !text.trim().is_empty() => text.clone(), | |
| _ => { | |
| tracing::warn!("📝 No lyrics content or empty lyrics, not writing to file"); | |
| return Ok(()); // Nothing to write | |
| } | |
| }; |
| if let Some(lyrics_data) = &self.current_lyrics { | ||
| if let Some(player) = &self.player { | ||
| if let Some(track) = &player.selected_track { | ||
| if let Err(e) = | ||
| crate::app::lyrics::LyricsService::write_lyrics_to_file( | ||
| track.path(), | ||
| lyrics_data, | ||
| ) | ||
| { | ||
| tracing::warn!("⚠️ Failed to cache lyrics in ID3 tag: {}", e); | ||
| } else { | ||
| tracing::info!("📝 Successfully cached lyrics in ID3 tag"); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The nested if let statements here can be flattened to improve readability. You can use Option::and_then to chain the checks for player and selected_track, which makes the code more concise and less nested.
if let Some(lyrics_data) = &self.current_lyrics {
if let Some(track) = self.player.as_ref().and_then(|p| p.selected_track.as_ref()) {
if let Err(e) =
crate::app::lyrics::LyricsService::write_lyrics_to_file(
track.path(),
lyrics_data,
)
{
tracing::warn!("⚠️ Failed to cache lyrics in ID3 tag: {}", e);
} else {
tracing::info!("📝 Successfully cached lyrics in ID3 tag");
}
}
}| use id3::frame::Lyrics; | ||
| tag.add_frame(Lyrics { | ||
| lang: "eng".to_string(), | ||
| description: "Lyrics".to_string(), | ||
| text: lyrics_text, | ||
| }); |
There was a problem hiding this comment.
Using a use statement inside a function is generally discouraged in Rust as it can make it harder to see where a type is coming from. It's better to qualify the type directly at the point of use.
In this case, since Lyrics is also the name of a struct in this module and a parameter to the function, qualifying it with id3::frame::Lyrics makes the code clearer.
tag.add_frame(id3::frame::Lyrics {
lang: "eng".to_string(),
description: "Lyrics".to_string(),
text: lyrics_text,
});
No description provided.