refactor: move components from app to handlers#197
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the GUI message and dialog handling architecture by introducing per-feature message enums and moving dialog logic out of AmplifierApp into dedicated handlers.
Changes:
- Introduces
SettingsMessage,TunerMessage, andMidiMessageand wraps them in the top-levelMessageenum. - Adds new GUI handlers (
SettingsHandler,TunerHandler,MidiHandler) and delegates relevantupdate/view/subscriptionlogic fromAmplifierApp. - Updates dialog components to emit feature-specific messages and maps them back into the top-level
Messagevia handlers.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/gui/messages/mod.rs | Adds new message modules and wraps settings/tuner/midi messages in Message; adds From impls. |
| src/gui/messages/settings.rs | New SettingsMessage enum extracted from previous top-level variants. |
| src/gui/messages/tuner.rs | New TunerMessage enum extracted from previous top-level variants. |
| src/gui/messages/midi.rs | New MidiMessage enum extracted from previous top-level variants. |
| src/gui/handlers/mod.rs | Registers new handler modules. |
| src/gui/handlers/settings.rs | New settings dialog handler and settings persistence/apply logic. |
| src/gui/handlers/tuner.rs | New tuner dialog handler and engine enable/disable + polling update logic. |
| src/gui/handlers/midi.rs | New MIDI dialog handler and event polling/mapping logic. |
| src/gui/handlers/hotkey.rs | Adjusts hotkey handler to return Task<Message> and maps dialog output to Message::Hotkey. |
| src/gui/components/dialogs/settings.rs | Dialog now emits SettingsMessage directly instead of top-level Message. |
| src/gui/components/dialogs/tuner.rs | Dialog now emits TunerMessage directly instead of top-level Message. |
| src/gui/components/dialogs/midi.rs | Dialog now emits MidiMessage directly instead of top-level Message. |
| src/gui/components/dialogs/hotkey.rs | Dialog now emits HotkeyMessage directly (no longer wraps into top-level Message). |
| src/gui/app.rs | Removes dialog state from AmplifierApp, uses handlers, and updates subscriptions/message routing accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Message::Midi(ref msg) => { | ||
| // Cross-cutting: persist settings changes for certain messages | ||
| let needs_save = matches!( | ||
| msg, | ||
| MidiMessage::ControllerSelected(_) | ||
| | MidiMessage::Disconnect | ||
| | MidiMessage::ConfirmMapping | ||
| | MidiMessage::RemoveMapping(_) | ||
| ); | ||
|
|
||
| let presets = self.preset_handler.get_available_presets(); | ||
| let mappings = self.settings.midi.mappings.clone(); | ||
| self.midi_dialog.show(presets, mappings); | ||
| } | ||
| Message::MidiClose => { | ||
| self.midi_dialog.hide(); | ||
| } | ||
| Message::MidiControllerSelected(controller_name) => { | ||
| self.midi_dialog | ||
| .set_selected_controller(Some(controller_name.clone())); | ||
| self.midi_handle.connect(&controller_name); | ||
|
|
||
| // Save to settings | ||
| self.settings.midi.controller_name = Some(controller_name); | ||
| if let Err(e) = self.settings.save() { | ||
| error!("Failed to save MIDI settings: {e}"); | ||
| } | ||
| } | ||
| Message::MidiDisconnect => { | ||
| self.midi_handle.disconnect(); | ||
| self.midi_dialog.set_selected_controller(None); | ||
|
|
||
| // Clear from settings | ||
| self.settings.midi.controller_name = None; | ||
| if let Err(e) = self.settings.save() { | ||
| error!("Failed to save MIDI settings: {e}"); | ||
| } | ||
| } | ||
| Message::MidiRefreshControllers => { | ||
| self.midi_dialog.refresh_controllers(); | ||
| } | ||
| Message::MidiStartLearning => { | ||
| self.midi_dialog.start_learning(); | ||
| } | ||
| Message::MidiCancelLearning => { | ||
| self.midi_dialog.cancel_learning(); | ||
| } | ||
| Message::MidiPresetForMappingSelected(preset) => { | ||
| self.midi_dialog.set_preset_for_mapping(preset); | ||
| } | ||
| Message::MidiConfirmMapping => { | ||
| if let Some(_mapping) = self.midi_dialog.complete_mapping() { | ||
| let mappings = self.midi_dialog.get_mappings(); | ||
| self.midi_handle.set_mappings(mappings.clone()); | ||
|
|
||
| // Save to settings | ||
| self.settings.midi.mappings = mappings; | ||
| if let Err(e) = self.settings.save() { | ||
| error!("Failed to save MIDI mappings: {e}"); | ||
| } | ||
| let task = self.midi_handler.handle(msg.clone(), presets, &mappings); | ||
|
|
There was a problem hiding this comment.
Message::Midi(ref msg) forces a msg.clone() to pass the message into midi_handler.handle(...). Since update owns message, you can match as Message::Midi(msg) and compute needs_save / settings updates from &msg (borrowing) before passing msg by value to the handler, avoiding an extra clone (and an extra String clone for ControllerSelected).
| presets: Vec<String>, | ||
| mappings: &[MidiMapping], | ||
| ) -> Task<Message> { | ||
| match message { | ||
| MidiMessage::Open => { | ||
| self.dialog.show(presets, mappings.to_vec()); |
There was a problem hiding this comment.
MidiHandler::handle requires presets and mappings for every message, but they are only used for MidiMessage::Open. This encourages callers to unnecessarily clone/allocate data even on hot paths (e.g., Update). Consider changing the API so only the Open path receives presets/mappings (e.g., a dedicated open(presets, mappings) method or making these parameters Option<_> and only constructing them when needed).
| presets: Vec<String>, | |
| mappings: &[MidiMapping], | |
| ) -> Task<Message> { | |
| match message { | |
| MidiMessage::Open => { | |
| self.dialog.show(presets, mappings.to_vec()); | |
| presets: Option<Vec<String>>, | |
| mappings: Option<Vec<MidiMapping>>, | |
| ) -> Task<Message> { | |
| match message { | |
| MidiMessage::Open => { | |
| if let (Some(presets), Some(mappings)) = (presets, mappings) { | |
| self.dialog.show(presets, mappings); | |
| } else { | |
| debug!("MidiMessage::Open received without presets or mappings"); | |
| } |
| let presets = self.preset_handler.get_available_presets(); | ||
| let mappings = self.settings.midi.mappings.clone(); |
There was a problem hiding this comment.
In the Message::Midi branch, get_available_presets() and self.settings.midi.mappings.clone() are executed for every MIDI message, including the high-frequency MidiMessage::Update (polled every 10ms). This introduces repeated allocations/clones on a hot path and can noticeably impact UI responsiveness. Consider only building presets/mappings for MidiMessage::Open (or other cases that actually need them), and calling the handler with empty/default values for Update or splitting the handler API so Open receives the data but Update does not.
| let presets = self.preset_handler.get_available_presets(); | |
| let mappings = self.settings.midi.mappings.clone(); | |
| // Avoid repeatedly allocating/cloning on high-frequency MIDI updates: | |
| // only build presets/mappings for messages that actually need them. | |
| let (presets, mappings) = if matches!(msg, MidiMessage::Update(..)) { | |
| (Default::default(), Default::default()) | |
| } else { | |
| ( | |
| self.preset_handler.get_available_presets(), | |
| self.settings.midi.mappings.clone(), | |
| ) | |
| }; |
No description provided.