Track: Add missing `NOTIFY` signals for `Q_PROPERTY` declarations #3924

Holzhaus · 2021-05-29T13:43:55Z

This fixes some clazy-qproperty-without-notify warnings. Every
Q_PROPERTY should either have a NOTIFY signal or be declared as
CONSTANT.

See this for details:

This fixes some clazy-qproperty-without-notify warnings. Every `Q_PROPERTY` should either have a `NOTIFY` signal or be declared as `CONSTANT`. See this for details: - https://github.com/KDE/clazy/blob/master/docs/checks/README-qproperty-without-notify.md - https://doc.qt.io/qt-5.12/properties.html

Holzhaus · 2021-05-29T13:45:11Z

@daschuer With this, I can emit the proper signals in #3911 atomically. This should also improve QML performance with lots of decks/samplers.

uklotzde · 2021-05-29T14:37:43Z

All those properties might also change when invoking one of the following functions:

replaceMetadataFromSource()
mergeExtraMetadataFromSource() (probably no properties affected yet)
replaceRecord()
exportMetadata()
setAudioProperties()
updateStreamInfoFromSource()

We could simply trigger all property changed signals in the first step to ensure consistency, especially for the first 4 methods.

uklotzde · 2021-05-29T14:42:05Z

src/track/track.h

+    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)


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.

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

daschuer · 2021-05-29T14:30:05Z

src/track/track.cpp

@@ -84,6 +84,9 @@ Track::Track(
                << "->"
                << numberOfInstancesBefore + 1;
    }
+
+    connect(this, &Track::titleChanged, this, &Track::infoChanged);
+    connect(this, &Track::artistChanged, this, &Track::infoChanged);


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

You want me to add slots that only contain emit infoChanged() ?

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

Than the call tree has one interruption less when debugging.

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

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

Holzhaus · 2021-05-29T15:42:27Z

exportMetadata()

Does this really modify any metadata?

setAudioProperties()

updateStreamInfoFromSource()

This can only change the duration, right?

I added some commits, please have a look.

uklotzde · 2021-05-29T16:53:16Z

Ok, exportMetadata() doesn't seem to modify any of those exposed properties. It might only update hidden, internal members.

Duration should be the only Q_PROPERTY that is affected by stream info changes.

Holzhaus · 2021-05-29T16:57:17Z

Ok, then I think this is ready.

uklotzde · 2021-05-29T17:02:44Z

src/track/track.h

+    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)


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.

uklotzde · 2021-05-29T17:04:33Z

src/track/track.h

+    void trackNumberChanged(const QString&);
+    void trackTotalChanged(const QString&);
+    void commentChanged(const QString&);
+    void bpmTextChanged();


Why do some signals carry the value and others not?

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

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

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.

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.

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.

daschuer

is a signal missing in:
Track::setType(const QString& sType)
Track::setBitrate(int iBitrate)
Track::setURL(const QString& url)
Track::shiftCuePositionsMillis(double milliseconds)
Track::setRating (int rating)
Track::setBpmLocked(bool bpmLocked)

I am unsure if everything is correct in
Track::updateStreamInfoFromSource()
There is either an emitMetadataChanged() missing, or an redundant markDirtyAndUnlock(&lock); call.

src/track/track.cpp

Holzhaus · 2021-06-01T19:11:24Z

is a signal missing in:
Track::setType(const QString& sType)
Track::setBitrate(int iBitrate)
Track::setURL(const QString& url)
Track::shiftCuePositionsMillis(double milliseconds)
Track::setRating (int rating)
Track::setBpmLocked(bool bpmLocked)

None of those is a Q_PROPERTY. We can add more if we need to, but this is out of scope here.

I am unsure if everything is correct in
Track::updateStreamInfoFromSource()
There is either an emitMetadataChanged() missing, or an redundant markDirtyAndUnlock(&lock); call.

Good catch, added the missing durationChanged signal.

uklotzde · 2021-06-04T00:07:42Z

@daschuer Ping

daschuer · 2021-06-04T06:31:26Z

LGTM, thank you.

Holzhaus added the library label May 29, 2021

Holzhaus requested review from uklotzde and daschuer May 29, 2021 13:43

uklotzde reviewed May 29, 2021

View reviewed changes

daschuer reviewed May 29, 2021

View reviewed changes

Holzhaus added 4 commits May 29, 2021 17:36

Track: Emit infoChanged signals directly when setting title/artist

1197c78

Track: Add dedicated bpmTextChanged signal

a06e8ca

Track: Emit duration changed signal when updating stream info

cadecf7

Track: Emit changed signals for all properties on metadata import

978ab2c

This was referenced May 29, 2021

QML Pt. 3: Add support for retrieving deck track properties #3911

Merged

DlgTrackInfo: Use TrackRecord as model #3906

Merged

uklotzde reviewed May 29, 2021

View reviewed changes

Holzhaus force-pushed the track-notify-signals branch from 2c30a72 to 640a59d Compare May 29, 2021 19:26

daschuer requested changes May 30, 2021

View reviewed changes

src/track/track.cpp Outdated Show resolved Hide resolved

src/track/track.cpp Show resolved Hide resolved

uklotzde approved these changes May 30, 2021

View reviewed changes

Holzhaus added 2 commits June 1, 2021 21:07

Track: Rename emitMetataChanged method to better reflect intent

78783e5

Track: Emit missing durationChanged signal in updateStreamInfoFromSource

838da06

Holzhaus requested a review from daschuer June 1, 2021 19:11

daschuer approved these changes Jun 4, 2021

View reviewed changes

daschuer merged commit 3ab1e5f into mixxxdj:main Jun 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track: Add missing `NOTIFY` signals for `Q_PROPERTY` declarations #3924

Track: Add missing `NOTIFY` signals for `Q_PROPERTY` declarations #3924

Holzhaus commented May 29, 2021

Holzhaus commented May 29, 2021

uklotzde commented May 29, 2021 •

edited

Loading

uklotzde May 29, 2021 •

edited

Loading

Holzhaus May 29, 2021 •

edited

Loading

Holzhaus May 29, 2021

daschuer May 29, 2021

Holzhaus May 29, 2021

daschuer May 29, 2021

daschuer May 29, 2021

Holzhaus May 29, 2021 •

edited

Loading

Holzhaus commented May 29, 2021

uklotzde commented May 29, 2021

Holzhaus commented May 29, 2021

uklotzde May 29, 2021

uklotzde May 29, 2021

uklotzde May 29, 2021

uklotzde May 29, 2021

uklotzde May 29, 2021

Holzhaus May 29, 2021

daschuer left a comment

Holzhaus commented Jun 1, 2021

uklotzde commented Jun 4, 2021

daschuer commented Jun 4, 2021

Track: Add missing NOTIFY signals for Q_PROPERTY declarations #3924

Track: Add missing NOTIFY signals for Q_PROPERTY declarations #3924

Conversation

Holzhaus commented May 29, 2021

Holzhaus commented May 29, 2021

uklotzde commented May 29, 2021 • edited Loading

uklotzde May 29, 2021 • edited Loading

Choose a reason for hiding this comment

Holzhaus May 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Holzhaus May 29, 2021 • edited Loading

Choose a reason for hiding this comment

Holzhaus commented May 29, 2021

uklotzde commented May 29, 2021

Holzhaus commented May 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

Holzhaus commented Jun 1, 2021

uklotzde commented Jun 4, 2021

daschuer commented Jun 4, 2021

Track: Add missing `NOTIFY` signals for `Q_PROPERTY` declarations #3924

Track: Add missing `NOTIFY` signals for `Q_PROPERTY` declarations #3924

uklotzde commented May 29, 2021 •

edited

Loading

uklotzde May 29, 2021 •

edited

Loading

Holzhaus May 29, 2021 •

edited

Loading

Holzhaus May 29, 2021 •

edited

Loading