More lyrics feature #29
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 upgrades the application's lyrics functionality, focusing on improving the user experience for displaying, synchronizing, and managing lyrics. Key changes include real-time synced lyrics updates with highlighting and auto-scrolling, a new dedicated lyrics column in the playlist for better visibility and management, and more accurate audio timestamp processing to ensure precise lyric synchronization. The underlying lyrics storage and retrieval mechanisms have also been refined to handle synced lyrics more effectively. 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 several valuable enhancements to the lyrics feature, such as support for synced lyrics, a lyrics type indicator, and the ability to remove lyrics. The UI responsiveness improvements for synced lyrics are also a great addition. However, I've identified some significant performance issues and a few bugs that need to be addressed. The most critical issue is a performance bottleneck in the playlist view, which performs file I/O for every track on every frame, likely causing UI freezes. There are also bugs related to playlist column configuration and track duration calculation that could impact user experience. I've provided detailed comments and suggestions for resolving these issues.
| let has_lyrics = crate::app::lyrics::LyricsService::read_lyrics_from_file( | ||
| track.path(), | ||
| &track_artist, | ||
| &track_title, | ||
| ) | ||
| .is_some(); |
There was a problem hiding this comment.
This code calls read_lyrics_from_file for every track in the playlist on every single UI frame. This performs repeated file I/O on the main UI thread, which is a major performance bottleneck. It will cause the UI to freeze or stutter, especially with larger playlists.
The lyrics status should be determined once when the track is added to the library and stored as a property on the LibraryItem. The UI should then just read this boolean flag.
Here's a suggested approach:
- Add a
has_lyrics: boolfield to theLibraryItemstruct. - When scanning music files and creating
LibraryItems, check for lyrics and set this flag. This can be done by checking for the presence of the lyrics tag without reading the full content. - Store this flag in the database along with other track metadata.
- In this component, simply read the
track.has_lyricsboolean instead of performing file I/O.
| let column_proportions = [0.05, 0.30, 0.18, 0.20, 0.12, 0.15]; | ||
| let num_columns = 5; // Changed from 6 to 5 (we don't need empty columns) |
There was a problem hiding this comment.
The num_columns is hardcoded to 5, but you've added a new "Lyrics" column, making the total 6. This will cause layout issues in the egui::Grid as it's expecting a different number of columns than what is being provided per row. The comment on this line is also now incorrect and misleading.
| let column_proportions = [0.05, 0.30, 0.18, 0.20, 0.12, 0.15]; | |
| let num_columns = 5; // Changed from 6 to 5 (we don't need empty columns) | |
| let column_proportions = [0.05, 0.30, 0.18, 0.20, 0.12, 0.15]; | |
| let num_columns = column_proportions.len(); | |
| if ui.small_button("❌").on_hover_text(t("remove_lyrics")).clicked() { | ||
| // Remove lyrics from the file | ||
| if let Err(e) = crate::app::lyrics::LyricsService::remove_lyrics_from_file(track.path()) { | ||
| tracing::error!("Failed to remove lyrics from file: {}", e); | ||
| } else { | ||
| tracing::info!("Successfully removed lyrics from file: {:?}", track.path()); | ||
| } | ||
| } |
There was a problem hiding this comment.
The remove_lyrics_from_file function is called directly within the UI update loop when the button is clicked. This performs a file write operation on the UI thread, which can block it and make the application unresponsive, especially with slow storage or network drives. This operation should be moved to a background thread, similar to how other long-running tasks are handled in the application.
| // Ensure minimum duration for music files (2 minutes = 120,000 ms) | ||
| audio_engine_state.duration = estimated_duration_ms.max(120000); |
There was a problem hiding this comment.
Enforcing a minimum duration of 2 minutes (120000 ms) is incorrect. This will cause any song shorter than 2 minutes to have an inaccurate duration displayed, and the seek bar will behave incorrectly. It's better to use the estimated duration, even if it's short.
| // Ensure minimum duration for music files (2 minutes = 120,000 ms) | |
| audio_engine_state.duration = estimated_duration_ms.max(120000); | |
| // Set the duration from the estimation | |
| audio_engine_state.duration = estimated_duration_ms; | |
| let is_synced = lyrics_frame.description == "SyncedLyrics" | ||
| || text.contains("[00:") | ||
| || text.contains("[01:") | ||
| || text.contains("[02:") | ||
| || text.contains("[03:") | ||
| || text.contains("[04:") | ||
| || text.contains("[05:"); |
There was a problem hiding this comment.
The current check for synced lyrics is a bit brittle as it only checks for timestamps in the first 6 minutes of a song (e.g., [00: to [05:). This might fail for longer songs or different LRC formats. A more robust heuristic would be to check for the general LRC timestamp pattern on any line.
| let is_synced = lyrics_frame.description == "SyncedLyrics" | |
| || text.contains("[00:") | |
| || text.contains("[01:") | |
| || text.contains("[02:") | |
| || text.contains("[03:") | |
| || text.contains("[04:") | |
| || text.contains("[05:"); | |
| let is_synced = lyrics_frame.description == "SyncedLyrics" | |
| || text.lines().any(|l| { | |
| let trimmed = l.trim_start(); | |
| trimmed.starts_with('[') && trimmed.contains(':') && trimmed.contains(']') | |
| }); | |
| let file_size_bytes = std::fs::metadata(path).map(|m| m.len()).unwrap_or(0); | ||
| let mut estimated_duration_ms = 0; | ||
|
|
||
| if file_size_bytes > 100000 { | ||
| // Only use file size if it's a reasonable size (>100KB) | ||
| // Assume 160 kbps MP3 = 160 * 1024 / 8 = 20480 bytes per second | ||
| let bytes_per_second = 160 * 1024 / 8; // 20480 | ||
| estimated_duration_ms = (file_size_bytes * 1000) / bytes_per_second as u64; | ||
| tracing::debug!( | ||
| "Estimated duration from file size: {} ms (file size: {} bytes, {} bytes/sec)", | ||
| estimated_duration_ms, | ||
| file_size_bytes, | ||
| bytes_per_second | ||
| ); | ||
| } | ||
|
|
||
| // Ensure minimum duration for music files (2 minutes = 120,000 ms) | ||
| audio_engine_state.duration = estimated_duration_ms.max(120000); | ||
|
|
||
| if estimated_duration_ms < 120000 { | ||
| tracing::debug!( | ||
| "Using minimum duration: {} ms (estimated was {} ms)", | ||
| audio_engine_state.duration, | ||
| estimated_duration_ms | ||
| ); | ||
| } |
There was a problem hiding this comment.
This block uses several magic numbers for duration estimation (100000, 160, 120000). It's better to define these as named constants to improve readability and make them easier to change in the future.
No description provided.