Effects refactoring: loaded chain #4431
Conversation
|
As I have already said, I am firmly opposed to this renaming. A preset is loaded into a chain. Conflating a preset with a chain creates a confusing situation. |
src/effects/effectchain.cpp
Outdated
| // Implement a check if Effects in the unit are matching a | ||
| // chain preset |
There was a problem hiding this comment.
What would be a use case for that? And what exactly does "matching" mean?
There was a problem hiding this comment.
This ControlObject is now meaningless. There is no such thing as an effect unit not being loaded.
There was a problem hiding this comment.
@ronso0 can you have a look here?
My expectation is that once we have moved the GUI to the the effect chain basis, this can indicate that no chain is loaded = every effect slot is empty.
Is there something else to consider?
There was a problem hiding this comment.
indicate that no chain is loaded = every effect slot is empty
I can't recall anyone ever asking for this nor can I think of any use case for it.
There was a problem hiding this comment.
Please rebase to remove this commit.
There was a problem hiding this comment.
@Swiftb0y I think you are not fully understanding. This CO has existed since Mixxx 2.0 but it never indicated anything useful. The legacy implementation had a concept of loading chains, but this was never actually made use of, it only added tons of complexity to the old code. If anyone used it the old CO for something I consider that a bug.
There was a problem hiding this comment.
I just grepped the source code and controller mappings and nothing uses this nonsense CO. Let's remove it.
There was a problem hiding this comment.
well then deprecate the CO and give it a dummy value? grepping for "loaded" reveals 0 skins but 3 mappings using it.
There was a problem hiding this comment.
Those are all for the EffectUnit_EffectX, loaded CO, not the EffectUnit, loaded CO.
There was a problem hiding this comment.
Ok than lets remove this entirely.
daschuer
force-pushed
the
effects_refactoring
branch
from
4141b2d to
02e7b6a
Compare
…Unit1],loaded_chain, to match other COs like next_chain and prev_chain
…or in EffectSlot::slotEffectSelector
…ng with the other COs
daschuer
force-pushed
the
effects_refactoring_loaded_chain
branch
from
84b9c1f to
f175f9e
Compare
|
I had to rebase this because unwanted commits from #2618 have slipped in. |
|
There are still unwanted commits in here. |
|
|
||
| m_pControlChainNextPreset = std::make_unique<ControlPushButton>( | ||
| m_pControlNextChainPreset = std::make_unique<ControlPushButton>( | ||
| ConfigKey(m_group, "next_chain")); |
There was a problem hiding this comment.
| ConfigKey(m_group, "next_chain")); | |
| ConfigKey(m_group, "next_chain_preset")); |
and add aliases
There was a problem hiding this comment.
I don't want to clutter the CO interface with aliases for such a minor issue. I think it is obvious already what the CO does even without the alias. We can add the alias at any time later.
There was a problem hiding this comment.
You have already changed one. Now change the others or you leave it in an inconsistent state.
There was a problem hiding this comment.
This PR is against the mixxxdj/effects_refactoring. If you think the renaming + alias is rectified, you can create your own PR. Maybe another team member is convinced that an this is a good idea. I like to move on the other issues.
|
Done. |
|
Still no justification has been made why this was so important to delay merging #2618 for months against unanimous consensus of everyone else. |
Ensure a consistent naming in EffectChain and fix some code issues.