Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Num deck steamline #14112

Open
wants to merge 9 commits into
base: 2.5
Choose a base branch
from
Open

Num deck steamline #14112

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d5a419e
make numberOfDecks() and friends int to match other functions
daschuer Jan 1, 2025
6687077
Make getDeck() and friends zero based and improve implementation.
daschuer Jan 1, 2025
82a2158
Don't call static numDecks() via base class pointer.
daschuer Jan 1, 2025
3bdfc03
Avoid String roundtrip in Auto DJ when looking up decks.
daschuer Jan 1, 2025
e460466
implement numberOf.. functions by direct CO access for speed up
daschuer Jan 1, 2025
4c1560e
move num functions to singleton PlayerInfo class, to get around stati…
daschuer Jan 1, 2025
9c24d1a
!fixup Swap COs and adjust name
daschuer Jan 3, 2025
c89f06e
Improve the comment of getDeckBase()
daschuer Jan 3, 2025
6ec70aa
Calc numTracks outside of the loop
daschuer Jan 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions src/mixer/playermanager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -587,33 +587,33 @@ BaseTrackPlayer* PlayerManager::getPlayer(const ChannelHandle& handle) const {
return nullptr;
}

Deck* PlayerManager::getDeck(unsigned int deck) const {
Deck* PlayerManager::getDeck(int deckIndex) const {
const auto locker = lockMutex(&m_mutex);
VERIFY_OR_DEBUG_ASSERT(deck > 0 && deck <= numDecks()) {
qWarning() << "getDeck() called with invalid number:" << deck;
VERIFY_OR_DEBUG_ASSERT(deckIndex >= 0 && deckIndex < m_decks.size()) {
qWarning() << "getDeck() called with invalid index:" << deckIndex;
return nullptr;
}
return m_decks[deck - 1];
return m_decks[deckIndex];
}

PreviewDeck* PlayerManager::getPreviewDeck(unsigned int libPreviewPlayer) const {
PreviewDeck* PlayerManager::getPreviewDeck(int previewDeckIndex) const {
const auto locker = lockMutex(&m_mutex);
if (libPreviewPlayer < 1 || libPreviewPlayer > numPreviewDecks()) {
VERIFY_OR_DEBUG_ASSERT(previewDeckIndex >= 0 && previewDeckIndex < m_previewDecks.size()) {
kLogger.warning() << "Warning getPreviewDeck() called with invalid index: "
<< libPreviewPlayer;
<< previewDeckIndex;
return nullptr;
}
return m_previewDecks[libPreviewPlayer - 1];
return m_previewDecks[previewDeckIndex];
}

Sampler* PlayerManager::getSampler(unsigned int sampler) const {
Sampler* PlayerManager::getSampler(int samplerIndex) const {
const auto locker = lockMutex(&m_mutex);
if (sampler < 1 || sampler > numSamplers()) {
VERIFY_OR_DEBUG_ASSERT(samplerIndex >= 0 && samplerIndex < m_samplers.size()) {
kLogger.warning() << "Warning getSampler() called with invalid index: "
<< sampler;
<< samplerIndex;
return nullptr;
}
return m_samplers[sampler - 1];
return m_samplers[samplerIndex];
}

TrackPointer PlayerManager::getLastEjectedTrack() const {
Expand Down
24 changes: 12 additions & 12 deletions src/mixer/playermanager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,19 +36,18 @@ class PlayerManagerInterface {
virtual BaseTrackPlayer* getPlayer(const QString& group) const = 0;
virtual BaseTrackPlayer* getPlayer(const ChannelHandle& channelHandle) const = 0;

// 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 Deck* getDeck(int deckIndex) const = 0;

virtual int numberOfDecks() const = 0;

// Get the preview deck by its deck number. Preview decks are numbered
// starting with 1.
virtual PreviewDeck* getPreviewDeck(unsigned int libPreviewPlayer) const = 0;
// Get the preview deck by its index.
virtual PreviewDeck* getPreviewDeck(int previewPlayerIndex) const = 0;

virtual int numberOfPreviewDecks() const = 0;

// Get the sampler by its number. Samplers are numbered starting with 1.
virtual Sampler* getSampler(unsigned int sampler) const = 0;
// Get the sampler by its index
virtual Sampler* getSampler(int samplerIndex) const = 0;

virtual int numberOfSamplers() const = 0;
};
Expand Down Expand Up @@ -102,23 +101,24 @@ class PlayerManager : public QObject, public PlayerManagerInterface {
// Get a BaseTrackPlayer (Deck, Sampler or PreviewDeck) by its handle.
BaseTrackPlayer* getPlayer(const ChannelHandle& handle) const override;

// Get the deck by its deck number. Decks are numbered starting with 1.
Deck* getDeck(unsigned int player) const override;
// Get the deck by its index.
Deck* getDeck(int deckIndex) const override;
// Return the number of players. Thread-safe.
static int numDecks();
int numberOfDecks() const override {
return numDecks();
}

PreviewDeck* getPreviewDeck(unsigned int libPreviewPlayer) const override;
// Get the preview deck by its index.
PreviewDeck* getPreviewDeck(int previewDeckIndex) const override;
// Return the number of preview decks. Thread-safe.
static int numPreviewDecks();
int numberOfPreviewDecks() const override {
return numPreviewDecks();
}

// Get the sampler by its number. Samplers are numbered starting with 1.
Sampler* getSampler(unsigned int sampler) const override;
// Get the sampler by its index.
Sampler* getSampler(int samplerIndex) const override;
// Return the number of samplers. Thread-safe.
static int numSamplers();
int numberOfSamplers() const override {
Expand Down
2 changes: 1 addition & 1 deletion src/mixer/samplerbank.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ bool SamplerBank::saveSamplerBankToPath(const QString& samplerBankPath) {
doc.appendChild(root);

for (int i = 0; i < m_pPlayerManager->numSamplers(); ++i) {
Sampler* pSampler = m_pPlayerManager->getSampler(i + 1);
Sampler* pSampler = m_pPlayerManager->getSampler(i);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we benefit form the 0 based index

if (!pSampler) {
continue;
}
Expand Down
6 changes: 3 additions & 3 deletions src/test/autodjprocessor_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,9 @@ class MockPlayerManager : public PlayerManagerInterface {

MOCK_CONST_METHOD1(getPlayer, BaseTrackPlayer*(const QString&));
MOCK_CONST_METHOD1(getPlayer, BaseTrackPlayer*(const ChannelHandle&));
MOCK_CONST_METHOD1(getDeck, Deck*(unsigned int));
MOCK_CONST_METHOD1(getPreviewDeck, PreviewDeck*(unsigned int));
MOCK_CONST_METHOD1(getSampler, Sampler*(unsigned int));
MOCK_CONST_METHOD1(getDeck, Deck*(int));
MOCK_CONST_METHOD1(getPreviewDeck, PreviewDeck*(int));
MOCK_CONST_METHOD1(getSampler, Sampler*(int));

int numberOfDecks() const {
return static_cast<int>(numDecks.get());
Expand Down
12 changes: 6 additions & 6 deletions src/test/playermanagertest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ class PlayerManagerTest : public MixxxDbTest, SoundSourceProviderRegistration {

TEST_F(PlayerManagerTest, UnEjectTest) {
// Ejecting an empty deck with no previously-recorded ejected track has no effect.
auto deck1 = m_pPlayerManager->getDeck(1);
auto deck1 = m_pPlayerManager->getDeck(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

deck1->slotEjectTrack(1.0);
ASSERT_EQ(nullptr, deck1->getLoadedTrack());

Expand All @@ -154,7 +154,7 @@ TEST_F(PlayerManagerTest, UnEjectTest) {
deck1->slotLoadTrack(pTrack2, false);

// Ejecting in an empty deck loads the last-ejected track.
auto deck2 = m_pPlayerManager->getDeck(2);
auto deck2 = m_pPlayerManager->getDeck(1);
ASSERT_EQ(nullptr, deck2->getLoadedTrack());
// make sure eject does not trigger 'unreplace'
QTest::qSleep(kUnreplaceDelay); // millis
Expand All @@ -166,7 +166,7 @@ TEST_F(PlayerManagerTest, UnEjectTest) {
// Loading a new track in a deck causes the old one to be ejected.
// That old track can be unejected into a different deck.
TEST_F(PlayerManagerTest, UnEjectReplaceTrackTest) {
auto deck1 = m_pPlayerManager->getDeck(1);
auto deck1 = m_pPlayerManager->getDeck(0);
// Load a track and the load another one
TrackPointer pTrack1 = getOrAddTrackByLocation(getTestDir().filePath(kTrackLocationTest1));
ASSERT_NE(nullptr, pTrack1);
Expand All @@ -186,7 +186,7 @@ TEST_F(PlayerManagerTest, UnEjectReplaceTrackTest) {
waitForTrackToBeLoaded(deck1);

// Ejecting in an empty deck loads the last-ejected track.
auto deck2 = m_pPlayerManager->getDeck(2);
auto deck2 = m_pPlayerManager->getDeck(1);
ASSERT_EQ(nullptr, deck2->getLoadedTrack());
// make sure eject does not trigger 'unreplace'
QTest::qSleep(kUnreplaceDelay);
Expand All @@ -201,7 +201,7 @@ TEST_F(PlayerManagerTest, UnEjectInvalidTrackIdTest) {
getTestDir().filePath(kTrackLocationTest1), TrackId(QVariant(10)));
ASSERT_NE(nullptr, pTrack);
m_pPlayerManager->slotSaveEjectedTrack(pTrack);
auto deck1 = m_pPlayerManager->getDeck(1);
auto deck1 = m_pPlayerManager->getDeck(0);
// Does nothing -- no crash.
// make sure eject does not trigger 'unreplace'
QTest::qSleep(kUnreplaceDelay);
Expand All @@ -211,7 +211,7 @@ TEST_F(PlayerManagerTest, UnEjectInvalidTrackIdTest) {

TEST_F(PlayerManagerTest, UnReplaceTest) {
// Trigger eject twice within 500 ms to undo track replacement
auto deck1 = m_pPlayerManager->getDeck(1);
auto deck1 = m_pPlayerManager->getDeck(0);
// Load a track
TrackPointer pTrack1 = getOrAddTrackByLocation(getTestDir().filePath(kTrackLocationTest1));
ASSERT_NE(nullptr, pTrack1);
Expand Down