refactor: extract shared UI constants, colors, and dialog helpers#208
refactor: extract shared UI constants, colors, and dialog helpers#208
Conversation
- Add shared constants to widgets/common.rs (text sizes, spacing, padding, border radii, semantic colors)
- Add helper functions: section_title, section_container, labeled_picker
- Create dialogs/common.rs with 7 shared dialog helpers (title row, containers, learning-state views, mapping list)
- Replace all hardcoded magic numbers across 22 files with named constants
- Deduplicate ~200 lines of near-identical code between hotkey and MIDI dialogs
- Fix missing padding in ir_cabinet_control.rs section container
- Fix tuner dialog title casing ("TUNER" → "Tuner") to match other dialogs
There was a problem hiding this comment.
Pull request overview
This PR centralizes UI layout/styling values and refactors repeated dialog UI into shared helpers to reduce duplication and improve visual consistency across the app.
Changes:
- Added shared UI constants + helpers (
section_title,section_container,labeled_picker, semantic colors, spacing/padding/text sizes) inwidgets/common.rs. - Introduced
dialogs/common.rswith shared dialog layout/helpers and refactored Hotkey/MIDI/Settings/Tuner dialogs to use them. - Replaced several hardcoded spacing/padding/font-size values with named constants; adjusted tuner title casing in i18n.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/i18n/mod.rs | Updates tuner dialog title string casing. |
| src/gui/stages/tonestack.rs | Switches model dropdown to labeled_picker and uses shared spacing constants. |
| src/gui/stages/reverb.rs | Uses shared spacing constant for stage layout. |
| src/gui/stages/preamp.rs | Switches clipper dropdown to labeled_picker and uses shared spacing constants. |
| src/gui/stages/poweramp.rs | Switches amp-type dropdown to labeled_picker and uses shared spacing constants. |
| src/gui/stages/noise_gate.rs | Uses shared spacing constant for stage layout. |
| src/gui/stages/multiband_saturator.rs | Replaces multiple hardcoded sizes/spacings with shared constants. |
| src/gui/stages/level.rs | Uses shared spacing constant for stage layout. |
| src/gui/stages/delay.rs | Uses shared spacing constant for stage layout. |
| src/gui/stages/compressor.rs | Uses shared spacing constant for stage layout. |
| src/gui/components/widgets/common.rs | Adds shared UI constants (sizes/spacing/padding/colors) and reusable widget helpers. |
| src/gui/components/preset_bar.rs | Replaces hardcoded padding/spacing/border radius with shared constants. |
| src/gui/components/pitch_shift_control.rs | Uses shared text size + spacing constants. |
| src/gui/components/peak_meter.rs | Uses shared spacing/text size and semantic colors for status indicators. |
| src/gui/components/ir_cabinet_control.rs | Refactors section header/container to shared section_title/section_container helpers; fixes padding. |
| src/gui/components/dialogs/tuner.rs | Refactors to shared dialog helpers and semantic colors; updates UI glyph usage. |
| src/gui/components/dialogs/settings.rs | Refactors to shared dialog helpers/section container and shared sizing/color constants. |
| src/gui/components/dialogs/mod.rs | Adds dialogs::common module and maps dialog spacing/padding constants to shared UI constants. |
| src/gui/components/dialogs/midi.rs | Refactors to shared dialog helpers (title/section/waiting/captured/mapping list/muted text). |
| src/gui/components/dialogs/hotkey.rs | Refactors to shared dialog helpers (title/section/waiting/captured/mapping list). |
| src/gui/components/dialogs/common.rs | New shared dialog helpers for consistent dialog/section layout and mapping-related subviews. |
| src/gui/app.rs | Replaces hardcoded UI spacing/padding/colors and refactors IO sections to shared section helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ] | ||
| .spacing(10) | ||
| .padding(5), | ||
| .spacing(SPACING_NORMAL) | ||
| .padding(SPACING_TIGHT), | ||
| column![ |
There was a problem hiding this comment.
padding(SPACING_TIGHT) mixes spacing constants into padding. Since this module already defines padding constants (e.g., PADDING_SMALL/PADDING_NORMAL), using those here would keep intent clear and avoid future confusion about whether the value is meant as inter-widget spacing or container padding.
| column![header, sample_rate_row, buffer_size_row, warning,] | ||
| .spacing(8) | ||
| .padding(10), | ||
| .padding(SPACING_NORMAL) |
There was a problem hiding this comment.
There is still a hardcoded .spacing(8) here (and the PR description claims all magic numbers were replaced). Either replace this with an appropriate named spacing constant (e.g., SPACING_TIGHT) or adjust the PR description to reflect remaining intentional literals.
| pub fn input_captured_view<'a, M: Clone + 'a>( | ||
| description: &str, | ||
| presets: &[String], | ||
| selected_preset: Option<String>, | ||
| on_select: impl Fn(String) -> M + 'a, | ||
| confirm_msg: M, | ||
| has_preset: bool, | ||
| ) -> Element<'a, M> { |
There was a problem hiding this comment.
input_captured_view takes both selected_preset: Option<String> and a separate has_preset: bool, which duplicates state and allows them to get out of sync. Consider deriving enabled/disabled state from selected_preset.is_some() inside the helper (or accept only one source of truth) to make the helper API harder to misuse.
| .map(|m| (m.description.clone(), m.preset_name.clone())) | ||
| .collect(), |
There was a problem hiding this comment.
mapping_list_view currently requires allocating/cloning all mapping strings each render (description.clone(), preset_name.clone(), then collect()). Since views are rebuilt frequently in Iced, this can become a noticeable per-frame cost. Consider changing the helper to accept borrowed data (e.g., an iterator/slice of (&str, &str) or &(String, String)) so callers can avoid cloning.
| .map(|m| (m.description.clone(), m.preset_name.clone())) | |
| .collect(), | |
| .map(|m| (m.description.as_str(), m.preset_name.as_str())), |
| let mappings_list = mapping_list_view( | ||
| self.mappings | ||
| .iter() | ||
| .map(|m| (m.description.clone(), m.preset_name.clone())) |
There was a problem hiding this comment.
mapping_list_view is fed by cloning strings into a new Vec on every render (description.clone(), preset_name.clone()). Because Iced rebuilds views frequently, this introduces avoidable per-frame allocations. Consider updating the helper to accept borrowed mapping data so this call site can pass references instead of cloning.
| .map(|m| (m.description.clone(), m.preset_name.clone())) | |
| .map(|m| (&m.description, &m.preset_name)) |
- Use PADDING_NORMAL instead of SPACING_NORMAL for container padding in settings dialog - Replace remaining hardcoded .spacing(8) with SPACING_NORMAL in JACK status section - Remove redundant has_preset parameter from input_captured_view, derive from selected_preset.is_some()
Summary
widgets/common.rs(text sizes, spacing, padding, border radii, semantic colors) and helper functions (section_title,section_container,labeled_picker)dialogs/common.rswith 7 shared dialog helpers — deduplicates ~200 lines of near-identical code between hotkey and MIDI dialogsir_cabinet_control.rssection containerTest plan
make lintpasses cleanmake testpasses all 76 tests