Pref > Mixer: apply & save settings only in slotApply(), fix bugs, improve UX #11527

ronso0 · 2023-05-01T22:23:31Z

This is like #4667 but for the entire new Mixer page.

update (read from config) only in slotUpdate()
apply settings only in slotApply()
~~add checkboxes to apply EQ shelves and Main EQ instantly (default: off) to allow the previous (unexpected) behaviour~~ applied instantly but saved to config in slotApply() so that Cancel (> slotUpdate() resets to saved values

UX tweaks, small bug fixes, review proposals from Preferences: join EQ and crossfader settings into new Mixer page #11272

~~This makes quick fix #11271 obsolete but I'll leave that open for now.~~
The last commits adds debug output that allows easy tracing. To be removed after review.

There's still a round-trip issue with the EQ shelves' getEqFreq() and getSliderPosition(). This is noticable when resetting to defaults (250, 2500), the resulting values are 246 and 2484 🤷‍♂️ Maybe someone has an idea...

ronso0 · 2023-05-02T22:47:38Z

There's still a round-trip issue with the EQ shelves' getEqFreq() and getSliderPosition(). This is noticable when resetting to defaults (250, 2500), the resulting values are 246 and 2484 man_shrugging Maybe someone has an idea...

Fixed with much larger slider ranges and std::round instead of casting to int.

edit I also set the steps for Left/Right/PageUp/PageDown keys relative to the slider range. This should make it easier to make precise adjustments with the keyboard when the slieris focused.

ronso0 · 2023-06-29T13:13:53Z

Back to draft to avoid early reviews. I did some polishing and rebased. Also, merging #11663 will create some annoying conflicts, so I prefer rebasing when that is merged.

Note that I very much still consider this an relevant bugfix and usability improvement for 2.4!

ronso0 · 2023-06-30T13:13:25Z

Ready to roll this is, woohoo!

I did extensive testing and eliminated all UX hickups, mostly around the infamous 'Single EQ effect for all decks' checkbox, so WYSIWYG. All deck EQ & QuickEffect selectors are now always visible, only deactivated in the 'Single EQ ..' is checked. Consider this a temporary state, we may as well hide those if desired.

All settings are applied only in slotApply(), and read from config/effect engine in update slots.

~~Both EQ shelves and main EQ sliders now have a 'Apply instantly' checkbox for quick testing.~~
reverted, still applied instantly but Cancel would revert to the previous state.

Please test!

…otApply()

…ip errors Increase slider range (int, linear) for more precision when calculating the corresponding EQ frequency (double, exponential) and vice versa. Previously there was a round-trip rounding issue that would decrease certain high EQ frequencies on each update until they'd settle at certain values that are appearantly not affected. This was also noticeable when recalling the defaults (250 Hz, 2500 Hz), which resulted in 2484 Hz and 246 Hz.

src/preferences/dialog/dlgprefmixer.cpp

ronso0 · 2023-09-15T14:44:22Z

The xfader graph looks like this now:

…3 (not 1990 ;)

Swiftb0y

minor comments, still didn't get through everything, but I feel like thats just the nature of QWidgets...

src/preferences/dialog/dlgprefmixer.cpp

Swiftb0y · 2023-09-18T22:40:50Z

src/preferences/dialog/dlgprefmixer.cpp


        QLabel* label = new QLabel(QObject::tr("Deck %1").arg(deckNo), this);

-        // Create the drop down list for deck EQs
+        // Create the EQ selector //////////////////////////////////////////////
        QComboBox* pEqComboBox = new QComboBox(this);


can you go the extra mile and make this a parented_ptr as well? if the same is done for the members of m_deckQuickEffectSelectors we can remove the destructor entirely.

👍
though I run into this now with the lambda in slotNumDecksChanged() here

src/preferences/dialog/dlgprefmixer.cpp: In member function ‘void DlgPrefMixer::slotNumDecksChanged(double)’: src/preferences/dialog/dlgprefmixer.cpp:230:17: error: use of deleted function ‘parented_ptr<T>::parented_ptr(const parented_ptr<T>&) [with T = QComboBox]’ 230 | [this, pQuickEffectComboBox](const QString& name) { | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 231 | m_ignoreEqQuickEffectBoxSignals = true; | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 232 | pQuickEffectComboBox->setCurrentIndex(pQuickEffectComboBox->findText(name)); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 233 | m_ignoreEqQuickEffectBoxSignals = false; | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 234 | }); | ~ In file included from src/preferences/dialog/dlgprefmixer.h:9, from src/preferences/dialog/dlgprefmixer.cpp:1: src/util/parented_ptr.h:35:5: note: declared here 35 | parented_ptr(const parented_ptr<T>&) = delete; | ^~~~~~~~~~~~

and I have no clue what this is about 🤷‍♂️

The lambda is capturing pQuickEffectComboBox by value (which is fine for a raw pointer, but not fine for a smart-ish-pointer such as parented_ptr). Thus the compiler wants to create a copy which it can't because the copy-ctor is deleted (intentional and correct for now).
This is solved most easily by capturing by reference: [this, &pQuickEffectComboBox]

@daschuer do we want to rethink the parented_ptr semantics? Eg enabling moves and and copies, giving the parented_ptr reference semantics essentially. it's not really owning anyways.

src/preferences/dialog/dlgprefmixer.cpp

Swiftb0y · 2023-09-18T23:07:34Z

src/preferences/dialog/dlgprefmixer.cpp

+        QLabel* pCenterFreqLabel = new QLabel(this);
+        QString labelText = param->name();
+        pCenterFreqLabel->setText(labelText);
+        m_mainEQLabels.append(pCenterFreqLabel);
+        slidersGridLayout->addWidget(pCenterFreqLabel, 0, i + 1, Qt::AlignCenter);
+
+        QSlider* pSlider = new QSlider(this);


these are being leaked right now from what I can tell. seems like a not-insignificant memory leak to me.

👍 using parented_ptr for all spontaneously created widgets now, and for the numDecks ControlProxy.
no new anymore in this file.

Thank you 👍

but I messed it up when using it for the Main EQ widgets,crashes with qDeleteAll and/or QList in slotMainEqEffectChanged(int) here 5f43d7e#diff-c4e587c957428bd6afd10136eb4dad00d554179984d800b2252947ef2c27e666L984

I'll take a look at this later on.
Or we stick to new and re-add the destructor to delete those widgets.

I'll try to look into it later as well. You can probably just get rid of the qDeleteAll call.

Okay, I double checked, simply removing the qDeleteAlls is the correct choice. The compiler should've warned you if you were to call qDeleteAll on a QList<parented_ptr<T>>, but it apparently didn't. The resulting code still won't be leaky since all objects in the QList will have a parent (as DEBUG_ASSERTed when calling .clear() on the container).

The thing I'm rather confused by is that apparently QList::appending a parented_ptr<T> works, even though it shouldn't because copy and move have been deleted...

I commented the entire function and I'm sure the crash happens in of the Quick Effect slots. I'll check this soonish

Then its probably from this lambda. It's probably that the lambda (the connection its bound to) outlives the pQuickEffectComboBox reference, which lives shorter because of some m_deckQuickEffectSelectors.clear() call. But I wonder why that bug wasn't there previously.

It's not the Main EQ (and therefore: false alarm, qDeleteAll doesn't cause any issues), and there is no m_deckQuickEffectSelectors.clear().

But it's indeed that lambda and that pQuickEffectComboBox reference, though the crash happens during startup and the presetChanged() signal was not emitted.

I worked around it by capturing just the deck group string and the preset name and update the index in a separate slot (just to have a compact lambda body).

...and I added a note about the crash and the workaround.

src/preferences/dialog/dlgprefmixer.cpp

Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com>

daschuer

LGTM and works. Thank you.

ronso0 · 2023-10-16T21:04:06Z

Finally off my table.. Thanks for your reviews!

ronso0 · 2023-10-19T13:23:51Z

Whoops, had to fix the 'Bypass EQ procesing' default, it didn't consider empty string (no cfg value).
e39db38

github-actions bot added the ui label May 1, 2023

ronso0 force-pushed the pref-mixer-apply branch from 6437aa0 to 485c72c Compare May 2, 2023 22:46

ronso0 added this to the 2.4.0 milestone May 8, 2023

ronso0 mentioned this pull request May 8, 2023

main EQ: properly restore parameters + also save 'no effect' to config #11271

Closed

ronso0 changed the title ~~Pref EQ: apply & save settings only in slotApply(), fix bugs, improve UX~~ Pref > Mixer: apply & save settings only in slotApply(), fix bugs, improve UX Jun 13, 2023

ronso0 force-pushed the pref-mixer-apply branch 4 times, most recently from de7810d to 5ccbfc0 Compare June 28, 2023 11:47

ronso0 marked this pull request as draft June 29, 2023 13:09

ronso0 force-pushed the pref-mixer-apply branch from 5ccbfc0 to 2c92245 Compare June 30, 2023 12:47

ronso0 marked this pull request as ready for review June 30, 2023 13:05

ronso0 added 14 commits July 1, 2023 00:54

Pref Mixer, ui: set xfader button group in ui file

f888df7

Pref Mixer, ui: fix widget tab focus order

c400306

Pref Mixer, xFader: update settings only when touching xfader widgets

ca18756

Pref Mixer, xfader: don't store pointer to curve scene

f230382

Pref Mixer: read config only in slotUpdate() pt I

14f6811

Pref Mixer, bypass EQ: add bool for checkbox state, apply 'only in sl…

555b02d

…otApply()

Pref Mixer, deck EQ & QuickEffects: prevent unneeded reload during init

9554e84

Pref Mixer: improve function names, remove b prefix from bools, comments

f731e9d

Pref Mixer: apply settings only in slotApply(), except Main EQ

96929a0

Pref Mixer, main EQ: rework loading, updating & saving

770c643

Pref Mixer, main EQ: add checkbox to apply parameter changes instantly

6b248d5

Pref Mixer, main EQ: tweak parameter slider steps and value labels

2fdaee3

Pref Mixer, EQ shelves: add checkbox to apply shelves instantly

b4c880b

ronso0 force-pushed the pref-mixer-apply branch from 271284e to 870a22d Compare June 30, 2023 23:08

ronso0 force-pushed the pref-mixer-apply branch from f079e16 to e4007f9 Compare September 14, 2023 17:05

Swiftb0y reviewed Sep 14, 2023

View reviewed changes

ronso0 force-pushed the pref-mixer-apply branch from e4007f9 to 1dcab7e Compare September 15, 2023 14:44

Pref Mixer: polish xfader graph, smooth lines, style it like it's 202…

20bd4fa

…3 (not 1990 ;)

ronso0 force-pushed the pref-mixer-apply branch from 1dcab7e to 20bd4fa Compare September 15, 2023 16:08

Merge remote-tracking branch 'mixxx/2.4' into pref-mixer-apply

6f20086

Swiftb0y reviewed Sep 18, 2023

View reviewed changes

ronso0 added 4 commits September 19, 2023 11:00

Pref Mixer: don't use indexOf(QComboBox*) in for loops

17422ed

Pref Mixer: use parented_ptr for all widgets created on request

5aa6b32

Pref Mixer: contruct combobox tooltips with string args

84ddc77

Pref Mixer: qAsConst -> std::as_const

4c1e867

ronso0 force-pushed the pref-mixer-apply branch from c11e3a3 to 4c1e867 Compare September 21, 2023 08:16

Swiftb0y reviewed Sep 21, 2023

View reviewed changes

src/preferences/dialog/dlgprefmixer.cpp Outdated Show resolved Hide resolved

src/preferences/dialog/dlgprefmixer.cpp Outdated Show resolved Hide resolved

Swiftb0y reviewed Sep 21, 2023

View reviewed changes

src/preferences/dialog/dlgprefmixer.cpp Outdated Show resolved Hide resolved

ronso0 and others added 3 commits September 21, 2023 20:40

Pref Mixer: auto* + simpler combobox item deactivation

c222ed4

Pref Mixer: more efficient EQ list splitting EQs / non-EQs

8e571ee

Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com>

Pref Mixer: show effects' display name, separate EQs / non-EQs

2132b97

ronso0 force-pushed the pref-mixer-apply branch from e865b25 to 2132b97 Compare September 21, 2023 20:01

ronso0 force-pushed the pref-mixer-apply branch from 395ce54 to 07409a7 Compare October 16, 2023 10:36

Merge remote-tracking branch 'mixxx/2.4' into pref-mixer-apply

268e278

ronso0 force-pushed the pref-mixer-apply branch from 07409a7 to 268e278 Compare October 16, 2023 11:53

daschuer approved these changes Oct 16, 2023

View reviewed changes

daschuer merged commit b695252 into mixxxdj:2.4 Oct 16, 2023
13 checks passed

ronso0 deleted the pref-mixer-apply branch October 16, 2023 21:04

This was referenced Oct 19, 2023

unify behavior of preference options #7440

Open

Merge 2.4 #12192

Closed

ronso0 mentioned this pull request Nov 9, 2023

no Quick Effect loaded for decks 3 & 4 by default #12277

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pref > Mixer: apply & save settings only in slotApply(), fix bugs, improve UX #11527

Pref > Mixer: apply & save settings only in slotApply(), fix bugs, improve UX #11527

ronso0 commented May 1, 2023 •

edited

ronso0 commented May 2, 2023 •

edited

ronso0 commented Jun 29, 2023

ronso0 commented Jun 30, 2023 •

edited

ronso0 commented Sep 15, 2023 •

edited

Swiftb0y left a comment

Swiftb0y Sep 18, 2023

ronso0 Sep 19, 2023

Swiftb0y Sep 19, 2023 •

edited

Swiftb0y Sep 18, 2023

ronso0 Sep 19, 2023

Swiftb0y Sep 19, 2023

ronso0 Sep 19, 2023

Swiftb0y Sep 19, 2023

Swiftb0y Sep 19, 2023

ronso0 Sep 19, 2023

Swiftb0y Sep 19, 2023

ronso0 Sep 19, 2023 •

edited

ronso0 Sep 21, 2023

daschuer left a comment

ronso0 commented Oct 16, 2023

ronso0 commented Oct 19, 2023

Pref > Mixer: apply & save settings only in slotApply(), fix bugs, improve UX #11527

Pref > Mixer: apply & save settings only in slotApply(), fix bugs, improve UX #11527

Conversation

ronso0 commented May 1, 2023 • edited

ronso0 commented May 2, 2023 • edited

ronso0 commented Jun 29, 2023

ronso0 commented Jun 30, 2023 • edited

ronso0 commented Sep 15, 2023 • edited

Swiftb0y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y Sep 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronso0 Sep 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

ronso0 commented Oct 16, 2023

ronso0 commented Oct 19, 2023

ronso0 commented May 1, 2023 •

edited

ronso0 commented May 2, 2023 •

edited

ronso0 commented Jun 30, 2023 •

edited

ronso0 commented Sep 15, 2023 •

edited

Swiftb0y Sep 19, 2023 •

edited

ronso0 Sep 19, 2023 •

edited