Effects: allow clearing chains with empty '---' preset

[ronso0]

Alternative to #4892
This implementation works without hacks only minor adjustments in the GUI counterparts (EQ / effects preferences, skins).

An empty preset named --- is created by EffectChainPresetManager on each start.

• it is added at the top of the common chain preset list and the QuickEffect preset list
• it is shown in the chain preset selectors in Preferences > EQ & WEffectChainPresetSelector
• it is hidden from the preset lists in Preferences > Effects to avoid manipulation
  when those lists are changed they are pushed back to EffectChainPrestManager to update the lists in effect widgets, --- is missing and needs to be re-added to the standard and QuickEffect preset lists
• it is not saved to [settings_dir]/effects/chains
• --- is not accepted as preset name when saving or importing chain presets to avoid conflicts
• --- can be selected with [fx chain], chain_preset_selector and .., next/prev_chain_preset
• loaded_chain_preset called with 0 will now clear the chain

Update:
--- is only added to the QuickEffect chain presets list to avoid a few incosistencies with standard chains.

When picked in standard chains' preset selectors, EffectChain::loadChainPreset will clear the name (more details in #10859 (comment)), so

• chainPresetChanged() signal will clear the selector combobox (index = -1) and the selection in the chain preset menu
• the empty preset can not be overwritten via 'Update Preset' in the WEffectChainPresetButton menu
• it is not stored as preset name for standard effect chains in effects.xml

When picked in QuickEffect chains' preset selectors or in Preferences > Equalizer, the name --- persists

• --- remains selected in preset selectors to signal the current chain is empty (and will remain empty because there's no (skin) interface to load effects into chain slots)
• it is used as preset identifier for QuickEffect chains in effects.xml:

   <QuickEffectChains>
    <ChainPresetName group="[Channel1]">Echo</ChainPresetName>
    <ChainPresetName group="[Channel2]">---</ChainPresetName>
    <ChainPresetName group="[Channel3]">Filter</ChainPresetName>
    <ChainPresetName group="[Channel4]">Filter</ChainPresetName>
   </QuickEffectChains>
  

  which is valid to restore the empty preset when Mixxx is started

[ronso0]

Currently the --- item can also be selected with [fx_unit],next_/previous_chain_preset and [fx_unit],preset_selector.
I don't consider that a good solution and would like to prevent that, though if it's desired I won't insist, see my comment here

[Swiftb0y]

sorry I'm a bit short on time atm. I'll try to get back to this PR and #4892 next week. Thanks for understanding

[ronso0]

I don't have any expectations this will get merged soonish, so no need to excuse and no need to hurry. I just gave this another shot while I'm still into the details of the preset management.

[daschuer]

Thank you. This looks like a good solution.
Can we close #4892 then?

[daschuer]

I must admit that I have not understand this, even after reading the comment. When is it "---" and when ""?
Can we just dispose the "" case in favor of "---"?

[ronso0]

No, we can't.
This is required to properly deal with how QuickEffect presets vs. regular chain presets can be edited and how both are stored. It is the least invasive solution, and it works perfectly for both chain types.

I try to explain it once more, hopefully I get the point across well this time. This is best understood if you look at your effects.xml.

If --- is selected in a standard chain, EffectChain::loadChainPreset clears the name so all widget that use or display the preset name (for example WEffectChainPresetSelector::slotChainPresetChanged will be cleared because (empty string) can't be found in the preset list (a preset must have a name).
This is exactly the state when Mixxx is started the first time: empty effect units waiting to be populated, edited, named & exported.

How chain states are stored:

• QuickEffect chains are not editable, only super knob and enable switch are exposed¹
• for Quick Effect chains only the preset name is stored in effects.xml
• on next start, this name is looked up in the comon preset list
• if this field is empty or preset not found, the default preset is loaded (Filter)
• so compared to (empty string) --- will accomplish that the empty chain is restored correctly

__

• for standard chains (e.g. EffectRack1) the entire chains state is stored
• this maybe an unchanged default preset, or some custom chain that wasn't even saved or exported, yet, so the preset name is optional
• the empty --- preset itself must not be edited, it is just a shortcut to clear the entire chain and allow the user to start over
  = eject preset = clear slots & preset name = the user can immediatley start to create a new chain
• thus, if the chain would adopt the --- as preset name, that would be incorrect as soon as an effect slot is populated
  = selecting another preset, then going back to --- would not clear the chain!
• also, importing, exporting and saving as ---, as well as renaming --- is prohibited to avoid any conflict with the auto-generated --- preset, so keeping the --- preset after selecting it and thereby allowing to edit it will be a rather disappointing experience

¹ note that this is only true with official skins. IIUC it's not prohibited in any way to create WEffectChainPresetButton and WEffectSeletor widgets for a QuickEffect chain, so with those one could alter the --- preset, though that state would not be saved automatically and thus not be restored on next start.
Idk if we need to add a guard to not allow this in the first place to avoid disaapointment with custom skins that implemet those widgets.

[ronso0]

I updated the above description. Hopefully it's comprehensible ths time.

[daschuer]

I have tested the LatNight effect chain selector and label and I have noticed that WEffectChain is cleared instead of dashed out. Is this desired, or only a side effect of the this code here?
For my understanding it should be "---" instead and the code above needs to be removed.

The solution with
m_pEffectsManager->chainIsQuickEffectChain(this)
is also a bit unusual because now we have QuickEffectChain code in the base class even though QuickEffectChain has already an implementation of loadChainPreset()

I have just did a test with replacing the code above with
m_presetName = pPreset->name();
And it works IMHO flawlessly after deleting the assertion in EffectChainPresetManager::saveEffectsXml() which is IMHO wrong, because it saves the current state, independent from which chain was loaded.

This way the preset name is "---" but the changes after selecting "---" are still recorded, which is valid and happens with any other chain.

If you want to reflect that the chain is no longer empty, we may rename it in "New Chain". This however happens with any other chain you pick and than change the effect. For instance if you Pick the Reverb preset and replace the Reverb effect with a bit-chusher you have the same inconsistent state.

the empty --- preset itself must not be edited,

Why not, like any other preset. It must not be stored as preset though, but that is already disabled elsewhere.

thus, if the chain would adopt the --- as preset name,

This is not a problem of this PR, because it happens with any other preset in the same way.
If we want to fix that, it is a topic of another PR.

= selecting another preset, then going back to --- would not clear the chain!

This works in my test.

Maybe I am still missing something?

[ronso0]

The solution with
m_pEffectsManager->chainIsQuickEffectChain(this)
is also a bit unusual because now we have QuickEffectChain code in the base class even though QuickEffectChain has already an implementation of loadChainPreset()

If chosen this path to avoid code duplication. If you think this needs to be resolved I can move it the quickeffectchain.cpp

[ronso0]

The basic rule I want to adhere to is that --- always means "empty" in any other selector in skins and preferences.
I built everything on this to find a way to clear the QuickEffect chains.

It gets tricky only with standart effect chains, because IMO for those the --- item should be an action not a state, it should allow the user to restore the initial state (empty chain) like after starting Mixxx the first time.
Yes, the inconsistency of keeping --- (or any other preset name) after changing an effect applies to all preset, though IMO it is hard to distinguish if a user alters EchoVerbHP by selecting a different filter or adds a entirely different effect. But we should not accept this inconsistency for the very special --- item.

Btw I just notice a somewhat big mistake: actually the chain preset selectors are not part of official skins (and neither are WEffectChain labels) so --- should be in the menu of WEffectChainPresetButton (and not be marked selected after selecting it). Or we add a separate "Clear chain" item. I think --- would be more intuitive, though I have no strong preference.

[ronso0]

= selecting another preset, then going back to --- would not clear the chain!

This works in my test.

True, it works, even when --- is already selected. But definitely expereinced that, probably with an early state of this branch, maybe in the preset menu, idk.

[daschuer]

But we should not accept this inconsistency for the very special --- item.

Accepting it would make the behavior consistent that the chain label only displays the last action. Special case the "---" case and replace it with "" does not solve the underlying issue, that you expectation is to display the state.
"" is also shown in case the chain is still "---" which feels wrong as well.

This can be solved by removing the label once the user has re-added an effect, or in case of other preset once they have changes any effect. For this PR I would prefer to keep the last action display feature working for all.

[daschuer]

I experience a segfault:
This happens after selecting a new effect before it was "---"

Details

``` Thread 1 "mixxx" received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7fffeb677140 (LWP 57663)] 0x0000555555be0ed6 in WEffectChainPresetButton::populateMenu ( this=0x555568927870) at ../../src/effects/presets/effectchainpreset.h:47 47 return m_readOnly; (gdb) bt #0 0x0000555555be0ed6 in WEffectChainPresetButton::populateMenu() (this=0x555568927870) at ../../src/effects/presets/effectchainpreset.h:47 #1 0x00007ffff3b42328 in QtPrivate::QSlotObjectBase::call(QObject*, void**) (a=0x7fffffffcab0, r=0x555568927870, this=0x555568498640) at ../../include/QtCore/../../src/corelib/kernel/qobjectdefs_impl.h:394 #2 QMetaObject::activate(QObject*, int, int, void**) (sender=0x55555b61ad10, signalOffset=, local_signal_index=, argv=) at kernel/qobject.cpp:3784 #3 0x0000555555903d67 in EffectSlot::effectChanged() (this=0x55555b61ad10) at mixxx-lib_autogen/include/moc_effectslot.cpp:194 #4 EffectSlot::loadEffectInner(QSharedPointer, QSharedPointer, bool) (this=0x55555b61ad10, pManifest=..., pEffectPreset=..., adoptMetaknobFromPreset=) at ../../src/effects/effectslot.cpp:368 #5 0x0000555555904356 in EffectSlot::loadEffectWithDefaults(QSharedPointer) (this=0x55555b61ad10, pManifest=...) at /usr/include/c++/9/bits/atomic_base.h:318 #6 0x0000555555e9a16a in WEffectSelector::slotEffectSelected(int) (this=0x555569488980, newIndex=1) at /usr/include/c++/9/bits/atomic_base.h:318 #7 0x00007ffff3b42328 in QtPrivate::QSlotObjectBase::call(QObject*, void**) (a=0x7fffffffce20, r=0x555569488980, this=0x555569a4c1b0) at ../../include/QtCore/../../src/corelib/kernel/qobjectdefs_impl.h:394 --Type for more, q to quit, c to continue without paging-- #8 QMetaObject::activate(QObject*, int, int, void**) (sender=0x555569488980, signalOffset=, local_signal_index=, argv=) at kernel/qobject.cpp:3784 #9 0x00007ffff6cf30c5 in QComboBox::activated(int) (this=this@entry=0x555569488980, _t1=) at .moc/moc_qcombobox.cpp:481 #10 0x00007ffff6cf54d2 in QComboBoxPrivate::emitActivated(QModelIndex const&) (this=this@entry=0x5555686a7d10, index=...) at ../../include/QtCore/../../src/corelib/itemmodels/qabstractitemmodel.h:62 #11 0x00007ffff6cf7fa0 in QComboBoxPrivate::_q_itemSelected(QModelIndex const&) (this=0x5555686a7d10, item=...) at widgets/qcombobox.cpp:1346 #12 0x00007ffff6cfdaf5 in QComboBox::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) (_o=, _c=, _id=, _a=0x7fffffffd000) at widgets/qcombobox.h:252 #13 0x00007ffff3b421d0 in QMetaObject::activate(QObject*, int, int, void**) (sender=0x555567956ab0, signalOffset=, local_signal_index=, argv=) at kernel/qobject.cpp:3804 #14 0x00007ffff6cf33b6 in QComboBoxPrivateContainer::itemSelected(QModelIndex const&) (this=this@entry=0x555567956ab0, _t1=...) at .moc/moc_qcombobox_p.cpp:347 #15 0x00007ffff6cf3aca in QComboBoxPrivateContainer::eventFilter(QObject*, QEvent*) (this=0x555567956ab0, o=0x5555699fd2b0, e=0x7fffffffd570) --Type for more, q to quit, c to continue without paging-- at widgets/qcombobox.cpp:758 #16 0x00007ffff3b1651b in QCoreApplicationPrivate::sendThroughObjectEventFilters(QObject*, QEvent*) (event=, receiver=) at kernel/qcoreapplication.cpp:1214 #17 QCoreApplicationPrivate::sendThroughObjectEventFilters(QObject*, QEvent*) (receiver=receiver@entry=0x5555699fd2b0, event=event@entry=0x7fffffffd570) at kernel/qcoreapplication.cpp:1203 #18 0x00007ffff6beca55 in QApplicationPrivate::notify_helper(QObject*, QEvent*) (this=this@entry=0x55555663e270, receiver=receiver@entry=0x5555699fd2b0, e=e@entry=0x7fffffffd570) at kernel/qapplication.cpp:3694 #19 0x00007ffff6bf6343 in QApplication::notify(QObject*, QEvent*) (this=, receiver=0x5555699fd2b0, e=0x7fffffffd570) at kernel/qapplication.cpp:3160 #20 0x00007ffff3b1680a in QCoreApplication::notifyInternal2(QObject*, QEvent*) (receiver=0x5555699fd2b0, event=0x7fffffffd570) at ../../include/QtCore/../../src/corelib/kernel/qobject.h:142 #21 0x00007ffff6bf5457 in QApplicationPrivate::sendMouseEvent(QWidget*, QMouseEvent*, QWidget*, QWidget*, QWidget**, QPointer&, bool, bool) (receiver=0x5555699fd2b0, event=event@entry= 0x7fffffffd570, alienWidget=0x5555699fd2b0, nativeWidget=0x555567956ab0, buttonDown=buttonDown@entry=0x7ffff711c8d0 , lastMouseReceiver=..., spontaneous=true, onlyDispatchEnterLeave=false) at kernel/qapplication.cpp:2646 --Type for more, q to quit, c to continue without paging-- #22 0x00007ffff6c4bce4 in QWidgetWindow::handleMouseEvent(QMouseEvent*) (this=0x55556ffa62a0, event=0x7fffffffd9f0) at /usr/include/c++/9/bits/atomic_base.h:413 #23 0x00007ffff6c4e1ec in QWidgetWindow::event(QEvent*) (event=0x7fffffffd9f0, this=0x55556ffa62a0) at kernel/qwidgetwindow.cpp:289 #24 QWidgetWindow::event(QEvent*) (this=0x55556ffa62a0, event=0x7fffffffd9f0) at kernel/qwidgetwindow.cpp:232 #25 0x00007ffff6beca66 in QApplicationPrivate::notify_helper(QObject*, QEvent*) (this=this@entry=0x55555663e270, receiver=receiver@entry=0x55556ffa62a0, e=e@entry=0x7fffffffd9f0) at kernel/qapplication.cpp:3700 #26 0x00007ffff6bf60f0 in QApplication::notify(QObject*, QEvent*) (this=0x7fffffffde60, receiver=0x55556ffa62a0, e=0x7fffffffd9f0) at kernel/qapplication.cpp:3446 #27 0x00007ffff3b1680a in QCoreApplication::notifyInternal2(QObject*, QEvent*) (receiver=0x55556ffa62a0, event=0x7fffffffd9f0) at ../../include/QtCore/../../src/corelib/kernel/qobject.h:142 #28 0x00007ffff65727d3 in QGuiApplicationPrivate::processMouseEvent(QWindowSystemInterfacePrivate::MouseEvent*) (e=e@entry=0x55556ffdfb20) at kernel/qguiapplication.cpp:2107 #29 0x00007ffff657410b in QGuiApplicationPrivate::processWindowSystemEvent(QWindowSystemInterfacePrivate::WindowSystemEvent*) (e=e@entry=0x55556ffdfb20) at kernel/qguiapplication.cpp:1842 #30 0x00007ffff654e35b in QWindowSystemInterface::sendWindowSystemEvents(QFlags<--Type for more, q to quit, c to continue without paging-- QEventLoop::ProcessEventsFlag>) (flags=flags@entry=...) at kernel/qwindowsysteminterface.cpp:1151 #31 0x00007fffeaf2532e in xcbSourceDispatch(GSource*, GSourceFunc, gpointer) (source=) at qxcbeventdispatcher.cpp:105 #32 0x00007ffff274d17d in g_main_context_dispatch () at /lib/x86_64-linux-gnu/libglib-2.0.so.0 #33 0x00007ffff274d400 in () at /lib/x86_64-linux-gnu/libglib-2.0.so.0 #34 0x00007ffff274d4a3 in g_main_context_iteration () at /lib/x86_64-linux-gnu/libglib-2.0.so.0 #35 0x00007ffff3b6e435 in QEventDispatcherGlib::processEvents(QFlags) (this=0x5555566faec0, flags=...) at kernel/qeventdispatcher_glib.cpp:422 #36 0x00007ffff3b153ab in QEventLoop::exec(QFlags) (this=this@entry=0x7fffffffdd90, flags=..., flags@entry=...) at ../../include/QtCore/../../src/corelib/global/qflags.h:140 #37 0x00007ffff3b1d116 in QCoreApplication::exec() () at ../../include/QtCore/../../src/corelib/global/qflags.h:120 #38 0x000055555573908d in (anonymous namespace)::runMixxx (args=..., pApp=0x7fffffffde60) at ../../src/main.cpp:70 #39 main(int, char**) (argc=, argv=) at ../../src/main.cpp:198 ```

[ronso0]

I experience a segfault: This happens after selecting a new effect before it was "---"

Yep, already fixed. incorrect usage of isReadOnly()

[ronso0]

Note that I need to rebase anyway to remove the skin debug commit.
I'll make fixup commits for all proposed changes and squash on final rebase.
Alright?

[ronso0]

For debugging / stress-testing I added a complete QuickEffect rack above the decks in LateNight.

[Be-ing]

Exposing all the effects and parameters within QuickEffect chains makes them no longer any different from regular effect chains, which therefore makes them pointless.

Sorry, I didn't see that was just for debugging. Disregard this comment.

[ronso0]

Exposing all the effects and parameters within QuickEffect chains makes them no longer any different from regular effect chains, which therefore makes them pointless.

Sorry, I didn't see that was just for debugging. Disregard this comment.

And even more for doing things that are possible, though not available in official skins (by concept), because it would be a pity if a custom skin would be able to break the concept I propose.

@Be-ing Besides that, do you have any idea how to improve this solution, especially the --- string comparison?

[ronso0]

[fxChain],chain_preset_selector is broken now: it get's stuck due preset (empty string) not being found in the presetlist (presetIndex() returns -1) here.
Here, this happens because the preset name was changed to (empty string) intentionally, though if it happens due to other inconsistencies the index should be reset to 0 to jump to a working index again.

[ronso0]

@daschuer I addressed all comments, this is ready

[daschuer]

Works and looks good. Thank you.

You can trick yourself by calling effect chains "--" or "----" but I rgink this is a corner case not worth a fix. is it?

@Be-ing You review is still not approved. Is there anything else to do?

[ronso0]

You can trick yourself by calling effect chains "--" or "----" but I rgink this is a corner case not worth a fix. is it?

Sure we could have a helper function that disallows multipple -, but if users prefer ultra-descriptive chain names like -- or ++#-- it's up to them I'd say

[ronso0]

*ding dong

@Be-ing's only comment was about the debugging interface for QuickEffects, #10859 (review)

[daschuer]

OK, It should be safe to merge now. Thank you.

[ronso0]

🎉 since the various attempts to get this done have caused me quite some headaches : )

[ronso0]

Thanks @daschuer and @Swiftb0y for your patience!