Support updating of effect presets #4686

ywwg · 2022-02-26T17:32:21Z

Adds a menu item to the effectunit cog menu below "Save as new preset".

daschuer · 2022-02-26T21:07:02Z

src/effects/presets/effectchainpresetmanager.cpp

+        return;
+    }
+
+    auto existing = m_effectChainPresets.find(name);


I do not understand why you look up the EffectChainPresetPointer by name, when we already have the pointer.
Please add a source code comment that explains the code.

the payload of the pointer is different because it is newly created. But I found a simpler way of doing this

ah, I thought I could call reset() on the shared pointer and have it update both lists, but that does not work.

src/effects/presets/effectchainpresetmanager.cpp

Co-authored-by: Daniel Schürmann <daschuer@mixxx.org>

daschuer · 2022-02-27T12:37:32Z

The code looks good now, however there is a usability issue with the default state.
If you have no preset loaded the feature tries to update an "Unnamed Preset". In this state, we should either hide the feature or default to the normal save dialog.

I also think that the menu can make use of an improvement. We have now:

Fancy Name 1
Fancy Name 2
--
Save as new preset
Update preset: Fency Name 1

The word "preset" = "Voreinstellung" sounds wrong in my ears because a preset is something that is "adjusted or applied in advance" but we have no advance in this case.

How about treating it all like a loaded file with a change tracking asterisk

✔ Fancy Name 1 *
Fancy Name 2
--
Create new
Save

Or show hide/gray out "Save" if nothing has changes.

In a first version I think we even can omit the idea of changes tracking.

ywwg · 2022-02-27T17:21:53Z

If you have no preset loaded the feature tries to update an "Unnamed Preset". In this state, we should either hide the feature or default to the normal save dialog.

I can't reproduce this. The code already checks for an empty preset name and hides the option if none is loaded:

the phrase "unnamed preset" does not seem to exist in the mixxx code so I'm not sure where you're seeing that.

But I like the checkmark idea and will update that part.

…check mark

ywwg · 2022-02-27T17:30:22Z

The word "preset" = "Voreinstellung" sounds wrong in my ears because a preset is something that is "adjusted or applied in advance" but we have no advance in this case.

in english this word choice is not confusing. "Create new preset" is fine.

daschuer · 2022-02-27T19:44:55Z

The "preset" = "Voreinstellung" thing is now less of an issue, because of the check mark.
We may only ask our self if the word preset adds any value here or if it can be removed. I think it is not a "Preset" we save it is more the "current effect chain state".

The "Unnamed Preset" is now no longer visible. But the underlying issue still persists

I have just tested with an empty preferences folder and hit a debug assertion:

critical [Main] DEBUG ASSERT: "pManifest.isNull() == pEffectPreset.isNull()" in function void EffectSlot::loadEffectInner(EffectManifestPointer, EffectPresetPointer, bool) at /home/sperry/workspace/mixxx2/src/effects/effectslot.cpp:289

When I continue anyway, I see no check mark, but I am able to select "Save preset"
I think in this case the "Save preset" should be grayed out.

ywwg · 2022-02-27T23:20:38Z

I see the loadEffectInner issue on main, not just my branch, are you sure that's caused by my changes?

critical [Main] DEBUG ASSERT: "pManifest.isNull() == pEffectPreset.isNull()" in function void EffectSlot::loadEffectInner(EffectManifestPointer, EffectPresetPointer, bool) at /home/owen/src/github/mixxx/src/effects/effectslot.cpp:289

ywwg · 2022-02-27T23:23:26Z

We may only ask our self if the word preset adds any value here or if it can be removed. I think it is not a "Preset" we save it is more the "current effect chain state".

the current effects code uses the word "preset" extensively. This is not the right place to re-raise the question of word choice.

I am still not able to reproduce the circumstance where a preset is not loaded but the "Save Preset" menu item is still visible. Can you post a screenshot?

daschuer · 2022-02-28T00:00:31Z

That's odd, now I cannot even start with a fresh profile anymore. There is something bad going on with the sound drivers.

Thread 1 (Thread 0x7fffeb355140 (LWP 45160)):
#0  futex_abstimed_wait_cancelable (private=<optimized out>, abstime=0x7fffffffd710, clockid=<optimized out>, expected=0, futex_word=0x555557909844) at ../sysdeps/nptl/futex-internal.h:320
#1  __pthread_cond_wait_common (abstime=0x7fffffffd710, clockid=<optimized out>, mutex=0x5555579097f0, cond=0x555557909818) at pthread_cond_wait.c:520
#2  __pthread_cond_timedwait (cond=0x555557909818, mutex=0x5555579097f0, abstime=0x7fffffffd710) at pthread_cond_wait.c:656
#3  0x00007ffff7b4b496 in WaitCondition () at /usr/local/lib/libportaudio.so.19.8
#4  0x00007ffff7b4d626 in StartStream () at /usr/local/lib/libportaudio.so.19.8
#5  0x00007ffff7b445ef in Pa_StartStream () at /usr/local/lib/libportaudio.so.19.8
#6  0x0000555555b06b20 in SoundDevicePortAudio::open(bool, int) (this=0x5555597e7ca0, isClkRefDevice=<optimized out>, syncBuffers=<optimized out>) at /home/sperry/workspace/mixxx2/src/soundio/sounddeviceportaudio.cpp:344
--Type <RET> for more, q to quit, c to continue without paging--
#7  0x00005555557987be in SoundManager::setupDevices() (this=0x555556e45ae0) at /usr/include/x86_64-linux-gnu/qt5/QtCore/qsharedpointer_impl.h:742
#8  0x000055555580e589 in MixxxMainWindow::initialize() (this=0x7fffffffde80) at /usr/include/c++/9/ext/atomicity.h:96
#9  0x00005555557352c3 in (anonymous namespace)::runMixxx (args=..., pApp=0x7fffffffde40) at /home/sperry/workspace/mixxx2/src/main.cpp:59
#10 main(int, char**) (argc=<optimized out>, argv=0x7fffffffe058) at /home/sperry/workspace/mixxx2/src/main.cpp:198
(gdb)

daschuer · 2022-02-28T00:08:55Z

I have copied over the soundconfig.xml and now it is working. I cannot longer produce the debug assertion and the save entry is hidden as desired. I think this is related to the LV2 plug-Ins.

My guess is that a preset with a missing LV2 Plug-In is removed from the list, but the required remove of the save menu entry is missing:

ywwg · 2022-02-28T02:36:05Z

My guess is that a preset with a missing LV2 Plug-In is removed from the list, but the required remove of the save menu entry is missing:

ahh ok. There are two lists of presets, one QHash and one sorted QList. It is not surprising that they can get out of sync. And it could be that the m_pChain tries to load itself but runs into an error and gets into a degraded state. I'll see if I can find another way to test whether a preset is actually loaded or not

… the list. Checking for an empty name in m_pChain was not reliable enough. Instead, only show the save option if we found a preset that matches our name.

ywwg · 2022-02-28T02:41:48Z

OK I only show the save option if we added the check mark. I believe that should be sufficient.

We should also file a bug for the issue you found with bad lv2 plugins causing issues.

daschuer · 2022-02-28T07:54:15Z

The hiding of the menu entry works now. Thank you.

"Create New Preset..." does not work well for me, because it implies that I create a new precondition for something from the scratch, which is not the case here. "Save Preset" is understandable but sounds wrong, because we have not yet preset to save. It becomes a preset during the process. I would prefer to avoid that abstract word "preset" entirely in the menu, but its OK if it used in the correct relation. The full sentence is "Save the current Effect Chain state as Effect Chain Preset"

From this I see the following alternatives:

Save
Save As...

Or

Save Effect Chain
Save Effect Chain As...

Or back to the original

Update Preset
Save As New Preset...

ywwg · 2022-02-28T14:46:52Z

these are called "presets" everywhere:

The "Create new" item creates a new item in the "chain preset" list with a new name. It's creating a new chain preset.

"save" and "save as" without a noun is confusing because nothing else in the menu indicates that these are effect chain presets. (Note that this object is the WEffectChainPresetButton.)

Calling it "save effect chain" without the word "preset" is inconsistent with the rest of the UI.

We could do "Save Chain Preset" and "Save Chain Preset As..."

But honestly I don't know how to say this well, this language choice is not confusing to a native English speaker and I don't agree with you. Our saved chains are called "presets", and creating a new saved chain is saving a new preset.

daschuer · 2022-02-28T15:23:09Z

these are called "presets" everywhere:

Your screenshots shows that the term Preset is never used alone. In these contexts the preset term makes sense, because they are uses a precondition for the effect chain. We have a list of selectable presets. See the definition:

preset
    a setting on a device that is adjusted or applied before use or by the manufacturer, especially on electronic audio or video equipment.

Loaded into a chain the Preset becomes the current setting of the chain, that can be freely adjusted. When I now read "Save Chain Preset As..." it sounds like I just give the original Preset a new name. Actually we want to Update the original preset from the chain and store it as a preset with a new name.

We could do "Save Chain Preset" and "Save Chain Preset As..."

If you like to keep "Preset", "Update Preset" and "Save As New Preset..." works much better for me and translates well.
This matches also the function name.

ywwg · 2022-02-28T15:39:20Z

https://docs.aws.amazon.com/elastictranscoder/latest/developerguide/create-preset.html
https://support.captureone.com/hc/en-us/articles/360002620598-Creating-a-Custom-Preset

ywwg · 2022-02-28T15:41:57Z

When I now read "Save Chain Preset As..." it sounds like I just give the original Preset a new name.

I do agree with this.

I'll go with "Update Preset" and "Save As New Preset..." to stop the argument, although I don't like using two different verbs

daschuer · 2022-02-28T16:41:02Z

Thank you, for this nice addition. LGTM

Support updating of presets

f62dc9d

Adds a menu item to the effectunit cog menu below "Save as new preset".

github-actions bot added the ui label Feb 26, 2022

ywwg requested a review from Be-ing February 26, 2022 17:55

Repopulate preset button menu when preset changes.

948a56a

daschuer reviewed Feb 26, 2022

View reviewed changes

daschuer requested changes Feb 26, 2022

View reviewed changes

src/effects/presets/effectchainpresetmanager.cpp Outdated Show resolved Hide resolved

ywwg and others added 3 commits February 26, 2022 18:35

Update Presets: Clarify why we need to do lookups in the preset lists

bd361fe

Update Preset: use replace instead of remove/insert

e806365

Co-authored-by: Daniel Schürmann <daschuer@mixxx.org>

Use .replace instead of remove/insert in two other places

7a8e4ca

Update Preset: Clarify menu language and indicate loaded preset with …

77bfe4f

…check mark

ywwg added 2 commits February 27, 2022 21:39

Update Presets: Only show "save preset" option if we found a match in…

fe30f44

… the list. Checking for an empty name in m_pChain was not reliable enough. Instead, only show the save option if we found a preset that matches our name.

Update Preset: style requires ellipses for menu items that open a dialog

ebf3101

Update Preset: tweak language

87c4eb8

daschuer approved these changes Feb 28, 2022

View reviewed changes

daschuer merged commit 3f30722 into mixxxdj:main Feb 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support updating of effect presets #4686

Support updating of effect presets #4686

ywwg commented Feb 26, 2022

daschuer Feb 26, 2022

ywwg Feb 26, 2022

ywwg Feb 26, 2022

daschuer commented Feb 27, 2022

ywwg commented Feb 27, 2022

ywwg commented Feb 27, 2022

daschuer commented Feb 27, 2022

ywwg commented Feb 27, 2022

ywwg commented Feb 27, 2022

daschuer commented Feb 28, 2022

daschuer commented Feb 28, 2022

ywwg commented Feb 28, 2022

ywwg commented Feb 28, 2022

daschuer commented Feb 28, 2022

ywwg commented Feb 28, 2022 •

edited

daschuer commented Feb 28, 2022

ywwg commented Feb 28, 2022

ywwg commented Feb 28, 2022

daschuer commented Feb 28, 2022

Support updating of effect presets #4686

Support updating of effect presets #4686

Conversation

ywwg commented Feb 26, 2022

daschuer Feb 26, 2022

Choose a reason for hiding this comment

ywwg Feb 26, 2022

Choose a reason for hiding this comment

ywwg Feb 26, 2022

Choose a reason for hiding this comment

daschuer commented Feb 27, 2022

ywwg commented Feb 27, 2022

ywwg commented Feb 27, 2022

daschuer commented Feb 27, 2022

ywwg commented Feb 27, 2022

ywwg commented Feb 27, 2022

daschuer commented Feb 28, 2022

daschuer commented Feb 28, 2022

ywwg commented Feb 28, 2022

ywwg commented Feb 28, 2022

daschuer commented Feb 28, 2022

ywwg commented Feb 28, 2022 • edited

daschuer commented Feb 28, 2022

ywwg commented Feb 28, 2022

ywwg commented Feb 28, 2022

daschuer commented Feb 28, 2022

ywwg commented Feb 28, 2022 •

edited