Delegate creation and multithreaded property updating
-
@felsi Properties are for single-threaded usage only. Both reading from and writing to them must happen in the same thread. If you are using them from GUI, reading and writing must happen in the GUI thread.
Easiest way to change your code is to
- Make all setters slots
- Make sure the object with the properties lives in the GUI thread (see QObject::moveToThread and associated functions)
- Create a queued (or automatic) connection between the thread that wants to set the property and the QObject with the property, thereby calling the setting asynchronously in the context of the GUI thread.
@Asperamanca
For reference, see https://doc.qt.io/qt-6/bindableproperties.html:
The bindable property system is not thread-safe. Properties used in the binding expression on one thread must not be read or modified by any other thread. -
@Asperamanca
For reference, see https://doc.qt.io/qt-6/bindableproperties.html:
The bindable property system is not thread-safe. Properties used in the binding expression on one thread must not be read or modified by any other thread.@Asperamanca
Actually, i thought about 1 and 3, but it was working so well...
And i would have never mentioned the thread affinity, thank you very very much!
Farewell, nice guy from the internet! -
F felsi has marked this topic as solved on
-
F felsi has marked this topic as unsolved on
-
@Asperamanca
Actually, i thought about 1 and 3, but it was working so well...
And i would have never mentioned the thread affinity, thank you very very much!
Farewell, nice guy from the internet!Hello, again!
The bug is still alive.I did what Asperamanca suggested and verified it with:
qDebug() << "setTrack: running in main thread:" << (QApplication::instance()->thread() == QThread::currentThread()); qDebug() << "setTrack: playlistTrack living in main thread:" << (QApplication::instance()->thread() == this->thread()); qDebug() << "setTrack: track living in main thread:" << (QApplication::instance()->thread() == track->thread());But i still got the following behaviour:
onPlaylistTrackChanged 1
updateTrack 1 : NULL
onCompleted 1
onPlaylistTrackChanged 2
updateTrack 2 : NULL
onCompleted 2
onPlaylistTrackChanged 3
updateTrack 3 : NULL
onTrackChanged 1
updateTrack 1 : Track 1
onTrackChanged 2
updateTrack 2 : Track 2
onCompleted 3
onPlaylistTrackChanged 4
updateTrack 4 : Track 4
onCompleted 4
onPlaylistTrackChanged 5
updateTrack 5 : Track 5
onCompleted 5As you can see, there is running qml code intermediately while the delegate for track 3 is created.
So my guess would be, that the ListView creates the delegate in a separate qml thread.
And the problem is caused similar to the way i described in my former posts after all.Which would mean, that using connections in delegates is always unsafe...
Because you can always miss signals during creation. -
Could you post your current code, like you did in the OP?
-
Could you post your current code, like you did in the OP?
@Asperamanca
The code hasn't changed, i only changed the way i call it and added the lines to verify.
Aren't connections between C++ and QML blocking queued?
Therefore i don't think, that the mixed-up creation and updating on the qml side is caused on the C++ side.
I think my multithreaded approach only increased the likelyhood of happening and is apart from that coincidental. -
Does the qDebug() for setTrack appear with track 3? And in the main thread?
-
Side note:
I just noticed that this was pointless anyway:Track *PlaylistTrack::getTrackRef() { QReadLocker locker(&this->lock); return this->track.data(); }You hand out a pointer to the internal data of the track. The locker goes out of scope, and the pointer keeps being used in QML.
When setTrack assigns a new pointer to this->track, the old shared_ptr goes out of scope, gets destroyed, but might still be used by QML (use-after-free)
So your locking mechanism wouldn't have worked anyway.
Reference semantics are fraught with danger... -
Side note:
I just noticed that this was pointless anyway:Track *PlaylistTrack::getTrackRef() { QReadLocker locker(&this->lock); return this->track.data(); }You hand out a pointer to the internal data of the track. The locker goes out of scope, and the pointer keeps being used in QML.
When setTrack assigns a new pointer to this->track, the old shared_ptr goes out of scope, gets destroyed, but might still be used by QML (use-after-free)
So your locking mechanism wouldn't have worked anyway.
Reference semantics are fraught with danger...Yes, i just checked. (it appears more often, due to unrelated changes)
The lock is needed to safely access the member on the C++ side, not to protect between C++ and QML.
Yes, that's a bit ugly, but i am aware of that and there is always one shared pointer left holding the reference.
And what alternatives do i have? I think this is a general problem with shared pointers and qml. -
Yes, i just checked. (it appears more often, due to unrelated changes)
The lock is needed to safely access the member on the C++ side, not to protect between C++ and QML.
Yes, that's a bit ugly, but i am aware of that and there is always one shared pointer left holding the reference.
And what alternatives do i have? I think this is a general problem with shared pointers and qml.@felsi The alternative is to provide a stable pointer (the Q_PROPERTY is then a CONSTANT and cannot change), and change the content of the object.
The object itself has Q_PROPERTY entries in turn for all the single properties that can change (and some of them could - again - be stable pointers) -
But that was a side note.
Is your problem with Track 3 still open, or does it work now? -
I didn't see whether your setter for Track 3 was ever called. Your logging does not contain that information.
-
I didn't see whether your setter for Track 3 was ever called. Your logging does not contain that information.
It looks like this:
onPlaylistTrackChanged 1
updateTrack 1 : NULL
onCompleted 1
onPlaylistTrackChanged 2
updateTrack 2 : NULL
onCompleted 2
onPlaylistTrackChanged 3
updateTrack 3 : NULL
setTrack 1
onTrackChanged 1
updateTrack 1 : Track 1
setTrack 2
onTrackChanged 2
updateTrack 2 : Track 2
setTrack 3
setTrack 4
setTrack 5
onCompleted 3
onPlaylistTrackChanged 4
updateTrack 4 : Track 4
onCompleted 4
onPlaylistTrackChanged 5
updateTrack 5 : Track 5
onCompleted 5 -
I would really try this:
class Track : public QObject { Q_OBJECT Q_PROPERTY(QString labeling READ getLabeling NOTIFY notifyLabelingChanged) // I would prefer BINDABLE, it's much simpler to code, but not relevant to the issue at hand Q_PROPERTY(QString duration_string READ getDurationString NOTIFY notifyDurationStringChanged) public: // Add the obvious getters and setters private: QString m_Labeling{"loading..."}; QString m_DurationString{"00:00"}; }; class PlaylistTrack : public QObject { Q_OBJECT Q_PROPERTY(Track* track READ getTrackPtr CONSTANT) // I would add FINAL, but not really relevant public: Track* getTrack() {return &m_Track;} private: Track m_Track; // If you want PIMPL, I'd use std::unique_ptr<Track> and create it in the constrcutor };The track will be created with PlaylistTrack , and will live as long as PlaylistTrack does.
Initially, it holds the default strings you previously specified in your QML.
Once you have data, you simply change the properties, and QML should update.
There are some variants to this pattern, but the important thing is that object points stay where they are for as long as the QML scene exists. This saves you from a lot of tricky edge cases and extra checks on the QML side. -
I would really try this:
class Track : public QObject { Q_OBJECT Q_PROPERTY(QString labeling READ getLabeling NOTIFY notifyLabelingChanged) // I would prefer BINDABLE, it's much simpler to code, but not relevant to the issue at hand Q_PROPERTY(QString duration_string READ getDurationString NOTIFY notifyDurationStringChanged) public: // Add the obvious getters and setters private: QString m_Labeling{"loading..."}; QString m_DurationString{"00:00"}; }; class PlaylistTrack : public QObject { Q_OBJECT Q_PROPERTY(Track* track READ getTrackPtr CONSTANT) // I would add FINAL, but not really relevant public: Track* getTrack() {return &m_Track;} private: Track m_Track; // If you want PIMPL, I'd use std::unique_ptr<Track> and create it in the constrcutor };The track will be created with PlaylistTrack , and will live as long as PlaylistTrack does.
Initially, it holds the default strings you previously specified in your QML.
Once you have data, you simply change the properties, and QML should update.
There are some variants to this pattern, but the important thing is that object points stay where they are for as long as the QML scene exists. This saves you from a lot of tricky edge cases and extra checks on the QML side.Tracks are widely spread across my whole program. My collection points to them, sorted trees of my collection point to them and different PlaylistTracks in different playlists can point to the same Track. They can even get replaced program-wide.
They are created by the collection and are only looked up in a hash table. All for low memory, fast loading, etc.
I can live with my little qml workaround, i encountered no other problems.
I posted more, because maybe there is a bug, when using connections in delegates. -
I admit I don't fully understand the issue you have myself. However, eliminating the state change from "null" to "something" in QML can't hurt. If you want to manage the tracks in a more centralized way, pass the shared_ptr as part of the constructor of PlaylistTrack and never change it during the lifetime of PlaylistTrack. Make the Q_PROPERTY CONSTANT. From the perspective of QML, that should be the same as my suggestion.
-
I admit I don't fully understand the issue you have myself. However, eliminating the state change from "null" to "something" in QML can't hurt. If you want to manage the tracks in a more centralized way, pass the shared_ptr as part of the constructor of PlaylistTrack and never change it during the lifetime of PlaylistTrack. Make the Q_PROPERTY CONSTANT. From the perspective of QML, that should be the same as my suggestion.
Thank you very much, i also prefer the safe way, if possible. :)
And your suggestion is clean and safe. But i really need to be able to set and change the track for a variety of reasons.
In fact, it also appears when i change the track.And for the missed signal problem, it should not matter whether it's an address or a value, it's not even a signal-parameter.
To me, it looks like, that the delegate gets created in a separate thread, otherwise "updateTrack 2" couldn't appear during creation of track 3. But i don't have a clue about the Qt source code, so i can't look that up easily...
And my guess would further be, that signals emitted after the creation has started and before the connection is established, are simply lost. -
To the best of my knowledge, all the scene setup and JavaScript runs in the main thread. Only parts of rendering may run in a different thread, but they would not change your properties.
Is the "updateTrack" in the Button's onCompleted still commented out? Because your logging shows that track 3 gets the onCompleted after some properties have already been set, so I would not expect any signals to arrive anymore. -
To the best of my knowledge, all the scene setup and JavaScript runs in the main thread. Only parts of rendering may run in a different thread, but they would not change your properties.
Is the "updateTrack" in the Button's onCompleted still commented out? Because your logging shows that track 3 gets the onCompleted after some properties have already been set, so I would not expect any signals to arrive anymore.Fortunately, the problem appears every few runs...
By the way, there is of course no problem, when all are set before or after the delegate creations.
And also not everytime when in between.
After a few checks, i would say, the workarounds behave like expected.
For completeness:With connectTrack() hackmack:
onPlaylistTrackChanged 1
connectTrack 1
updateTrack 1 : NULL
onCompleted 1
onPlaylistTrackChanged 2
connectTrack 2
updateTrack 2 : NULL
onCompleted 2
onPlaylistTrackChanged 3
connectTrack 3
updateTrack 3 : NULL
setTrack 1
onTrackChanged 1
updateTrack 1 : Track 1
setTrack 2
onTrackChanged 2
updateTrack 2 : Track 2
setTrack 3
onTrackChanged 3
updateTrack 3 : Track 3
setTrack 4
setTrack 5
onCompleted 3
onPlaylistTrackChanged 4
connectTrack 4
updateTrack 4 : Track 4
onCompleted 4
onPlaylistTrackChanged 5
connectTrack 5
updateTrack 5 : Track 5
onCompleted 5With updateTrack() in onCompleted:
onPlaylistTrackChanged 1
updateTrack 1 : NULL
onCompleted 1
updateTrack 1 : NULL
onPlaylistTrackChanged 2
updateTrack 2 : NULL
onCompleted 2
updateTrack 2 : NULL
onPlaylistTrackChanged 3
updateTrack 3 : NULL
setTrack 1
onTrackChanged 1
updateTrack 1 : Track 1
setTrack 2
onTrackChanged 2
updateTrack 2 : Track 2
setTrack 3
setTrack 4
setTrack 5
onCompleted 3
updateTrack 3 : Track 3
onPlaylistTrackChanged 4
updateTrack 4 : Track 4
onCompleted 4
updateTrack 4 : Track 4
onPlaylistTrackChanged 5
updateTrack 5 : Track 5
onCompleted 5
updateTrack 5 : Track 5