[Merge] lp:~phablet-team/media-hub/mpris-take-2 into lp:media-hub/stable

[Simon Fels]

Diff comments:

> 
> === modified file 'src/core/media/service_skeleton.cpp'
> --- src/core/media/service_skeleton.cpp	2015-08-11 16:10:14 +0000
> +++ src/core/media/service_skeleton.cpp	2015-09-23 08:43:51 +0000
> @@ -612,17 +634,47 @@
>                  player.properties.position->set(position);
>              });
>  
> -            connections.playback_status_changed = cp->playback_status().changed().connect([this](core::ubuntu::media::Player::PlaybackStatus status)
> +            connections.playback_status_changed = cp->playback_status().changed().connect(
> +                [this](core::ubuntu::media::Player::PlaybackStatus status)
>              {
>                  player.properties.playback_status->set(mpris::Player::PlaybackStatus::from(status));
>              });
>  
> -            connections.loop_status_changed = cp->loop_status().changed().connect([this](core::ubuntu::media::Player::LoopStatus status)
> +            connections.loop_status_changed = cp->loop_status().changed().connect(
> +                [this](core::ubuntu::media::Player::LoopStatus status)
>              {
>                  player.properties.loop_status->set(mpris::Player::LoopStatus::from(status));
>              });
>  
> -            connections.meta_data_changed = cp->meta_data_for_current_track().changed().connect([this](const core::ubuntu::media::Track::MetaData& md)
> +            connections.can_go_previous_changed = cp->can_go_previous().changed().connect(
> +                [this](bool can_go_previous)
> +            {
> +                player.properties.can_go_previous->set(can_go_previous);
> +            });
> +
> +            connections.can_go_next_changed = cp->can_go_next().changed().connect(
> +                [this](bool can_go_next)
> +            {
> +                player.properties.can_go_next->set(can_go_next);
> +            });
> +
> +            // Sync property values between session and player mpris::Player instances
> +            // TODO Getters from media::Player actually return values from a
> +            // mpris::Player::Skeleton instance different from "player". Each of them use
> +            // different DBus object paths. Does this make any sense? Discuss.
> +            player.properties.duration->set(cp->duration().get());
> +            player.properties.position->set(cp->position().get());
> +            player.properties.playback_status->set(mpris::Player::PlaybackStatus::from(
> +                                                       cp->playback_status().get()));
> +            player.properties.loop_status->set(mpris::Player::LoopStatus::from(
> +                                                   cp->loop_status().get()));
> +            player.properties.can_go_previous->set(cp->can_go_previous().get());
> +            player.properties.can_go_next->set(cp->can_go_next().get());
> +
> +#if 0
> +            // TODO Lambda currently crashing, needs further research

Do we have a bug / backlog item to research that?

> +            connections.meta_data_changed = cp->meta_data_for_current_track().changed().connect(
> +                [this](const core::ubuntu::media::Track::MetaData& md)
>              {
>                  mpris::Player::Dictionary dict;
>  
> @@ -648,32 +700,21 @@
>  
>                  player.signals.properties_changed->emit(
>                              std::make_tuple(
> -                                dbus::traits::Service<mpris::Player::Properties::Metadata::Interface>::interface_name(),
> +                                dbus::traits::Service<
> +                                    mpris::Player::Properties::Metadata::Interface>
> +                                        ::interface_name(),
>                                  wrap,
>                                  std::vector<std::string>()));
> -            });
> +                                });
> +#endif
>          }
>  
> -        void unset_current_player()
> +        void reset_current_player()
>          {
> +            std::cout << __PRETTY_FUNCTION__ << std::endl;

Still needed? Otherwise drop left over debug statement.

> +            // And announce that we can no longer be controlled.
> +            player.properties.can_control->set(false);
>              current_player.reset();
> -
> -            // We disconnect all previous event connections.
> -            connections.seeked_to.disconnect();
> -            connections.duration_changed.disconnect();
> -            connections.position_changed.disconnect();
> -            connections.playback_status_changed.disconnect();
> -            connections.loop_status_changed.disconnect();
> -            connections.meta_data_changed.disconnect();
> -
> -            // And announce that we cannot be controlled anymore.
> -            player.properties.can_control->set(false);
> -        }
> -
> -        void unset_if_current(const std::shared_ptr<media::Player>& cp)
> -        {
> -            if (cp == current_player.lock())
> -                unset_current_player();
>          }
>  
>          dbus::Bus::Ptr bus;
> 
> === modified file 'src/core/media/track_list_skeleton.cpp'
> --- src/core/media/track_list_skeleton.cpp	2015-08-11 15:25:50 +0000
> +++ src/core/media/track_list_skeleton.cpp	2015-09-23 08:43:51 +0000
> @@ -241,16 +241,31 @@
>  
>  bool media::TrackListSkeleton::has_next()
>  {
> -    if (tracks().get().empty())
> +    auto n_tracks = tracks().get().size();
> +
> +    if (n_tracks == 0)
>          return false;
>  
> +    // TODO Using current_iterator() makes media-hub crash later. Logic for

Do we have a bug / backlog item to research that?

> +    // handling the iterators must be reviewed. As a minimum updates to the
> +    // track list should update current_track instead of the list being sneakly
> +    // changed in player_implementation.cpp.
> +    // To avoid the crash we consider that current_track will be eventually
> +    // initialized to the first track when current_iterator() gets called.
> +    if (d->current_track == d->empty_iterator) {
> +        if (n_tracks < 2)
> +            return false;
> +        else
> +            return true;
> +    }
> +
>      const auto next_track = std::next(current_iterator());
>      return !is_last_track(next_track);
>  }
>  
>  bool media::TrackListSkeleton::has_previous()
>  {
> -    if (tracks().get().empty())
> +    if (tracks().get().empty() || d->current_track == d->empty_iterator)
>          return false;
>  
>      // If we are looping over the entire list, then there is always a previous track
> @@ -328,8 +346,11 @@
>  media::Track::Id media::TrackListSkeleton::previous()
>  {
>      std::cout << __PRETTY_FUNCTION__ << std::endl;
> -    if (tracks().get().empty())
> -        return *(d->empty_iterator);
> +    if (tracks().get().empty()) {
> +        // TODO Change ServiceSkeleton to return with error from DBus call

Do we have a bug / backlog item for this?

> +        std::cerr << "ERROR: no tracks, cannot go to previous" << std::endl;
> +        return "";
> +    }
>  
>      bool do_go_to_previous_track = false;
>  


-- 
https://code.launchpad.net/~phablet-team/media-hub/mpris-take-2/+merge/271942
Your team Ubuntu Phablet Team is requested to review the proposed merge of lp:~phablet-team/media-hub/mpris-take-2 into lp:media-hub/stable.