-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
AutoDJ crossfader reset option #13156
Conversation
@@ -39,6 +39,9 @@ DlgPrefAutoDJ::DlgPrefAutoDJ(QWidget* pParent, | |||
// Auto DJ random enqueue | |||
RandomQueueCheckBox->setChecked(m_pConfig->getValue( | |||
ConfigKey("[Auto DJ]", "EnableRandomQueue"), false)); | |||
// Auto DJ XFaderCenterResetState | |||
XFaderCenterResetStateCheckBox->setChecked(m_pConfig->getValue( | |||
ConfigKey("[Auto DJ]", "XFaderCenterResetState"), true)); |
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.
Let's name it
CenterXFaderWhenDisabling
and the default should be false
m_pConfig->setValue(ConfigKey("[Auto DJ]", "XFaderCenterResetState"), | ||
enable); |
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.
This basically circumvents slotApply() and applies the selection right away.
All this "buffering" to the config is not good pratice as it unnnecessarily increases complexity and pollutes the config file with temp values. This page hasn't been touched for years. So essentially, if you want start working on (and learning about) the preferences, the AutoDJ page is not a good example.
IMO we should not built upon this, but instead seize the opportunity to refactor this page.
We can either
-
- stick with these checkbox / spinbox slots and store the temp values in member variables
- use the temp values in slotApply()
- (preferred IMO) simply read all checkboxes and spinboxes in slotApply() and write to the config
Do you want to work on this refactoring?
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 indeed tried to imitate the given code as much as possible. With a lot of trial and error.
I saw the not cleaned buff-occurences in the cfg-file and searched for a method avoiding those extra lines,
I struggled myself through the code :-)
I don't have a clue how old certain files are, but I can imagine there were different people adding their own part (like I did) puzzling out how it could be done, finding a way and making it all more complex.
I compaired other dialog settings files, and the structure is each time different, there is no consistency (not even about checkbox in front or after the text), making it hard for newbies.
I think it started with making a spinbox depending on a checkbox and ended with a lot of buffers and rules.
I am honoured with your question and I would like to improve this part. I just can't give a timeline, my students are studying these days, next week I start taking exams, so the spare time willl be less.
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.
Sure, I understand, no need to excuse, I think that's how most contributors start.
If you decide to do the refactoring, please start over with a new branch. Or reorder commits and force-puhs, as you like.
<string>Reset the Crossfader back to center after disabling AutoDJ</string> | ||
</property> | ||
</widget> | ||
</item> |
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 think we definitely have to add a note here explaining the consequences (main gain drop), see #10683
Can be closed, now that #13303 has been merged. |
AutoDJ, Added Settings to reset XFader to center after disabling Auto Dj