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.

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

[WIP] Livemetadata PR #1675

Closed
wants to merge 79 commits into from
Closed
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
79 commits
Select commit Hold shift + click to select a range
32cece7
Added .vscode on gitignore
davidhm May 5, 2018
ba829a2
First few files
davidhm May 14, 2018
5169748
So far manual tests are working
davidhm May 16, 2018
3d8c501
Pre automatic tests
davidhm May 18, 2018
523d09b
Using github as a backup
davidhm May 18, 2018
23ef862
Error when compiling moc generated cpp
davidhm May 20, 2018
0a4a4d6
WIP: Automatic tests
davidhm May 23, 2018
d203e85
Moved timers away from Track object
davidhm May 24, 2018
1a6bc0d
Pre volume scrobbling
davidhm May 26, 2018
ff27795
Pre tests, scrobbling manager
davidhm May 27, 2018
db2db23
Deadlock
davidhm May 30, 2018
7f55150
Solved the deadlock, simple tests pass
davidhm Jun 1, 2018
121443e
WIP: Adding file listener
davidhm Jun 2, 2018
2cd5bbc
Fixed file buffer, it's automatically updated now
davidhm Jun 3, 2018
b9c4a6c
[Untested] Fixed file info, modified metadatabroadcast
davidhm Jun 5, 2018
e10df9e
[WIP] Adding tests for scrobbling manager
davidhm Jun 7, 2018
c1de7db
Added first test for scrobbling manager
davidhm Jun 9, 2018
50019b0
File listener template + factory
davidhm Jun 10, 2018
d805306
Scrobbling tests done
davidhm Jun 12, 2018
4e714d5
[WIP] Adding file preferences
davidhm Jun 12, 2018
b3f8ea8
Preferences mock-up
davidhm Jun 13, 2018
8cd9c16
Changed everything to a weak pointer
davidhm Jun 13, 2018
d4b9aa5
Added file listener path in options
davidhm Jun 14, 2018
ec98ef6
[WIP] Persistent config now playing file
davidhm Jun 14, 2018
0209b4a
[WIP] Requested changes
davidhm Jun 15, 2018
2d8f4a2
Adding new tab
davidhm Jun 17, 2018
88c4819
Added mock-up in preferences
davidhm Jun 17, 2018
ab8b524
Table view mockup
davidhm Jun 18, 2018
97daf77
Pre changing prefmetadata class
davidhm Jun 19, 2018
4bc2de3
Added file settings
davidhm Jun 20, 2018
49b8b93
[Untested] Added options to file listener
davidhm Jun 20, 2018
567e9b5
Modified settings, added concurrency
davidhm Jun 23, 2018
5748fa9
Added dedicated thread
davidhm Jun 26, 2018
3a677b6
Modified author and title string
davidhm Jun 26, 2018
9c43d09
Merge branch 'master' into Livemetadata
davidhm Jun 26, 2018
2c21183
Added connections to lambda expressions in player manager, file liste…
davidhm Jun 28, 2018
a6d67d7
Fixed tests failing
davidhm Jun 29, 2018
ba8a6fc
Added mock network manager, gotta test with mock server
davidhm Jul 1, 2018
6b7f207
Now listening scrobbles work with ListenBrainz
davidhm Jul 5, 2018
f7c6a83
Deleted log file
davidhm Jul 5, 2018
1057b06
Merge branch 'master' into Livemetadata
davidhm Jul 6, 2018
152925c
ListenBrainz full scrobbles work now too
davidhm Jul 6, 2018
ad43628
Fixed double delete in developer mode
davidhm Jul 7, 2018
751f567
Forgot to compile
davidhm Jul 7, 2018
64d35c5
Modified metadata file options
davidhm Jul 7, 2018
ddfb859
Editable combobox
davidhm Jul 9, 2018
e4dc8df
Added mpris stub, not working
davidhm Jul 11, 2018
a1ad4c6
MPRIS now reflects the playback state
davidhm Jul 12, 2018
ee4c5a5
Disabled failing test until a solution is found
davidhm Jul 13, 2018
4571d43
Fixed broken tests, continuing MPRIS implementation
davidhm Jul 18, 2018
185d2f2
Fixed non compiling build
davidhm Jul 18, 2018
0bc22be
Segfault on weak_ptr
davidhm Jul 19, 2018
0af2279
Fixed eject bug
davidhm Jul 19, 2018
6be0042
Mpris reflects AutoDJ enabled
davidhm Jul 19, 2018
62eae49
Pause, play and go next MPRIS functions work
davidhm Jul 21, 2018
c2317c7
Fixed correct fade-in in MPRIS
davidhm Jul 22, 2018
8b5e54a
Added volume, playback status and loop status as well as a LINUX define
davidhm Jul 23, 2018
cac8c1f
Moved MPRIS into a feature
davidhm Jul 24, 2018
6807838
Added MPRIS macro in includes too
davidhm Jul 24, 2018
d6880a1
MPRIS is seekable now
davidhm Jul 24, 2018
32e565a
MPRIS broadcasts current track and rate works now too
davidhm Jul 25, 2018
1b77c8b
Modified linked lists into hash maps
davidhm Aug 1, 2018
7ba10a7
Added space after commas
davidhm Aug 1, 2018
94ab525
Changed ref ampersand and disabled non working test
davidhm Aug 1, 2018
5e8fb60
Added more button in encoding combo box
davidhm Aug 3, 2018
41bdfb9
Resized combobox
davidhm Aug 6, 2018
0def3e3
Merge remote-tracking branch 'upstream/master' into Livemetadata
davidhm Aug 6, 2018
9fe5277
Added few UI suggestions
davidhm Aug 6, 2018
97271d8
Missing include
davidhm Aug 7, 2018
f4b9649
Added cover art to mpris
davidhm Aug 11, 2018
dde6c47
Added cover art to MPRIS player
davidhm Aug 12, 2018
e1cfd7a
Fixed few things
davidhm Aug 14, 2018
69a3020
Deleting all cover art files
davidhm Aug 17, 2018
7af4dd7
Single file image
davidhm Aug 17, 2018
170e6c9
Cover art file is now QTemporaryFile
davidhm Aug 18, 2018
8c6fa58
Revamped encoding combobox
davidhm Aug 18, 2018
899f207
Fixed some cover URL generation issues
daschuer Aug 19, 2018
4a13665
Small fixes
davidhm Aug 20, 2018
39dc901
Merge pull request #1 from daschuer/Livemetadata
davidhm Aug 20, 2018
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
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,5 @@ lib/*/*.lib
lib/*/lib/*.lib

lib/qtscript-bytearray/*.cc

*.vscode
Copy link
Member

@Holzhaus Holzhaus Aug 27, 2019

Choose a reason for hiding this comment

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

This file is not created by Mixxx or its build system. Adding patterns for every conceivable editor/IDE to this project's gitignore is neither desirable nor feasible. You should add it to your global gitignore instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, we should stop adding these editor generated files to our shared .gitignore and remove the ones that are already there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was already mentioned in that PR ages ago. I can't seem to find it.

7 changes: 7 additions & 0 deletions build/depends.py
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,7 @@ def configure(self, build, conf):
class TestHeaders(Dependence):
def configure(self, build, conf):
build.env.Append(CPPPATH="#lib/gtest-1.7.0/include")
build.env.Append(CPPPATH="#lib/gmock-1.7.0/include")

class FidLib(Dependence):
def sources(self, build):
Expand Down Expand Up @@ -663,6 +664,10 @@ def sources(self, build):
"control/controlttrotary.cpp",
"control/controlencoder.cpp",

"broadcast/metadatabroadcast.cpp",
"broadcast/scrobblingmanager.cpp",
"broadcast/filelistener.cpp",

"controllers/dlgcontrollerlearning.cpp",
"controllers/dlgprefcontroller.cpp",
"controllers/dlgprefcontrollers.cpp",
Expand Down Expand Up @@ -1093,6 +1098,8 @@ def sources(self, build):
"track/trackinfo.cpp",
"track/trackrecord.cpp",
"track/trackref.cpp",
"track/trackplaytimers.cpp",
"track/tracktiminginfo.cpp",

"mixer/auxiliary.cpp",
"mixer/baseplayer.cpp",
Expand Down
28 changes: 28 additions & 0 deletions src/broadcast/filelistener.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@

#include "broadcast/filelistener.h"


FileListener::FileListener(const QString &path) :
m_file(path)
{
Copy link
Member

Choose a reason for hiding this comment

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

no new line before {

QFileInfo fileInfo(path);
qDebug() << "Absolute path " << fileInfo.absoluteFilePath();
qDebug() << "File exists: " << fileInfo.exists();
m_file.open(QIODevice::WriteOnly |
QIODevice::Text |
QIODevice::Unbuffered);

}

void FileListener::broadcastCurrentTrack(TrackPointer pTrack) {
if (!pTrack)
return;
QTextStream stream(&m_file);
m_file.resize(0);
Copy link
Member

Choose a reason for hiding this comment

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

A comment helps here:
// clear file

stream << "Now listening " << pTrack->getTitle();
stream << " by " << pTrack->getArtist();
}

void FileListener::scrobbleTrack(TrackPointer pTrack) {

}
13 changes: 13 additions & 0 deletions src/broadcast/filelistener.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#pragma once

#include <QFile>
#include "broadcast/scrobblingservice.h"

class FileListener: public ScrobblingService {
public:
FileListener(const QString &path);
void broadcastCurrentTrack(TrackPointer pTrack) override;
Copy link
Contributor

Choose a reason for hiding this comment

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

Inheritance is unnecessary. The file listener could be implemented as a standalone class with 2 slots, connected to the ScrobblingService.

Copy link
Contributor Author

@davidhm davidhm Jun 5, 2018

Choose a reason for hiding this comment

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

The idea is to have N services that connect to the scrobbling manager that answer to both slots. If I take one implementation out of the hierarchy then I have to update that separately. It also doesn't allow me to mock up the file listener. I don't see any advantage on breaking the inheritance. Can you elaborate on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

The scrobbling manager can offer 2 public signals for this purpose and dependent services can connect to them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah but who owns the services? I was thinking of getting a list of services from the user settings, pass them to the scobbling manager and then simply keep them updated. I find the manager owning a list of services to be easier to mantain than creating a connection for every service and storing them somewhere else.

void scrobbleTrack(TrackPointer pTrack) override;
private:
QFile m_file;
};
27 changes: 27 additions & 0 deletions src/broadcast/metadatabroadcast.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#include "metadatabroadcast.h"
#include "mixer/playerinfo.h"

MetadataBroadcaster::MetadataBroadcaster(TrackTimers::ElapsedTimer *timer) :
m_pElapsedTimer(timer)
{

}

void MetadataBroadcaster::slotReadyToBeScrobbled(TrackPointer pTrack) {

}

void MetadataBroadcaster::slotNowListening(TrackPointer pTrack) {
for (auto &service : m_scrobblingServices) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is essentially a manually implemented synchronous pub/sub communication. Why don't you use Qt signal/slots for this purpose with all their thread-safety guarantees?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't use slots because I control when to call the services and I own the metadata broadcast object, which lives in the same thread as the scrobbling manager. Since I know when I will call the services and I don't need queued connections I didn't see any reason to use signals & slots.

Copy link
Contributor

Choose a reason for hiding this comment

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

Send everything that you need at the receiver side by value, i.e. wrap it into a queueable meta-type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean all the track metadata so the services don't have to store a pointer? Like a DTO?

Copy link
Member

Choose a reason for hiding this comment

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

const auto&

service->broadcastCurrentTrack(pTrack);
}
}

QLinkedList<TrackPointer> MetadataBroadcaster::getTrackedTracks() {
return m_trackedTracks;
}

void MetadataBroadcaster::addNewScrobblingService(ScrobblingService *service) {
m_scrobblingServices.push_back(
std::move(std::unique_ptr<ScrobblingService>(service)));
}
25 changes: 25 additions & 0 deletions src/broadcast/metadatabroadcast.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
#pragma once

#include <QObject>
#include <QLinkedList>
#include <list>

#include "broadcast/scrobblingservice.h"
#include "track/track.h"
#include "track/trackplaytimers.h"

class MetadataBroadcaster : public QObject {
Q_OBJECT
public:
MetadataBroadcaster(TrackTimers::ElapsedTimer *timer);
QLinkedList<TrackPointer> getTrackedTracks();
void addNewScrobblingService(ScrobblingService *service);

public slots:
void slotReadyToBeScrobbled(TrackPointer pTrack);
void slotNowListening(TrackPointer pTrack);
private:
QLinkedList<TrackPointer> m_trackedTracks;
std::unique_ptr<TrackTimers::ElapsedTimer> m_pElapsedTimer;
std::list<std::unique_ptr<ScrobblingService>> m_scrobblingServices;
};
210 changes: 210 additions & 0 deletions src/broadcast/scrobblingmanager.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,210 @@
#include <QObject>

#include "broadcast/scrobblingmanager.h"
#include "broadcast/filelistener.h"
#include "control/controlproxy.h"
#include "engine/enginexfader.h"
#include "mixer/deck.h"
#include "mixer/playerinfo.h"
#include "mixer/playermanager.h"

ScrobblingManager::ScrobblingManager(PlayerManager *manager) :
m_CPGuiTick("[Master]", "guiTick50ms",this),
Copy link
Member

Choose a reason for hiding this comment

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

Some style nits:
We use a new line and 4 or 8 space indent before the colon. All initializer are following in a column + 2 spaces.
Opening brace is always at the line end.

ScrobblingManager::ScrobblingManager(PlayerManager *manager) 
        : m_CPGuiTick("[Master]", "guiTick50ms",this),
          ...
          m_CPCrossfader("[Master]","crossfader", this) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okey, thanks.

m_CPCrossfader("[Master]","crossfader", this),
m_CPXFaderCurve(ConfigKey(EngineXfader::kXfaderConfigKey,
"xFaderCurve"),this),

m_CPXFaderCalibration(ConfigKey(EngineXfader::kXfaderConfigKey,
Copy link
Member

Choose a reason for hiding this comment

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

Can't you call the function that calculates the xfader gain instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I need the correct parameters which are given by those Control objects, aren't they? I copied the code from EngineMaster.

"xFaderCalibration"),this),

m_CPXFaderMode(ConfigKey(EngineXfader::kXfaderConfigKey,
"xFaderMode"),this),

m_CPXFaderReverse(ConfigKey(EngineXfader::kXfaderConfigKey,
"xFaderReverse"),this),

m_pManager(manager),
m_broadcaster(new TrackTimers::ElapsedTimerQt)
{
m_CPGuiTick.connectValueChanged(SLOT(slotGuiTick(double)));
connect(&PlayerInfo::instance(),SIGNAL(currentPlayingTrackChanged(TrackPointer)),
&m_broadcaster,SLOT(slotNowListening(TrackPointer)));
m_broadcaster
.addNewScrobblingService(new FileListener("nowListening.txt"));
Copy link
Member

Choose a reason for hiding this comment

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

I think it is: addNewCurrentTrackConsumer() or so. I think it is better to call this form outside the class. The manager should have no knowledge which type of consumers are connected. It should only track a list of generic consumers if required.

Copy link
Member

Choose a reason for hiding this comment

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

The file should be finally written into a user directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's what I was thinking too, but since the user will choose the services this is a WIP solution. The file path is WIP too because the user should be able to choose the path of the file.

startTimer(1000);
}


void ScrobblingManager::slotTrackPaused(TrackPointer pPausedTrack) {
QMutexLocker locker(&m_mutex);
bool allPaused = true;
TrackInfo *pausedTrackInfo = nullptr;
for (TrackInfo *trackInfo : m_trackList) {
if (trackInfo->m_pTrack == pPausedTrack) {
pausedTrackInfo = trackInfo;
for (QString playerGroup : trackInfo->m_players) {
BaseTrackPlayer *player = m_pManager->getPlayer(playerGroup);
if (!player->isTrackPaused())
Copy link
Member

Choose a reason for hiding this comment

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

check the pointer before using. If it is always valid ensure it by a VERIFY_OR_DEBUG_ASSERT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright.

allPaused = false;
}
break;
}
}
if (allPaused && pausedTrackInfo)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use curly braces even for single line blocks with a consistent indentation and continuation style as proposed in our style guide. Of course, we should have an automatic code formatter that enforces the rules and fixes those issues automatically ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okey

pausedTrackInfo->m_trackInfo.pausePlayedTime();
Copy link
Member

Choose a reason for hiding this comment

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

You crash is not a deadlock, it is a Segmentation fault, which happens when you access memory, not allocated by the process. In this case, you access memory via the uninitialized pointer pausedTrackInfo.

It is a good idea to always initialize empty pointers with "nullptr". Than you can decorate your code with
DEBUG_ASSERT(pausedTrackInfo) in various places to detect such design issues early.
If you start mixxx with --debugAssertBreak under GDB, it will hit a breakpoint, instead of just abort.
Very handy :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, how can a seg fault keep all threads in a futex? Wouldn't it kill the process? Thanks for the help.

Copy link
Member

@daschuer daschuer Jun 1, 2018

Choose a reason for hiding this comment

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

It actually kills the process here on my Ubuntu Trusty.

Copy link
Member

Choose a reason for hiding this comment

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

Mybe we have an additional deadlock?

}

void ScrobblingManager::slotTrackResumed(TrackPointer pResumedTrack) {
BaseTrackPlayer *player = qobject_cast<Deck*>(sender());
DEBUG_ASSERT(player);
if (player == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

This can be combined as VERIFY_OR_DEBUG_ASSERT.

qDebug() << "Didn't load track in a deck yet scrobbling "
"received resumed signal.";
return;
}
if (isTrackAudible(pResumedTrack,player)) {
QMutexLocker locker(&m_mutex);
for (TrackInfo *trackInfo : m_trackList) {
if (trackInfo->m_pTrack == pResumedTrack) {
trackInfo->m_trackInfo.resumePlayedTime();
break;
}
}
}
}

void ScrobblingManager::slotLoadingTrack(TrackPointer pNewTrack, TrackPointer pOldTrack) {
BaseTrackPlayer *sourcePlayer =
qobject_cast<BaseTrackPlayer*>(sender());
DEBUG_ASSERT(sourcePlayer);
if (pOldTrack) {
m_tracksToBeReset.append(TrackToBeReset(pOldTrack,
sourcePlayer->getGroup()));
}
}

void ScrobblingManager::slotNewTrackLoaded(TrackPointer pNewTrack) {
//Empty player gives a null pointer.
if (!pNewTrack)
return;
BaseTrackPlayer *player = qobject_cast<BaseTrackPlayer*>(sender());
DEBUG_ASSERT(player);
QMutexLocker locker(&m_mutex);
bool trackAlreadyAdded = false;
for (TrackInfo *trackInfo : m_trackList) {
if (trackInfo->m_pTrack == pNewTrack) {
trackInfo->m_players.append(player->getGroup());
trackAlreadyAdded = true;
break;
}
}
if (!trackAlreadyAdded) {
TrackInfo *newTrackInfo = new TrackInfo(pNewTrack);
newTrackInfo->m_players.append(player->getGroup());
m_trackList.append(newTrackInfo);
connect(&m_trackList.last()->m_trackInfo,SIGNAL(readyToBeScrobbled(TrackPointer)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you establish a signal/slot connection between all instances of the internal TrackTimingInfo class and the broadcaster instead of only using a single connection between the ScrobblingManager and the broadcaster? You send the affected track with as the payload data with every signal and the track is bound to each instance of TrackTimingInfo anyway. If you need more context than just the TrackPointer then you could always collect this in a dedicated signal data class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see how it is any different, you're just adding one level of indirection. In the end, the broadcaster manages the info on the grace period timer, the scrobbling manager doesn't care about it. Could you please explain why you think this is a bad idea?

Copy link
Contributor

Choose a reason for hiding this comment

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

Simple and robust design? It doesn't make sense to use n connections when 1 is sufficient and all the receiver needs to know about the context is already part of the message.

The track info objects are an implementation detail of your class that should not be visible from the outside. The connections make them visible, indirectly. You are also not allowed to delete those objects like you do now while some signals might still be pending. You have to delete them with QObject::deleteLater(). Otherwise the receiver will get a dangling sender pointer. But all this doesn't matter if you lift the connection up to the next level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct me if I'm wrong but if they live on the same thread aren't they under a direct connection? Then there are no pending signals because the slot is called immediately. However, I didn't think about exposing implementation details so you're right, I will call them from the scrobbling manager.

Copy link
Member

Choose a reason for hiding this comment

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

A direct connection is finally a simple callback, Unfortunately, Qt has a lot of boilerplate code to detect that and to make connect and reconnect save in all circumstances.
Signal and slot connection suffer also a big issue regarding readability. You cannot instantly be sure who is connected where. So would should always prefer not to use signal and slots or similar messaging.

&m_broadcaster,SLOT(slotReadyToBeScrobbled(TrackPointer)));
}
//A new track has been loaded so must unload old one.
resetTracks();
}

void ScrobblingManager::slotPlayerEmpty() {
QMutexLocker locker(&m_mutex);
resetTracks();
}

void ScrobblingManager::resetTracks() {
for (TrackToBeReset candidateTrack : m_tracksToBeReset) {
QLinkedList<TrackInfo*>::Iterator trackListIterator =
m_trackList.begin();
for (;trackListIterator != m_trackList.end(); ++trackListIterator) {
TrackInfo *trackInfo = *trackListIterator;
if (trackInfo->m_pTrack == candidateTrack.m_pTrack) {
if (!trackInfo->m_players.contains(candidateTrack.
m_playerGroup)) {
qDebug() << "Track doesn't contain player"
"yet is requested for deletion.";
break;
}
//Load error, stray from engine buffer.
BaseTrackPlayer *player =
m_pManager->getPlayer(candidateTrack.m_playerGroup);
if (player->getLoadedTrack() ==
candidateTrack.m_pTrack)
break;
QLinkedList<QString>::iterator it =
trackInfo->m_players.begin();
while (it != trackInfo->m_players.end() &&
Copy link
Contributor

Choose a reason for hiding this comment

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

3 nested loops with lots of conditional statements feels way too heavy for the task of synchronizing internal state. Please rethink this piece of code. It also contains the sensitive memory management as I mentioned out elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is inefficient, yes, but the number of tracks is so low I haven't seen any performance hits. I didn't want to optimize unless I saw any so I think the code is fine this way, there will be at most 8 different tracks between both lists. Also I don' t understand what you mean with the sensitive memory management. I delete a pointer I created in the slotNewTrack. Since I know where the pointer is coming from I don't see any problem with it. I can wrap it in a unique_ptr though if you think it's better.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not complaining about efficiency, I'm talking about clean and readable code ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If its not about efficiency, how would you go about iterating through the list of tracks to be reset, picking each track to be reset, looking for it on the list of current tracks and then deleting only the player that's been ejected from the list of players each tracked track has without 3 loops? I'm open to suggestions.

*it != candidateTrack.m_playerGroup) {
++it;
}
if (*it == candidateTrack.m_playerGroup) {
trackInfo->m_players.erase(it);
}
if (trackInfo->m_players.empty()) {
trackInfo->m_trackInfo.pausePlayedTime();
trackInfo->m_trackInfo.resetPlayedTime();
delete trackInfo;
m_trackList.erase(trackListIterator);
}
break;
}
}
}
}

bool ScrobblingManager::isTrackAudible(TrackPointer pTrack, BaseTrackPlayer * pPlayer) {
if (pPlayer->getLoadedTrack() != pTrack) {
qDebug() << "Track can't be audible because is not in player";
return false;
}
return getPlayerVolume(pPlayer) >= 0.20;
}

double ScrobblingManager::getPlayerVolume(BaseTrackPlayer *pPlayer) {
double finalVolume;
ControlProxy trackPreGain(pPlayer->getGroup(),"pregain",this);
double preGain = trackPreGain.get();
ControlProxy trackVolume(pPlayer->getGroup(),"volume",this);
double volume = trackVolume.get();
ControlProxy deckOrientation(pPlayer->getGroup(),"orientation",this);
int orientation = deckOrientation.get();

double xFaderLeft,xFaderRight;

EngineXfader::getXfadeGains(m_CPCrossfader.get(),
m_CPXFaderCurve.get(),
m_CPXFaderCalibration.get(),
m_CPXFaderMode.get(),
m_CPXFaderReverse.toBool(),
&xFaderLeft,&xFaderRight);
finalVolume = preGain * volume;
if (orientation == EngineChannel::LEFT)
finalVolume *= xFaderLeft;
else if (orientation == EngineChannel::RIGHT)
finalVolume *= xFaderRight;
return finalVolume;
}

void ScrobblingManager::slotGuiTick(double timeSinceLastTick) {
for (TrackInfo *trackInfo : m_trackList) {
trackInfo->m_trackInfo.slotGuiTick(timeSinceLastTick);
}
}

void ScrobblingManager::timerEvent(QTimerEvent *timerEvent) {
for (TrackInfo *trackInfo : m_trackList) {
bool inaudible = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please try to avoid naming a boolean variables with a negation names. The name "audible" would work much better here. But the local variable is not needed anyway, the code can be simplified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change the name of the variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need the variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate?

for (QString playerGroup : trackInfo->m_players) {
BaseTrackPlayer *player = m_pManager->getPlayer(playerGroup);
if (isTrackAudible(trackInfo->m_pTrack,player)) {
inaudible = false;
break;
}
}
if (inaudible) {
trackInfo->m_trackInfo.pausePlayedTime();
}
}
}
Loading