Conversation
daschuer
left a comment
daschuer
left a comment
There was a problem hiding this comment.
We need some more string tweaks.
| } | ||
|
|
||
| bool EffectChainManager::getAdoptMetaknobPosition() const { | ||
| bool EffectChainManager::getAdoptMetaknobPositionSetting() const { |
There was a problem hiding this comment.
The setting prefix is unusual in Mixxx.
How about isAdoptMetaknobPositionEnabled
Position can also be missleasing.
IsAdoptMetaknobValueEnabled()
| checkBoxAdoptMetaknobPosition->setChecked(m_pConfig->getValue( | ||
| ConfigKey("[Effects]", "AdoptMetaknobPosition"), true)); | ||
| bool effectLoadMetaknobBehavior = m_pConfig->getValue( | ||
| ConfigKey("[Effects]", "EffectLoadMetaknobBehavior"), true); |
There was a problem hiding this comment.
In general I prefer a bool config value, according to my comment above AdoptMetaknobValue.
If you think we need a third option in future. We need an enum type, for now with two elements.
| <item> | ||
| <widget class="QRadioButton" name="radioButtonKeepMetaknobPosition"> | ||
| <property name="text"> | ||
| <string>Keep metaknob position</string> |
There was a problem hiding this comment.
Keeping the position is not the most important thing.
It is that the effect parameter are forced to the metaknob value instead of the default.
That's why I prefer "Adopt metaknob value" or any other short phrase that expresses this.
There was a problem hiding this comment.
I do not think people who have not been following this discussion will understand those implementation details. What they will see is whether the metaknob moves or not when they switch effects.
There was a problem hiding this comment.
A more detailed description might help:
[keep/preserve/respect] metaknob position and [adopt/adjust/apply to] effect parameters
Please choose whatever wording fits best here, since I'm not a native speaker.
Technically it is "loading", but the actual use case is "switching" from one effect to another. "Loading and switching effects" instead of "Load behaviour" might be easier for identifying the use case.
There was a problem hiding this comment.
Those are too many words for the preferences UI:
I'd like to keep it how it is here.
| comboBoxScreensaver->setCurrentIndex(comboBoxScreensaver->findData(inhibitsettings)); | ||
|
|
||
| bool effectLoadMetaknobBehavior = m_pConfig->getValue( | ||
| ConfigKey("[Effects]", "EffectLoadMetaknobBehavior"), true); |
There was a problem hiding this comment.
"behavior" is not a good naming for a boolean value.
When switching effects we can only preserve one of the following knob positions without inconsistencies:
- effect parameters
- metaknob
- superknob
You choose one these. The positions of the chosen one and all following in downwards direction are kept/preserved, while the corresponding parameters of all others in upwards direction need to be adjusted/adopted accordingly. Currently I don't see any more possible choices. The ordering with the implications might even give hints how a good user interface for this selection might look like.
Just brainstorming in the morning to help you find a good name.
|
@Be-ing: will you fix the remaining issue or should I? |
|
I don't think I'll have time. It's just code cleanup at this point. If you want to take care of that for the release candidate, go ahead but it's no big deal if it doesn't get done. |
|
Here it is: Be-ing#18 |
…ehavior use isAdoptMetaknobValueEnabled and "[Effects]", "AdoptMetaknobValue"
|
This broke MetaLinkTest.SuperToMeta_Softtakeover_EffectEnabled |
|
I cannot confirm this. |
|
The build servers, Travis, and AppVeyor are confirming it. |
|
Not in the latest 2.1 |
|
Ok, I was able to reproduce it here. I will push the fix directly to 2.1 |
3 participants
Follow up to #1517, which I accidentally pressed the merge button for too early