CO Renaming (Pt. 1) #11980

Holzhaus · 2023-09-13T22:36:48Z

This PR is part of the ongoing effort to remove offensive language from the Mixxx codebase (#11931).

Here is the first bunch of renamings discussed in #11960 (comment). I removed the num_mics_configured CO since it's used nowhere outside the EngineMixer class (not even mappings or skins) and can be replaced with a simple member variable.

Old Name	New Name	Alias?
`[Master],guiTick50ms`	`[App],gui_tick_50ms_period_s`	✔️ (although undocumented)
`[Master],guiTickTime`	`[App],gui_tick_full_period_s`	✔️ (although undocumented)
`[Master],indicator_250millis`	`[App],indicator_250ms`	✔️
`[Master],indicator_500millis`	`[App],indicator_500ms`	✔️
`[Master],keylock_engine`	`[App],keylock_engine`	❌
`[Master],num_auxiliaries`	`[App],num_auxiliaries`	✔️ (although undocumented)
`[Master],num_decks`	`[App],num_decks`	✔️
`[Master],num_microphones`	`[App],num_microphones`	✔️ (although undocumented)
`[Master],num_mics_configured`	Removed	n/A
`[Master],num_preview_decks`	`[App],num_preview_decks`	✔️
`[Master],num_samplers`	`[App],num_samplers`	✔️
`[Master],samplerate`	`[App],samplerate`	✔️

Swiftb0y · 2023-09-13T22:51:22Z

While were at it, I'd also like to propose to retire the millis suffix. We had that as a suffix for constants and member variables for some time, but this will get obsolete with <chrono> types and literals fast. So I'd propose to stick with the ms actually since that mirrors std::chrono_literals. Wdyt?

src/engine/enginebuffer.cpp

src/engine/enginemixer.h

JoergAtGithub · 2023-09-13T23:12:02Z

src/test/autodjprocessor_test.cpp

-            : numDecks(ConfigKey("[Master]", "num_decks"), true),
-              numSamplers(ConfigKey("[Master]", "num_samplers"), true),
-              numPreviewDecks(ConfigKey("[Master]", "num_preview_decks"),
-                              true) {
+            : numDecks(ConfigKey(kAppGroup, QStringLiteral("num_decks")), true),
+              numSamplers(ConfigKey(kAppGroup, QStringLiteral("num_samplers")), true),
+              numPreviewDecks(ConfigKey(kAppGroup, QStringLiteral("num_preview_decks")),
+                      true) {


Please only add a test for the new CO name and leave the existing one as it is

The plan was to remove the word from the codebase as much as possible which includes tests. Also, these tests are supposed to check if the AutoDJ processor works correctly, not if old CO names still work. I agree that it makes sense to test this, too, but that should be a different test IMHO.

I was planning to add a new test that checks if the aliases exist. This would ideally go into coreservicestest.cpp since most other tests do not create all COs. Unfortunately this test is broken at the moment. I'm currently looking into it.

this would / could result in warnings raised by tests though if we decided to implement the warnings on deprecated CO access.

We need to ensure the backward compatibility of our legacy API. I'm open for any test implementation, that ensures this.

Holzhaus · 2023-09-13T23:25:44Z

While were at it, I'd also like to propose to retire the millis suffix. We had that as a suffix for constants and member variables for some time, but this will get obsolete with <chrono> types and literals fast. So I'd propose to stick with the ms actually since that mirrors std::chrono_literals. Wdyt?

Okay for me. By the way, should I also rename gui_tick_time to gui_tick_s then? I think it's in seconds if I remember correctly.

Swiftb0y · 2023-09-13T23:32:03Z

Okay for me. By the way, should I also rename gui_tick_time to gui_tick_s then? I think it's in seconds if I remember correctly.

Sure. though I didn't check either, never heard of that CO before tbh...

daschuer · 2023-09-14T08:30:03Z

'guiTickTime' and 'guiTick50ms' have are both in seconds in terms of std::chrono.
The difference is that the guiTickTime is advanced in refresh rate steps and guiTick50ms is advanced in 50ms steps. The diffrence is the period in terms of chono.

So we may think of
'gui_full_period_tick_s'
'gui_50ms_period_tick_s'

daschuer · 2023-09-14T08:36:48Z

Derived form that are
'indicator_250millis'
'indicator_500millis'

They just cycle 0.0 ad 1.0.
Rename as well?

Swiftb0y

LGTM

Swiftb0y · 2023-09-14T13:57:42Z

src/util/timer.cpp

 GuiTickTimer::GuiTickTimer(QObject* pParent)
        : QObject(pParent),
          m_pGuiTick(make_parented<ControlProxy>(
-              "[Master]", "guiTickTime", this)),
+                  QStringLiteral("[App]"), QStringLiteral("gui_tick_time"), this)),
          m_bActive(false) {
 }


this was removed anyway in #11850, just fyi

Swiftb0y · 2023-09-14T13:59:47Z

src/waveform/guitick.cpp

+            ConfigKey(kAppGroup, QStringLiteral("gui_tick_full_period_s")));
+    m_pCOGuiTickTime->addAlias(ConfigKey(kLegacyGroup, QStringLiteral("guiTickTime")));
+    m_pCOGuiTick50ms = std::make_unique<ControlObject>(
+            ConfigKey(kAppGroup, QStringLiteral("gui_tick_50ms_period_s")));
+    m_pCOGuiTick50ms->addAlias(ConfigKey(kLegacyGroup, QStringLiteral("guiTick50ms")));


just for the future, we should probably not document these are they will be obsolete with QML, or rather they would have to be reimplemented.

Swiftb0y · 2023-09-14T14:01:52Z

src/widget/wtracktableview.cpp

-    m_pCOTGuiTick = new ControlProxy("[Master]", "guiTick50ms", this);
+    m_pCOTGuiTick = new ControlProxy(
+            QStringLiteral("[App]"), QStringLiteral("gui_tick_50ms_period_s"), this);
    m_pCOTGuiTick->connectValueChanged(this, &WTrackTableView::slotGuiTick50ms);


this dependency is... interesting... worrisome?
Anyways, obsolete with QML.

Swiftb0y · 2023-09-14T14:03:27Z

res/controllers/Traktor-Kontrol-S4-MK3.js

-        const meterConnection = engine.makeConnection("[Master]", "guiTick50ms", function(_value) {
+        const meterConnection = engine.makeConnection("[App]", "gui_tick_50ms_period_s", function(_value) {


we should probably track this in an issue on the QML project board, otherwise this will silently break under QML. putting this on an engine.beginTimer instead should be sufficient.

Swiftb0y

LGTM. last chance for anyone to object.

Holzhaus · 2023-09-14T23:24:11Z

I was planning to add tests for the legacy aliases as @JoergAtGithub requested, but the CoreServicesTest (which would be the ideal location for such a test because it creates all COs) does not work on CI. I'll try to find another way.

JoergAtGithub · 2023-09-14T23:29:49Z

I was planning to add tests for the legacy aliases as @JoergAtGithub requested, but at the CoreServicesTest (which would be the ideal location for such a test because it creates all COs) does not work on CI. I'll try to find another way.

I appreciate your effort! I think it's safer to have this test case in place before merging this. Otherwise this is LGTM!

Holzhaus · 2023-09-16T13:46:04Z

src/test/controlobjectaliastest.cpp

+class ControlObjectAliasTest : public MixxxTest {
+};
+
+#if QT_VERSION < QT_VERSION_CHECK(6, 0, 0)


I think this needs to be removed when merging into main, right?

Holzhaus · 2023-09-16T13:46:50Z

I was planning to add tests for the legacy aliases as @JoergAtGithub requested, but at the CoreServicesTest (which would be the ideal location for such a test because it creates all COs) does not work on CI. I'll try to find another way.

I appreciate your effort! I think it's safer to have this test case in place before merging this. Otherwise this is LGTM!

Tests are in place now.

JoergAtGithub

LGTM! Waiting for CI

JoergAtGithub · 2023-09-16T14:40:52Z

Thank you!

Swiftb0y · 2023-09-16T14:51:39Z

Thank you as well.

Holzhaus · 2023-09-17T15:12:13Z

mixxxdj/manual#579

Holzhaus added the code quality label Sep 13, 2023

github-actions bot added controller mappings library skins ui labels Sep 13, 2023

Swiftb0y reviewed Sep 13, 2023

View reviewed changes

src/engine/enginebuffer.cpp Show resolved Hide resolved

Holzhaus mentioned this pull request Sep 13, 2023

Remove offensive language (Pt. 3) #11960

Draft

Swiftb0y reviewed Sep 13, 2023

View reviewed changes

src/engine/enginemixer.h Show resolved Hide resolved

JoergAtGithub requested changes Sep 13, 2023

View reviewed changes

Holzhaus mentioned this pull request Sep 14, 2023

Use QLatin1String for ConfigKey #11983

Open

Holzhaus force-pushed the controlobject-splitup branch from 4692347 to 87407d6 Compare September 14, 2023 12:02

Swiftb0y approved these changes Sep 14, 2023

View reviewed changes

Holzhaus added 6 commits September 14, 2023 22:33

refactor(waveform/guitick): Rename guiTick COs

4957f89

refactor(controlindicator): Move indicator_ COs to [App] group

859bd45

refactor(enginemixer): Move samplerate CO to [App] group

cfca585

refactor(engine): Move keylock_engine CO into [App] group

375e0bc

refactor(playermanager): Move player num_* COs to [App] group

f324542

refactor(enginemixer): Remove unnecessary [Master],num_mics_configured

a086ce3

Holzhaus force-pushed the controlobject-splitup branch from 87407d6 to a086ce3 Compare September 14, 2023 20:34

Swiftb0y approved these changes Sep 14, 2023

View reviewed changes

github-actions bot added the build label Sep 16, 2023

test: Add ControlObjectAliasTest test to ensure backwards compat

08f3696

Holzhaus force-pushed the controlobject-splitup branch from 2ff9a88 to 08f3696 Compare September 16, 2023 13:31

Holzhaus commented Sep 16, 2023

View reviewed changes

Holzhaus requested a review from JoergAtGithub September 16, 2023 14:13

JoergAtGithub approved these changes Sep 16, 2023

View reviewed changes

JoergAtGithub merged commit e336dfa into mixxxdj:2.4 Sep 16, 2023
10 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CO Renaming (Pt. 1) #11980

CO Renaming (Pt. 1) #11980

Holzhaus commented Sep 13, 2023 •

edited

Loading

Swiftb0y commented Sep 13, 2023

JoergAtGithub Sep 13, 2023

Holzhaus Sep 13, 2023

Swiftb0y Sep 13, 2023

JoergAtGithub Sep 13, 2023

Holzhaus commented Sep 13, 2023 •

edited

Loading

Swiftb0y commented Sep 13, 2023

daschuer commented Sep 14, 2023

daschuer commented Sep 14, 2023

Swiftb0y left a comment

Swiftb0y Sep 14, 2023

Swiftb0y Sep 14, 2023

Swiftb0y Sep 14, 2023

Swiftb0y Sep 14, 2023

Swiftb0y left a comment

Holzhaus commented Sep 14, 2023 •

edited

Loading

JoergAtGithub commented Sep 14, 2023

Holzhaus Sep 16, 2023

Holzhaus commented Sep 16, 2023

JoergAtGithub left a comment

JoergAtGithub commented Sep 16, 2023

Swiftb0y commented Sep 16, 2023

Holzhaus commented Sep 17, 2023

		const meterConnection = engine.makeConnection("[Master]", "guiTick50ms", function(_value) {
		const meterConnection = engine.makeConnection("[App]", "gui_tick_50ms_period_s", function(_value) {

CO Renaming (Pt. 1) #11980

CO Renaming (Pt. 1) #11980

Conversation

Holzhaus commented Sep 13, 2023 • edited Loading

Swiftb0y commented Sep 13, 2023

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

Holzhaus commented Sep 13, 2023 • edited Loading

Swiftb0y commented Sep 13, 2023

daschuer commented Sep 14, 2023

daschuer commented Sep 14, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y left a comment

Choose a reason for hiding this comment

Holzhaus commented Sep 14, 2023 • edited Loading

JoergAtGithub commented Sep 14, 2023

Choose a reason for hiding this comment

Holzhaus commented Sep 16, 2023

JoergAtGithub left a comment

Choose a reason for hiding this comment

JoergAtGithub commented Sep 16, 2023

Swiftb0y commented Sep 16, 2023

Holzhaus commented Sep 17, 2023

Holzhaus commented Sep 13, 2023 •

edited

Loading

Holzhaus commented Sep 13, 2023 •

edited

Loading

Holzhaus commented Sep 14, 2023 •

edited

Loading