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

[Alfonso Sanchez-Beato]

Review: Needs Fixing

Looks good, just some minor comments.

In my changes to notify that that all tracks have been removed after calling media-hub's Reset call, I have done the same: assume that after the call returns the action has already been performed. So same approach :). I think we should change the provider to do this in all cases except for the calls for which we need some information back in the signal.

Diff comments:

> 
> === modified file 'src/aal/aalmediaplaylistcontrol.h'
> --- src/aal/aalmediaplaylistcontrol.h	2015-07-24 18:29:42 +0000
> +++ src/aal/aalmediaplaylistcontrol.h	2015-11-05 21:06:44 +0000
> @@ -24,6 +24,7 @@
>  
>  #include <core/connection.h>
>  
> +#include <atomic>

This include is not needed anymore

>  #include <memory>
>  
>  QT_BEGIN_NAMESPACE
> 
> === modified file 'src/aal/aalmediaplaylistprovider.cpp'
> --- src/aal/aalmediaplaylistprovider.cpp	2015-11-05 21:06:44 +0000
> +++ src/aal/aalmediaplaylistprovider.cpp	2015-11-05 21:06:44 +0000
> @@ -230,6 +230,76 @@
>      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;
> +
> +    // This must be emitted before the move_track occurs or things in AalMediaPlaylistControl
> +    // such as m_currentId won't be accurate
> +    Q_EMIT startMoveTrack(from, 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;
> +    }
> +
> +    qDebug() << "************ New track move:" << from << "to" << to;
> +    // Moves the track in local track_index_lut
> +    const bool ret = moveTrack(from, to);

We should do the call m_hubTrackList->move_track() before this.

> +    if (!ret)
> +    {
> +        qWarning() << Q_FUNC_INFO << "- Failed to move track";
> +        return false;
> +    }
> +
> +    try {
> +        m_hubTrackList->move_track(fromTrack, toTrack);
> +    }
> +    catch (const media::TrackList::Errors::FailedToMoveTrack &e) {
> +        qWarning() << "Failed to move track: " << e.what();
> +        return false;
> +    }
> +    catch (const media::TrackList::Errors::FailedToFindMoveTrackSource &e) {
> +        qWarning() << "Failed to move track: " << e.what();
> +        return false;
> +    }
> +    catch (const media::TrackList::Errors::FailedToFindMoveTrackDest &e) {
> +        qWarning() << "Failed to move track: " << e.what();
> +        return false;
> +    }
> +    catch (const std::runtime_error &e) {
> +        qWarning() << "Failed to move track: " << e.what();
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
>  bool AalMediaPlaylistProvider::removeMedia(int pos)
>  {
>      if (!m_hubTrackList) {
> @@ -249,11 +319,13 @@
>          return false;
>      }
>  
> +

Unneeded blank line

>      return true;
>  }
>  
>  bool AalMediaPlaylistProvider::removeMedia(int start, int end)
>  {
> +    qWarning() << Q_FUNC_INFO;
>      for (int i=start; i<=end; i++)
>      {
>          if (!removeMedia(i))
> @@ -391,6 +473,69 @@
>          m_trackAddedConnection.disconnect();
>  }
>  
> +bool AalMediaPlaylistProvider::moveTrack(int from, int to)
> +{
> +    const int listSize = static_cast<int>(track_index_lut.size());
> +    if (from < 0 or from >= listSize or to < 0 or to >= listSize)
> +    {
> +        qWarning() << Q_FUNC_INFO
> +            << "- failed to move track, 'from' or 'to' are invalid index values";
> +        qDebug() << "from:" << from << ", to:" << to
> +            << "track_index_lut.size():" << track_index_lut.size();
> +        return false;
> +    }
> +
> +    qDebug() << "track_index_lut before erase:";
> +    for (int i=0; i<static_cast<int>(track_index_lut.size()); ++i)
> +        qDebug() << track_index_lut.at(i).c_str();

Please remove these traces, must be very noisy

> +
> +    const media::Track::Id fromTrack = trackOfIndex(from);
> +    const media::Track::Id toTrack = trackOfIndex(to);
> +    qDebug() << "fromTrack:" << fromTrack.c_str();
> +    qDebug() << "toTrack:" << toTrack.c_str();
> +
> +    if (fromTrack.empty() or toTrack.empty())

We have already checked the indices, so this check is not needed.

> +    {
> +        qWarning() << Q_FUNC_INFO
> +            << "- failed to move track, 'fromTrack' or 'toTrack' track ids are empty";
> +        return false;
> +    }
> +
> +    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())
> +    {
> +        qWarning() << "Failed to find position to move track to:" << toTrack.c_str();
> +        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 mediaRemoved(from, from);
> +
> +    qDebug() << "track_index_lut before reinsert:";
> +    for (size_t i=0; i<track_index_lut.size(); ++i)
> +        qDebug() << track_index_lut.at(i).c_str();
> +
> +    Q_EMIT mediaAboutToBeInserted(to, to);
> +    track_index_lut.insert(insertIt, fromTrack);
> +
> +    qDebug() << "track_index_lut after full move:";
> +    for (size_t i=0; i<track_index_lut.size(); ++i)
> +        qDebug() << track_index_lut.at(i).c_str();

Please remove these traces too

> +
> +    Q_EMIT mediaInserted(to, to);
> +
> +    return true;
> +}
> +
>  bool AalMediaPlaylistProvider::removeTrack(const core::ubuntu::media::Track::Id &id)
>  {
>      if (id.empty())


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