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

[Alfonso Sanchez-Beato]

Review: Needs Fixing

Please see inline comments.

Diff comments:

> 
> === modified file 'src/core/media/track_list_implementation.cpp'
> --- src/core/media/track_list_implementation.cpp	2015-11-03 17:20:38 +0000
> +++ src/core/media/track_list_implementation.cpp	2015-11-03 17:20:38 +0000
> @@ -198,9 +198,62 @@
>          on_track_changed()(current_id);
>  }
>  
> +void 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())

This case and others below should be notified to the caller of the function. move_track() should return a boolean for this. The caller should eventually return with a DBus error if there has been an error.

> +    {
> +        std::cerr << "Can't move track since 'id' or 'to' are empty" << std::endl;
> +        return;
> +    }
> +
> +    if (id == to)
> +    {
> +        std::cerr << "Can't move track to it's same position" << std::endl;
> +        return;
> +    }
> +
> +    if (tracks().get().size() == 1)

Return with error here

> +    {
> +        std::cerr << "Can't move track since TrackList contains only one track" << std::endl;
> +        return;
> +    }
> +
> +    // 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())

We should return with error if "to" is not found

> +    {
> +        const auto result = tracks().update([this, id, to, &insert_point_it]
> +                (TrackList::Container& container)
> +        {
> +            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);
> +
> +            // Insert id at the location just before insert_point_it
> +            container.insert(insert_point_it, id);

current_iterator() will be invalid after these erase/insert, we must save the current id before doing anything and restore after changing the container.

Also, not finding id on the list is an error. What would we be inserting in that case? There would be no associated URI.

> +
> +            return true;
> +        });
> +
> +        if (result)
> +        {
> +            std::cout << "TrackList after move" << std::endl;
> +            for(auto track : tracks().get())
> +            {
> +                std::cout << track << std::endl;
> +            }
> +            on_track_moved()(id);
> +        }
> +    }
> +}
> +
>  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;
> 
> === modified file 'src/core/media/track_list_skeleton.cpp'
> --- src/core/media/track_list_skeleton.cpp	2015-11-03 17:20:38 +0000
> +++ src/core/media/track_list_skeleton.cpp	2015-11-03 17:20:38 +0000
> @@ -172,6 +173,18 @@
>          });
>      }
>  
> +    void handle_move_track(const core::dbus::Message::Ptr& msg)
> +    {
> +        media::Track::Id id;
> +        media::Track::Id to;
> +        msg->reader() >> id >> to;
> +
> +        impl->move_track(id, to);

We should return DBus errors when this call fails.

> +
> +        auto reply = dbus::Message::make_method_return(msg);
> +        bus->send(reply);
> +    }
> +
>      void handle_remove_track(const core::dbus::Message::Ptr& msg)
>      {
>          media::Track::Id track;


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