[Merge] lp:~phablet-team/qtubuntu-media/fix-1510221 into lp:qtubuntu-media/stable

[Alfonso Sanchez-Beato]

Review: Needs Fixing

Some inline comments.

Also, I think we need to handle track_index_lut differently when we receive onTrackAdded (I am not able to see the changes for the insert track case in the page, although I saw them locally). The MPRIS spec says that the TrackAdded signal has an "AfterTrack" field that we should use when inserting the id in track_index_lut.

Diff comments:

> === modified file 'src/aal/aalmediaplaylistprovider.cpp'
> --- src/aal/aalmediaplaylistprovider.cpp	2015-11-03 21:29:40 +0000
> +++ src/aal/aalmediaplaylistprovider.cpp	2015-11-03 21:29:40 +0000
> @@ -230,6 +233,69 @@
>      return false;
>  }
>  
> +bool AalMediaPlaylistProvider::moveMedia(int from, int to)
> +{
> +    if (!m_hubTrackList) {
> +        qWarning() << "Track list does not exist so can't add a new track";
> +        return false;
> +    }
> +
> +    if (from < 0 or from >= static_cast<int>(track_index_lut.size())) {
> +        qWarning() << "Failed to moveMedia(), index 'from' is out of valid range";
> +        return false;
> +    }
> +
> +    if (to < 0 or to >= static_cast<int>(track_index_lut.size())) {
> +        qWarning() << "Failed to moveMedia(), index 'to' is out of valid range";
> +        return false;
> +    }
> +
> +    if (from == to)
> +        return true;
> +
> +    m_moveTrackStartIndex = from;
> +    m_moveTrackDestIndex = to;
> +
> +    const media::Track::Id fromTrack = trackOfIndex(from);
> +    if (fromTrack.empty()) {
> +        qWarning() << Q_FUNC_INFO
> +            << "failed to moveMedia due to failure to look up correct track id to move";
> +        return false;
> +    }
> +
> +    const media::Track::Id toTrack = trackOfIndex(to);
> +    if (toTrack.empty()) {
> +        qWarning() << Q_FUNC_INFO
> +            << "failed to moveMedia due to failure to look up correct track id to move to";
> +        return false;
> +    }
> +
> +    try {
> +        m_hubTrackList->move_track(fromTrack, toTrack);
> +    }
> +    catch (const std::runtime_error &e) {
> +        qWarning() << "Failed to move track: " << e.what();
> +        return false;
> +    }
> +
> +    // Update the locally cached tracklist
> +    Q_EMIT mediaAboutToBeRemoved(from, from);
> +    std::vector<media::Track::Id>::const_iterator eraseIt = std::find(track_index_lut.begin(), track_index_lut.end(), fromTrack);
> +    if (eraseIt != track_index_lut.end())
> +        track_index_lut.erase(eraseIt);
> +    else
> +        return false;
> +
> +    Q_EMIT mediaAboutToBeInserted(to, to);
> +    const std::vector<media::Track::Id>::const_iterator insertIt = std::find(track_index_lut.begin(), track_index_lut.end(), toTrack);
> +    if (insertIt != track_index_lut.end())
> +        track_index_lut.insert(insertIt, fromTrack);

Why don't we wait for the OnTrackMoved signal to modify track_index_lut, like for added tracks? We should try to follow the same design as for the calls specified by MPRIS.

> +    else
> +        return false;
> +
> +    return true;
> +}
> +
>  bool AalMediaPlaylistProvider::removeMedia(int pos)
>  {
>      if (!m_hubTrackList) {
> @@ -249,11 +315,13 @@
>          return false;
>      }
>  
> +

Unneeded blank line here.

>      return true;
>  }
>  
>  bool AalMediaPlaylistProvider::removeMedia(int start, int end)
>  {
> +    qWarning() << Q_FUNC_INFO;
>      for (int i=start; i<=end; i++)
>      {
>          if (!removeMedia(i))
> 
> === modified file 'src/aal/aalmediaplaylistprovider.h'
> --- src/aal/aalmediaplaylistprovider.h	2015-11-03 21:29:40 +0000
> +++ src/aal/aalmediaplaylistprovider.h	2015-11-03 21:29:40 +0000
> @@ -79,6 +83,10 @@
>      // Did the client perform an insertTrack() (as opposed to an addTrack()) operation?
>      // If yes, the index will be zero or greater, if not index will be -1;
>      std::atomic<int> m_insertTrackIndex;
> +    // Stores the starting index of the moveTrack request
> +    std::atomic<int> m_moveTrackStartIndex;
> +    // Stores the destination index of the moveTrack request
> +    std::atomic<int> m_moveTrackDestIndex;
>  };

I do not think that making these variables atomic really fixes any concurrency problems we might have. If the dbus-cpp signals use a different thread to calls to, say, insertMedia, this is not enough. We would need a lock to protect the whole transaction (we need to protect the access to the lut too, for instance).

Even more, what would happen if insertMedia is called several times before we receive/process signals back from media-hub? m_insertTrackIndex would only have the value of the last call. I would add more on this in the review comment.

>  
>  QT_END_NAMESPACE


-- 
https://code.launchpad.net/~phablet-team/qtubuntu-media/fix-1510221/+merge/276559
Your team Ubuntu Phablet Team is subscribed to branch lp:~phablet-team/qtubuntu-media/bg-playlist-fixes.