Implement Save as Preset functionality - #62
Conversation
📊 PR SummaryChanges Overview
CI ChecksThis PR will trigger the following checks: Please ensure all checks pass before merging. |
Agent-Logs-Url: https://github.com/DirektDSP/PluginConfiguratorApp/sessions/17450483-d605-459a-b1d1-d3069f749212 Co-authored-by: SeamusMullan <43112447+SeamusMullan@users.noreply.github.com>
📊 PR SummaryChanges Overview
CI ChecksThis PR will trigger the following checks:
Please ensure all checks pass before merging. |
There was a problem hiding this comment.
Pull request overview
Implements an actual “Save as Preset” workflow in the UI by introducing a dedicated dialog to capture preset metadata and wiring MainWindow.save_current_as_preset() to persist the collected configuration via ConfigManager.save_preset().
Changes:
- Added
SavePresetDialogto collect/validate preset name + optional description. - Replaced the previous no-op
save_current_as_presetstub with dialog → collect config → persist → user feedback flow. - Added a focused test suite for
SavePresetDialogvalidation and accessors.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/ui/dialogs/save_preset_dialog.py |
New dialog for preset name/description capture and validation. |
src/ui/main_window.py |
Hooks up Save-as-Preset flow to ConfigManager.save_preset() and adds error handling. |
src/ui/dialogs/__init__.py |
Exposes SavePresetDialog from the dialogs package. |
tests/test_save_preset_dialog.py |
Adds tests for dialog initialization, validation behavior, and trimmed accessors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _on_accept(self) -> None: | ||
| """Validate the name field before accepting the dialog.""" | ||
| if not self._name_edit.text().strip(): | ||
| self._error_label.setText("Preset name cannot be empty.") | ||
| self._error_label.setVisible(True) | ||
| self._name_edit.setFocus() | ||
| return |
There was a problem hiding this comment.
The preset name is later used as part of the filename (via ConfigManager.save_preset), but the dialog currently only checks for non-empty text. This allows path separators (/, \\) or traversal segments (..) and OS-invalid characters, which can lead to saving outside the presets directory or save failures on some platforms. Add validation to reject/normalize to a safe file stem (e.g., disallow separators and reserved characters) and surface a clear error message to the user.
| config = self.collect_configuration() | ||
| config["meta"] = {"name": preset_name, "description": description} | ||
|
|
||
| try: | ||
| self._config_manager.save_preset(config, preset_name) | ||
| self.status_bar.showMessage(f'Preset "{preset_name}" saved successfully') | ||
| except ValueError as exc: |
There was a problem hiding this comment.
Saving a preset will overwrite an existing *.xml with the same name without any confirmation (ElementTree write() overwrites). This is inconsistent with the overwrite confirmation used in PresetManagementDialog._on_import and can cause silent data loss. Before calling save_preset, check whether the target preset file already exists and prompt the user to confirm overwrite (or offer a rename).
save_current_as_presetwas a no-op stub — it collected config but never persisted it, always emitting a misleading "Preset saved" status message.Changes
src/ui/dialogs/save_preset_dialog.py(new)SavePresetDialog: prompts for a preset name (required) and optional descriptionpreset_name()/description()accessors return trimmed textsrc/ui/main_window.pySavePresetDialog; early-return on Cancelcollect_configuration()→ attachmetadict (name + description)ConfigManager.save_preset()→ XML in~/.plugin_configurator/presets/QMessageBox.criticalonValueError/unexpected errorsrc/ui/dialogs/__init__.pySavePresetDialogtests/test_save_preset_dialog.py(new)