unify `[effect chain],loaded_chain_preset` control (all 0-indexed now) #12561

ronso0 · 2024-01-13T15:30:21Z

In #10859 I introduced an inconsistency, [QuickEffectRack1_[ChannelN]] loaded_chain_preset is offset by one compared to [EffectRack1_EffectUnitN] because of the prepended empty preset.

Fix1
The '---' preset is now also available for standard chains, e.g. via the effect chain preset button (⚙️ icon in effect units), which removes the offset. As before, selecting this preset clears the effect chain and loaded_chain_preset is set to 0 (works with all effect chains).
(and -1 for nameless items, the default state of standard effect units and the main output effect unit)
(fixes the issue discovered by @Swiftb0y in #11378 (comment))
I didn't do this before to avoid the UX issue I tried to describe here #11378 (comment)
which is now fixed by

Fix2
When an effect is about to be loaded while the '---' preset is loaded (and slots are empty)

an empty, nameless preset is loaded (loaded_chain_preset is set to -1)
the effect is loaded
'---' is again available for clearing the effect chain

Tests

Don't store/reload '---' preset

clear an effect unit by selecting the '---' preset in the chain preset menu
loaded_chain_preset is 0
restart Mixxx
check
- no preset is selected in the chain preset menu
- loaded_chain_preset is -1

Don't allow loading an effect into the '---' preset

clear an effect unit by selecting the '---' preset in the chain preset menu
trigger loading an effect (via effect selector or [(unit)_Effect1],next_/prev_effect/effect_selector
check
- desired effect is loaded
- no preset is selected in the chain preset menu
- loaded_chain_preset is -1

ronso0 · 2024-01-13T15:42:58Z

IMO this is definitely a 2.4.0 candidate since it restores the API consistency.

I'm sorry for not taking care of it sooner, #10859 got too big / took too long, I had no motivation to fix it right away.
Now, with a bit of distance, the fix was pretty easy. And low-risk IMO.

Swiftb0y · 2024-01-13T18:01:57Z

Thank you. I'll try to take a look ASAP.

ywwg

if I understand correctly, the major issue is that we want effects to be 0-based, but we also want a nullable-style "no effect" option. Is that right? I don't love the hackiness of this solution but at this late date I don't have a better idea. Something to look at later, perhaps.

ywwg · 2024-01-14T16:13:09Z

src/effects/presets/effectchainpresetmanager.cpp

@@ -394,6 +394,20 @@ void EffectChainPresetManager::setPresetOrder(
                m_effectChainPresets.value(chainPresetName));
    }

+    // After having changed the presets order in DlgPrefEffects, we received the
+    // new lists. The '---' was not displayed there, so it's not in the list.
+    // Make sure the empty preset "---" is the first item in the list.


please use consistent quote marks in the comments just for neatness

ywwg · 2024-01-14T16:14:31Z

src/effects/presets/effectchainpresetmanager.cpp

@@ -896,6 +931,10 @@ void EffectChainPresetManager::saveEffectsXml(QDomDocument* pDoc, const EffectsX
    QDomElement chainsElement = pDoc->createElement(EffectXml::kChainsRoot);
    rackElement.appendChild(chainsElement);
    for (const auto& pPreset : std::as_const(data.standardEffectChainPresets)) {
+        // Don't store the name '---', see readEffectsXml() for explanation.
+        if (pPreset->name() == kNoEffectString) {


if we don't want to store the noeffect, shouldn't we skip it (continue) instead of setting the name empty and adding it?

(as you did below)

I added this only as safeguard to not dump loaded effects in the (unexpected) case that there are effects loaded despite the '---' preset name.
I can turn it into a VERIFY_OR_DEBUG_ASSERT.

Fixed by d9b104b

ronso0 · 2024-01-14T20:10:21Z

I don't love the hackiness of this solution but at this late date I don't have a better idea.

I understand. IMO the hackiness (filtering '---' during preset export/import, Read/SaveEffectsXml and in the preferences) stems from the fact that we don't want to expose the '---' in the GUI except in the chain selectors and the chain preset menu.

IMO this fix can be considered a hack only because the preset switch happens after the effect was loaded into the '---' preset.
Would be better to that when the effect change is requested.
Just now I realize EffectSlot knows its parent EffectChain and also has a pointer to the preset manager, so since only EffectSlot::loadEffectInner emits the effectChanged() signal we care about, the preset swap can very well be triggered from there.

I'll push fixups in response to your review, and another commit moving the preset swap to EffectSlot.

…eferences

ronso0 · 2024-01-14T23:15:15Z

Okay, I'm happy with this now. Definitely less hacky. Works as desired.
I also added a Tests section to the PR description.

…into the '---' preset

daschuer · 2024-01-19T07:27:08Z

@ywwg Is this ready for merge form your perspective?

…n effects.xml

daschuer

The code Looks good and it works without any issues. Than you.

github-actions bot added effects ui preferences labels Jan 13, 2024

ronso0 mentioned this pull request Jan 13, 2024

Implement CO to set all QuickEffectRack Effects to the same Chain. #11378

Draft

3 tasks

ronso0 force-pushed the effectchain-loaded_preset-offset branch from e710d6b to 0e7dfbb Compare January 13, 2024 21:58

ywwg reviewed Jan 14, 2024

View reviewed changes

Effects: expose '---' preset to all chains, adjust saveEfectsXml & pr…

3d5134e

…eferences

ronso0 force-pushed the effectchain-loaded_preset-offset branch 2 times, most recently from 74581ea to c42d5ab Compare January 14, 2024 23:05

daschuer added this to the 2.4.0 milestone Jan 15, 2024

ronso0 added 2 commits January 15, 2024 12:30

Effects: make 'loaded_chain_preset' 0-indexed

12d1a7c

EffectChainPresetManager: rename createEmptyChainPreset()

d7ddaf7

ronso0 force-pushed the effectchain-loaded_preset-offset branch from c42d5ab to 79878d5 Compare January 15, 2024 11:30

Effect chain: create new preset when an effect is about to be loaded …

6bd44b4

…into the '---' preset

ronso0 force-pushed the effectchain-loaded_preset-offset branch from 79878d5 to 6bd44b4 Compare January 15, 2024 11:57

ronso0 added 2 commits January 19, 2024 10:02

Effects: ensure we rename a non-empty '---' preset before saving it i…

d9b104b

…n effects.xml

EffectChain: explain 'loaded_chain_preset' range

9264d2e

daschuer approved these changes Jan 23, 2024

View reviewed changes

daschuer merged commit b3439e9 into mixxxdj:2.4 Jan 23, 2024
13 checks passed

ronso0 deleted the effectchain-loaded_preset-offset branch February 9, 2024 02:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unify `[effect chain],loaded_chain_preset` control (all 0-indexed now) #12561

unify `[effect chain],loaded_chain_preset` control (all 0-indexed now) #12561

ronso0 commented Jan 13, 2024 •

edited

Loading

ronso0 commented Jan 13, 2024 •

edited

Loading

Swiftb0y commented Jan 13, 2024

ywwg left a comment

ywwg Jan 14, 2024

ronso0 Jan 14, 2024

ywwg Jan 14, 2024

ywwg Jan 14, 2024

ronso0 Jan 14, 2024

ronso0 Jan 19, 2024 •

edited

Loading

ronso0 commented Jan 14, 2024

ronso0 commented Jan 14, 2024

daschuer commented Jan 19, 2024

daschuer left a comment

unify [effect chain],loaded_chain_preset control (all 0-indexed now) #12561

unify [effect chain],loaded_chain_preset control (all 0-indexed now) #12561

Conversation

ronso0 commented Jan 13, 2024 • edited Loading

Tests

Don't store/reload '---' preset

Don't allow loading an effect into the '---' preset

ronso0 commented Jan 13, 2024 • edited Loading

Swiftb0y commented Jan 13, 2024

ywwg left a comment

Choose a reason for hiding this comment

ywwg Jan 14, 2024

Choose a reason for hiding this comment

ronso0 Jan 14, 2024

Choose a reason for hiding this comment

ywwg Jan 14, 2024

Choose a reason for hiding this comment

ywwg Jan 14, 2024

Choose a reason for hiding this comment

ronso0 Jan 14, 2024

Choose a reason for hiding this comment

ronso0 Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

ronso0 commented Jan 14, 2024

ronso0 commented Jan 14, 2024

daschuer commented Jan 19, 2024

daschuer left a comment

Choose a reason for hiding this comment

unify `[effect chain],loaded_chain_preset` control (all 0-indexed now) #12561

unify `[effect chain],loaded_chain_preset` control (all 0-indexed now) #12561

ronso0 commented Jan 13, 2024 •

edited

Loading

ronso0 commented Jan 13, 2024 •

edited

Loading

ronso0 Jan 19, 2024 •

edited

Loading