[Merge] lp:~phablet-team/media-hub/add-tracks-batch into lp:media-hub/stable

[Jim Hodapp]

Addressed code review comments.

Diff comments:

> 
> === modified file 'src/core/media/track_list_implementation.cpp'
> --- src/core/media/track_list_implementation.cpp	2015-09-28 13:20:01 +0000
> +++ src/core/media/track_list_implementation.cpp	2015-10-22 19:06:46 +0000
> @@ -138,8 +146,49 @@
>          std::cout << "Signaling that we just added track id: " << id << std::endl;
>          // Signal to the client that a track was added to the TrackList
>          on_track_added()(id);
> -        std::cout << "Signaled that we just added track id: " << id << std::endl;
> -    }
> +    }
> +}
> +
> +void media::TrackListImplementation::add_tracks_with_uri_at(const ContainerURI& uris, const Track::Id& position)
> +{
> +    std::cout << __PRETTY_FUNCTION__ << std::endl;
> +
> +    ContainerURI tmp;
> +    for (const auto uri : uris)
> +    {
> +        // TODO: Refactor this code to use a smaller common function shared with add_track_with_uri_at()
> +        std::stringstream ss; ss << d->object->path().as_string() << "/" << d->track_counter++;

Agreed. This was original code from tvoss that I just moved around. But I like them on separate lines too, so I'll make the suggested change.

> +        Track::Id id{ss.str()};
> +        std::cout << "Adding Track::Id: " << id << std::endl;
> +        std::cout << "\tURI: " << uri << std::endl;
> +
> +        tmp.push_back(id);
> +
> +        Track::Id insert_position = position;
> +
> +        auto it = std::find(tracks().get().begin(), tracks().get().end(), insert_position);
> +        auto result = tracks().update([this, id, position, it, &insert_position](TrackList::Container& container)
> +        {
> +            container.insert(it, id);
> +            // Make sure the next insert position is after the current insert position
> +            // Update the Track::Id after which to insert the next one from uris
> +            insert_position = id;
> +
> +            return true;
> +        });
> +
> +        if (result)
> +        {
> +            d->updateCachedTrackMetadata(id, uri);
> +
> +            // Signal to the client that the current track has changed for the first track added to the TrackList
> +            if (tracks().get().size() == 1)
> +                on_track_changed()(id);
> +        }
> +    }
> +
> +    std::cout << "Signaling that we just added " << tmp.size() << " tracks to the TrackList" << std::endl;
> +    on_tracks_added()(tmp);
>  }
>  
>  void media::TrackListImplementation::remove_track(const media::Track::Id& id)
> 
> === modified file 'src/core/media/track_list_skeleton.cpp'
> --- src/core/media/track_list_skeleton.cpp	2015-09-28 15:31:46 +0000
> +++ src/core/media/track_list_skeleton.cpp	2015-10-22 19:06:46 +0000
> @@ -113,6 +114,36 @@
>          });
>      }
>  
> +    void handle_add_tracks_with_uri_at(const core::dbus::Message::Ptr& msg)
> +    {
> +        std::cout << "*** " << __PRETTY_FUNCTION__ << std::endl;
> +        request_context_resolver->resolve_context_for_dbus_name_async(msg->sender(), [this, msg](const media::apparmor::ubuntu::Context& context)

I think we should standardize on 100 characters in a line. We all have modern high resolution monitors and just like two spaces after a period is an antiquated thing, not having long lines should also be antiquated in my opinion. I'll add a comment to the media-hub README that 100 columns is the convention we'll agree to.

> +        {
> +            ContainerURI uris; media::Track::Id after;

Done

> +            msg->reader() >> uris >> after;
> +
> +            media::apparmor::ubuntu::RequestAuthenticator::Result result;
> +            for (const auto uri : uris)
> +            {
> +                // Make sure the client has adequate apparmor permissions to open the URI
> +                result = request_authenticator->authenticate_open_uri_request(context, uri);

This check categorically belongs in the skeleton since it is part of verifying the dbus API call (In dbus terminology, the skeleton is the part that handles the dbus API).

> +                if (not std::get<0>(result))
> +                {
> +                    std::cerr << "Warning: Not adding track " << uri <<
> +                        " to TrackList because of inadequate client apparmor permissions." << std::endl;
> +                    break;
> +                }
> +            }
> +
> +            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_tracks_with_uri_at(uris, after);
> +
> +            bus->send(reply);

Done

> +        });
> +    }
> +
>      void handle_remove_track(const core::dbus::Message::Ptr& msg)
>      {
>          media::Track::Id track;
> 
> === modified file 'src/core/media/track_list_stub.cpp'
> --- src/core/media/track_list_stub.cpp	2015-07-27 22:15:33 +0000
> +++ src/core/media/track_list_stub.cpp	2015-10-22 19:06:46 +0000
> @@ -184,18 +197,28 @@
>  
>  void media::TrackListStub::add_track_with_uri_at(
>          const media::Track::UriType& uri,
> -        const media::Track::Id& id,
> +        const media::Track::Id& position,
>          bool make_current)
>  {
>      auto op = d->object->invoke_method_synchronously<mpris::TrackList::AddTrack, void>(
>                  uri,
> -                id,
> +                position,
>                  make_current);
>  
>      if (op.is_error())
>          throw std::runtime_error("Problem adding track: " + op.error());
>  }
>  
> +void media::TrackListStub::add_tracks_with_uri_at(const ContainerURI& uris, const Track::Id& position)
> +{
> +    auto op = d->object->invoke_method_synchronously<mpris::TrackList::AddTracks, void>(

There were issues with the bus locking up when using the async version of the same call. This is the only safe way to use dbus-cpp. Hopefully we'll be able to use the async version when we convert away from using dbus-cpp.

> +                uris,
> +                position);
> +
> +    if (op.is_error())
> +        throw std::runtime_error("Problem adding tracks: " + op.error());
> +}
> +
>  void media::TrackListStub::remove_track(const media::Track::Id& track)
>  {
>      auto op = d->object->invoke_method_synchronously<mpris::TrackList::RemoveTrack, void>(


-- 
https://code.launchpad.net/~phablet-team/media-hub/add-tracks-batch/+merge/275426
Your team Ubuntu Phablet Team is subscribed to branch lp:media-hub/stable.