-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[WIP] Livemetadata PR #1675
Changes from 15 commits
32cece7
ba829a2
5169748
3d8c501
523d09b
23ef862
0a4a4d6
d203e85
1a6bc0d
ff27795
db2db23
7f55150
121443e
2cd5bbc
b9c4a6c
e10df9e
c1de7db
50019b0
d805306
4e714d5
b3f8ea8
8cd9c16
d4b9aa5
ec98ef6
0209b4a
2d8f4a2
88c4819
ab8b524
97daf77
4bc2de3
49b8b93
567e9b5
5748fa9
3a677b6
9c43d09
2c21183
a6d67d7
ba8a6fc
6b7f207
f7c6a83
1057b06
152925c
ad43628
751f567
64d35c5
ddfb859
e4dc8df
a1ad4c6
ee4c5a5
4571d43
185d2f2
0bc22be
0af2279
6be0042
62eae49
c2317c7
8b5e54a
cac8c1f
6807838
d6880a1
32e565a
1b77c8b
7ba10a7
94ab525
5e8fb60
41bdfb9
0def3e3
9fe5277
97271d8
f4b9649
dde6c47
e1cfd7a
69a3020
7af4dd7
170e6c9
8c6fa58
899f207
4a13665
39dc901
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,3 +57,5 @@ lib/*/*.lib | |
lib/*/lib/*.lib | ||
|
||
lib/qtscript-bytearray/*.cc | ||
|
||
nowListening.txt |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
|
||
#include "broadcast/filelistener.h" | ||
|
||
|
||
FileListener::FileListener(const QString &path) : | ||
m_file(path) | ||
{ | ||
QFileInfo fileInfo(path); | ||
qDebug() << "Absolute path " << fileInfo.absoluteFilePath(); | ||
qDebug() << "File exists: " << fileInfo.exists(); | ||
m_file.open(QIODevice::WriteOnly | | ||
QIODevice::Text | | ||
QIODevice::Unbuffered); | ||
|
||
} | ||
|
||
FileListener::~FileListener() { | ||
m_file.resize(0); | ||
} | ||
|
||
void FileListener::broadcastCurrentTrack(TrackPointer pTrack) { | ||
if (!pTrack) | ||
return; | ||
QTextStream stream(&m_file); | ||
m_file.resize(0); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A comment helps here: |
||
stream << "Now listening " << pTrack->getTitle(); | ||
stream << " by " << pTrack->getArtist(); | ||
} | ||
|
||
void FileListener::scrobbleTrack(TrackPointer pTrack) { | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
#pragma once | ||
|
||
#include <QFile> | ||
#include "broadcast/scrobblingservice.h" | ||
|
||
class FileListener: public ScrobblingService { | ||
public: | ||
FileListener(const QString &path); | ||
~FileListener(); | ||
void broadcastCurrentTrack(TrackPointer pTrack) override; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
|
||
#include <QLinkedListIterator> | ||
|
||
#include "broadcast/metadatabroadcast.h" | ||
#include "mixer/playerinfo.h" | ||
|
||
MetadataBroadcaster::MetadataBroadcaster() | ||
{ | ||
|
||
} | ||
|
||
void MetadataBroadcaster::slotAttemptScrobble(TrackPointer pTrack) { | ||
for (auto it = m_trackedTracks.begin(); | ||
it != m_trackedTracks.end(); | ||
++it) { | ||
if (*it == GracePeriod(0,pTrack)) { | ||
GracePeriod &trackPeriod = *it; | ||
if (trackPeriod.hasBeenEjected && | ||
trackPeriod.m_msElapsed > | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We use 8 space indent for line brakes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand what you mean here, which line break? All of them? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. line brakes because of long lines = 8 spaces.
|
||
static_cast<double>(m_gracePeriodSeconds)*1000.0) { | ||
for (auto &service : m_scrobblingServices) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use universal reference, for non const for loops: auto&& and const auto& for const loops. If the container is an implicit shared qt container you need also to ad as_const cast to avoid a deep copy of the container. We allign & && and * with the type (no space) please check the whole PR for this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, thanks. |
||
service->scrobbleTrack(pTrack); | ||
} | ||
trackPeriod.hasBeenEjected = false; | ||
trackPeriod.m_numberOfScrobbles++; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please comment these conditions. They are hard to understand. Maybe we can optimize the variable names as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the names are ok? Do you have any proposal? |
||
} | ||
break; | ||
} | ||
} | ||
} | ||
|
||
void MetadataBroadcaster::slotNowListening(TrackPointer pTrack) { | ||
for (auto &service : m_scrobblingServices) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. const auto& |
||
service->broadcastCurrentTrack(pTrack); | ||
} | ||
} | ||
|
||
QLinkedList<TrackId> MetadataBroadcaster::getTrackedTracks() { | ||
//Stub | ||
return QLinkedList<TrackId>(); | ||
} | ||
|
||
MetadataBroadcaster& MetadataBroadcaster::addNewScrobblingService(ScrobblingService *service) { | ||
m_scrobblingServices.push_back( | ||
std::move(std::unique_ptr<ScrobblingService>(service))); | ||
return *this; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does retiring this makes sense? The caller had it anyway. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh this was intended to chain adds, I guess it doesn't matter now. |
||
} | ||
|
||
void MetadataBroadcaster::newTrackLoaded(TrackPointer pTrack) { | ||
QLinkedListIterator<GracePeriod> it(m_trackedTracks); | ||
if (!it.findNext(GracePeriod(0,pTrack))) { | ||
GracePeriod newPeriod(0,pTrack); | ||
m_trackedTracks.append(newPeriod); | ||
} | ||
} | ||
|
||
void MetadataBroadcaster::trackUnloaded(TrackPointer pTrack) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should use one word for the same action |
||
for (auto it = m_trackedTracks.begin(); | ||
it != m_trackedTracks.end(); | ||
++it) { | ||
if (*it == GracePeriod(0,pTrack)) { | ||
it->hasBeenEjected = true; | ||
it->m_msElapsed = 0; | ||
} | ||
break; | ||
} | ||
} | ||
|
||
void MetadataBroadcaster::slotGuiTick(double timeSinceLastTick) { | ||
for (auto it = m_trackedTracks.begin(); | ||
it != m_trackedTracks.end(); | ||
++it) { | ||
if (it->hasBeenEjected) { | ||
it->m_msElapsed += timeSinceLastTick; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
#pragma once | ||
|
||
#include <QObject> | ||
#include <QLinkedList> | ||
#include <list> | ||
|
||
#include "broadcast/scrobblingservice.h" | ||
#include "track/track.h" | ||
#include "track/trackid.h" | ||
#include "track/trackplaytimers.h" | ||
|
||
class MetadataBroadcaster : public QObject { | ||
Q_OBJECT | ||
private: | ||
struct GracePeriod { | ||
double m_msElapsed; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since what? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps m_msSinceEjection? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, if it count the time since ejection. |
||
unsigned int m_numberOfScrobbles = 0; | ||
TrackId m_trackId; | ||
bool hasBeenEjected = false; | ||
GracePeriod(double msElapsed,TrackPointer pTrack) : | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please improve line breakers, to match the rest of Mixxx There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, sorry, I forgot. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please pass directly the Id, to document that this class ties not store a reference. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright. |
||
m_msElapsed(msElapsed),m_trackId(pTrack->getId()) {} | ||
bool operator==(const GracePeriod &other) const { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When is this used? It is not a fully compare of al elements so it requires some comments. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. QLinkedListIterator uses it, I don't know if it's better to use that class or do a manual loop. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is better to use a QHash with a m_trackId as key. That makes the code more explicit. |
||
return m_trackId == other.m_trackId; | ||
} | ||
}; | ||
public: | ||
|
||
MetadataBroadcaster(); | ||
QLinkedList<TrackId> getTrackedTracks(); | ||
MetadataBroadcaster& addNewScrobblingService(ScrobblingService *service); | ||
void newTrackLoaded(TrackPointer pTrack); | ||
void trackUnloaded(TrackPointer pTrack); | ||
void setGracePeriod(unsigned int seconds); | ||
|
||
public slots: | ||
void slotAttemptScrobble(TrackPointer pTrack); | ||
void slotNowListening(TrackPointer pTrack); | ||
void slotGuiTick(double timeSinceLastTick); | ||
private: | ||
unsigned int m_gracePeriodSeconds; | ||
QLinkedList<GracePeriod> m_trackedTracks; | ||
std::list<std::unique_ptr<ScrobblingService>> m_scrobblingServices; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no new line before {