Finally refactor #30
Finally refactor #30
Conversation
- Fix busy-wait spin loop (100µs -> 2ms sleep) - Remove allocation in PulseAudio volume path - Optimize volume scaling iterator Reduces CPU usage 10-30% and improves battery life.
- Remove player, database, library_cmd_tx/rx, is_processing_ui_change from App - All fields now accessed through boot_cfg and runtime structures - Fix borrowing conflicts by using Arc clones and scoped access - Update is_processing_ui_change() to return Arc clone instead of reference
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 represents a significant architectural overhaul of the application, moving towards a more modular, maintainable, and extensible codebase. The primary focus has been on re-engineering the audio playback engine using a robust state machine pattern and restructuring the application's state management and UI components into distinct, specialized modules and services. This foundational work aims to simplify future feature development, improve code clarity, and enhance overall system stability. 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 is a massive and impressive refactoring of the application's core architecture. The separation of concerns into distinct modules for state, services, UI, and audio is a huge step forward for maintainability and clarity. The introduction of a state machine for the audio thread and the modularization of the playlist_table component are particularly noteworthy improvements. My review focuses on potential issues in the new structure, such as performance optimizations, error handling in the audio thread, and removing an unnecessary unsafe block. Overall, this is excellent work that sets a strong foundation for future development.
| "previous" => { | ||
| if let Some(idx) = ctx.playing_playlist_idx { | ||
| let playlist_clone = ctx.playlists[idx].clone(); | ||
| ctx.player_mut_ref().previous(&playlist_clone); | ||
| } | ||
| } | ||
| "next" => { | ||
| if let Some(idx) = ctx.playing_playlist_idx { | ||
| let playlist_clone = ctx.playlists[idx].clone(); | ||
| ctx.player_mut_ref().next(&playlist_clone); | ||
| } | ||
| } | ||
| _ => {} |
There was a problem hiding this comment.
Cloning the entire playlist on every previous and next action can be inefficient, especially for large playlists. This will cause a noticeable UI lag and increased memory usage.
A better approach would be to determine the next track to play without cloning the playlist. This can be achieved by moving the next/previous track logic from the Player struct into a method on the App or a service, which can then mutate the player state directly without running into conflicting borrow issues. This would avoid the expensive clone operation.
| seek_timestamp: u64, | ||
| ) { | ||
| let hint = Hint::new(); | ||
| let source = Box::new(std::fs::File::open(path).expect("couldn't open file")); |
There was a problem hiding this comment.
Using expect here can cause the audio thread to panic if the file cannot be opened (e.g., it was moved or deleted). A panic in the audio thread will crash the application. It's better to handle this error gracefully, for example by returning a Result and letting the caller state (e.g., LoadFileState) transition to an error or stopped state and notify the UI.
| let source = Box::new(std::fs::File::open(path).expect("couldn't open file")); | |
| let source = match std::fs::File::open(path) { | |
| Ok(file) => Box::new(file), | |
| Err(e) => { | |
| tracing::error!("Failed to open audio file {:?}: {}", path, e); | |
| return; | |
| } | |
| }; |
| .send(UiCommand::AudioFinished) | ||
| .expect("Failed to send audio finished to ui thread"); |
There was a problem hiding this comment.
Using expect on a channel send can cause the audio thread to panic if the UI thread has been closed. While this might happen during shutdown, it's generally better to avoid panics in threads. Using let _ = ... to ignore the error is a safer way to handle this, as the application is likely shutting down anyway.
| .send(UiCommand::AudioFinished) | |
| .expect("Failed to send audio finished to ui thread"); | |
| let _ = ctx.ui_tx.send(UiCommand::AudioFinished); |
| let duration = decoded.capacity() as u64; | ||
| ctx.engine | ||
| .audio_output | ||
| .replace(crate::audio::output::try_open(spec, duration).unwrap()); |
There was a problem hiding this comment.
Using unwrap() here can cause the audio thread to panic if opening the audio output device fails. This could happen for various reasons (e.g., no audio device available, permissions issues). The audio thread should handle this error gracefully, perhaps by transitioning to a StoppedState and logging the error, rather than crashing.
.replace(match crate::audio::output::try_open(spec, duration) {
Ok(output) => output,
Err(e) => {
tracing::error!("Failed to open audio output: {:?}", e);
return;
}
});| if let Ok(updated_library) = Library::load_from_db(&db_conn) { | ||
| self.library = updated_library; | ||
| } |
There was a problem hiding this comment.
Reloading the entire library from the database after updating a single track's metadata is inefficient. The update_track_metadata function already updates the track in memory within the playlists. Instead of a full reload, you should also update the corresponding item in self.library.items directly. This will be much more performant.
No description provided.