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

[Jim Hodapp]

Review: Needs Fixing code

A few comments inline.

Diff comments:

> 
> === modified file 'src/core/media/player_implementation.cpp'
> --- src/core/media/player_implementation.cpp	2015-10-22 17:31:32 +0000
> +++ src/core/media/player_implementation.cpp	2015-11-02 16:20:51 +0000
> @@ -676,6 +676,13 @@
>  bool media::PlayerImplementation<Parent>::open_uri(const Track::UriType& uri)
>  {
>      d->track_list->reset();
> +
> +    // If empty uri, give the same meaning as QMediaPlayer::setMedia("")
> +    if (uri.empty()) {
> +        cout << __PRETTY_FUNCTION__ << ": cleaning current media" << endl;

I don't understand what this cout means. How is this cleaning the Player? Do you mean instead to say it's resetting the current media?

> +        return TRUE;

Use lowercase true

> +    }
> +
>      const bool ret = d->engine->open_resource_for_uri(uri, false);
>      // Don't set new track as the current track to play since we're calling open_resource_for_uri above
>      static const bool make_current = false;
> 
> === modified file 'src/core/media/service_skeleton.cpp'
> --- src/core/media/service_skeleton.cpp	2015-10-14 20:54:19 +0000
> +++ src/core/media/service_skeleton.cpp	2015-11-02 16:20:51 +0000
> @@ -442,9 +446,19 @@
>      {
>          Player::PlayerKey key;
>          msg->reader() >> key;
> -        impl->set_current_player(key);
> -
> -        auto reply = dbus::Message::make_method_return(msg);
> +
> +        core::dbus::Message::Ptr reply;
> +        if (not configuration.player_store->has_player_for_key(key)) {
> +            cerr <<  __PRETTY_FUNCTION__ << " player key not found - " << key << endl;
> +            reply = dbus::Message::make_error(
> +                            msg,
> +                            mpris::Service::Errors::SettingCurrentPlayer::name(),

I would rename this error to be something like PlayerKeyNotFound which I think would be a more accurate error name.

> +                            "Player key not found");
> +        } else {
> +            impl->set_current_player(key);
> +            reply = dbus::Message::make_method_return(msg);
> +        }
> +
>          impl->access_bus()->send(reply);
>      }
>  


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