[Merge] lp:~phablet-team/media-hub/add_move_track into lp:media-hub/stable
Jim Hodapp
jim.hodapp at canonical.com
Wed Nov 18 17:04:47 UTC 2015
Review: Needs Fixing code
Several inline comments below.
Diff comments:
>
> === modified file 'src/core/media/track_list_implementation.cpp'
> --- src/core/media/track_list_implementation.cpp 2015-11-18 15:56:20 +0000
> +++ src/core/media/track_list_implementation.cpp 2015-11-18 15:56:21 +0000
> @@ -31,6 +32,8 @@
> namespace dbus = core::dbus;
> namespace media = core::ubuntu::media;
>
> +using namespace std;
I don't think we should introduce this change without changing the entire codebase. I don't want it to be where some files use std explicitly and some don't. So I think we should do this in it's own commit, or not do it at all.
> +
> struct media::TrackListImplementation::Private
> {
> typedef std::map<Track::Id, std::tuple<Track::UriType, Track::MetaData>> MetaDataCache;
> @@ -66,6 +70,19 @@
> std::get<0>(meta_data_cache[id]) = uri;
> }
> }
> +
> + media::TrackList::Container::iterator get_shuffled_insert_it()
> + {
> + media::TrackList::Container::iterator rand_it = shuffled_tracks.begin();
Just to make the variable name more consistent with others in this source file, please rename to random_it;
> + if (rand_it == shuffled_tracks.end())
> + return rand_it;
> +
> + // This is slightly biased, but not much, as RAND_MAX >= 32767, which is
> + // much more than the average number of tracks.
> + // Note that for N tracks we have N + 1 possible insertion positions.
> + advance(rand_it, rand() % (shuffled_tracks.size() + 1));
> + return rand_it;
> + }
> };
>
> media::TrackListImplementation::TrackListImplementation(
>
> === modified file 'src/core/media/track_list_skeleton.cpp'
> --- src/core/media/track_list_skeleton.cpp 2015-11-18 15:56:20 +0000
> +++ src/core/media/track_list_skeleton.cpp 2015-11-18 15:56:21 +0000
> @@ -372,6 +440,10 @@
> {
> }
>
> +/*
> + * NOTE We do not consider the loop status in this function due to the use of it
I don't quite understand what this comment means. Can you reword it or explain to me in IRC what you mean by it?
> + * we do in TrackListSkeleton::next()
> + */
> bool media::TrackListSkeleton::has_next()
> {
> const auto n_tracks = tracks().get().size();
> @@ -385,37 +457,48 @@
> // 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 (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);
> + if (shuffle())
> + {
> + auto it = get_current_shuffled();
> + return ++it != shuffled_tracks().end();
> + }
> + else
> + {
> + const auto next_track = std::next(current_iterator());
> + return !is_last_track(next_track);
> + }
> }
>
> +/*
> + * NOTE We do not consider the loop status in this function due to the use of it
> + * we do in TrackListSkeleton::previous()
> + */
> bool media::TrackListSkeleton::has_previous()
> {
> 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
> - if (d->loop_status == media::Player::LoopStatus::playlist)
> - return true;
> -
> - 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());
> + if (shuffle())
> + return get_current_shuffled() != shuffled_tracks().begin();
> + else
> + return d->current_track != std::begin(tracks().get());
> +}
> +
> +media::TrackList::ConstIterator media::TrackListSkeleton::get_current_shuffled()
> +{
> + auto current_id = *current_iterator();
> + TrackList::ConstIterator it = find(shuffled_tracks().begin(),
> + shuffled_tracks().end(), current_id);
> +
> + return it;
You could just make this be: return std::find(shuffled_tracks().begin(), shuffled_tracks().end(), *current_iterator());
> }
>
> media::Track::Id media::TrackListSkeleton::next()
> @@ -427,51 +510,67 @@
> return media::Track::Id{};
> }
>
> - const auto next_track = std::next(current_iterator());
> - bool do_go_to_next_track = false;
> + bool set_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;
> + set_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;
> +
> + if (shuffle())
> + {
> + auto id = *shuffled_tracks().begin();
const
> + set_current_track(id);
> + }
> + else
> + {
> + d->current_track = tracks().get().begin();
> + }
> + set_track = true;
> }
> else
> {
> - // Next track is not the last track
> - if (not is_last_track(next_track))
> + if (shuffle())
> {
> - std::cout << "Advancing to next track: " << *(next_track) << std::endl;
> - d->current_track = next_track;
> - do_go_to_next_track = true;
> + auto it = get_current_shuffled();
> + if (++it != shuffled_tracks().end()) {
> + cout << "Advancing to next track: " << *it << endl;
> + set_current_track(*it);
> + set_track = true;
I like the original variable name better as it's less confusing to me in instances like this where you call a set_current_track() and then do set_track = true; Once I've not looked at this code for a while this will be a source of confusion. do_go_to_next_track = true is more clear and specific in my opinion.
> + }
> }
> - // 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()();
> + auto it = std::next(current_iterator());
const
> + if (not is_last_track(it))
> + {
> + cout << "Advancing to next track: " << *it << endl;
> + d->current_track = it;
> + set_track = true;
> + }
> }
> +
> }
>
> - if (do_go_to_next_track)
> + if (set_track)
> {
> cout << "next track id is " << *(current_iterator()) << endl;
> 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);
> + on_go_to_track()(id);
> + }
> + else
> + {
> + // At the end of the tracklist and not set to loop
> + cout << "End of tracklist reached" << endl;
> + on_end_of_tracklist()();
> }
>
> return *(current_iterator());
> @@ -495,48 +594,60 @@
> if (d->current_position > max_position)
> {
> std::cout << "Repeating current track..." << std::endl;
> - do_go_to_previous_track = true;
> + set_track = true;
> }
> // Loop on the current track forever
> else if (d->loop_status == media::Player::LoopStatus::track)
> {
> std::cout << "Looping on the current track..." << std::endl;
> - do_go_to_previous_track = true;
> + set_track = true;
> }
> // Loop over the whole playlist and repeat
> - else if (d->loop_status == media::Player::LoopStatus::playlist && is_first_track(current_iterator()))
> + else if (d->loop_status == media::Player::LoopStatus::playlist && not has_previous())
> {
> std::cout << "Looping on the entire TrackList..." << std::endl;
> - d->current_track = std::prev(tracks().get().end());
> - do_go_to_previous_track = true;
> +
> + if (shuffle())
> + {
> + auto id = *std::prev(shuffled_tracks().end());
const
> + set_current_track(id);
> + }
> + else
> + {
> + d->current_track = std::prev(tracks().get().end());
> + }
> +
> + set_track = true;
> }
> else
> {
> - // Current track is not the first track
> - if (not is_first_track(current_iterator()))
> + if (shuffle())
> + {
> + auto it = get_current_shuffled();
> + if (it != shuffled_tracks().begin()) {
> + set_current_track(*(--it));
> + set_track = true;
> + }
> + }
> + else 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()();
> + set_track = true;
Same comment about clarity above.
> }
> }
>
> - if (do_go_to_previous_track)
> + if (set_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);
> + on_go_to_track()(id);
> + }
> + else
> + {
> + // At the beginning of the tracklist and not set to loop
> + cout << "Beginning of tracklist reached" << endl;
> + on_end_of_tracklist()();
> }
>
> return *(current_iterator());
--
https://code.launchpad.net/~phablet-team/media-hub/add_move_track/+merge/276560
Your team Ubuntu Phablet Team is subscribed to branch lp:~phablet-team/media-hub/bg-playlist-fixes.
More information about the Ubuntu-reviews
mailing list