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

Track: Add missing NOTIFY signals for Q_PROPERTY declarations #3924

Merged
merged 8 commits into from
Jun 4, 2021
Merged

Track: Add missing NOTIFY signals for Q_PROPERTY declarations #3924

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
54ddf25
Track: Add missing `NOTIFY` signals for `Q_PROPERTY` declarations
Holzhaus May 29, 2021
1197c78
Track: Emit infoChanged signals directly when setting title/artist
Holzhaus May 29, 2021
a06e8ca
Track: Add dedicated bpmTextChanged signal
Holzhaus May 29, 2021
cadecf7
Track: Emit duration changed signal when updating stream info
Holzhaus May 29, 2021
978ab2c
Track: Emit changed signals for all properties on metadata import
Holzhaus May 29, 2021
640a59d
Track: Make property naming consistent and use new NOTIFY signal scheme
Holzhaus May 29, 2021
78783e5
Track: Rename emitMetataChanged method to better reflect intent
Holzhaus Jun 1, 2021
838da06
Track: Emit missing durationChanged signal in updateStreamInfoFromSource
Holzhaus Jun 1, 2021
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
47 changes: 37 additions & 10 deletions src/track/track.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,9 @@ Track::Track(
<< "->"
<< numberOfInstancesBefore + 1;
}

connect(this, &Track::titleChanged, this, &Track::infoChanged);
connect(this, &Track::artistChanged, this, &Track::infoChanged);
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to call the additional signals explicit.
For less magic to consider in case of future refactoring.

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 want me to add slots that only contain emit infoChanged() ?

Copy link
Member

Choose a reason for hiding this comment

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

No, I mean copy emit infoChanged() below emit artistChanged()

Than the call tree has one interruption less when debugging.

Copy link
Member

Choose a reason for hiding this comment

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

Than we can consider to get rid of infoChanged() in favor of changed()

Copy link
Member Author

@Holzhaus Holzhaus May 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

No, the signal signature is incompatible. I added the direct emit statements as you suggested.

}

Track::~Track() {
Expand Down Expand Up @@ -506,6 +509,7 @@ void Track::setDuration(mixxx::Duration duration) {
m_record.refMetadata().refStreamInfo().ptrDuration(),
duration)) {
markDirtyAndUnlock(&lock);
emit durationChanged();
}
}

Expand Down Expand Up @@ -536,8 +540,10 @@ QString Track::getTitle() const {

void Track::setTitle(const QString& s) {
QMutexLocker lock(&m_qMutex);
if (compareAndSet(m_record.refMetadata().refTrackInfo().ptrTitle(), s.trimmed())) {
const QString value = s.trimmed();
if (compareAndSet(m_record.refMetadata().refTrackInfo().ptrTitle(), value)) {
markDirtyAndUnlock(&lock);
emit titleChanged(value);
}
}

Expand All @@ -548,8 +554,10 @@ QString Track::getArtist() const {

void Track::setArtist(const QString& s) {
QMutexLocker lock(&m_qMutex);
if (compareAndSet(m_record.refMetadata().refTrackInfo().ptrArtist(), s.trimmed())) {
const QString value = s.trimmed();
if (compareAndSet(m_record.refMetadata().refTrackInfo().ptrArtist(), value)) {
markDirtyAndUnlock(&lock);
emit artistChanged(value);
}
}

Expand All @@ -560,8 +568,10 @@ QString Track::getAlbum() const {

void Track::setAlbum(const QString& s) {
QMutexLocker lock(&m_qMutex);
if (compareAndSet(m_record.refMetadata().refAlbumInfo().ptrTitle(), s.trimmed())) {
const QString value = s.trimmed();
if (compareAndSet(m_record.refMetadata().refAlbumInfo().ptrTitle(), value)) {
markDirtyAndUnlock(&lock);
emit albumChanged(value);
}
}

Expand All @@ -572,8 +582,10 @@ QString Track::getAlbumArtist() const {

void Track::setAlbumArtist(const QString& s) {
QMutexLocker lock(&m_qMutex);
if (compareAndSet(m_record.refMetadata().refAlbumInfo().ptrArtist(), s.trimmed())) {
const QString value = s.trimmed();
if (compareAndSet(m_record.refMetadata().refAlbumInfo().ptrArtist(), value)) {
markDirtyAndUnlock(&lock);
emit albumArtistChanged(value);
}
}

Expand All @@ -584,8 +596,10 @@ QString Track::getYear() const {

void Track::setYear(const QString& s) {
QMutexLocker lock(&m_qMutex);
if (compareAndSet(m_record.refMetadata().refTrackInfo().ptrYear(), s.trimmed())) {
const QString value = s.trimmed();
if (compareAndSet(m_record.refMetadata().refTrackInfo().ptrYear(), value)) {
markDirtyAndUnlock(&lock);
emit yearChanged(value);
}
}

Expand All @@ -596,8 +610,10 @@ QString Track::getGenre() const {

void Track::setGenre(const QString& s) {
QMutexLocker lock(&m_qMutex);
if (compareAndSet(m_record.refMetadata().refTrackInfo().ptrGenre(), s.trimmed())) {
const QString value = s.trimmed();
if (compareAndSet(m_record.refMetadata().refTrackInfo().ptrGenre(), value)) {
markDirtyAndUnlock(&lock);
emit genreChanged(value);
}
}

Expand All @@ -608,8 +624,10 @@ QString Track::getComposer() const {

void Track::setComposer(const QString& s) {
QMutexLocker lock(&m_qMutex);
if (compareAndSet(m_record.refMetadata().refTrackInfo().ptrComposer(), s.trimmed())) {
const QString value = s.trimmed();
if (compareAndSet(m_record.refMetadata().refTrackInfo().ptrComposer(), value)) {
markDirtyAndUnlock(&lock);
emit composerChanged(value);
}
}

Expand All @@ -620,8 +638,10 @@ QString Track::getGrouping() const {

void Track::setGrouping(const QString& s) {
QMutexLocker lock(&m_qMutex);
if (compareAndSet(m_record.refMetadata().refTrackInfo().ptrGrouping(), s.trimmed())) {
const QString value = s.trimmed();
if (compareAndSet(m_record.refMetadata().refTrackInfo().ptrGrouping(), value)) {
markDirtyAndUnlock(&lock);
emit groupingChanged(value);
}
}

Expand All @@ -637,15 +657,19 @@ QString Track::getTrackTotal() const {

void Track::setTrackNumber(const QString& s) {
QMutexLocker lock(&m_qMutex);
if (compareAndSet(m_record.refMetadata().refTrackInfo().ptrTrackNumber(), s.trimmed())) {
const QString value = s.trimmed();
if (compareAndSet(m_record.refMetadata().refTrackInfo().ptrTrackNumber(), value)) {
markDirtyAndUnlock(&lock);
emit trackNumberChanged(value);
}
}

void Track::setTrackTotal(const QString& s) {
QMutexLocker lock(&m_qMutex);
if (compareAndSet(m_record.refMetadata().refTrackInfo().ptrTrackTotal(), s.trimmed())) {
const QString value = s.trimmed();
if (compareAndSet(m_record.refMetadata().refTrackInfo().ptrTrackTotal(), value)) {
markDirtyAndUnlock(&lock);
emit trackTotalChanged(value);
}
}

Expand All @@ -658,6 +682,7 @@ void Track::setPlayCounter(const PlayCounter& playCounter) {
QMutexLocker lock(&m_qMutex);
if (compareAndSet(m_record.ptrPlayCounter(), playCounter)) {
markDirtyAndUnlock(&lock);
emit timesPlayedChanged();
}
}

Expand All @@ -667,6 +692,7 @@ void Track::updatePlayCounter(bool bPlayed) {
playCounter.updateLastPlayedNowAndTimesPlayed(bPlayed);
if (compareAndSet(m_record.ptrPlayCounter(), playCounter)) {
markDirtyAndUnlock(&lock);
emit timesPlayedChanged();
}
}

Expand All @@ -692,6 +718,7 @@ void Track::setComment(const QString& s) {
QMutexLocker lock(&m_qMutex);
if (compareAndSet(m_record.refMetadata().refTrackInfo().ptrComment(), s)) {
markDirtyAndUnlock(&lock);
emit commentChanged(s);
}
}

Expand Down
64 changes: 43 additions & 21 deletions src/track/track.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,27 +47,34 @@ class Track : public QObject {
const QString& filePath,
TrackId trackId);

Q_PROPERTY(QString artist READ getArtist WRITE setArtist)
Q_PROPERTY(QString title READ getTitle WRITE setTitle)
Q_PROPERTY(QString album READ getAlbum WRITE setAlbum)
Q_PROPERTY(QString albumArtist READ getAlbumArtist WRITE setAlbumArtist)
Q_PROPERTY(QString genre READ getGenre WRITE setGenre)
Q_PROPERTY(QString composer READ getComposer WRITE setComposer)
Q_PROPERTY(QString grouping READ getGrouping WRITE setGrouping)
Q_PROPERTY(QString year READ getYear WRITE setYear)
Q_PROPERTY(QString track_number READ getTrackNumber WRITE setTrackNumber)
Q_PROPERTY(QString track_total READ getTrackTotal WRITE setTrackTotal)
Q_PROPERTY(int times_played READ getTimesPlayed)
Q_PROPERTY(QString comment READ getComment WRITE setComment)
Q_PROPERTY(double bpm READ getBpm)
Q_PROPERTY(QString bpmFormatted READ getBpmText STORED false)
Q_PROPERTY(QString key READ getKeyText WRITE setKeyText)
Q_PROPERTY(double duration READ getDuration)
Q_PROPERTY(QString durationFormatted READ getDurationTextSeconds STORED false)
Q_PROPERTY(QString durationFormattedCentiseconds READ getDurationTextCentiseconds STORED false)
Q_PROPERTY(QString durationFormattedMilliseconds READ getDurationTextMilliseconds STORED false)
Q_PROPERTY(QString info READ getInfo STORED false)
Q_PROPERTY(QString titleInfo READ getTitleInfo STORED false)
Q_PROPERTY(QString artist READ getArtist WRITE setArtist NOTIFY artistChanged)
Q_PROPERTY(QString title READ getTitle WRITE setTitle NOTIFY titleChanged)
Q_PROPERTY(QString album READ getAlbum WRITE setAlbum NOTIFY albumChanged)
Q_PROPERTY(QString albumArtist READ getAlbumArtist WRITE setAlbumArtist
NOTIFY albumArtistChanged)
Q_PROPERTY(QString genre READ getGenre WRITE setGenre NOTIFY genreChanged)
Q_PROPERTY(QString composer READ getComposer WRITE setComposer NOTIFY composerChanged)
Q_PROPERTY(QString grouping READ getGrouping WRITE setGrouping NOTIFY groupingChanged)
Q_PROPERTY(QString year READ getYear WRITE setYear NOTIFY yearChanged)
Q_PROPERTY(QString trackNumber READ getTrackNumber WRITE setTrackNumber
NOTIFY trackNumberChanged)
Q_PROPERTY(QString trackTotal READ getTrackTotal WRITE setTrackTotal NOTIFY trackTotalChanged)
Q_PROPERTY(int timesPlayed READ getTimesPlayed NOTIFY timesPlayedChanged)
Q_PROPERTY(QString comment READ getComment WRITE setComment NOTIFY commentChanged)
Q_PROPERTY(double bpm READ getBpm NOTIFY bpmUpdated)
Q_PROPERTY(QString bpmFormatted READ getBpmText STORED false NOTIFY bpmUpdated)
Copy link
Contributor

@uklotzde uklotzde May 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The change signal has a different type than the property. Is this intended? I would expect that all change signals emit the actual property type.

This also applies to the key text and formatted duration.

Copy link
Member Author

@Holzhaus Holzhaus May 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The signals without parameters are intended:

A NOTIFY signal is optional. If defined, it should specify one existing signal in that class that is emitted whenever the value of the property changes. NOTIFY signals for MEMBER variables must take zero or one parameter, which must be of the same type as the property.

https://doc.qt.io/qt-5.12/properties.html

The type mismatch is unintended though :D

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Q_PROPERTY(QString key READ getKeyText WRITE setKeyText NOTIFY keysUpdated)
Q_PROPERTY(double duration READ getDuration NOTIFY durationChanged)
Q_PROPERTY(QString durationFormatted READ getDurationTextSeconds
STORED false NOTIFY durationChanged)
Q_PROPERTY(QString durationFormattedCentiseconds READ
getDurationTextCentiseconds
STORED false NOTIFY durationChanged)
Q_PROPERTY(QString durationFormattedMilliseconds READ
getDurationTextMilliseconds
STORED false NOTIFY durationChanged)
Q_PROPERTY(QString info READ getInfo STORED false NOTIFY infoChanged)
Q_PROPERTY(QString titleInfo READ getTitleInfo STORED false NOTIFY infoChanged)

mixxx::FileAccess getFileAccess() const {
// Copying QFileInfo is thread-safe due to implicit sharing,
Expand Down Expand Up @@ -363,6 +370,21 @@ class Track : public QObject {
const mixxx::audio::StreamInfo& streamInfo);

signals:
void artistChanged(const QString&);
void titleChanged(const QString&);
void albumChanged(const QString&);
void albumArtistChanged(const QString&);
void genreChanged(const QString&);
void composerChanged(const QString&);
void groupingChanged(const QString&);
void yearChanged(const QString&);
void trackNumberChanged(const QString&);
void trackTotalChanged(const QString&);
void commentChanged(const QString&);
void timesPlayedChanged();
void durationChanged();
void infoChanged();

void waveformUpdated();
void waveformSummaryUpdated();
void coverArtUpdated();
Expand Down