Mixer controls pt2 - #15603
Conversation
38bb831 to
487396a
Compare
|
@mixxxdj/developers There were many conflicts so I need more eyes on this, especially the c++ changes. |
|
This PR is marked as stale because it has been open 90 days with no activity. |
|
I can throw copilot at this if you resolve the conflicts |
487396a to
5435d47
Compare
|
I rebased, @ywwg go ahead! |
There was a problem hiding this comment.
Pull request overview
This PR is part of #11931 (removal of offensive terms from codebase) and renames mixer controls from [Master] to [Mixer] group. It updates EngineMixer to remove the hardcoded group parameter, moves control objects to the new [Mixer] group, renames controls for clarity (e.g., headMix → headphone_mix, gain → main_gain), and adds backward compatibility aliases for the legacy [Master] group.
Changes:
- EngineMixer constructor no longer takes group parameter; uses
[Mixer]internally - Core mixer controls moved from
[Master]to[Mixer]group - Control names updated for consistency and clarity
- Backward compatibility aliases added for
[Master]group - Tests, skins, and controller mappings updated to use new names
Reviewed changes
Copilot reviewed 209 out of 209 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/engine/enginemixer.h/cpp | Removed group parameter from constructor, moved controls to [Mixer] group, added aliases |
| src/engine/enginetalkoverducking.h/cpp | Added alias support, updated control names |
| src/test/*.cpp/h | Updated tests to use [Mixer] group and new control names |
| src/mixer/playerinfo.cpp | ISSUE: Still uses [Master] for crossfader |
| src/preferences/dialog/*.cpp | Updated to use [Mixer] group and new control names |
| src/controllers/controlpickermenu.cpp | Updated control picker to use new names |
| res/skins/* | ISSUES: Several inconsistencies in control names |
| res/controllers/* | ISSUES: Some controller scripts not fully updated |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
TODO
|
|
One question before I do a mass-renaming: |
|
I addressed all valid review comments. Question remains: do we want to mass-reformat mappings because of the eslint complaints about indentation, missing whitespaces etc.? |
|
Since this is already pretty much a no-op refactor, I am ok with fixing the eslint issues |
|
we mostly use the singluar form of "headphone" in the code |
oh, we need #16049 in order to get a patch file. Btw applying the eslint fixes leads to inconsistencies, for example: engine.softTakeover("[Mixer]", "crossfader", true); // <-- fix
engine.softTakeover("[Mixer]","crossfader",true); // <-- old code
engine.softTakeover("[Channel1]","volume",true);
engine.softTakeover("[Channel1]","rate",true);
engine.softTakeover("[Channel2]","volume",true);
engine.softTakeover("[Channel2]","rate",true);
engine.softTakeover("[Sampler1]","volume",true);and also nonsense proposals like this CMDMM.crossfader = new components.Pot({
midi: [MIDI.CC,0x40],
inKey: "crossfader",
group: "[Mixer]", // eslint breaks indentationI'll apply what makes sense IMO and ignore the rest. |
| EXPECT_DOUBLE_EQ(indicator500ms.get(), indicator500msLegacy.get()); | ||
| } | ||
|
|
||
| TEST_F(ControlObjectAliasTest, EngineMixer) { |
There was a problem hiding this comment.
I think I fixed that in 5842d3b
CI is stuck though.. will push again
There was a problem hiding this comment.
Fixed. The alias for talkover_mix is talkoverDucking
There was a problem hiding this comment.
Not sure if this was the issue, but ducking aliases were added twice, once in EngineTalkoverDucking ctor and again in EngineMixer by calling EngineTalkoverDucking::addAlias().
Remove the duplication, let's see how this works
(aah, forgot to append something.. pushed again)
7550b2d to
a523693
Compare
84db5b8 to
eae6851
Compare
|
Nice, all green. |
eae6851 to
b8a3440
Compare
|
CI is all green, pre-commits format fixes are mostly nonsense (read: they are formally correct but break existing indentaion schemes, and fixing those would lead to huge diffs) |
Rebased version of #12023
Part of #11931.
There were many conflicts, not sure if I resolved all correctly.
(there may even be unresolved ones in res/controllers)
Targetting 2.7 so we have the beta period to find regressions in mappings.
Next up: remove only remaining [Master] and [MasterOutput] from the effects engine