Effect chain selectors: add empty item to allow clearing the chain

[ronso0]

• selecting --- in effect chain preset selectors and in EQ preferences (Quick Effects) clears the respective effect chain
• for Quick Effects chains, the preset selector (skins & prefs) will show --- which will also be stored in effects.xml to keep the empty chain on next start
• for other effect chains, the preset selector will be cleared and an empty preset name will be stored as before (see inline comments for details)

Primary purpose is to clear the Quick Effect chain (which I don't use) to get a clean GUI and have one aspect less I need to check when deck output is... unexpected / silent.
https://bugs.launchpad.net/mixxx/+bug/1980755

Instead of a built-in empty chain preset, this approach uses the already present --- items in DlgPrefEQ and adjusts the effect chain manager to handle it appropriately, i.e. all widgets (skin & preferences) as well as effect slots and loaded_chain_preset are updated. Also, the empty chain dummy item does not show up in Preferences > Effects, and thus doesn't need to be taken care of anywhere else.

@Be-ing Do you mind taking a look?

TODO

• remove commit with extra fx chain preset widgets in LateNight

[Swiftb0y]

alternative solution proposal: introduce a concept of read-only presets and make the "empty chain preset" read-only.

[Swiftb0y]

that'll break in the future because the preset index is unsigned. (It works right now because the underflow and overflow cancel each other out, but thats still not a solution).

[Swiftb0y]

I think the best option would be to change setControlLoadedPresetIndex to take a std::optional. Not super efficient, but IMO the safest approach.

void EffectChain::setControlLoadedPresetIndex(std::optional<uint> index) {
    // add 1 to make the ControlObject 1-indexed like other ControlObjects
    // if the effect chain is cleared (no valid index), the CO is 0;
    m_pControlLoadedChainPreset->setAndConfirm(index ? *index + 1 : 0);
}

[ronso0]

Thanks for the hint, I overlooked uint.
std::optional looks like a good fit, I'll try that.

[ronso0]

Added by 4e64f7a

[ronso0]

Thanks for your review!

alternative solution proposal: introduce a concept of read-only presets and make the "empty chain preset" read-only.

Well, IMO these empty items are only useful for the effect chain selectors and in the EQ preferences (to cover Shade which has no quick effect selectors). I tried the effect chain approach first because it seemed to be the simplest solution. Though it's not. Besides the read-only attribute, it also needs to be hidden from the effects preferences and the WEffectChainMenuButton (it's kinda useless/confusing in both), and probably also prevent it from being copied to the user directory, which complicates things more than necessary IMO, considering the simple purpose (quickly clear an effect chain).

[Swiftb0y]

Well we need to distinguish between whats easy to implement (hack in) and whats easy to maintain. I think that introducing a notion of read-only presets would be easier to reason about later. IMO what I proposed is decent tradeoff between both. We should also document that niche (0 = No preset) in the manual.

[Swiftb0y]

Also, should we make the next_chain (and related) COs include the ----preset?

[ronso0]

I think that introducing a notion of read-only presets would be easier to reason about later.

Probably, even though I don't see the benefit of read-only chains. Did you think of protecting built-in chains from deletion?

Well we need to distinguish between whats easy to implement (hack in) and whats easy to maintain.

Sure. Though in this exact case I'm fear the more elaborate 'read-only' approach is actually harder to maintain.
In addition to the issues I mentioned above which we'd need to take care of when using an empty preset (hiding it from effect prefs and effect chain menu, which I consider hacks tbh), some parts of the GUI require special treatment like dealing with chain changes in the effect chain menu ("Update preset"?). Since there is no signal emitted when a chain's slots are edited, read-only chains could be edited, but there's no easy way to reset it to defaults (yet).
Also there'd be a crosslink between xml effect chain presets and c++ effect chain managing that needs to be maintained (where the empty chain is visible it has to be at the top of the list).

Thus, I think for the purpose of clearing a chain the internal '---' approach works well and is easy to maintain as long as it's documented properly in the relevant places.
Actually it's only the EQ/QuickEffect preferences where GUI and backend need to be kept in sync ('---' in combobox / nullptr in presetlist for emitting the chain change). I'll add comments to clarify that interdependency.
The change in WEffectChainPresetSelector does not depend on the backend, only thing I still have to add is preventing --- from being used as preset name ("Save as new.." in skins / "Rename preset" + "Import preset" in prefs).

Botton line: I'm not very interested to invest a lot of time in the effects management/backend, I still want to finish work more important to me, and since implementing the read-only attribute incl. all GUI and backend adjustments is a lot of work, someone else would have to take care of it. (if there actually is someone caring about this ; )
If there's a consensus to not use this approach, I'm well served with a custom empty preset just to clear the Quick Effects.

[Swiftb0y]

First of, don't get me wrong, IMO the current approach lgtm IMO if you address the review comment above.

Second, I just wanted to express my concerns about shoehorning features into a system retroactively, making the system unmaintainable. As said previously, this PR seems mostly fine to me. I just wanted to voice my thoughts on a possibly more coherent solution (though it doesn't make much more sense either the more I think about it).

[ronso0]

Thanks, I appreciate and acknowledge your concerns, maybe it's just that I had to express (for myself?) that this is not just another hack (like those I propose every now & then ; ) but that I seriously considered the alternative.

I'll check how std::optional works here, rebase to fix the conflicts and add some comments.

[daschuer]

Should "---" be considered when toggling through the presets? I think yes.
We have

    m_pControlLoadedChainPreset->setAndConfirm(index + 1);

an of by one problem anyway. How about solving both by treating 0 as "---"?

[ronso0]

Yes, also I think --- would need to be the default name for empty effect chains right from the start so WEffectChain widget always displays the correct string, not just after clearing a chain.
I'll look into it.

Also, reloading quick effect chains after start does not yet work (even though it was working at some intermediate state).
Maybe that's related...

So I'll revert to 'draft' until that works.

[ronso0]

also I think --- would need to be the default name for empty effect chains right from the start so WEffectChain widget always displays the correct string, not just after clearing a chain.
I'll look into it.

🙄 it is already. here and here
So, I got it to work without too much hazzle.

Though there will be issues due to the fact that chains are stored differently in effects.xml for Quick Effect slots and regular [EffectRack1] effect units:
Quick Effect slots: <ChainPresetName group="[Channel3]">Filter</ChainPresetName>
regular effect units: complete snapshot of the current state, all knob and button states.
Thus, using --- for Quick Effect slots is easy and rather safe, for regular effect units it's more elaborate (and might require hacks)
:
I'll try this and that, and maybe come up with a maintainable solution.
If not, I'll drop it.

[ronso0]

Alright, this is now working beatifully:tm:

• selecting --- in effect chain preset selectors and in EQ preferences (Quick Effects) clears the respective effect chain
• for Quick Effects chains, the preset selector (skins & prefs) will show --- which will also be stored in effects.xml to keep the empty chain on next start
• for other effect chains, the preset selector will be cleared and an empty preset name will be stored (see inline comments for details)

Testing:

• in the settings directory, delete effects.xml and effects/chain directory
• start Mixxx
• verify Filter is the Quick Effect in all decks
• Preferences > EQ: verify that the Quick Effect name is the same in skins and prefs when selecting a preset or --- in either skin or prefs
• verify no preset is selected in Fx units 1-4 and that the chain name label is empty (IIRC the previous fx unit state should be restored)
• select presets and --- in Fx units 1-4 (verify that selecting --- clears the selector and the chain name label)
• close Mixxx
• restart
• verify all chain preset selections are restored correctly (Quick Effects may shown ---, Fx units 1-4 must not)
• verify that you can neither save presets as or rename to ---, or import any chain preset named --- (<Name> node in effects/chains/any-chain.xml, not the file name)

[ronso0]

Oh, commit Effect chain preset: disallow '---' on import, rename & save as new is not polished, yet. Will fix that and push soon..,

Nevertheless, all other aspects may be tested.

[ronso0]

... and revert to Draft to fix the CI errors (missing includes I guess)

[ronso0]

Note that I added Fx chain preset selectors and chain name labels to LateNight for testing.

TODO
make sure that commit is removed before merge

[ronso0]

Oh, commit Effect chain preset: disallow '---' on import, rename & save as new is not polished, yet. Will fix that and push soon..,

Done, ready for review.

[ronso0]

Should "---" be considered when toggling through the presets? I think yes. We have

    m_pControlLoadedChainPreset->setAndConfirm(index + 1);

an of by one problem anyway. How about solving both by treating 0 as "---"?

Well, we could make (effect chain), loaded_chain_preset and (effect slot), loaded_effect controls accept 0 to load a nullptr, if that's what you mean.
But we already have (effect chain), clear this purpose (clears the chain's effect slots, though not the preset name -- which may not be desired anyway, for example when trying to set up a new chain with a given name).
As equivalent for --- we may also add (effect chain), eject_preset to clear slots and preset name.

Note: I would not change the behaviour of (effect chain), chain_preset_selector CO or (effect slot), effect_selector.
Those are meant to select available chain presets 1-n with encoders (or up/down buttons) and wraps around. Especially because of the wrapping I don't consider scrolling an easy way to clear a chain/slot -- compared to comboxes where one could press Up or PageUp repeatedly (or Home) until the selector 'hits the ceiling' (first item).

[ronso0]

ping*

[Swiftb0y]

fyi I did spend an hour to review this but got frustrated when I tried to find a way to eliminate m_isQuickEffectChain. Also the changes in EffectChainPresetManager are difficult to understand, so I gave up.

[ronso0]

I appreciate your effort. Guess how long it took me to figure out all effect chain signals, preset loading etc. There needs to be more documentation in many places to avoid days of reverse-engineering.

However, I can take another look how to avoid m_isQuickEffectChain.
I can also try to split the preset manager commit if that makes reviewing easier?

[ronso0]

Also the changes in EffectChainPresetManager are difficult to understand, so I gave up.

What exactly? I still rememeber the context and reason for every change : ) so I can sure help.
I don't know how you review (diff only, or gh checkout [#pr] + IDE) but I really tried to make the changes as comprehensible as possible.

I'll add/tweak some more inline comments.

[ronso0]

However, I can take another look how to avoid m_isQuickEffectChain.

I added bool EffectsManager::chainIsQuickEffectChain(const EffectChain* pEffectChain).
Please check 79aee27 (I'd sqash that later on if that's okay)

[Swiftb0y]

Thanks, that's slightly better but still not ideal. The primary concern is that the baseclass needs to know about a child class. Not only does this essentially create a circular reference, it also introduces tricky dependencies. The ideal solution would be to re-implement the quickeffectchain-specific behavior by overwriting loadChainPreset but thats tricky without unnecessary code duplication because loadChainPreset already does too much.

Also the changes in EffectChainPresetManager are difficult to understand, so I gave up.

What exactly?

I forgot unfortunately.

I'll take another stab at reviewing this PR when I find the time.

[Swiftb0y]

going through all the code, it really makes me rethink whether we shouldn't introduce a pseudo-chain for the --- case... The current code makes so many "incisions" that make assumptions about the surrounding code that the result just seems too brittle.

[ronso0]

Thanks, that's slightly better but still not ideal. The primary concern is that the baseclass needs to know about a child class. Not only does this essentially create a circular reference, it also introduces tricky dependencies.

With 79aee27 it's only EffectsManager that deals with "is it a QuickEffect chain or not". I don't quite understand where there is a circular depency now.
Edit: Just noticed WEffectChainPresetSelector does it the same way WEffectChainPresetSelector::setup

I considered the Ideal Solution:tm:, too, but that's just too much code duplication + relevant info scattered = more maintenance burden. Thus I didn't choose it.

The current code makes so many "incisions" that make assumptions about the surrounding code that the result just seems too brittle.

Please point me to the places you're worried about.
I really tried hard to find the least invasive way to accomplish the goal. Maybe the implementation in WEffectChainSelector and DlgPrefEQ can be improved so it's easier to understand, if that's what bothers you. If you see ways to polish this approach I'm fine with investing more time to try those.

As I explained above #4892 (comment), I will not go the pseudo-chain route because I think that will require more new code and exceptions to have a consistent GUI/UX and chain state handling (e.g WEffectChainPresetButton, readEffectsXml / saveEffectsXml).

[ronso0]

As I explained above #4892 (comment), I will not go the pseudo-chain route because I think that will require more new code and exceptions to have a consistent GUI/UX and chain state handling (e.g WEffectChainPresetButton, readEffectsXml / saveEffectsXml).

This was bugging me, so I tried to ignore my previous assumptions and made another naive attempt.
@Swiftb0y @daschuer Please check #10859

[ronso0]

Closing this in favor of #10859
Thanks for reviewing so far @Swiftb0y @daschuer