Skip to content

add option to generate waveforms on analysis#998

Merged
daschuer merged 3 commits intomixxxdj:masterfrom
Be-ing:waveform_generate_on_analysis_option
Sep 4, 2016
Merged

add option to generate waveforms on analysis#998
daschuer merged 3 commits intomixxxdj:masterfrom
Be-ing:waveform_generate_on_analysis_option

Conversation

@Be-ing
Copy link
Copy Markdown
Contributor

@Be-ing Be-ing commented Sep 2, 2016

Taking care of https://bugs.launchpad.net/mixxx/+bug/1617589

Adds a new checkbox to Preferences > Interface > Waveforms > Caching:
waveform-option

Comment thread src/preferences/waveformsettings.h Outdated

bool waveformGenerateWithAnalysisEnabled() const {
return m_pConfig->getValue<bool>(
ConfigKey("[Library]", "GenerateWaveformsWithAnalysis"), true);
Copy link
Copy Markdown
Contributor

@uklotzde uklotzde Sep 3, 2016

Choose a reason for hiding this comment

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

The naming is inconsistent in various aspects. The new option in the config file uses a different wording for functions and the corresponding key in the config file: "waveform generate" (singular) vs. "generate waveforms" (plural) and ordering of words. And it also differs from the naming of related config options, namely "EnableWaveformCaching" -> "waveform caching enabled".

I would propose to follow the existing naming scheme for related options. Even if we don't like the current scheme, at least it is easier to remember if it is consistent.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for point that out. I didn't think much about the naming. Fixed.

AnalyzerQueue* ret = new AnalyzerQueue(pTrackCollection);

if (pConfig->getValue<bool>(ConfigKey("[Library]", "GenerateWaveformsWithAnalysis"))) {
if (pConfig->getValue<bool>(ConfigKey("[Library]", "EnableWaveformGenerationWithAnalysis"))) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is a very long name, but it is clear. Anyone have a suggestion for a shorter name that is still clear?

@daschuer
Copy link
Copy Markdown
Member

daschuer commented Sep 4, 2016

EnableWaveformGenerationWithAnalysis works for me.
Is this ready for mere now?

@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Sep 4, 2016

Yes, ready for merge.

@daschuer
Copy link
Copy Markdown
Member

daschuer commented Sep 4, 2016

LGTM Thank you!

@daschuer daschuer merged commit 6bdc4ab into mixxxdj:master Sep 4, 2016
@esbrandt esbrandt mentioned this pull request Jun 24, 2017
37 tasks
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.

3 participants