[Merge] lp:~phablet-team/media-hub/mpris-take-2 into lp:media-hub/stable
Jim Hodapp
jim.hodapp at canonical.com
Mon Sep 28 14:39:31 UTC 2015
Review: Needs Fixing code
A few comments inline below.
Diff comments:
>
> === modified file 'src/core/media/player_implementation.cpp'
> --- src/core/media/player_implementation.cpp 2015-07-27 22:15:33 +0000
> +++ src/core/media/player_implementation.cpp 2015-09-28 13:22:21 +0000
> @@ -376,13 +390,15 @@
>
> std::function<bool()> can_go_next_getter = [this]()
> {
> - return d->track_list->has_next();
> + // If LoopStatus == playlist, then there is always a next track
> + return d->track_list->has_next() or Parent::loop_status() != Player::LoopStatus::none;
Good catch on LoopStatus of playlist and track, both indeed mean that there's always a next/previous
> };
> Parent::can_go_next().install(can_go_next_getter);
>
> std::function<bool()> can_go_previous_getter = [this]()
> {
> - return d->track_list->has_previous();
> + // If LoopStatus == playlist, then there is always a next previous
> + return d->track_list->has_previous() or Parent::loop_status() != Player::LoopStatus::none;
> };
> Parent::can_go_previous().install(can_go_previous_getter);
>
>
> === 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-28 13:22:21 +0000
> @@ -612,68 +634,87 @@
> 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)
> - {
> - mpris::Player::Dictionary dict;
> -
> - bool has_title = md.count(xesam::Title::name) > 0;
> - bool has_album_name = md.count(xesam::Album::name) > 0;
> - bool has_artist_name = md.count(xesam::Artist::name) > 0;
> -
> - if (has_title)
> - dict[xesam::Title::name] = dbus::types::Variant::encode(md.get(xesam::Title::name));
> - if (has_album_name)
> - dict[xesam::Album::name] = dbus::types::Variant::encode(md.get(xesam::Album::name));
> - if (has_artist_name)
> - dict[xesam::Artist::name] = dbus::types::Variant::encode(md.get(xesam::Artist::name));
> -
> - dict[mpris::metadata::ArtUrl::name] = dbus::types::Variant::encode(
> - cover_art_resolver(
> - has_title ? md.get(xesam::Title::name) : "",
> - has_album_name ? md.get(xesam::Album::name) : "",
> - has_artist_name ? md.get(xesam::Artist::name) : ""));
> -
> - mpris::Player::Dictionary wrap;
> - wrap[mpris::Player::Properties::Metadata::name()] = dbus::types::Variant::encode(dict);
> -
> - player.signals.properties_changed->emit(
> - std::make_tuple(
> - dbus::traits::Service<mpris::Player::Properties::Metadata::Interface>::interface_name(),
> - wrap,
> - std::vector<std::string>()));
> - });
> + 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.
Let's discuss this. I need you to fully point out what you mean by this comment as it's not completely clear to me.
> + 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 cover_art_resolver() is not implemented yet
> + connections.meta_data_changed = cp->meta_data_for_current_track().changed().connect(
> + [this](const core::ubuntu::media::Track::MetaData& md)
> + {
> + mpris::Player::Dictionary dict;
> +
> + bool has_title = md.count(xesam::Title::name) > 0;
> + bool has_album_name = md.count(xesam::Album::name) > 0;
> + bool has_artist_name = md.count(xesam::Artist::name) > 0;
> +
> + if (has_title)
> + dict[xesam::Title::name] = dbus::types::Variant::encode(md.get(xesam::Title::name));
> + if (has_album_name)
> + dict[xesam::Album::name] = dbus::types::Variant::encode(md.get(xesam::Album::name));
> + if (has_artist_name)
> + dict[xesam::Artist::name] = dbus::types::Variant::encode(md.get(xesam::Artist::name));
> +
> + dict[mpris::metadata::ArtUrl::name] = dbus::types::Variant::encode(
> + cover_art_resolver(
> + has_title ? md.get(xesam::Title::name) : "",
> + has_album_name ? md.get(xesam::Album::name) : "",
> + has_artist_name ? md.get(xesam::Artist::name) : ""));
> +
> + mpris::Player::Dictionary wrap;
> + wrap[mpris::Player::Properties::Metadata::name()] = dbus::types::Variant::encode(dict);
> +
> + player.signals.properties_changed->emit(
> + std::make_tuple(
> + 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;
> + // 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-28 13:22:21 +0000
> @@ -241,16 +241,31 @@
>
> bool media::TrackListSkeleton::has_next()
> {
> - if (tracks().get().empty())
> + auto n_tracks = tracks().get().size();
const auto
> +
> + if (n_tracks == 0)
> return false;
>
> + // TODO Using current_iterator() makes media-hub crash later. Logic for
> + // 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
> @@ -273,8 +288,11 @@
> media::Track::Id media::TrackListSkeleton::next()
> {
> 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
> + std::cerr << "ERROR: no tracks, cannot go to next" << std::endl;
> + return "";
To be consistent with the style of the media-hub code, I'd rather this be "return media::Track::Id{};"
> + }
>
> const auto next_track = std::next(current_iterator());
> bool do_go_to_next_track = false;
> @@ -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
> + std::cerr << "ERROR: no tracks, cannot go to previous" << std::endl;
> + return "";
To be consistent with the style of the media-hub code, I'd rather this be "return media::Track::Id{};"
> + }
>
> 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 subscribed to branch lp:media-hub/stable.
More information about the Ubuntu-reviews
mailing list