Pref EQ: apply & save settings only in slotApply() #4667

ronso0 · 2022-02-07T22:10:47Z

before, "Use the same EQ for all decks" was reset to true, overriding the config value.

Noticed here https://mixxx.discourse.group/t/mixx-strongly-began-to-heavily-load-the-system-linux/24068/29

uklotzde · 2022-02-07T23:08:54Z

src/preferences/dialog/dlgprefeq.cpp

@@ -45,6 +45,9 @@ DlgPrefEQ::DlgPrefEQ(QWidget* pParent, EffectsManager* pEffectsManager, UserSett
    m_pOutputEffectRack = m_pEffectsManager->getOutputsEffectRack();

    setupUi(this);
+
+    loadSettings();


Leave a comment why the settings must be loaded here and not later?

Not really. It's fixed in main already.
Also the comment would actually belong to/into slotNumDecksChanged which is doing the reset.

ronso0 · 2022-02-07T23:48:35Z

more to come, after start the EQ boxes are still accessible even though "Bypass EQs" is checked (toggling it off and on again does de/activate comboboxes)

Be-ing · 2022-02-07T23:49:43Z

Note that there were a lot of changes to this code between 2.3 and main so someone is likely to have to deal with merge conflicts which might get messy.

ronso0 · 2022-02-07T23:58:44Z

I know, I'm already looking into the differences 2.3/main.
Until now it's only moving lines around in the initialisation, so conflicts seem manageable.

ronso0 · 2022-02-08T15:20:35Z

Unfortunately, this pref page is a bit of a mess, fixing one bug introduced another regression and so on.
There are places where config vaules are set based on checkbox states not initialized from config but the default states determined by the ui file, other functions write to config too early (before slotApply is invoked) so that clicking Cancel does not wipe unapplied settings but smply closes the dialog, reset to defaults loads settings from config?

Sooo, I could "fix" the actual bug with a few workarounds and additional repetetive widget updates calls, and overhaul the entire page after merging this to main. Thus leaving 2.3 in a rather sub-optimal bu "somehow working" state.
Or I do the overhaul now and deal with the conflicts...

Btw it seems to make sense to apply every Master EQ slider change immediately (so it's audible without having to press Apply every time). I'd leave that as is if no one has objections.

Be-ing · 2022-02-08T15:22:54Z

I agree this preference page is a mess to maintain. I think it was a mistake to introduce the nearly useless feature of allowing to configure different EQs for different decks; it adds a lot of complexity to this preferences code that I doubt anyone uses in practice. I think this bug is very minor and it's fine to leave it as is.

uklotzde · 2022-02-08T16:53:08Z

Do not spend any time on these dialogs. Only fix minor bugs (if you really can't hold yourself back) and don't touch anything else.

ronso0 · 2022-02-08T23:57:35Z

Well, after all we'll still have to deal with those dialogs for some time.
I already touched it and poked around to find out what's wrong -- that time would be wasted if I drop it now.

My aspiration is to -at least- make sure

settings are loaded from config, not from .ui file defaults
the GUI reliably shows the current config
nice to have: only Apply writes values to config (except Master EQ)

I'll pack bitesize commits for minimal review time.

ywwg · 2022-02-10T04:16:38Z

different EQs for different decks

how hard would it be to rip that out? (not for this PR). I would like to lose some cruft if we agree it's not needed

I agree that it's best to take a "do as little as possible" approach to the prefs code. Fixes are always appreciated, just don't get sucked too far in :)

Be-ing · 2022-02-10T04:30:46Z

how hard would it be to rip that out?

Not sure, but probably not all that hard. No changes would be required in the effects system, only DlgPrefEq.

ronso0 · 2022-02-10T09:45:17Z

just don't get sucked too far in :)

Too late. Though I'm not refactoring but consolidating all apply code in, well, slotApplyso that Cancel does.. cancel as it supposed to. That went unexpectedly quick, only packing small commits took a while.
Also, the parts I touched are almost identical in main, so I'm confident the conflicts are minimal (will resolves those of course).
IMO review should focus not focus on the code quality (as I just moved stuff around) but manual testing if and when changes are applied, since that is what this is all about. Cancel should cancel, Apply should apply (set COs and set config values).

I will not touch the code that allows 4 individual EQs / QuickEffects. Shouldn't be too hard though.

Also, I noticed a bug in the EQ shelve 'validation' / slider/value calculation: if you set low & high shelves above average the values would be lowered with each start until they settle at certain values. Won't fix that here either.

ronso0 · 2022-02-10T12:14:18Z

While testing the changes I noticed another, additional bug in main:
"Bypass EQ" ON is not taken into account after start -- until it's toggled Off and On again in the preferences.

I think it makes much sense to fix this stuff now, regardless of a future preferences interface.

Be-ing · 2022-02-10T12:42:41Z

How about getting rid of the redundant bypass EQ option? It does the same thing as picking None in the combobox.

ronso0 · 2022-02-10T17:42:04Z

"None" unloads the EQ effect (and hides the respective mixer controls).
"Bypass" is more like a (temporary?) switch that keeps the EQ config.
Idk, it's not a bug and not urgent IMO, removal can happen in main.

Btw: I'd move the Bypass checkbox up above Master EQ.
The current position at the bottom does not communicate that it only affects deck EQs.
Though that is more a UI inconvenience rather than a bug.

ronso0 · 2022-02-16T00:39:01Z

Okay, done:
now all settings are applied and saved to config only when hitting Apply (except Main Equalizer, as before).

IMO review should focus not focus on the code quality (as I just moved stuff around) but on manual testing and when changes are applied, since that is what this is all about. Cancel should cancel, Apply should apply (set COs and set config values).

When testing this, take care not to use your config from main. I had situations where EQs were not processed due to some weird CO being set to 0 (also in [MixerProfile], equivalent to [EqualizerRack1_[ChannelN]_Effect1], enabled) when switching between main and 2.3 Maybe I can figure out what that was.

Note:
I'll not be able to work on this or fix conflicts with main after Thursday, so either someone reviews this soonish and I'll merge to main ASAP -- or this has to wait for 3 weeks.

In main we can then discuss

if we want to remove the option to have deck-specific EQs
if Bypass EQs is still needed
if the main EQ should also be applied only when clicking Apply

…otApply so that slotNumDecksChanged uses the config values, not the default GUI state determined by the .ui file

ronso0 · 2022-02-16T18:33:40Z

Please don't panic, instead look at the single commits first.
You'll see most of what I did is moving all writing to config, setting COs and loading effects to slotApply() where it belongs, hence the lines diff.
You'll also see that this consolidation allowed to remove most of what you consider brittle / more complex than necessary: the entanglement of combobox stateChanged slots and slotPopulateDeckSelectors, restoring previous, with 'EQs effects only' unavailable EQ selections etc.
I think it's pretty straight-forward now: adjust settings, hit Apply or Cancel. No signal/slot auto-adjust magic anymore.
Please take a closer look and tell me what exactly you think could go wrong. You can put a6d99fc on top to have debug output for tracing when slots are called.

What I consider the actual bug here is that reading selections was inconsistent (from both .ui file defaults and config?) and saving/applying was done instantly all over the place.
I propose to fix exactly that in the stable 2.3 branch.

Without even considering the conflicts with main

Of course I'm not going to fix the conflicts chunk by chunk, that would be insane.
I'll git checkout --ours src/preferences/dialog/dlgprefeq* and re-apply the commits manually. That's faster and safer, since I'm into it atm.

ronso0 · 2022-02-17T10:39:18Z

I notice I will probably not have time to respond to reviews and merge to main before I'm travelling.
Marking as draft again, and finishing it when I'm back.

Holzhaus · 2022-02-17T10:46:15Z

I notice I will probably not have time to respond to reviews and merge to main before I'm travelling.
Marking as draft again, and finishing it when I'm back.

I don't understand, is this ready to review or not? If it is, I suggest to not mark it das draft so that people can review it until you are back.

ronso0 · 2022-02-17T10:52:49Z

Yes, ready for review. Though, there'll be non-trivial conflicts with main, and I want to avoid this being merged 'by accident', so "Finishing it" refers to me taking care of conflict resolution / re-implementation in main.

So, any reviewer aware of this may unmark this on his own, please. Thank you.

Be-ing · 2022-02-22T16:11:11Z

To clarify my previous comments: if someone else thinks it is a good idea to merge this to 2.3 and is motivated to thoroughly review & test the changes, go ahead. I don't think the risk is worth the minor benefits for a stable branch. I think it would be a better idea to rebase this on main and leave well enough alone on the stable branch.

ronso0 · 2022-03-24T13:19:57Z

All EQ settings are applied instantly, that itself is a bug because the EQ page is not working as expected.
Though, we can discuss if the shelf paramters should be applied instantly to simplify testing.

I can only recommend to review the entire EQ page in an IDE.Tthat needs to be done anyway, regardless if the changes go to 2.3 or main.
I did extensive manual tests with both QuickEffects and EQs. There are only improvements, no regression, and the EQ checkbox bug is fixed. The entire page is clearer and consistent now in terms of UX and signal-wise (except unchanged Master EQ behaviour).

ronso0 · 2022-04-11T20:19:21Z

Officially ready for review.
My offer to port the changes to main still stands.
Though, if I'm not available and someone needs to merge 2.3 into main in a hurry, simply resolve conflicts with git checkout --ours src/preferences/dialog/dlgprefeq*

github-actions · 2022-07-11T00:07:20Z

This PR is marked as stale because it has been open 90 days with no activity.

daschuer

I have reviewed an tested it and the code locks good and works like expected.

Before merge, I like to clarify the general approach/use case for this:

Due to this PR the frequency corners are no longer applied immediately.
They where original applied immediately to test them instantly:
- Kill Low pass
- Move the slider until it sound good
I understand that this does not fit to the overall concept of the preferences and might be surprising,
The main EQ is still applied directly even if you change the center frequency with the parametric EQ which is the same like the corner frequencies.
I do consider no longer "applying the frequency corners instantly" not as a 2.3 bugfix
Tweaking the frequency corners is only a on-time issue
Tweaking the main EQ will be done on each venue

This means we have here the issue between "tewaking EQ" vs "manage preferences". This PR shifts the issue a bit but does not solve it.

I am open to merge this anyway as long it fits to future plans.
What are they?

We may consider following changes:

Revert the to instant adoption of frequency corners, to keep the impact of this PR small
Add a label that describes the behaviour
Add a checkbox "[x] Apply changes immediately"
Move EQ slider to a separate pop-up window ....

What do you think?

ronso0 · 2022-08-10T10:40:29Z

Indeed, applying the slider values immediatly is probably desired as that is more practical. So the actual bug IMO is not that the parameters are applied immediatly but that they are written to the config immediatly.

Right now, I'd rather close this, and try to fix

before, "Use the same EQ for all decks" was reset to true, overriding the config value.
Noticed here https://mixxx.discourse.group/t/mixx-strongly-began-to-heavily-load-the-system-linux/24068/29

(in a rather hackish way) in 2.3.

Then rework this page for main so that all values are written to config only in slotApply(), so that Cancel (revert to previous state) is useful again.

daschuer · 2022-08-10T12:21:42Z

Ok, thank you.

ronso0 added the minor bug label Feb 7, 2022

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

uklotzde reviewed Feb 7, 2022

View reviewed changes

ronso0 marked this pull request as draft February 7, 2022 23:47

ronso0 force-pushed the pref-eq-reset branch 3 times, most recently from eb79d46 to 4c46cc0 Compare February 16, 2022 00:32

ronso0 added 9 commits February 16, 2022 02:24

Pref EQ: load settings only in init/slotUpdate, apply only in init/sl…

46b690e

…otApply so that slotNumDecksChanged uses the config values, not the default GUI state determined by the .ui file

Pref EQ: remove unneeded bools and functions

d032ef0

Pref EQ: load eq/effect selection in separate function

13a8dad

Pref EQ: read correct checkbox to clear Deck1 label

516a9d0

Pref EQ: remove obsolete UI restoration, update combobox iterators

5be3a2a

Pref EQ, loadSettings: consolidate EQ

8e41651

Pref EQ: in slotApply, write all loaded EQs/effects to config

180a329

Pref EQ: move deck EQ options above Master EQ

65aa3ee

Pref EQ: use const strings for ConfigKeys

f78151b

ronso0 marked this pull request as ready for review February 16, 2022 18:33

ronso0 changed the title ~~Pref EQ: load settings earlier to not use single EQ default~~ Pref EQ: apply & save settings on in slotApply() Feb 17, 2022

ronso0 marked this pull request as draft February 17, 2022 10:39

ronso0 changed the title ~~Pref EQ: apply & save settings on in slotApply()~~ Pref EQ: apply & save settings only in slotApply() Apr 8, 2022

ronso0 mentioned this pull request Apr 11, 2022

Pref controller: prevent false-positive 'dirty' flag #4721

Merged

ronso0 marked this pull request as ready for review April 11, 2022 20:16

ronso0 added the preferences label Apr 11, 2022

github-actions bot added the stale Stale issues that haven't been updated for a long time. label Jul 11, 2022

ronso0 mentioned this pull request Aug 8, 2022

DlgPrefEQ: fix loading/saving main EQ parameters #4884

Merged

daschuer added needs review and removed stale Stale issues that haven't been updated for a long time. labels Aug 9, 2022

daschuer approved these changes Aug 10, 2022

View reviewed changes

ronso0 marked this pull request as draft August 10, 2022 10:41

ronso0 mentioned this pull request Aug 10, 2022

DlgPrefEQ: properly restore 'One EQ for all decks' setting #4886

Merged

mixxxbot mentioned this pull request Aug 22, 2022

unify behavior of preference options #7440

Open

ronso0 mentioned this pull request Feb 13, 2023

Preferences: join EQ and crossfader settings into new Mixer page #11272

Merged

ronso0 closed this Feb 23, 2023

ronso0 mentioned this pull request May 1, 2023

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

Merged

mixxxbot mentioned this pull request May 24, 2023

EQs and QuickEffect disabled by inaccessible controls #10737

Open

ronso0 deleted the pref-eq-reset branch August 21, 2023 22:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pref EQ: apply & save settings only in slotApply() #4667

Pref EQ: apply & save settings only in slotApply() #4667

ronso0 commented Feb 7, 2022 •

edited

uklotzde Feb 7, 2022

ronso0 Feb 7, 2022

ronso0 commented Feb 7, 2022

Be-ing commented Feb 7, 2022

ronso0 commented Feb 7, 2022

ronso0 commented Feb 8, 2022

Be-ing commented Feb 8, 2022

uklotzde commented Feb 8, 2022

ronso0 commented Feb 8, 2022

ywwg commented Feb 10, 2022

Be-ing commented Feb 10, 2022

ronso0 commented Feb 10, 2022

ronso0 commented Feb 10, 2022

Be-ing commented Feb 10, 2022

ronso0 commented Feb 10, 2022

ronso0 commented Feb 16, 2022

ronso0 commented Feb 16, 2022 •

edited

ronso0 commented Feb 17, 2022

Holzhaus commented Feb 17, 2022

ronso0 commented Feb 17, 2022 •

edited

Be-ing commented Feb 22, 2022

ronso0 commented Mar 24, 2022

ronso0 commented Apr 11, 2022

github-actions bot commented Jul 11, 2022

daschuer left a comment

ronso0 commented Aug 10, 2022

daschuer commented Aug 10, 2022

Pref EQ: apply & save settings only in slotApply() #4667

Pref EQ: apply & save settings only in slotApply() #4667

Conversation

ronso0 commented Feb 7, 2022 • edited

uklotzde Feb 7, 2022

Choose a reason for hiding this comment

ronso0 Feb 7, 2022

Choose a reason for hiding this comment

ronso0 commented Feb 7, 2022

Be-ing commented Feb 7, 2022

ronso0 commented Feb 7, 2022

ronso0 commented Feb 8, 2022

Be-ing commented Feb 8, 2022

uklotzde commented Feb 8, 2022

ronso0 commented Feb 8, 2022

ywwg commented Feb 10, 2022

Be-ing commented Feb 10, 2022

ronso0 commented Feb 10, 2022

ronso0 commented Feb 10, 2022

Be-ing commented Feb 10, 2022

ronso0 commented Feb 10, 2022

ronso0 commented Feb 16, 2022

ronso0 commented Feb 16, 2022 • edited

ronso0 commented Feb 17, 2022

Holzhaus commented Feb 17, 2022

ronso0 commented Feb 17, 2022 • edited

Be-ing commented Feb 22, 2022

ronso0 commented Mar 24, 2022

ronso0 commented Apr 11, 2022

github-actions bot commented Jul 11, 2022

daschuer left a comment

Choose a reason for hiding this comment

ronso0 commented Aug 10, 2022

daschuer commented Aug 10, 2022

ronso0 commented Feb 7, 2022 •

edited

ronso0 commented Feb 16, 2022 •

edited

ronso0 commented Feb 17, 2022 •

edited