Add Setting Screen#50
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a comprehensive audio settings system to the application, including a GUI settings dialog and fixes a pre-amp bias issue. The changes introduce configurable audio port connections, buffer sizes, and sample rates while resolving DC bias problems in the preamp stage.
- Added a complete settings management system with persistent storage and GUI controls
- Fixed preamp bias DC compensation using asymmetric soft clipping and DC blocking
- Introduced flexible audio port configuration replacing hardcoded connections
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/sim/stages/preamp.rs |
Reimplemented preamp processing with DC blocking filter and asymmetric bias handling |
src/sim/stages/filter.rs |
Added comprehensive tests for highpass filter functionality |
src/io/processor.rs |
Removed hardcoded port connections and added settings parameter |
src/io/manager.rs |
Enhanced with dynamic port connection management and settings application |
src/gui/settings.rs |
New settings data structures with JSON serialization and file persistence |
src/gui/mod.rs |
Updated to accept and pass through settings configuration |
src/gui/messages.rs |
Added message types for settings dialog interactions |
src/gui/components/settings_dialog.rs |
New modal dialog component for audio settings configuration |
src/gui/components/mod.rs |
Exported the new settings dialog module |
src/gui/app.rs |
Integrated settings dialog and management into main application |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| toggler(self.temp_settings.auto_connect).on_toggle(Message::AutoConnectToggled) | ||
| ] | ||
| .spacing(10) | ||
| .align_y(Alignment::Center); |
There was a problem hiding this comment.
The toggler widget should include a label parameter or proper accessibility labeling. Consider using toggler("Auto-connect on startup", self.temp_settings.auto_connect) to ensure screen readers can properly identify the control.
| .align_y(Alignment::Center); | |
| let auto_connect_section = iced::widget::toggler( | |
| "Auto-connect on startup", | |
| self.temp_settings.auto_connect, | |
| Message::AutoConnectToggled | |
| ) | |
| .size(16); |
|
|
||
| if let Some(port) = client.port_by_name("rustortion:out_port_left") { | ||
| client.disconnect(&port).unwrap_or_else(|e| { | ||
| error!("Failed to disconnect in_port: {e}"); |
There was a problem hiding this comment.
The error message incorrectly refers to 'in_port' when disconnecting the left output port. It should be 'Failed to disconnect out_port_left: {e}'.
| error!("Failed to disconnect in_port: {e}"); | |
| error!("Failed to disconnect out_port_left: {e}"); |
| .center_x(Length::Fill) | ||
| .center_y(Length::Fill) | ||
| .style(|_theme: &iced::Theme| { | ||
| container::Style::default().background(iced::Color::from_rgba(0.0, 0.0, 0.0, 0.7)) |
There was a problem hiding this comment.
[nitpick] The overlay background color (black with 70% opacity) is a magic number. Consider defining this as a constant like const MODAL_OVERLAY_COLOR: iced::Color = iced::Color::from_rgba(0.0, 0.0, 0.0, 0.7); to improve maintainability.
| container::Style::default().background(iced::Color::from_rgba(0.0, 0.0, 0.0, 0.7)) | |
| container::Style::default().background(MODAL_OVERLAY_COLOR) |
|
|
||
| // Main content with settings button in top bar | ||
| let top_bar = row![ | ||
| Space::new(Length::Fill, Length::Fixed(0.0)), |
There was a problem hiding this comment.
[nitpick] Using Length::Fixed(0.0) for height is unusual and could be confusing. Consider using Length::Shrink or a small fixed value if specific spacing is needed.
| Space::new(Length::Fill, Length::Fixed(0.0)), | |
| Space::new(Length::Fill, Length::Shrink), |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Closes #20