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

Provide a way for skins to specify default values for ConfigKeys. #387

Closed
wants to merge 4 commits into from

Conversation

ywwg
Copy link
Member

@ywwg ywwg commented Nov 15, 2014

Default show_coverart to true.

bool bPersist = m_pContext->selectAttributeBool(keyElement, "persist", false);

return controlFromConfigKey(key, bPersist, created);
ControlObject* co = controlFromConfigKey(key, bPersist, created);
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this works. Isn't *created just an indicator that the Co is crated by the skin. You need to know if the key exists in the mixxx.cfg.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, I thought when a configkey was persisted it would set created to false. Yeah I guess it doesn't work then.

@ywwg
Copy link
Member Author

ywwg commented Nov 22, 2014

That should work. Unfortunately sometimes the default value is processed before the pushbutton has iNoStates set. Not sure whose bug that is yet.

@ywwg
Copy link
Member Author

ywwg commented Feb 1, 2015

I'd like to revive this patch -- for new users installing Mixxx, the library does not appear if the default is not explicitly set. This makes the first-run experience very bad. (You can try it, remove the show_library setting from mixxx.cfg and you'll see it defaults to False)

@daschuer
Copy link
Member

There is an issue with the default value of COs setDefaultValue().
Strictly spoken, you do not set THE default value of the CO, you set an initial value, for the case the the value of the previous run is not there.
This can be fixes in to ways actually change the COs default value as well or rename the attribute, "initial" or something.

Setting the Inital/default value should be done in ControlDoublePrivate::initialize()

The other issue is that defining the default value in a '' iss misplaces.
See: https://bugs.launchpad.net/mixxx/+bug/1277201
The skin can only set a default, for COs it has just created.
If you looking for a CO that changes its value every time the skin is initialized, we have already the <attributes> region. I think we should also (or exclusive) be possible to set the persist and default flags there.
https://github.com/ywwg/mixxx/blob/default_show_coverart/src/skin/legacyskinparser.cpp#L333

@ywwg
Copy link
Member Author

ywwg commented Mar 25, 2015

you set an initial value, for the case the the value of the previous run is not there.

how is that different from the current use of "defaultvalue"?

default value in a '' iss misplaces

I can't understand this.

attributes region

this region does not work. I don't think it does anything at all, actually. I will see if I can figure out why

@ywwg
Copy link
Member Author

ywwg commented Mar 25, 2015

how about this instead: #534

@daschuer
Copy link
Member

I can't understand this.

Because of the confusion described in the linked bug.

how is that different from the current use of "defaultvalue"

Every CO already has a default value, defaults to 0, accessed by the child co "XXX_set_default".
You are about to introduce a initial value for a persistent control a different thing with the same name.
You can either make it the same, or use different names.

how about this instead: #534

A merge of both would be nice.

@ywwg
Copy link
Member Author

ywwg commented Mar 26, 2015

Because of the confusion described in the linked bug.

no I meant I can't interpret the words you have written: "iss misplaces"

@daschuer
Copy link
Member

sorry.

@ywwg
Copy link
Member Author

ywwg commented Apr 22, 2015

obsolete

@ywwg ywwg closed this Apr 22, 2015
@ywwg ywwg deleted the default_show_coverart branch April 22, 2015 13:25
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.

None yet

2 participants