[Merge] lp:~phablet-team/media-hub/fix-1498962 into lp:media-hub

[Alfonso Sanchez-Beato]

Review: Needs Fixing

Some minor nits around.

Diff comments:

> 
> === modified file 'src/core/media/codec.h'
> --- src/core/media/codec.h	2015-04-14 19:11:15 +0000
> +++ src/core/media/codec.h	2016-06-08 21:03:16 +0000
> @@ -60,16 +68,72 @@
>  template<>
>  struct Codec<core::ubuntu::media::Track::MetaData>
>  {
> -    static void encode_argument(core::dbus::Message::Writer& out, const core::ubuntu::media::Track::MetaData& in)
> +    static void encode_argument(core::dbus::Message::Writer& writer, const core::ubuntu::media::Track::MetaData& md)
>      {
> -        (void) out;
> -        (void) in;
> +        typedef std::pair<std::string, dbus::types::Variant> Pair;
> +        auto dict = writer.open_array(dbus::types::Signature{dbus::helper::TypeMapper<Pair>::signature()});
> +
> +        for (const auto& pair : *md)
> +        {
> +            auto de = dict.open_dict_entry();
> +            {
> +                if (pair.first == "mpris:length" and not pair.second.empty())

Can "mpris:length" definition be shared with track.h?

> +                {
> +                    Codec<Pair>::encode_argument(de, std::make_pair(pair.first, dbus::types::Variant::encode(boost::lexical_cast<std::int64_t>(pair.second))));

Too long line here, and some others in this file too.

> +                }
> +                else if (pair.first == xesam::Album::name and not pair.second.empty())
> +                {
> +                    Codec<Pair>::encode_argument(de, std::make_pair(pair.first, dbus::types::Variant::encode(pair.second)));
> +                }
> +                else if (pair.first == xesam::AlbumArtist::name and not pair.second.empty())
> +                {
> +#if 0
> +                    // TODO: This code doesn't work but will be needed for full MPRIS compliance. Technically
> +                    // there can be more than one album artist stuffed into an array.

Any hint on why this does not work? Add to the comment if that is the case.

> +                    auto array = de.open_array(dbus::types::Signature{dbus::helper::TypeMapper<Pair>::signature()});
> +                    {
> +                        // TODO: this should really iterate over all artists, but seems we only extract one artist from playbin
> +                        Codec<Pair>::encode_argument(array, std::make_pair(pair.first, dbus::types::Variant::encode(pair.second)));
> +                    }
> +                    de.close_array(std::move(array));
> +#else
> +                    Codec<Pair>::encode_argument(de, std::make_pair(pair.first, dbus::types::Variant::encode(pair.second)));
> +#endif
> +                }
> +                else
> +                {
> +                    Codec<Pair>::encode_argument(de, std::make_pair(pair.first, dbus::types::Variant::encode(pair.second)));
> +                }
> +            }
> +            dict.close_dict_entry(std::move(de));
> +        }
> +        writer.close_array(std::move(dict));
>      }
>  
> -    static void decode_argument(core::dbus::Message::Reader& out, core::ubuntu::media::Track::MetaData& in)
> +    static void decode_argument(core::dbus::Message::Reader& reader, core::ubuntu::media::Track::MetaData& md)
>      {
> -        (void) out;
> -        (void) in;
> +        auto array = reader.pop_array();
> +
> +        while (array.type() != dbus::ArgumentType::invalid)
> +        {
> +            auto entry = array.pop_dict_entry();
> +            {
> +                std::string key {entry.pop_string()};
> +                auto variant = entry.pop_variant();
> +                {
> +                    if (key == xesam::Album::name)
> +                    {
> +                        const std::string album = variant.pop_string();
> +                        MH_DEBUG("Getting key \"%s\" and value \"%s\"", key, album);
> +                        md.set_album(album);
> +                    }
> +                    else
> +                    {
> +                        MH_WARNING("Unknown metadata key \"%s\" while decoding dbus message", key);
> +                    }
> +                }
> +            }
> +        }
>      }
>  };
>  
> 
> === modified file 'src/core/media/player_implementation.cpp'
> --- src/core/media/player_implementation.cpp	2016-05-03 19:08:00 +0000
> +++ src/core/media/player_implementation.cpp	2016-06-08 21:03:16 +0000
> @@ -325,6 +336,27 @@
>          parent->can_go_next().set(has_next);
>      }
>  
> +    // Makes sure all relevant metadata fields are set to current data and
> +    // will trigger the track_meta_data().changed() signal to go out over dbus
> +    void update_mpris_metadata(const media::Track::MetaData& md)
> +    {
> +        media::Track::MetaData metadata{md};
> +        if (not metadata.is_set(media::Track::MetaData::TrackIdKey))
> +        {
> +            const std::size_t last_slash = track_list->current().find_last_of("/");
> +            const std::string track_id = track_list->current().substr(last_slash+1);

Spaces around + symbol

> +            if (not track_id.empty())
> +                metadata.set_track_id("/org/mpris/MediaPlayer2/Track/" + track_id);
> +            else
> +                MH_WARNING("Failed to set MPRIS track id since the id value is NULL");
> +        }
> +        // TODO: This needs to be extracted from GStreamer and dynamically set
> +        if (not metadata.is_set(media::Track::MetaData::TrackArtlUrlKey))
> +            metadata.set_art_url("file:///usr/lib/arm-linux-gnueabihf/unity-scopes/mediascanner-music/album_missing.svg");
> +
> +        parent->meta_data_for_current_track().set(metadata);
> +    }
> +
>      bool pause_other_players(media::Player::PlayerKey key)
>      {
>          if (not config.parent.player_service)


-- 
https://code.launchpad.net/~phablet-team/media-hub/fix-1498962/+merge/296604
Your team Ubuntu Phablet Team is subscribed to branch lp:media-hub.