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

Allow to set inital persistant value of COs #559

Merged
merged 6 commits into from
Apr 22, 2015

Conversation

daschuer
Copy link
Member

This is a follow up of #534
It removes a hack for setting the initial persistent value, and allows whole Mixxx to benefit from it.

@daschuer
Copy link
Member Author

Open discussion form: #534 (comment)

Please pull

@ywwg:

I'm not sure this is right, because there may be a difference between the default value (which is what you get when you reset the object) and the initial value. So I don't want to make this change yet.

Yes, you are right, there is one, even though in most cases a inital Mixxx start should move its persistent controls to the default value.
But in a rare case for a different value, it is perfectly OK to set a different default after the CO is constructed. This is the way Mixxx works in master. The default value is initialized with a fixed 0.0 and can be changed afterwards. This PR just adopts the initial persistent value as default this is slightly better than the random 0.0.

@ywwg
Copy link
Member

ywwg commented Apr 22, 2015

The argument should probably be called defaultValue so people understand it's not just an initial value, and then the header comments should also say that the co will be initialized to the default value on construction.

@daschuer
Copy link
Member Author

Names changed, comments added.

if (m_bPersistInConfiguration) {
ConfigObject<ConfigValue>* pConfig = ControlDoublePrivate::s_pUserConfig;
if (pConfig != NULL) {
// Assume toDouble() returns 0 if conversion fails.
value = pConfig->getValueString(m_key).toDouble();
initalValue = pConfig->getValueString(m_key).toDouble();
Copy link
Member

Choose a reason for hiding this comment

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

inital -> initial

@ywwg
Copy link
Member

ywwg commented Apr 22, 2015

Just some spelling fixes and a slight rewording of the comment, then LGTM

@rryan
Copy link
Member

rryan commented Apr 22, 2015

I'll do the fixes post merge since I'd like to get this into master before #411.

rryan added a commit that referenced this pull request Apr 22, 2015
Allow to set initial persistant value of COs.
@rryan rryan merged commit 7f1c0ea into mixxxdj:master Apr 22, 2015
@daschuer daschuer deleted the attribute-fix branch January 8, 2016 16:39
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