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 5 commits
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
75 changes: 65 additions & 10 deletions src/track/track.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,8 @@ void Track::replaceMetadataFromSource(
if (colorModified) {
emit colorUpdated(newColor);
}

emitMetadataChanged();
}

// TODO: Import Serato metadata within the locking scope and not
Expand All @@ -240,6 +242,7 @@ bool Track::mergeExtraMetadataFromSource(
}
markDirtyAndUnlock(&lock);
// Modified
emitMetadataChanged();
return true;
}

Expand Down Expand Up @@ -303,6 +306,7 @@ bool Track::replaceRecord(
if (bpmUpdatedFlag) {
emit bpmUpdated(newBpm.getValue());
emit beatsUpdated();
emit bpmTextChanged();
}
if (oldKey != newKey) {
emitKeysUpdated(newKey);
Expand All @@ -313,6 +317,8 @@ bool Track::replaceRecord(
if (oldColor != newColor) {
emit colorUpdated(newColor);
}

emitMetadataChanged();
return true;
}

Expand Down Expand Up @@ -440,6 +446,27 @@ void Track::emitBeatsAndBpmUpdated(
mixxx::Bpm newBpm) {
emit bpmUpdated(newBpm.getValue());
emit beatsUpdated();
emit bpmTextChanged();
}

void Track::emitMetadataChanged() {
Holzhaus marked this conversation as resolved.
Show resolved Hide resolved
emit artistChanged(getArtist());
emit titleChanged(getTitle());
emit albumChanged(getAlbum());
emit albumArtistChanged(getAlbumArtist());
emit genreChanged(getGenre());
emit composerChanged(getComposer());
emit groupingChanged(getGrouping());
emit yearChanged(getYear());
emit trackNumberChanged(getTrackNumber());
emit trackTotalChanged(getTrackTotal());
emit commentChanged(getComment());
emit bpmTextChanged();
emit timesPlayedChanged();
emit durationChanged();
emit infoChanged();
emit keysUpdated();
emit bpmUpdated(getBpm());
}

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

Expand Down Expand Up @@ -536,8 +564,11 @@ 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);
emit infoChanged();
}
}

Expand All @@ -548,8 +579,11 @@ 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);
emit infoChanged();
}
}

Expand All @@ -560,8 +594,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 +608,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 +622,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 +636,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 +650,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 +664,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 +683,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 +708,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 +718,7 @@ void Track::updatePlayCounter(bool bPlayed) {
playCounter.updateLastPlayedNowAndTimesPlayed(bPlayed);
if (compareAndSet(m_record.ptrPlayCounter(), playCounter)) {
markDirtyAndUnlock(&lock);
emit timesPlayedChanged();
}
}

Expand All @@ -692,6 +744,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 Expand Up @@ -1588,6 +1641,7 @@ void Track::setAudioProperties(
m_record.refMetadata().ptrStreamInfo(),
streamInfo)) {
markDirtyAndUnlock(&lock);
emit durationChanged();
}
}

Expand All @@ -1603,6 +1657,7 @@ void Track::updateStreamInfoFromSource(
// Nothing more to do
if (updated) {
markDirtyAndUnlock(&lock);
emit durationChanged();
}
return;
}
Expand Down
68 changes: 47 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 bpmTextChanged)
Copy link
Contributor

Choose a reason for hiding this comment

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

How about consistently adopting the Text prefix for all formatted (= derived text) properties, i.e. bpmFormatted -> bpmText for consistency?

This also applies to key -> keyText / keysUpdated -> keyTextUpdated.

Even worse are the naming inconsistencies of the various duration properties. I didn't check how much code would be affected.

Managing all those updates and their dependencies is tedious and error prone.. Unfortunately, I have no idea how to adopt and apply FRP principles in Qt/QML or what idiomatic patterns they propose instead.

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,22 @@ 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 bpmTextChanged();
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 some signals carry the value and others not?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, even the docs don't apply a consistent approach:

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

Copy link
Contributor

Choose a reason for hiding this comment

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

0 or 1 parameter are allowed, but if the new value is provided as a parameter than it's type must match the type of the property.

In order to keep the number of signals low let's apply the following rationale:

  • For native properties without any derived properties provide the new value if possible
  • For properties with derived properties use the name of the native property, reuse the signal for all derived properties, and don't provide any value.

By applying this rule we need to revert bpmTextChanged to bpmChanged.

Copy link
Contributor

Choose a reason for hiding this comment

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

The Track object with its thread synchronization is a bastard and not really suitable for this purpose. What we actually would need is a distinction between view model and domain model classes.

Copy link
Member Author

Choose a reason for hiding this comment

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

640a59d

void timesPlayedChanged();
void durationChanged();
void infoChanged();

void waveformUpdated();
void waveformSummaryUpdated();
void coverArtUpdated();
Expand Down Expand Up @@ -406,6 +429,9 @@ class Track : public QObject {
void afterBeatsAndBpmUpdated(QMutexLocker* pLock);
void emitBeatsAndBpmUpdated(mixxx::Bpm newBpm);

/// Emits a changed signal for each Q_PROPERTY
void emitMetadataChanged();

/// Sets beats and returns a boolean to indicate if BPM/Beats were updated.
/// Only supposed to be called while the caller guards this a lock.
bool setBeatsWhileLocked(mixxx::BeatsPointer pBeats);
Expand Down