Skip to content
  • Categories
  • Recent
  • Tags
  • Popular
  • Users
  • Groups
  • Search
  • Get Qt Extensions
  • Unsolved
Collapse
Brand Logo
  1. Home
  2. Qt Development
  3. QML and Qt Quick
  4. Delegate creation and multithreaded property updating
Forum Updated to NodeBB v4.3 + New Features

Delegate creation and multithreaded property updating

Scheduled Pinned Locked Moved Solved QML and Qt Quick
47 Posts 3 Posters 8.6k Views 2 Watching
  • Oldest to Newest
  • Newest to Oldest
  • Most Votes
Reply
  • Reply as topic
Log in to reply
This topic has been deleted. Only users with topic management privileges can see it.
  • F Offline
    F Offline
    felsi
    wrote on last edited by
    #1

    Hello!
    I am writing a music player which can read and write playlists multithreaded. It works fine, but for a single time it didn't...
    Entries in these playlists are called PlaylistTracks. PlaylistTracks can get selected and point to Tracks.
    Tracks are unique for every media file and contain mainly metadata. When opening a playlist, all PlaylistTracks are created with track=NULL.
    A thread parses the playlist-file, looks up the Track and sets the pointer.
    The list of PlaylistTracks gets displayed by a custom model and a ListView. The delegate of the ListView shows "loading.." while track=NULL.

    Finally my problem: For a single time (in months), "loading..." didn't disappear.
    My guess would be, that the problem occured during the creation of the delegate.
    Because the onPlaylistTrackChanged triggered updateTrack() while the connection didn't.
    I would further assume, that calling updateTrack() within Component.onCompleted solves the problem.
    Unfortunatly i can't reproduce the problem and i am not sure about my assumption.
    But maybe, someone else knows more..
    I would be very glad about hints.

    My simplified code:

    class PlaylistTrack : public QObject
    {
        Q_OBJECT
    
        Q_PROPERTY(Track *track READ getTrackRef NOTIFY trackChanged)
    
    public:
    
        Track *PlaylistTrack::getTrackRef()
        {
            QReadLocker locker(&this->lock);
            return this->track.data();
        }
    
        void PlaylistTrack::setTrack(QSharedPointer<Track> track)
        {
            this->lock.lockForWrite();
            this->track = track;
            this->lock.unlock();
    
            emit this->trackChanged();
        }
    
    signals:
        void trackChanged();
    
    private:
        QReadWriteLock lock;
        QSharedPointer<Track> track = NULL;
    
    }
    

    And the delegate:

    Button {
        id: root_item
    
        Component.onCompleted:{
            //updateTrack();
        }
    
        property PlaylistTrack playlistTrack
        onPlaylistTrackChanged: {updateTrack();}
    
        Connections{
            target: playlistTrack
            function onTrackChanged(){updateTrack();}
        }
    
        function updateTrack()
        {
            if(playlistTrack==null)
            {
                labeling_item.text = "no track";
                duration_item.text = "-";
                return;
            }
            else if(playlistTrack.track==null)
            {
                labeling_item.text = "loading...";
                duration_item.text = "00:00";
                return;
            }
    
            let track = playlistTrack.track;
            labeling_item.text = track.labeling
            duration_item.text = track.duration_string;
        }
    }
    
    1 Reply Last reply
    0
    • F Offline
      F Offline
      felsi
      wrote on last edited by
      #2

      Maybe the property binding "target: playlistTrack" causes the problem, when:

      1. onPlaylistTrackChanged: {updateTrack();} (shows "loading...")
      2. thread sets track and "emit this->trackChanged()" goes nowhere, because
      3. "target: playlistTrack" gets re-evaluated and trackChanged is connected properly
        Normally 3. happens before 2. and everything's fine...

      Then it could always happen when i set a new playlistTrack...
      But maybe this could be fixed with:

      property PlaylistTrack playlistTrack
      onPlaylistTrackChanged: {connection.target=playlistTrack; updateTrack();}
      
      Connections{
          id: connection
          target: playlistTrack
          function onTrackChanged(){updateTrack();}
      }
      

      I would be very glad, if some QML-Pro, who knows, when exactly certain things happen, could verify or clarify this, because i can't test it...

      1 Reply Last reply
      0
      • F Offline
        F Offline
        felsi
        wrote on last edited by
        #3

        Ok, to do anything, the above should more look like this...

        Button {
            id: root_item
        
            Component.onCompleted:{
                //updateTrack();
            }
        
            property PlaylistTrack playlistTrack
            onPlaylistTrackChanged: {connectTrack(); updateTrack();}
        
            function connectTrack()
            {
                if(playlistTrack == null)
                    return;
        
                playlistTrack.trackChanged.connect(updateTrack);
            }
        
            function updateTrack()
            {
                if(playlistTrack==null)
                {
                    labeling_item.text = "no track";
                    duration_item.text = "-";
                    return;
                }
                else if(playlistTrack.track==null)
                {
                    labeling_item.text = "loading...";
                    duration_item.text = "00:00";
                    return;
                }
        
                let track = playlistTrack.track;
                labeling_item.text = track.labeling
                duration_item.text = track.duration_string;
            }
        }
        

        But i am still not sure, if this solves my issue or if the bug is just sitting somewhere else...
        Even a "looks good to me" would make me feel more confident, that i actually killed the bug...

        A 1 Reply Last reply
        0
        • F felsi

          Ok, to do anything, the above should more look like this...

          Button {
              id: root_item
          
              Component.onCompleted:{
                  //updateTrack();
              }
          
              property PlaylistTrack playlistTrack
              onPlaylistTrackChanged: {connectTrack(); updateTrack();}
          
              function connectTrack()
              {
                  if(playlistTrack == null)
                      return;
          
                  playlistTrack.trackChanged.connect(updateTrack);
              }
          
              function updateTrack()
              {
                  if(playlistTrack==null)
                  {
                      labeling_item.text = "no track";
                      duration_item.text = "-";
                      return;
                  }
                  else if(playlistTrack.track==null)
                  {
                      labeling_item.text = "loading...";
                      duration_item.text = "00:00";
                      return;
                  }
          
                  let track = playlistTrack.track;
                  labeling_item.text = track.labeling
                  duration_item.text = track.duration_string;
              }
          }
          

          But i am still not sure, if this solves my issue or if the bug is just sitting somewhere else...
          Even a "looks good to me" would make me feel more confident, that i actually killed the bug...

          A Offline
          A Offline
          Asperamanca
          wrote on last edited by
          #4

          @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

          1. Make all setters slots
          2. Make sure the object with the properties lives in the GUI thread (see QObject::moveToThread and associated functions)
          3. 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.
          A 1 Reply Last reply
          2
          • A Asperamanca

            @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

            1. Make all setters slots
            2. Make sure the object with the properties lives in the GUI thread (see QObject::moveToThread and associated functions)
            3. 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.
            A Offline
            A Offline
            Asperamanca
            wrote on last edited by
            #5

            @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.

            F 1 Reply Last reply
            2
            • A Asperamanca

              @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.

              F Offline
              F Offline
              felsi
              wrote on last edited by
              #6

              @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 1 Reply Last reply
              0
              • F felsi has marked this topic as solved on
              • F felsi has marked this topic as unsolved on
              • F felsi

                @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 Offline
                F Offline
                felsi
                wrote on last edited by felsi
                #7

                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 5

                As 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.

                1 Reply Last reply
                0
                • A Offline
                  A Offline
                  Asperamanca
                  wrote on last edited by
                  #8

                  Could you post your current code, like you did in the OP?

                  F 1 Reply Last reply
                  0
                  • A Asperamanca

                    Could you post your current code, like you did in the OP?

                    F Offline
                    F Offline
                    felsi
                    wrote on last edited by
                    #9

                    @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.

                    1 Reply Last reply
                    0
                    • A Offline
                      A Offline
                      Asperamanca
                      wrote on last edited by
                      #10

                      Does the qDebug() for setTrack appear with track 3? And in the main thread?

                      1 Reply Last reply
                      0
                      • A Offline
                        A Offline
                        Asperamanca
                        wrote on last edited by Asperamanca
                        #11

                        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...

                        F 1 Reply Last reply
                        1
                        • A Asperamanca

                          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...

                          F Offline
                          F Offline
                          felsi
                          wrote on last edited by felsi
                          #12

                          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.

                          A 1 Reply Last reply
                          0
                          • F Offline
                            F Offline
                            felsi
                            wrote on last edited by
                            #13

                            Oh, sorry, i misunderstood, exactly, you're absolutely wright.
                            No problem with gui thread...

                            1 Reply Last reply
                            0
                            • F felsi

                              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.

                              A Offline
                              A Offline
                              Asperamanca
                              wrote on last edited by
                              #14

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

                              1 Reply Last reply
                              0
                              • A Offline
                                A Offline
                                Asperamanca
                                wrote on last edited by
                                #15

                                But that was a side note.
                                Is your problem with Track 3 still open, or does it work now?

                                1 Reply Last reply
                                0
                                • F Offline
                                  F Offline
                                  felsi
                                  wrote on last edited by
                                  #16

                                  Yes, still open, nothing changed about that problem.
                                  And pretty sure not related to threading anymore.

                                  Uh, thank you very much, i will check out these topics!

                                  1 Reply Last reply
                                  0
                                  • A Offline
                                    A Offline
                                    Asperamanca
                                    wrote on last edited by
                                    #17

                                    I didn't see whether your setter for Track 3 was ever called. Your logging does not contain that information.

                                    F 1 Reply Last reply
                                    0
                                    • A Asperamanca

                                      I didn't see whether your setter for Track 3 was ever called. Your logging does not contain that information.

                                      F Offline
                                      F Offline
                                      felsi
                                      wrote on last edited by
                                      #18

                                      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

                                      1 Reply Last reply
                                      0
                                      • A Offline
                                        A Offline
                                        Asperamanca
                                        wrote on last edited by
                                        #19

                                        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.

                                        F 1 Reply Last reply
                                        0
                                        • A Asperamanca

                                          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.

                                          F Offline
                                          F Offline
                                          felsi
                                          wrote on last edited by felsi
                                          #20

                                          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.

                                          1 Reply Last reply
                                          0

                                          • Login

                                          • Login or register to search.
                                          • First post
                                            Last post
                                          0
                                          • Categories
                                          • Recent
                                          • Tags
                                          • Popular
                                          • Users
                                          • Groups
                                          • Search
                                          • Get Qt Extensions
                                          • Unsolved