Skip to content
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

Respect build options in ExportProjectDialog #3714

Merged
merged 4 commits into from Jul 26, 2017
Merged

Conversation

@irrenhaus3
Copy link
Contributor

@irrenhaus3 irrenhaus3 commented Jul 21, 2017

Addresses issue #3713 by implementing a workaround on the existing hard-coded ordering. This is obviously not the optimal solution because the core problem is the hard-coded ordering itself.

@tresf
Copy link
Member

@tresf tresf commented Jul 21, 2017

Do not have a hard-coded order at all

This seems like the best solution.

return ProjectRenderer::MP3File;
#endif // If neither ogg nor mp3 are available, index 1 is OOB; fall through to default case

This comment has been minimized.

@tresf

tresf Jul 21, 2017
Member

This is hard to read and will be even harder to add new items to. Let's instead parse "OGG, WAV, MP3" from the dropdown and make our assumptions there.

This comment has been minimized.

@irrenhaus3

irrenhaus3 Jul 21, 2017
Author Contributor

Agreed. But I don't think parsing strings is the best solution either. I think it's best to use the User Data field in QItem to tag each combo box item directly with the ProjectRenderer::ExportFileFormat, converted to int. That way, we can extract this value in onFileFormatChanged, convert it back to the enum type, and work directly with it. This would also make this dialog a lot more extensible for the purpose of adding more renderers.
I'll work on it and update this pull request once I have something that works.

This comment has been minimized.

@tresf

tresf Jul 21, 2017
Member

I don't think parsing strings is the best solution either.

The dialog mandates a file extension filter. Parsing string is perfectly sane here. I prefer enum approaches too and we need them elsewhere, but I see no logic flaw in .ogg = OGG at this point. It's not going to change.

…t in ExportProjectDialog

For compatibility with QVariant, ExportFileFormats is now explicitly an int.
@tresf
Copy link
Member

@tresf tresf commented Jul 21, 2017

@irrenhaus3 this is very clean, thank you. Can you please reformat the code to use tabs instead of spaces? Sorry for the inconvenience.

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Jul 25, 2017

@irrenhaus3 @tresf Did you test it for every configuration?

@tresf
Copy link
Member

@tresf tresf commented Jul 25, 2017

I did not test this PR at all.

@irrenhaus3
Copy link
Contributor Author

@irrenhaus3 irrenhaus3 commented Jul 25, 2017

@PhysSong @tresf I didn't test it for every configuration, just for (OGG enabled, MP3 disabled) and (both enabled). However, this PR specifically removes the need to check these configurations in the UI code and replaces them with runtime checks on whatever is available after this initialization: https://github.com/irrenhaus3/lmms/blob/6368bf1c886b961cbafe168c5d650eede62eb980/src/core/ProjectRenderer.cpp#L39
It should therefore work for any arbitrary flag configuration and continue to work when new flags are added in the future.

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Jul 26, 2017

I'll do a final test and merge it soon. Thanks for your work, @irrenhaus3!

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Jul 26, 2017

Tested and works good. I'll merge it soon if there's no objection.

@PhysSong PhysSong merged commit c8af34a into LMMS:master Jul 26, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
PhysSong added a commit that referenced this pull request Jul 27, 2017
* Respect build options in ExportProjectDialog

* Use QItem user data instead of hard ordering to identify export format in ExportProjectDialog

* For compatibility with QVariant, ExportFileFormats is now explicitly an int.

* Don't break out of format identifier loop prematurely in Song export.
@PhysSong
Copy link
Member

@PhysSong PhysSong commented Jul 27, 2017

Backported via b83c1bd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.