Fix #22551: ensure .drm extension when saving drumset#32824
Fix #22551: ensure .drm extension when saving drumset#32824jvcastroo-ist wants to merge 3 commits intomusescore:masterfrom
Conversation
When saving a custom drumset, if the user did not manually type the .drm extension, the file was saved without it. Since the load dialog filters by *.drm, the file would become invisible and unusable. Added ensureDrmSuffix() to guarantee the extension is always appended if missing, and set a default filename (drumset.drm) in the save dialog to guide the user. Also added unit tests for ensureDrmSuffix() under src/palette/tests/, along with the necessary CMake setup to enable the palette test suite.
| { | ||
| }; | ||
|
|
||
| TEST_F(DrumsetFileSaveTests, ensureDrumsetFileExtension) |
There was a problem hiding this comment.
This test does not make much sense; it tests io::path_t::appendingSuffix, rather than CustomizeKitDialog::ensureDrmSuffix. I suggest removing it.
That, in combination with the PR description, leaves the impression that this PR was AI-generated. Please be reluctant to submit AI-generated PRs, when you (apparently) don't fully understand the generated code yourself. Such PRs take the team (and volunteers like me) a lot of time to review; it slows us down instead of speeding us up. That's not meant as an offence, but as a honest request to keep software development sustainable.
|
|
||
| if (fname.empty()) { | ||
| return; | ||
| } | ||
|
|
||
| fname = ensureDrmSuffix(fname); |
There was a problem hiding this comment.
Writing to a different path than what the user selected comes with the risk of unintentionally overwriting existing files, because the OS file dialog only displays a warning if a file exists at the user-specified path, and not if a file exists at the modified path with the extension.
This is a general problem, and should probably be solved at the level of interactive::Interactive rather than here at one specific caller. See also PRs like #26950, #18645.
When saving a custom drumset, if the user did not manually type the .drm extension, the file was saved without it. Since the load dialog filters by *.drm, the file would become invisible and unusable.
Added ensureDrmSuffix() to guarantee the extension is always appended if missing, and set a default filename (drumset.drm) in the save dialog to guide the user.
Also added unit tests for ensureDrmSuffix() under src/palette/tests/, along with the necessary CMake setup to enable the palette test suite.
Resolves: #22551
Cause of the bug
The save operation for custom drumsets did not enforce the .drm file extension programmatically. Although the file dialog provided a filter suggesting the .drm format, this mechanism only acted as a visual hint and did not automatically append the extension to the filename. As a result, when users entered a filename without specifying the extension, the file was saved exactly as provided — and since the load dialog filters by *.drm, the file would become invisible and unusable on the next open.
Fix
A helper method ensureDrmSuffix() was introduced in CustomizeKitDialog to verify whether the selected file path includes the .drm extension and append it if missing. This method is applied after the file selection step, ensuring all saved drumsets follow the correct format regardless of user input. Additionally, a default filename (drumset.drm) was set in the save dialog to guide the user upfront.
Development notes
During development, the current HEAD of the repository caused the application to crash whenever the "Customize Kit" button was clicked, which prevented access to the save functionality entirely and made it impossible to reproduce and validate the bug. For this reason, the work was based on an older commit (fc1e049) where the feature was stable and the bug was reproducible.
Additionally, when running the existing unit test suite against the unmodified codebase, test #28 consistently failed. This was identified as a known issue already reported in the repository, unrelated to this fix, and therefore out of scope.
Notes
Added ensureDrmSuffix() as a private method so the logic is isolated and testable independently of the dialog
Introduced the src/palette/tests/ test suite, which did not previously exist, along with the necessary CMake setup (MUE_BUILD_PALETTE_TESTS)
Added unit test DrumsetFileSaveTests.ensureDrumsetFileExtension to verify that the extension is appended when missing and not duplicated when already present