[Merge] lp:~phablet-team/media-hub/add_move_track into lp:media-hub/stable

[Alfonso Sanchez-Beato]

Review: Needs Fixing

Please see comments inline

Diff comments:

> 
> === modified file 'src/core/media/track_list_implementation.cpp'
> --- src/core/media/track_list_implementation.cpp	2015-11-04 17:25:30 +0000
> +++ src/core/media/track_list_implementation.cpp	2015-11-04 17:25:30 +0000
> @@ -198,9 +198,91 @@
>          on_track_changed()(current_id);
>  }
>  
> +bool media::TrackListImplementation::move_track(const media::Track::Id& id,
> +                                                const media::Track::Id& to)
> +{
> +    std::cout << __PRETTY_FUNCTION__ << std::endl;
> +
> +    if (id.empty() or to.empty())
> +    {
> +        std::cerr << "Can't move track since 'id' or 'to' are empty" << std::endl;
> +        return false;
> +    }
> +
> +    if (id == to)
> +    {
> +        std::cerr << "Can't move track to it's same position" << std::endl;
> +        return false;
> +    }
> +
> +    if (tracks().get().size() == 1)
> +    {
> +        std::cerr << "Can't move track since TrackList contains only one track" << std::endl;
> +        return false;
> +    }
> +
> +    bool ret = false;
> +    // Get an iterator that points to the track that is the insertion point
> +    auto insert_point_it = std::find(tracks().get().begin(), tracks().get().end(), to);
> +    if (insert_point_it != tracks().get().end())
> +    {
> +        const auto result = tracks().update([this, id, to, &insert_point_it]
> +                (TrackList::Container& container)
> +        {
> +            // Get an iterator that points to the track to move within the TrackList
> +            auto to_move_it = std::find(tracks().get().begin(), tracks().get().end(), id);
> +            std::cout << "Erasing old track position: " << *to_move_it << std::endl;
> +            if (to_move_it != tracks().get().end())
> +            {
> +                container.erase(to_move_it);
> +            }
> +            else
> +            {
> +                throw media::TrackList::Errors::FailedToFindMoveTrackDest
> +                        ("Failed to find destination track " + to);
> +                return false;

This return is not needed.

> +            }
> +
> +            // Insert id at the location just before insert_point_it
> +            container.insert(insert_point_it, id);
> +
> +            // Make sure the current track is still valid
> +            if (to_move_it == current_iterator())

I do not think we can safely access the iterator here - think for instance of it being a pointer to a list node that has been erased. We should instead save the id before any modification and after that use std::find to find the position and update the iterator with that value.

> +            {
> +                const bool r = update_current_iterator(insert_point_it);
> +                if (!r)
> +                {
> +                    throw media::TrackList::Errors::FailedToMoveTrack();
> +                    return false;
> +                }
> +            }
> +
> +            return true;
> +        });
> +
> +        if (result)
> +        {
> +            std::cout << "TrackList after move" << std::endl;
> +            for(auto track : tracks().get())
> +            {
> +                std::cout << track << std::endl;
> +            }
> +            const media::TrackList::TrackIdTuple ids = std::make_tuple(id, to);
> +            // Signal to the client that track 'id' was moved within the TrackList
> +            on_track_moved()(ids);
> +            ret = true;
> +        }
> +    }
> +    else
> +        throw media::TrackList::Errors::FailedToFindMoveTrackSource
> +                ("Failed to find source track " + id);
> +
> +    return ret;
> +}
> +
>  void media::TrackListImplementation::remove_track(const media::Track::Id& id)
>  {
> -    auto result = tracks().update([id](TrackList::Container& container)
> +    const auto result = tracks().update([id](TrackList::Container& container)
>      {
>          container.erase(std::find(container.begin(), container.end(), id));
>          return true;


-- 
https://code.launchpad.net/~phablet-team/media-hub/add_move_track/+merge/276560
Your team Ubuntu Phablet Team is subscribed to branch lp:~phablet-team/media-hub/bg-playlist-fixes.