Skip to content

Conversation

@embarc-gabriel
Copy link
Contributor

Resolves: audacity/audacity#8166

The file dialog is ignoring the selected filter extension on file save.

  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually
  • I created a unit test or vtest to verify the changes I made (if applicable)

@embarc-gabriel
Copy link
Contributor Author

@cbjeukendrup can you kindly take a look on this new attempt?
If there is something I need to adjust please let me know.
It is based on you previous work: #18645

@embarc-gabriel embarc-gabriel force-pushed the use_file_dialog_filter_extension_when_saving_file branch from aac3eb1 to fd6bdd2 Compare April 15, 2025 09:46
QVariantMap resultMap = rv.val.toQVariant().toMap();

io::path_t selectedPath = QUrl::fromUserInput(resultMap["path"].toString()).toLocalFile();
std::string extension{};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't extension be initialised to muse::io::suffix(path)?
Or is the muse::io::suffix(path).empty() condition maybe not necessary?
Or what is the intended logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah! Probably it was not a nice code and it is hard to get the intention.
The user can fill the filename with the extension. On that specific case I won´t add the extension once the selectedPath already contains one. Maybe I can check if there is already an extension on selectedPath and extract from it. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps InteractiveProvider::openFileDialog should just be "dumb", and just return the path that the user entered, and the extension that was selected. Like this:

io::path_t selectedPath = QUrl::fromUserInput(resultMap["path"].toString()).toLocalFile();
std::string extension = resultMap["extension"].toString().toStdString();
result.val = { selectedPath, extension };

Then the post processing can happen in InteractiveProvider::selectSavingFile.
I would think the logic should be something like this:

  • Look at the extension of fileInfo.val.path
  • If it matches one of the allowed extensions of the selected filter, then just return that path
  • If it does not match one of the allowed extensions, first allowed extension of the selected filter, and append it to the path, and return that.

(One filter can namely have multiple extensions, for example for MIDI files, the allowed extensions are *.mid and *.midi.)

And of course, we need to have special handling for the * filter, and I seem to recall that on Windows, there is a special filter for "no extension", namely *.. We use that for MSCX files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!
Is there anything else I should do to support the MSCX files?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@embarc-gabriel Sorry for the delay, it was not entirely like what I was thinking of, so I pushed a commit with what I had in mind. Could you please check if this works for you?
I'll also ask our QA team to double-check that it works well on all OS.

@embarc-gabriel embarc-gabriel force-pushed the use_file_dialog_filter_extension_when_saving_file branch 3 times, most recently from 09ee4aa to b737a55 Compare April 28, 2025 05:27
@cbjeukendrup cbjeukendrup force-pushed the use_file_dialog_filter_extension_when_saving_file branch from b737a55 to bd28b85 Compare May 5, 2025 21:00
@cbjeukendrup
Copy link
Member

@DmitryArefiev
Copy link
Contributor

DmitryArefiev commented May 6, 2025

@embarc-gabriel @cbjeukendrup Tested on Win10, Mac13.7.2, LinuxUbuntu24.04.2 LTS

On Linux it crashes on save for new scores (when file browser appears to indicate the location)

Screencast.from.2025-05-06.18-33-36.webm

@embarc-gabriel
Copy link
Contributor Author

@cbjeukendrup the problem is converting the extensions on FileDialog to RetVal.
There error is this one on toRetVal

Val::fromQVariant | Not supported type: QStringList

@embarc-gabriel
Copy link
Contributor Author

embarc-gabriel commented May 7, 2025

@cbjeukendrup we can do something like this, for example:

root.ret = { "errcode": 0, "value": { "path": root.currentFile.toString(), "extensions": root.selectedNameFilter.extensions.map(function (ext) { return ext; }), } }

We can also update the val.cpp file.
What do you think?

@cbjeukendrup
Copy link
Member

First of all, apologies, I thought I had tested this, but forgot that the code in question runs only on Linux (even though it is also compiled on other OSs).

Yet another option would be to change the type of QMap<QString, RetVal<Val> > m_retvals to QMap<QString, RetVal<QVariant> >, and only convert to Val where really needed, i.e. in methods that return RetVal<Val>.

But if that's too complicated, modifying val.cpp to handle QStringList in the same way as QVariantList is also okay.

@embarc-gabriel
Copy link
Contributor Author

embarc-gabriel commented May 8, 2025

@cbjeukendrup I have added a new commit with your first suggestion.
The code worked as expected on my Linux machine.
Feel free to request further changes.
Thank you!
FYI @DmitryArefiev

@cbjeukendrup
Copy link
Member

@DmitryArefiev Ready for second round :)

@DmitryArefiev
Copy link
Contributor

@cbjeukendrup There is a kind of regression (comparing with #11615) on Linux

Screencast.from.2025-05-08.15-15-52.webm

@embarc-gabriel
Copy link
Contributor Author

@DmitryArefiev @cbjeukendrup I didn´t understand the issue just watching the video.
Can you give some steps and what is expected?
Is there a way to test it on Au4 code?

@cbjeukendrup
Copy link
Member

The problem is that the word "experimental" gets picked up as if it is the extension specification, because it is between parentheses... easiest workaround would be to change the parentheses around that word to square brackets.

@embarc-gabriel
Copy link
Contributor Author

@cbjeukendrup should I change it on the selectScoreOpeningFile function, right? I am a little bit lost where this change should take place.

@cbjeukendrup
Copy link
Member

Yes, and also OpenSaveProjectScenario::askLocalPath.

Actually, in ProjectActionsController::selectScoreOpeningFile, it would be good to update all 5 cases, also for other file types, because they would have the same problem.

@embarc-gabriel
Copy link
Contributor Author

Fixed! Can we test it again @cbjeukendrup @DmitryArefiev ?

@DmitryArefiev
Copy link
Contributor

On Linux, when saving Untitled score.mscz , it adds one period (at the end) in folder name, and two periods in file name.

Screencast.from.2025-05-14.17-04-35.webm

@cbjeukendrup With folder name it's probably fine.. but can we remove one period in file name (Untitled score.mscz..mscx) ?

Comment on lines +198 to +199
|| muse::contains(fileInfo.val.allowedExtensions, ALL_FILES_FILTER)
|| muse::contains(fileInfo.val.allowedExtensions, WINDOWS_NO_EXTENSION_FILTER)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we made a mistake here: instead of a filter, we will see an extension. So maybe the logic should be muse::contains(fileInfo.val.allowedExtensions, std::string()), to check if an empty extension is one of the allowed extensions. Because in that case, we should not add an extra ..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Native Ubuntu File Picker Does Not Show Exported Presets

4 participants