Reintroduce the Quick effect selectors in the EQ preferences

[daschuer]

We need that to merge the effects_refactoring branch without a regression until the Skin effect GUI is in place. https://bugs.launchpad.net/mixxx/+bug/1948537

This does not revert to the 2.4 state where single effects where selected, It selects the effect chain presets and is fully aligned with the upcoming chain preset selectors in the skins.

This was already closed by @Be-ing without giving appropriate reasons.
#4485

I still think this is a good step forward and takes some pressure form all of us.
Please read also my reasons form https://bugs.launchpad.net/mixxx/+bug/1948537

What do other think?

[Swiftb0y]

I absolutely agree that we should keep the quickeffectselector in the EQ preferences until we have support for this in the GUI. @Be-ing Closing the previous PR without proper reasoning is inappropriate. If you insist that this has already been rejected, please support that fact by at least linking to the discussion where this was decided.

[Be-ing]

Adding technical debt instead of actually solving problems is not okay. It is really disappointing you chose to waste time on this instead of helping.

[Swiftb0y]

Right now, we have the problems, that the main branch is in a unreleasable state because there is no way for the average user to change the QuickEffect atm. That is the problem right now. In an effort to get the main branch into a releasable state, reverting the removal of that selector in the preferences would be the quickest way of solving the problem. You are arguing for the more work intensive option of actually getting the GUI to a point where this is possible outside the preferences. We don't know when that will happen and I dislike having main in this unreleasable state for longer than necessary. This is why I argue that this PR is a valid short-term solution.

[Be-ing]

This makes zero progress towards making the main branch releasable; it only adds technical debt that would have to be removed later.

No more bikeshedding. If this bothers you then actually help.

[Be-ing]

I refuse to contribute features if the bar to get anything merged is reimplementing it 4 times.

[Be-ing]

there is no way for the average user to change the QuickEffect atm

This is not true. You can switch to Tango then switch to another skin if you really want. If this is too much trouble for you, then help add the new widgets to the rest of the skins.

[Swiftb0y]

@daschuer what is your reasoning for favoring this approach over @Be-ing's suggestion of actually implementing the quickeffect changer in the skins where it ultimatively belongs?

[uklotzde]

there is no way for the average user to change the QuickEffect atm

This is not true. You can switch to Tango then switch to another skin if you really want. If this is too much trouble for you, then help add the new widgets to the rest of the skins.

I disagree. This is inaccessible for the regular user.

Why not include a temporary solution if someone has already taken care of it? If we properly annotate the bridging code with references to the open issue I consider this maintainable. It is not much code and restricted to a single UI class.

[Be-ing]

All this time spent bikeshedding could be spent actually adding the new widgets to other skins.

[Swiftb0y]

The bikeshedding is bipartisan here. Daschauer technically already did all the work for the stopgap solution. You are blocking/bikeshedding here because you consider your solution of actually adding the widgets to the skin superior.

We can just merge this PR as is and once we add all the widgets, we just need to revert this PR, should be pretty straightforward.

[Be-ing]

This does not help at all.

This is inaccessible for the regular user.

This is a development branch. If the standard for the development branch is perfection I will stop contributing.

[Holzhaus]

I refuse to contribute features if the bar to get anything merged is reimplementing it 4 times.
[...]
This is a development branch. If the standard for the development branch is perfection I will stop contributing.

I agree that perfection can be our goal, but not the requirement for the development branch. In fact, it's not even the standard for our releases (we did release 2.3.x even though we still have open issues at our bug tracker).

IIUC the effects refactoring branch aimed to remove stuff like the unnecessary EffectRack abstraction (YAGNI!), and opens more possibilities to combine effects.A side effect of that refactoring was that we can now use our own effect chains as quickeffects and an effect selector directly in the skins is a nice new feature.

It's arguably better to have a selector in the skins than putting that option into the preferences, but as you said I don't think we should raise the bar to "everything has to be absolutely perfect" and "you need to implement it for all 4 skins first" before we can make a release.

For that reason I agree with this PR as a good stopgap solution. If the new selectors have implemented for all 4 skins by the time we branch off 2.4, we can easily revert this. If not, we can still release Mixxx in good conscience.

[ronso0]

A few months ago I started working on the skin implementations but since not every skin has enough space that still needs some design decisions.
So thanks @daschuer for adding the stop gap solution which can easily be reverted once the GUI is updated.

Btw this needs to be rebased, checks are failing.

[Be-ing]

This makes no progress towards a release; it only adds code that would need to be removed before a release. Releases must have feature parity between skins; this should be self-evident.

@ronso0 could you open PRs with whatever you have started? Please make a separate PR for each skin.

[ronso0]

I'm currently traveling, so after rebase just merge this instead of wasting time talking about it.
there's a clear majority to do so.

[Be-ing]

Nobody has attempted to justify why it would be acceptable to make a release with different features in different skins.

[Be-ing]

I cannot help but observe that I am held to a different standard. This is not fair. When @daschuer says not to merge something, everyone else allows him to hold up the entire project indefinitely. When I do, apparently it doesn't matter.

[Be-ing]

I don't want to participate when different rules apply to different people.

[Holzhaus]

I cannot help but observe that I am held to a different standard. This is not fair. When @daschuer says not to merge something, everyone else allows him to hold up the entire project indefinitely. When I do, apparently it doesn't matter.

I don't want to participate when different rules apply to different people.

Why do you have the impression that it doesn't matter? It obviously matters since this PR is still unmerged. Instead of commenting "this bikeshedding helps nobody" and just hitting merge despite your concerns, we try to give reasons why we think that this makes sense to add.

Right now, the quick effect cannot be changed without switching skins multiple times, which is extremely inconvenient for everyone who is already using main. This PR would solve that issue for the time being, amd as soon as the effect selectors have been implemented we can simply revert the merge.

The alternative would be to stick with the current, bad situation and wait for someone to implement it. As @ronso0 already mentioned this is not trivial and I suppose it may take some time.

Nobody has attempted to justify why it would be acceptable to make a release with different features in different skins.

I vaguely remember that we didn't always have feature parity across all skins in the past. And to be honest, the reason why we aim for feature parity is because it would become impossible to use certain feature for users of skins that lag behind. In this case, the preferences option would still allow everyone to customize the quickeffect, it's just that it becomes more tedious to change during a set. For me, thast is an acceptable compromise.

[Swiftb0y]

Thank you @Holzhaus for taking the time to put our arguments into words. I 100% agree with your statements.

[ywwg]

LGTM, getting things back to a working state is a good stop-gap solution until the skins are updated.

[Be-ing]

extremely inconvenient for everyone who is already using main

Did anyone who uses main regularly actually change the Quick Effect from the preferences before? Did they do it frequently? Or are we talking about hypothetical people? Adding technical debt to fix things for hypothetical people is a bad idea.

I suppose it may take some time.

So what?

the reason why we aim for feature parity is because it would become impossible to use certain feature for users of skins that lag behind

Yes, right, otherwise we end up in a messy situation where controller mappings are coupled to skins, which actually happened in Mixxx 2.0.

In this case, the preferences option would still allow everyone to customize the quickeffect, it's just that it becomes more tedious to change during a set.

Changing it during a set is the point... so I think this PR is pointless.

[Be-ing]

Why do you have the impression that it doesn't matter?

Because I explicitly said multiple times to not do this and furthermore to stop escalating by making PRs for things that have already been explicitly rejected. I suggested how to help instead but @daschuer flagrantly ignored all of this and instead chose to present this nonsolution as somehow solving the problem. And somehow this is being treated as okay.

[ronso0]

Let's wrap this up to restore the easily accessible Quick effect configuration in the usual place until all skins are updated:
There's comprehensible reasoning to merge this and revert asap.
So far, no technical or UX-related reasons have been brought up that justify blocking this intermediate solution.

Due to unresolved (unresolvable?) dissent we need to vote in order to move on.

Merge? 👍 / 👎

[Be-ing]

I accept that I have been outvoted, but I can't help point out the unfair treatment here. When @daschuer is the only one opposed to merging something, he has been allowed to hold up everything for weeks or months regardless of what anyone else says, for example #4239 or #2920, sometimes without even an explanation (#2618).

[Swiftb0y]

I can understand that you feel treated unfairly. We're trying to avoid stagnating PRs by overruling vetos by voting more often. This is one of the first PRs where we're trying this. I understand this seems biased right now, but you'll see that this is not the case when we do this for more and more PRs.