[Merge] lp:~phablet-team/media-hub/bg-playlist-fixes into lp:media-hub/stable

[Jim Hodapp]

Review: Needs Fixing

A few comments below about the latest changes.

Diff comments:

> 
> === modified file 'src/core/media/track_list_skeleton.cpp'
> --- src/core/media/track_list_skeleton.cpp	2015-11-12 12:34:43 +0000
> +++ src/core/media/track_list_skeleton.cpp	2015-11-12 12:34:43 +0000
> @@ -110,6 +115,8 @@
>              // Only add the track to the TrackList if it passes the apparmor permissions check
>              if (std::get<0>(result))
>              {
> +                media::Track::Id next;
> +                //if (make_current)

Why is this commented out?

>                  impl->add_track_with_uri_at(uri, after, make_current);
>              }
>              else
> @@ -176,8 +183,56 @@
>          media::Track::Id track;
>          msg->reader() >> track;
>  
> +        auto id_it = find(impl->tracks().get().begin(), impl->tracks().get().end(), track);
> +        if (id_it == impl->tracks().get().end()) {
> +            ostringstream err_str;
> +            err_str << "Track " << track << " not found in play list";
> +            cout << __PRETTY_FUNCTION__ << " WARNING " << err_str.str() << endl;
> +            auto reply = dbus::Message::make_error(
> +                            msg,
> +                            mpris::TrackList::Error::TrackNotFound::name,
> +                            err_str.str());
> +            bus->send(reply);
> +            return;
> +        }
> +
> +        media::Track::Id next;
> +        bool deleting_current = false;
> +
> +        if (id_it == impl->current_iterator()) {
> +            cout << "Removing current track" << endl;
> +            deleting_current = true;
> +
> +            if (current_track != empty_iterator) {
> +                ++current_track;
> +
> +                if (   current_track == impl->tracks().get().end()

Remove the leading spaces in front of current_track

> +                    && loop_status == media::Player::LoopStatus::playlist)
> +                        current_track = impl->tracks().get().begin();
> +
> +                if (current_track == impl->tracks().get().end())
> +                    current_track = empty_iterator;
> +                else
> +                    next = *current_track;
> +            }
> +        } else if (current_track != empty_iterator) {
> +            next = *current_track;
> +        }
> +
>          impl->remove_track(track);
>  
> +        if (not next.empty()) {
> +            current_track = find(impl->tracks().get().begin(), impl->tracks().get().end(), next);
> +
> +            if (deleting_current) {
> +                const bool toggle_player_state = true;
> +                impl->go_to(next, toggle_player_state);
> +            } else {
> +                // To force an index update in the client
> +                impl->on_track_changed()(next);

I don't agree that we should emit the on_track_changed signal when deleting a track that isn't the currently playing one. The position in the list has changed, but the actual track hasn't changed. If a signal like this is necessary, then another option is to add a new signal to the TrackList interface or you could also fire on_track_list_replaced.

> +            }
> +        }
> +
>          auto reply = dbus::Message::make_method_return(msg);
>          bus->send(reply);
>      }
> @@ -510,6 +577,24 @@
>          d->current_track = d->empty_iterator;
>  }
>  
> +media::Track::Id media::TrackListSkeleton::get_current_track(void)
> +{
> +    if (d->current_track == d->empty_iterator || tracks().get().empty())
> +        return media::Track::Id{};
> +
> +    return *(current_iterator());
> +}
> +
> +void media::TrackListSkeleton::set_current_track(const media::Track::Id& id)
> +{
> +    auto id_it = find(tracks().get().begin(), tracks().get().end(), id);
> +    if (id_it != tracks().get().end()) {
> +        d->current_track = id_it;
> +        // To force an index update in the client
> +        on_track_changed()(id);

See previous comment about this.

> +    }
> +}
> +
>  const core::Property<bool>& media::TrackListSkeleton::can_edit_tracks() const
>  {
>      return *d->skeleton.properties.can_edit_tracks;


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