[Merge] lp:~phablet-team/media-hub/bg-playlists-vivid into lp:media-hub/stable
Alfonso Sanchez-Beato
alfonso.sanchez-beato at canonical.com
Tue Aug 11 07:34:33 UTC 2015
Review: Needs Fixing
Overall looks good, although I have some comments.
Diff comments:
> === modified file 'debian/changelog'
> --- debian/changelog 2015-07-10 18:12:22 +0000
> +++ debian/changelog 2015-08-07 14:47:21 +0000
> @@ -1,4 +1,10 @@
> -media-hub (3.1.0+15.10.20150710-0ubuntu1) wily; urgency=medium
> +media-hub (3.1.0+15.04.20150710-0ubuntu2) vivid; urgency=medium
Should be 0ubuntu1. Besides, I have seen that the version in the silo is 4.0.0+15.04.20150810-0ubuntu1, which is wrong as 4.0 should be only on wily indicating the C++ ABI change. I think that can happen if you previously attached a MP for media-hub trunk (4.0) in the silo and then changed it for this one ("stable"). Probably this needs some action from the trainguards to get fixed. Finally, I think you should change all entries in the changelog that are for 15.10 and wily to 15.04 and vivid.
> +
> + * Change version to vivid+overlay
> +
> + -- Jim Hodapp <jim.hodapp at canonical.com> Fri, 07 Aug 2015 10:46:42 -0400
> +
> +media-hub (3.1.0+15.04.20150710-0ubuntu1) vivid; urgency=medium
>
> [ CI Train Bot ]
> * New rebuild forced.
> @@ -31,6 +37,17 @@
>
> -- CI Train Bot <ci-train-bot at canonical.com> Mon, 01 Jun 2015 16:31:53 +0000
>
> +media-hub (3.1.0+15.10.20150601-0ubuntu1) wily; urgency=medium
> +
> + [ CI Train Bot ]
> + * New rebuild forced.
> +
> + [ Jim Hodapp ]
> + * Add next/previous track implementation for the Player and
> + can_go_next/previous.
> +
> + -- CI Train Bot <ci-train-bot at canonical.com> Mon, 01 Jun 2015 16:31:53 +0000
> +
This entry in the changelog is duplicated.
> media-hub (3.1.0+15.10.20150527.1-0ubuntu1) wily; urgency=medium
>
> [ CI Train Bot ]
>
> === modified file 'src/core/media/gstreamer/engine.cpp'
> --- src/core/media/gstreamer/engine.cpp 2015-06-01 16:30:59 +0000
> +++ src/core/media/gstreamer/engine.cpp 2015-08-07 14:47:21 +0000
> @@ -430,10 +429,14 @@
> {
> // No need to wait, and we can immediately return.
> if (d->state == media::Engine::State::stopped)
> + //if (d->state != media::Engine::State::playing ||
> + // d->state == media::Engine::State::paused)
These 2 commented lines should be removed.
> + {
> + std::cout << "Current player state is already stopped - no need to change state to stopped" << std::endl;
Probably "cerr" here because the caller should know the state, unless this is a very common situation.
> return true;
> -
> - auto result = d->playbin.set_state_and_wait(GST_STATE_NULL);
> -
> + }
> +
> + const auto result = d->playbin.set_state_and_wait(GST_STATE_NULL);
> if (result)
> {
> d->state = media::Engine::State::stopped;
>
> === modified file 'src/core/media/track_list_implementation.cpp'
> --- src/core/media/track_list_implementation.cpp 2015-05-22 21:19:28 +0000
> +++ src/core/media/track_list_implementation.cpp 2015-08-07 14:47:21 +0000
> @@ -59,17 +59,19 @@
>
> media::Track::UriType media::TrackListImplementation::query_uri_for_track(const media::Track::Id& id)
> {
> - auto it = d->meta_data_cache.find(id);
> + const auto it = d->meta_data_cache.find(id);
>
> if (it == d->meta_data_cache.end())
> return Track::UriType{};
>
> + //std::cout << "returning uri: " << std::get<0>(it->second) << std::endl;
> +
Please remove this commented-out line
> return std::get<0>(it->second);
> }
>
> media::Track::MetaData media::TrackListImplementation::query_meta_data_for_track(const media::Track::Id& id)
> {
> - auto it = d->meta_data_cache.find(id);
> + const auto it = d->meta_data_cache.find(id);
>
> if (it == d->meta_data_cache.end())
> return Track::MetaData{};
>
> === modified file 'src/core/media/track_list_skeleton.cpp'
> --- src/core/media/track_list_skeleton.cpp 2015-04-29 21:18:20 +0000
> +++ src/core/media/track_list_skeleton.cpp 2015-08-07 14:47:21 +0000
> @@ -90,7 +104,9 @@
> auto reply = dbus::Message::make_method_return(msg);
> // Only add the track to the TrackList if it passes the apparmor permissions check
> if (std::get<0>(result))
> + {
> impl->add_track_with_uri_at(uri, after, make_current);
> + }
I you prefer to add braces here I'd also add them to the "else" part of the statement.
> else
> std::cerr << "Warning: Not adding track " << uri <<
> " to TrackList because of inadequate client apparmor permissions." << std::endl;
> @@ -193,74 +230,182 @@
> std::bind(&Private::handle_go_to,
> std::ref(d),
> std::placeholders::_1));
> +
> + d->object->install_method_handler<mpris::TrackList::Reset>(
> + std::bind(&Private::handle_reset,
> + std::ref(d),
> + std::placeholders::_1));
> }
>
> media::TrackListSkeleton::~TrackListSkeleton()
> {
> }
>
> -bool media::TrackListSkeleton::has_next() const
> +bool media::TrackListSkeleton::has_next()
> {
> - const auto next_track = std::next(d->current_track);
> - std::cout << "has_next track? " << (next_track != tracks().get().end() ? "yes" : "no") << std::endl;
> - return next_track != tracks().get().end();
> + if (tracks().get().empty())
> + return false;
> +
> + const auto next_track = std::next(current_iterator());
> + return !is_last_track(next_track);
> }
>
> -bool media::TrackListSkeleton::has_previous() const
> +bool media::TrackListSkeleton::has_previous()
> {
> + if (tracks().get().empty())
> + return false;
> +
> // If we are looping over the entire list, then there is always a previous track
> if (d->loop_status == media::Player::LoopStatus::playlist)
> return true;
>
> - std::cout << "has_previous track? " << (d->current_track != tracks().get().begin() ? "yes" : "no") << std::endl;
> - return d->current_track != tracks().get().begin();
> + return d->current_track != std::begin(tracks().get());
> +}
> +
> +bool media::TrackListSkeleton::is_first_track(const ConstIterator &it)
> +{
> + return it == std::begin(tracks().get());
> +}
> +
> +bool media::TrackListSkeleton::is_last_track(const TrackList::ConstIterator &it)
> +{
> + return it == std::end(tracks().get());
> }
>
> media::Track::Id media::TrackListSkeleton::next()
> {
> std::cout << __PRETTY_FUNCTION__ << std::endl;
> if (tracks().get().empty())
> - return *(d->current_track);
> + return *(d->empty_iterator);
> +
> + const auto next_track = std::next(current_iterator());
> + bool do_go_to_next_track = false;
> +
> + // End of the track reached so loop around to the beginning of the track
> + if (d->loop_status == media::Player::LoopStatus::track)
> + {
> + std::cout << "Looping on the current track since LoopStatus is set to track" << std::endl;
> + do_go_to_next_track = true;
> + }
> + // End of the tracklist reached so loop around to the beginning of the tracklist
> + else if (d->loop_status == media::Player::LoopStatus::playlist && not has_next())
> + {
> + std::cout << "Looping on the tracklist since LoopStatus is set to playlist" << std::endl;
> + d->current_track = tracks().get().begin();
> + do_go_to_next_track = true;
> + }
> + else
> + {
> + // Next track is not the last track
> + if (not is_last_track(next_track))
> + {
> + std::cout << "Advancing to next track: " << *(next_track) << std::endl;
> + d->current_track = next_track;
> + do_go_to_next_track = true;
> + }
> + // At the end of the tracklist and not set to loop, so we stop advancing the tracklist
> + else
> + {
> + std::cout << "End of tracklist reached, not advancing to next since LoopStatus is set to none" << std::endl;
> + on_end_of_tracklist()();
> + }
> + }
> +
> + if (do_go_to_next_track)
> + {
> + on_track_changed()(*(current_iterator()));
> + // Don't automatically call stop() and play() in player_implementation.cpp on_go_to_track()
> + // since this breaks video playback when using open_uri() (stop() and play() are unwanted in
> + // this scenario since the qtubuntu-media will handle this automatically)
> + const bool toggle_player_state = false;
> + const media::Track::Id id = *(current_iterator());
> + const std::pair<const media::Track::Id, bool> p = std::make_pair(id, toggle_player_state);
> + // Signal the PlayerImplementation to play the next track
> + on_go_to_track()(p);
> + }
> +
> + return *(current_iterator());
> +}
> +
> +media::Track::Id media::TrackListSkeleton::previous()
> +{
> + std::cout << __PRETTY_FUNCTION__ << std::endl;
> + if (tracks().get().empty())
> + return *(d->empty_iterator);
> +
> + bool do_go_to_previous_track = false;
>
> // Loop on the current track forever
> if (d->loop_status == media::Player::LoopStatus::track)
> {
> std::cout << "Looping on the current track..." << std::endl;
> - return *(d->current_track);
> + do_go_to_previous_track = true;
> }
> // Loop over the whole playlist and repeat
> - else if (d->loop_status == media::Player::LoopStatus::playlist && !has_next())
> + else if (d->loop_status == media::Player::LoopStatus::playlist && is_first_track(current_iterator()))
> {
> std::cout << "Looping on the entire TrackList..." << std::endl;
> - d->current_track = tracks().get().begin();
> - return *(d->current_track);
> - }
> - else if (has_next())
> - {
> - // Keep returning the next track until the last track is reached
> - d->current_track = std::next(d->current_track);
> - std::cout << *this << std::endl;
> - }
> -
> - return *(d->current_track);
> -}
> -
> -media::Track::Id media::TrackListSkeleton::previous()
> -{
> - // TODO: Add logic to calculate the previous track
> - return *(d->current_track);
> + d->current_track = std::prev(tracks().get().end());
> + do_go_to_previous_track = true;
> + }
> + else
> + {
> + // Current track is not the first track
> + if (not is_first_track(current_iterator()))
> + {
> + // Keep returning the previous track until the first track is reached
> + d->current_track = std::prev(current_iterator());
> + do_go_to_previous_track = true;
> + }
> + // At the beginning of the tracklist and not set to loop, so we stop advancing the tracklist
> + else
> + {
> + std::cout << "Beginning of tracklist reached, not advancing to previous since LoopStatus is set to none" << std::endl;
> + on_end_of_tracklist()();
> + }
> + }
> +
> + if (do_go_to_previous_track)
> + {
> + on_track_changed()(*(current_iterator()));
> + // Don't automatically call stop() and play() in player_implementation.cpp on_go_to_track()
> + // since this breaks video playback when using open_uri() (stop() and play() are unwanted in
> + // this scenario since the qtubuntu-media will handle this automatically)
> + const bool toggle_player_state = false;
> + const media::Track::Id id = *(current_iterator());
> + const std::pair<const media::Track::Id, bool> p = std::make_pair(id, toggle_player_state);
> + on_go_to_track()(p);
> + }
> +
> + return *(current_iterator());
> }
>
> const media::Track::Id& media::TrackListSkeleton::current()
> {
> + return *(current_iterator());
> +}
> +
> +const media::TrackList::ConstIterator& media::TrackListSkeleton::current_iterator()
> +{
> // Prevent the TrackList from sitting at the end which will cause
> // a segfault when calling current()
> if (tracks().get().size() && (d->current_track == d->empty_iterator))
> + {
> + std::cout << "Wrapping d->current_track back to begin()" << std::endl;
> d->current_track = d->skeleton.properties.tracks->get().begin();
> + }
> else if (tracks().get().empty())
Please add the braces also to the "else if" part
> std::cerr << "TrackList is empty therefore there is no valid current track" << std::endl;
>
> - return *(d->current_track);
> + return d->current_track;
> +}
> +
> +void media::TrackListSkeleton::reset_current_iterator_if_needed()
> +{
> + // If all tracks got removed then we need to keep a sane current
> + // iterator for further use.
> + if (tracks().get().empty())
> + d->current_track = d->empty_iterator;
> }
>
> const core::Property<bool>& media::TrackListSkeleton::can_edit_tracks() const
--
https://code.launchpad.net/~phablet-team/media-hub/bg-playlists-vivid/+merge/266765
Your team Ubuntu Phablet Team is requested to review the proposed merge of lp:~phablet-team/media-hub/bg-playlists-vivid into lp:media-hub/stable.
More information about the Ubuntu-reviews
mailing list