-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Refactor ExportProjectDialog
#8215
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Connect signals only after the widgets are mapped to their respective parents
szeli1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did test the PR, here are my findings:
Exporting generally works, but there are some inconvenient things: In the file dialog, when you select .flac and click okay, the export dialog still defaults to wav. I also found a bug:
- export something as
wav - export again, in the file dialog select the previously exported file and select "mp3". A warning should show up.
- continue exporting, still as "mp3", finish exporting.
- the mp3 file doesn't show up where the original file was.
I repeated these steps, but used a different name for themp3file and it exported correctly.
|
I usually include a "how to test" section in my pull requests, because testers may not know what |
|
Thanks for the review @szeli1 👍
Just added this. Let me know if there's a need for more targeted testing instead of a general "test everything" approach, but basically we don't want any behavior to be missing from the refactor.
So are you saying that when you select "MP3', the extension used for the output path should change? I think I can do that, yeah. Because right now, if you select a WAV file, it will try to select a WAV for export but if you change it to MP3, it just replaces the WAV file that has the ".wav" extension with an MP3 file, which is misleading. |
Avoids assumptions for files with weird extensions
Just fixed the two issues you mentioned and addressed most of the review. |
If I reuse a filename with a new extension, I expect a file with the new extension to show up. If I reuse the filename and the extension, then I expect the original file to be overridden. What I described failed my first expectation: I reused a filename, but used "MP3" instead of "WAV" and I didn't see an "MP3" file show up. I hope this makes it clearer.
I will review again. |
szeli1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find any more bugs or issues. Thanks for making this PR!
|
There's a slight issue with the layout flow changing when you select a different format in the file format settings group. I'm going to try and see if I can fix this. Hopefully its simple enough. |
Simply hiding and showing the rows causes inconsistent padding between some of the rows that are visible
|
Fixed the spacing issue. This will be available for any testing for a few days before merge (I think this is good overall). |
This PR refactors
ExportProjectDialogto allow for greater flexibility, control, and simplicity.Changes
export_project.uiand convertsExportProjectDialogto use Qt layouts and widgets directly. This will help making changes to the dialog much easier, as controls and be removed or added without going through another file.export_project.uiwas also hardcoding the available sample rates, bit rates, etc, which is a problem since if we ever decide to change them, we have to remember to update the UI file as well. Instead, the file format combo boxes are dynamically populated according to the values inSUPPORTED_SAMPLERATES,SUPPORTED_BITRATES, etc.For testing the PR, the goal is that the export dialog does not have any regressions or missing behavior from the original export dialog since this is a refactor. Test by exporting different formats with different options, and overall confirm that this dialog keeps all the behavior of the old one.