Num deck streamline #14112

daschuer · 2025-01-03T00:24:27Z

This mainly fixes a lifetime issue with the non owning

static QAtomicPointer<ControlProxy> PlayerManager::m_pCOPNumDecks;

Since it is non owning it can become dangling. I have fixed it by moving it as a PollingControlProxy to the singleton PlayerInfo class

…c lifetime issues.

Swiftb0y · 2025-01-03T10:45:54Z

src/test/playermanagertest.cpp

-    auto deck1 = m_pPlayerManager->getDeck(1);
+    auto deck1 = m_pPlayerManager->getDeck(0);


our [ChannelN] COs are 1-based while this is now 0-based. Is that really an improvement? I'm concerned this will cause confusion and off-by-one errors later.

You point out the downside of the change.

The benefit is that it now matches PlayerManager::groupForDeck(i) and pevents cases where we have to + 1 before the call and - 1 after it.

Like here: (I will also place an inline comment)

for (int i = 0; i < m_pPlayerManager->numberOfSamplers(); ++i) {

In addition there was a naming clash with Index vs. Number.

In general we should stick to the rule to use 0 based counting inside the code base and 1 base in all user visible cases.

Please have a second look. If you still think we should keep the 1 based index here I will fix the issue in that direction.

Thank you for the explanation. I see the tension there. I too prefer 0-based indexing. I will take a second look.

daschuer · 2025-01-03T16:38:10Z

src/library/autodj/autodjprocessor.cpp

-        QString group = PlayerManager::groupForDeck(i);
-        BaseTrackPlayer* pPlayer = pPlayerManager->getPlayer(group);
+    for (int i = 0; i < pPlayerManager->numberOfDecks(); ++i) {
+        BaseTrackPlayer* pPlayer = pPlayerManager->getDeckBase(i);


Here we benefit form the 0 base.

daschuer · 2025-01-03T16:38:37Z

src/mixer/samplerbank.cpp

-    for (unsigned int i = 0; i < m_pPlayerManager->numSamplers(); ++i) {
-        Sampler* pSampler = m_pPlayerManager->getSampler(i + 1);
+    for (int i = 0; i < m_pPlayerManager->numberOfSamplers(); ++i) {
+        Sampler* pSampler = m_pPlayerManager->getSampler(i);


Here we benefit form the 0 based index

src/coreservices.cpp

Swiftb0y · 2025-01-03T20:12:00Z

src/mixer/playermanager.h

-    // Get the deck by its deck number. Decks are numbered starting with 1.
-    virtual Deck* getDeck(unsigned int player) const = 0;
+    // Get the deck by its deck index
+    virtual BaseTrackPlayer* getDeckBase(int deckIndex) const = 0;


Why this replacement?

This interface allows the unittest to return a mocked Deck instead of the real one. I will add a comment. Before getPlayer() has been used, which requires a QString group for lookup. I will add a comment.

Co-authored-by: Swiftb0y <[email protected]>

acolombier · 2025-02-14T09:53:50Z

src/mixer/playerinfo.cpp

@@ -14,10 +14,16 @@ constexpr int kPlayingDeckUpdateIntervalMillis = 2000;

 PlayerInfo* s_pPlayerInfo = nullptr;

+const QString kAppGroup = QStringLiteral("[App]");
+const QString kMasterGroup = QStringLiteral("[Master]");


Can we use [Main] instead here? I believe the [Master] group is deprecated

That's nothing we can do in a stable branch. We need to introduce an alias. I don't want to do it in this PR. Let's keep it separate.

acolombier · 2025-02-14T12:26:35Z

src/library/autodj/autodjprocessor.cpp

@@ -155,9 +155,8 @@ AutoDJProcessor::AutoDJProcessor(

    // TODO(rryan) listen to signals from PlayerManager and add/remove as decks


I know it's not related to your change, but anything we can do about this 10yo+ TODO?

acolombier · 2025-02-14T12:27:50Z

src/coreservices.cpp

-    for (int i = 0; i < (int)m_pPlayerManager->numDecks() && i < musicFiles.count(); ++i) {
+    const int numTracks = std::min(m_pPlayerManager->numberOfDecks(),
+            static_cast<int>(musicFiles.count()));
+    for (int i = 0; i < numTracks; ++i) {


What's the benefit of preincr vs postincr? Is this preferred?

i++ was the original one preferred in C and the origin of the name for C++. In C++ itself ++i is preferred if possible, because it returns the incremented value and not the original one which is considered to be faster in case of complex types, because the original one does not need to be kept in memory. In this particular case the optimized machine code will most likely be the same anyway because the return value is not used.

Thanks for the detailed explanation. I knew about the usecase where using the value, but I was wondering if there is reason why you would use this form when the value is irrelevant. Is that something we should unify? I know we have a few for-loop which use the two form, beside no apparent reason for switching between the two forms

I don't really mind. Both is correct and readable in this case IMHO.

…the base class

…ware of new decks.

daschuer · 2025-02-18T00:40:33Z

This should be now ready

daschuer added 6 commits January 3, 2025 00:21

make numberOfDecks() and friends int to match other functions

d5a419e

Make getDeck() and friends zero based and improve implementation.

6687077

Don't call static numDecks() via base class pointer.

82a2158

Avoid String roundtrip in Auto DJ when looking up decks.

3bdfc03

implement numberOf.. functions by direct CO access for speed up

e460466

move num functions to singleton PlayerInfo class, to get around stati…

4c1560e

…c lifetime issues.

github-actions bot added library autodj code quality browse controller backend labels Jan 3, 2025

Swiftb0y reviewed Jan 3, 2025

View reviewed changes

daschuer commented Jan 3, 2025

View reviewed changes

daschuer changed the base branch from main to 2.5 January 3, 2025 16:40

!fixup Swap COs and adjust name

9c24d1a

Swiftb0y reviewed Jan 3, 2025

View reviewed changes

Improve the comment of getDeckBase()

c89f06e

daschuer force-pushed the numDeckSteamline branch from 53ab2c0 to b47a282 Compare January 3, 2025 22:18

Calc numTracks outside of the loop

6ec70aa

Co-authored-by: Swiftb0y <[email protected]>

daschuer force-pushed the numDeckSteamline branch from b47a282 to 6ec70aa Compare January 3, 2025 22:44

acolombier reviewed Feb 14, 2025

View reviewed changes

daschuer added 2 commits February 18, 2025 00:20

move signals numberOfDecksChanged() and numberOfSamplersChanged() to …

cf2922d

…the base class

Introduce AutoDJProcessor::slotNumberOfDecksChanged to make Auto DJ a…

4675e7d

…ware of new decks.

ronso0 changed the title ~~Num deck steamline~~ Num deck streamline Feb 18, 2025

JoergAtGithub added the needs review label Mar 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Num deck streamline #14112

Num deck streamline #14112

daschuer commented Jan 3, 2025

Swiftb0y Jan 3, 2025

daschuer Jan 3, 2025

Swiftb0y Jan 3, 2025

daschuer Jan 3, 2025

daschuer Jan 3, 2025

Swiftb0y Jan 3, 2025

daschuer Jan 3, 2025

acolombier Feb 14, 2025

daschuer Feb 17, 2025

acolombier Feb 14, 2025

daschuer Feb 18, 2025

acolombier Feb 14, 2025

daschuer Feb 17, 2025

acolombier Feb 18, 2025

daschuer Feb 18, 2025

daschuer commented Feb 18, 2025

		auto deck1 = m_pPlayerManager->getDeck(1);
		auto deck1 = m_pPlayerManager->getDeck(0);

		@@ -155,9 +155,8 @@ AutoDJProcessor::AutoDJProcessor(

		// TODO(rryan) listen to signals from PlayerManager and add/remove as decks

Num deck streamline #14112

Are you sure you want to change the base?

Num deck streamline #14112

Conversation

daschuer commented Jan 3, 2025

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

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

daschuer commented Feb 18, 2025