[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