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

Kill buttons2 #357

Merged
merged 41 commits into from
Nov 10, 2014
Merged

Kill buttons2 #357

merged 41 commits into from
Nov 10, 2014

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Oct 7, 2014

This adds kill buttons to all three new three band EQs.
This commit is the successor of #297 and based on it.

I know there is a pending discussion if this is the right way to support push buttons.
So this is only a base to implement the final solution for 1.12

Any hints what to improve to be mergeable?

@daschuer
Copy link
Member Author

daschuer commented Oct 7, 2014

Parts of the discussion can be found here:
#281

Conflicts:
	src/effects/effectbuttonparameterslot.cpp
	src/effects/effectparameterslot.h
	src/effects/effectparameterslotbase.cpp
	src/effects/effectparameterslotbase.h
@daschuer
Copy link
Member Author

daschuer commented Oct 8, 2014

Hi @rryan

I really like to have this PR merge-able and I like to spend some time to move it in a direction.
The question is which.

I think it is required to stick on a separate CO namespace for button parameter (currently "button_parameterN") to allow to map them explicit to skins and controllers.

The current solution keeps this split all over the codebase, but has a common API for declaring them in the manifest (EffectManifestParameter). This is somehow inconsistent.

We can fix it by creating inheriting a button and a non button API from a Base API "EffectManifestParameterBase" to providing common things.

An other solution is to merge EffectManifest::m_parameters and EffectManifest::m_buttonParameters into a common list and deside acording to the hints if it is a button or a knob.

We can consider the same for Effect::m_parameters and Effect::m_buttonParameters since there is no difference between button and knobs inside the effect. The drawback is that we loose the strong connection between the Effect Index inside the effect and the N in the CO name.

The LV2 branch introduces dynamic mapping between the LV2 parameter index and the CO N to allow to pick the accessible parameters. So the drawback above is non..

What are your Ideas?
Thank you.

daschuer and others added 20 commits October 9, 2014 01:16
Conflicts:
	src/effects/effectmanifestparameter.h
	src/effects/effectparameter.cpp
	src/effects/effectparameter.h
	src/effects/effectparameterslot.cpp
	src/effects/effectparameterslot.h
	src/effects/native/butterworth8eqeffect.cpp
… available options when right clicking the button
Conflicts:
	src/effects/effectbuttonparameterslot.h
	src/effects/effectmanifestparameter.h
	src/effects/effectparameter.cpp
	src/effects/effectparameterslot.h
	src/effects/native/bessel4lvmixeqeffect.cpp
	src/effects/native/bessel8lvmixeqeffect.cpp
	src/effects/native/linkwitzriley8eqeffect.cpp
@daschuer
Copy link
Member Author

OK, I have done the later solution, merged the List of Buttons and Parameters into a common list.
Split it on the fly, when assigning parameter slots.

I have also merged the enum buttons from the LV2 Branch.
It is also prepared to have a step rotary, as replacement for valueHint INTEGRAL
For it we need to add a Behaviour later.
Imo a snapping of the knob is not worth the work, because we now have the enum Button,
button one can add it later.

CONTROL_KNOB_STEPPING, // A step rotary, steps given by m_steps
                       // are arranged with equal distance on scale
CONTROL_TOGGLE_STEPPING // For button and enum controls, not accessible
                        // form many controllers, no linking to super knob

A dynamic mapping (enable/disable) of parameters from the LV2 branch is prepared but not working.

This PR includes #356

@rryan: Please have a look. Thank you.

Conflicts:
	src/effects/effectmanifest.h
	src/effects/native/bessel4lvmixeqeffect.cpp
	src/effects/native/bessel8lvmixeqeffect.cpp
	src/effects/native/filtereffect.cpp
	src/effects/native/graphiceqeffect.cpp
	src/engine/effects/engineeffect.cpp
@daschuer
Copy link
Member Author

daschuer commented Nov 5, 2014

This is again mergeable.
I will merge this soon. This should be OK, since this reduced code duplication, for button parameters all ready merged to master.
the merge is required as base for the EQ/Filter effect Rack.

Any objections?

<ConfigKey>[EffectRack1_EffectUnit<Variable name="effectunitnum"/>_Effect<Variable name="effectnum"/>],parameter<Variable name="effectparameternum"/>_link_type</ConfigKey>
<ButtonState>LeftButton</ButtonState>
<ButtonState>LeftButton</ButtonState>
Copy link
Member

Choose a reason for hiding this comment

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

do you have some problems with your tab settings in xml files?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. This was an indentation fix.

@daschuer
Copy link
Member Author

No further objections. So I will merge it now. If there are remaining issues, we can discuss them in the EQ_Rack2 branch #364 witch is the next merge candidate.

Thank you for review and not at least a big thanks to @badescunicu, who did the majority of work.

daschuer added a commit that referenced this pull request Nov 10, 2014
@daschuer daschuer merged commit 193b532 into mixxxdj:master Nov 10, 2014
@rryan
Copy link
Member

rryan commented Nov 12, 2014

Thanks @daschuer and @badescunicu and I'm sorry that I did not make the time to review this.

Merging the knob and button lists went a long way towards fixing my concerns -- so thanks for that @daschuer. I prefer to remove the split CO API and make the skinning and controller language "smarter" but that can wait.

@daschuer daschuer deleted the kill_buttons2 branch May 30, 2015 21:57
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.

4 participants