Add lyrics #26
Add lyrics #26
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 introduces a new lyrics display feature, allowing users to view synchronized or plain lyrics for the currently playing track. It integrates an asynchronous lyrics fetching service with the application's UI and player controls, enhancing the user experience by providing immediate access to song lyrics. 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 successfully implements a lyrics fetching and display feature. It introduces a LyricsService that runs in a background thread to fetch lyrics from an external API without blocking the UI, and a new LyricsComponent to display them. The changes are well-integrated across the application, with lyrics being fetched automatically on track changes. My review focuses on improving code clarity, maintainability by reducing duplication, and ensuring the UI respects theming conventions.
| if let Some(lyrics_data) = &self.current_lyrics { | ||
| if lyrics_data.instrumental | ||
| || !lyrics_data.lines.is_empty() | ||
| || lyrics_data.plain_lyrics.is_some() | ||
| { | ||
| self.show_lyrics_panel = true; | ||
| } | ||
| } | ||
| if let Some(lyrics_data) = &self.current_lyrics { | ||
| if lyrics_data.instrumental { | ||
| tracing::info!("✅ Found instrumental track"); | ||
| } else if !lyrics_data.lines.is_empty() | ||
| || lyrics_data.plain_lyrics.is_some() | ||
| { | ||
| tracing::info!("✅ Lyrics loaded successfully"); | ||
| } else { | ||
| tracing::warn!("⚠️ Lyrics record found but no content available"); | ||
| } | ||
| } else { | ||
| tracing::warn!("❌ No lyrics found"); | ||
| } |
There was a problem hiding this comment.
The logic to handle the received lyrics can be simplified by merging the two if let Some(lyrics_data) = &self.current_lyrics blocks. This improves readability and reduces redundancy.
if let Some(lyrics_data) = &self.current_lyrics {
let has_content = lyrics_data.instrumental
|| !lyrics_data.lines.is_empty()
|| lyrics_data.plain_lyrics.is_some();
if has_content {
self.show_lyrics_panel = true;
if lyrics_data.instrumental {
tracing::info!("✅ Found instrumental track");
} else {
tracing::info!("✅ Lyrics loaded successfully");
}
} else {
tracing::warn!("⚠️ Lyrics record found but no content available");
}
} else {
tracing::warn!("❌ No lyrics found");
}| ui.add(egui::Label::new( | ||
| egui::RichText::new("♪ Instrumental ♪") | ||
| .color(egui::Color32::from_rgb(150, 150, 150)) | ||
| .size(16.0) | ||
| .italics(), | ||
| )); |
There was a problem hiding this comment.
Hardcoding colors like egui::Color32::from_rgb(150, 150, 150) and egui::Color32::BLACK prevents the UI from adapting to different themes (e.g., a dark theme). It's better to use theme-aware colors from ui.style().visuals. For example, use ui.style().visuals.text_color() for standard text and ui.style().visuals.weak_text_color() for subdued text. This applies to several places in this file.
| ui.add(egui::Label::new( | |
| egui::RichText::new("♪ Instrumental ♪") | |
| .color(egui::Color32::from_rgb(150, 150, 150)) | |
| .size(16.0) | |
| .italics(), | |
| )); | |
| ui.add(egui::Label::new( | |
| egui::RichText::new("♪ Instrumental ♪") | |
| .color(ui.style().visuals.weak_text_color()) | |
| .size(16.0) | |
| .italics(), | |
| )); |
| let label = if line.text.trim().is_empty() { | ||
| "♪".to_string() | ||
| } else { | ||
| line.text.clone() | ||
| }; |
There was a problem hiding this comment.
In the loop for displaying synced lyrics, line.text.clone() creates a new String on every iteration. This is inefficient. You can avoid the allocation by passing a string slice &line.text to egui::RichText::new(), as it accepts any type that implements Into<WidgetText>.
| let label = if line.text.trim().is_empty() { | |
| "♪".to_string() | |
| } else { | |
| line.text.clone() | |
| }; | |
| let label: &str = if line.text.trim().is_empty() { | |
| "♪" | |
| } else { | |
| &line.text | |
| }; |
| let search_url = format!( | ||
| "https://lrclib.net/api/search?artist_name={}&track_name={}", | ||
| urlencoding::encode(artist), | ||
| urlencoding::encode(title) | ||
| ); |
There was a problem hiding this comment.
The API URL is hardcoded inside the fetch_from_api function. It's good practice to define this as a constant at the module level. This makes it easier to find and change if the API endpoint ever moves.
For example, you could add this at the top of the file:
const LRC_API_SEARCH_URL: &str = "https://lrclib.net/api/search";And then use it here.
No description provided.