Merge lp:~phablet-team/media-hub/mpris into lp:media-hub/stable

[Jim Hodapp]

Re-enable MPRIS player controls and improves background playlists.

Re-enable MPRIS player controls and improves background playlists.

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

review: Approve

[lp:~phablet-team/media-hub/mpris]

173. By Jim Hodapp on 2015-09-08

Make sure the correct player is set as the current player controled by MPRIS. Also improved the can_go_next() and can_go_previous() logic to always return true if the loop_status is currently set to loop over the entire playlist.

174. By Jim Hodapp on 2015-09-08

Bump version for change in interface

175. By Jim Hodapp on 2015-09-08

Merged with upstream

176. By Jim Hodapp on 2015-09-08

Bumped the package version.

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

Some minor fixes required. A MP for trunk would be good too.

review: Needs Fixing

[lp:~phablet-team/media-hub/mpris]

177. By Jim Hodapp on 2015-09-09

Added link to a coding standard for this code base and fixed line length in debian/changelog entry.

[Jim Hodapp (jhodapp)]

Comments addressed.

[lp:~phablet-team/media-hub/mpris]

178. By Jim Hodapp on 2015-09-09

Address code review comments

179. By Jim Hodapp on 2015-09-09

Addressed review comment - check for if (sp) is now moved into is_multimedia_role()

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

review: Approve

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2015-04-10 16:21:16 +0000
+++ CMakeLists.txt	2015-09-09 16:32:06 +0000
@@ -3,7 +3,7 @@
 project(ubuntu-media-hub)
 
 set(UBUNTU_MEDIA_HUB_VERSION_MAJOR 3)
-set(UBUNTU_MEDIA_HUB_VERSION_MINOR 1)
+set(UBUNTU_MEDIA_HUB_VERSION_MINOR 2)
 set(UBUNTU_MEDIA_HUB_VERSION_PATCH 0)
 
 set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall -Wextra -fPIC -pthread")

=== modified file 'README'
--- README	2014-08-08 14:36:29 +0000
+++ README	2015-09-09 16:32:06 +0000
@@ -1,3 +1,12 @@
+Coding Convention:
+------------------
+
+NOTE: the media-hub code did not start out with the following coding convention, but it is being introduced to try and
+converge on a standard from this point forward:
+
+https://google-styleguide.googlecode.com/svn/trunk/cppguide.html
+
+
 To Build:
 ---------
 

=== modified file 'debian/changelog'
--- debian/changelog	2015-09-04 18:51:23 +0000
+++ debian/changelog	2015-09-09 16:32:06 +0000
@@ -1,3 +1,13 @@
+media-hub (3.2.0+15.04.20150904-0ubuntu1) UNRELEASED; urgency=medium
+
+  * Make sure the correct player is set as the current player controled by
+    MPRIS.
+  * Improved the can_go_next() and can_go_previous() logic to always return
+    true if the loop_status is currently set to loop over the entire
+    playlist.
+
+ -- phablet <phablet@ubuntu-phablet>  Tue, 08 Sep 2015 17:18:15 -0400
+
 media-hub (3.1.0+15.04.20150904-0ubuntu1) vivid; urgency=medium
 
   [ Alfonso Sanchez-Beato (email Canonical) ]

=== modified file 'include/core/media/service.h'
--- include/core/media/service.h	2015-04-14 02:43:56 +0000
+++ include/core/media/service.h	2015-09-09 16:32:06 +0000
@@ -60,6 +60,9 @@
     /** @brief Resumes a fixed-name session directly by player key. */
     virtual std::shared_ptr<Player> resume_session(Player::PlayerKey) = 0;
 
+    /** @brief Sets the current player that the MPRIS interface will control */
+    virtual void set_current_player(Player::PlayerKey) = 0;
+
     /** @brief Pauses sessions other than the supplied one. */
     virtual void pause_other_sessions(Player::PlayerKey) = 0;
 

=== modified file 'src/core/media/mpris/service.h'
--- src/core/media/mpris/service.h	2015-04-10 16:13:55 +0000
+++ src/core/media/mpris/service.h	2015-09-09 16:32:06 +0000
@@ -115,6 +115,7 @@
     DBUS_CPP_METHOD_WITH_TIMEOUT_DEF(DestroySession, Service, 1000)
     DBUS_CPP_METHOD_WITH_TIMEOUT_DEF(CreateFixedSession, Service, 1000)
     DBUS_CPP_METHOD_WITH_TIMEOUT_DEF(ResumeSession, Service, 1000)
+    DBUS_CPP_METHOD_WITH_TIMEOUT_DEF(SetCurrentPlayer, Service, 1000)
     DBUS_CPP_METHOD_WITH_TIMEOUT_DEF(PauseOtherSessions, Service, 1000)
 };
 }

=== modified file 'src/core/media/player_implementation.cpp'
--- src/core/media/player_implementation.cpp	2015-07-27 22:15:33 +0000
+++ src/core/media/player_implementation.cpp	2015-09-09 16:32:06 +0000
@@ -376,13 +376,15 @@
 
     std::function<bool()> can_go_next_getter = [this]()
     {
-        return d->track_list->has_next();
+        // If LoopStatus == playlist, then there is always a next track
+        return d->track_list->has_next() or Parent::loop_status() == Player::LoopStatus::playlist;
     };
     Parent::can_go_next().install(can_go_next_getter);
 
     std::function<bool()> can_go_previous_getter = [this]()
     {
-        return d->track_list->has_previous();
+        // If LoopStatus == playlist, then there is always a next previous
+        return d->track_list->has_previous() or Parent::loop_status() == Player::LoopStatus::playlist;
     };
     Parent::can_go_previous().install(can_go_previous_getter);
 

=== modified file 'src/core/media/service_implementation.cpp'
--- src/core/media/service_implementation.cpp	2015-04-17 16:44:30 +0000
+++ src/core/media/service_implementation.cpp	2015-09-09 16:32:06 +0000
@@ -246,6 +246,11 @@
   return std::shared_ptr<media::Player>();
 }
 
+void media::ServiceImplementation::set_current_player(Player::PlayerKey key)
+{
+  // no impl
+}
+
 void media::ServiceImplementation::pause_other_sessions(media::Player::PlayerKey key)
 {
     if (not d->configuration.player_store->has_player_for_key(key))

=== modified file 'src/core/media/service_implementation.h'
--- src/core/media/service_implementation.h	2015-04-16 15:19:35 +0000
+++ src/core/media/service_implementation.h	2015-09-09 16:32:06 +0000
@@ -49,6 +49,7 @@
     void destroy_session(const std::string&, const Player::Configuration&);
     std::shared_ptr<Player> create_fixed_session(const std::string& name, const Player::Configuration&);
     std::shared_ptr<Player> resume_session(Player::PlayerKey key);
+    void set_current_player(Player::PlayerKey key);
     void pause_other_sessions(Player::PlayerKey key);
 
 private:

=== modified file 'src/core/media/service_skeleton.cpp'
--- src/core/media/service_skeleton.cpp	2015-08-11 16:10:14 +0000
+++ src/core/media/service_skeleton.cpp	2015-09-09 16:32:06 +0000
@@ -91,6 +91,11 @@
                         &Private::handle_resume_session,
                         this,
                         std::placeholders::_1));
+        object->install_method_handler<mpris::Service::SetCurrentPlayer>(
+                    std::bind(
+                        &Private::handle_set_current_player,
+                        this,
+                        std::placeholders::_1));
         object->install_method_handler<mpris::Service::PauseOtherSessions>(
                     std::bind(
                         &Private::handle_pause_other_sessions,
@@ -102,7 +107,7 @@
     {
         static unsigned int session_counter = 0;
 
-        unsigned int current_session = session_counter++;
+        const unsigned int current_session = session_counter++;
         boost::uuids::uuid uuid = gen();
 
         std::stringstream ss;
@@ -134,7 +139,8 @@
             configuration.player_store->add_player_for_key(key, impl->create_session(config));
             uuid_player_map.insert(std::make_pair(uuid, key));
 
-            request_context_resolver->resolve_context_for_dbus_name_async(msg->sender(), [this, key, msg](const media::apparmor::ubuntu::Context& context)
+            request_context_resolver->resolve_context_for_dbus_name_async(msg->sender(),
+                    [this, key, msg](const media::apparmor::ubuntu::Context& context)
             {
                 fprintf(stderr, "%s():%d -- app_name='%s', attached\n", __func__, __LINE__, context.str().c_str());
                 player_owner_map.insert(std::make_pair(key, std::make_tuple(context.str(), true, msg->sender())));
@@ -213,7 +219,8 @@
                 ss << "/core/ubuntu/media/Service/sessions/" << key;
                 dbus::types::ObjectPath op{ss.str()};
 
-                request_context_resolver->resolve_context_for_dbus_name_async(msg->sender(), [this, msg, key, op](const media::apparmor::ubuntu::Context& context)
+                request_context_resolver->resolve_context_for_dbus_name_async(msg->sender(),
+                        [this, msg, key, op](const media::apparmor::ubuntu::Context& context)
                 {
                     auto info = player_owner_map.at(key);
                     fprintf(stderr, "%s():%d -- reattach app_name='%s', info='%s', '%s'\n", __func__, __LINE__, context.str().c_str(), std::get<0>(info).c_str(), std::get<2>(info).c_str());
@@ -224,6 +231,12 @@
                         // Signal player reconnection
                         auto player = configuration.player_store->player_for_key(key);
                         player->reconnect();
+                        // We only care to allow the MPRIS controls to apply to multimedia player (i.e. audio, video)
+                        if (player->audio_stream_role() == media::Player::AudioStreamRole::multimedia)
+                        {
+                            std::cout << "Setting current_player" << std::endl;
+                            exported.set_current_player(player);
+                        }
 
                         auto reply = dbus::Message::make_method_return(msg);
                         reply->writer() << op;
@@ -280,7 +293,8 @@
                 // the session is no longer usable.
                 uuid_player_map.erase(uuid);
 
-                request_context_resolver->resolve_context_for_dbus_name_async(msg->sender(), [this, msg, key](const media::apparmor::ubuntu::Context& context)
+                request_context_resolver->resolve_context_for_dbus_name_async(msg->sender(),
+                        [this, msg, key](const media::apparmor::ubuntu::Context& context)
                 {
                     auto info = player_owner_map.at(key);
                     fprintf(stderr, "%s():%d -- Destroying app_name='%s', info='%s', '%s'\n", __func__, __LINE__, context.str().c_str(), std::get<0>(info).c_str(), std::get<2>(info).c_str());
@@ -424,9 +438,18 @@
         }
     }
 
+    void handle_set_current_player(const core::dbus::Message::Ptr& msg)
+    {
+        Player::PlayerKey key;
+        msg->reader() >> key;
+        impl->set_current_player(key);
+
+        auto reply = dbus::Message::make_method_return(msg);
+        impl->access_bus()->send(reply);
+    }
+
     void handle_pause_other_sessions(const core::dbus::Message::Ptr& msg)
     {
-        std::cout << __PRETTY_FUNCTION__ << std::endl;
         Player::PlayerKey key;
         msg->reader() >> key;
         impl->pause_other_sessions(key);
@@ -477,20 +500,10 @@
             return defaults;
         }
 
-        static std::string service_name()
-        {
-            static const bool export_to_indicator_sound_via_mpris
-            {
-                core::posix::this_process::env::get("UBUNTU_MEDIA_HUB_EXPORT_TO_INDICATOR_VIA_MPRIS", "0") == "1"
-            };
-
-            return export_to_indicator_sound_via_mpris ? "org.mpris.MediaPlayer2.MediaHub" :
-                                                         "hidden.org.mpris.MediaPlayer2.MediaHub";
-        }
-
         explicit Exported(const dbus::Bus::Ptr& bus, const media::CoverArtResolver& cover_art_resolver)
             : bus{bus},
-              service{dbus::Service::add_service(bus, service_name())},
+              /* Export MediaHub service interface on dbus */
+              service{dbus::Service::add_service(bus, "org.mpris.MediaPlayer2.MediaHub")},
               object{service->add_object_for_path(dbus::types::ObjectPath{"/org/mpris/MediaPlayer2"})},
               media_player{mpris::MediaPlayer2::Skeleton::Configuration{bus, object, media_player_defaults()}},
               player{mpris::Player::Skeleton::Configuration{bus, object, player_defaults()}},
@@ -500,14 +513,15 @@
             object->install_method_handler<core::dbus::interfaces::Properties::GetAll>([this](const core::dbus::Message::Ptr& msg)
             {
                 // Extract the interface
-                std::string itf; msg->reader() >> itf;
+                std::string interface;
+                msg->reader() >> interface;
                 core::dbus::Message::Ptr reply = core::dbus::Message::make_method_return(msg);
 
-                if (itf == mpris::Player::name())
+                if (interface == mpris::Player::name())
                     reply->writer() << player.get_all_properties();
-                else if (itf == mpris::MediaPlayer2::name())
+                else if (interface == mpris::MediaPlayer2::name())
                     reply->writer() << media_player.get_all_properties();
-                else if (itf == mpris::Playlists::name())
+                else if (interface == mpris::Playlists::name())
                     reply->writer() << playlists.get_all_properties();
 
                 Exported::bus->send(reply);
@@ -516,9 +530,9 @@
             // Setup method handlers for mpris::Player methods.
             auto next = [this](const core::dbus::Message::Ptr& msg)
             {
-                auto sp = current_player.lock();
+                const auto sp = current_player.lock();
 
-                if (sp)
+                if (is_multimedia_role())
                     sp->next();
 
                 Exported::bus->send(core::dbus::Message::make_method_return(msg));
@@ -527,9 +541,9 @@
 
             auto previous = [this](const core::dbus::Message::Ptr& msg)
             {
-                auto sp = current_player.lock();
+                const auto sp = current_player.lock();
 
-                if (sp)
+                if (is_multimedia_role())
                     sp->previous();
 
                 Exported::bus->send(core::dbus::Message::make_method_return(msg));
@@ -538,9 +552,9 @@
 
             auto pause = [this](const core::dbus::Message::Ptr& msg)
             {
-                auto sp = current_player.lock();
+                const auto sp = current_player.lock();
 
-                if (sp)
+                if (is_multimedia_role() and sp->can_pause())
                     sp->pause();
 
                 Exported::bus->send(core::dbus::Message::make_method_return(msg));
@@ -549,9 +563,9 @@
 
             auto stop = [this](const core::dbus::Message::Ptr& msg)
             {
-                auto sp = current_player.lock();
+                const auto sp = current_player.lock();
 
-                if (sp)
+                if (is_multimedia_role())
                     sp->stop();
 
                 Exported::bus->send(core::dbus::Message::make_method_return(msg));
@@ -560,9 +574,9 @@
 
             auto play = [this](const core::dbus::Message::Ptr& msg)
             {
-                auto sp = current_player.lock();
+                const auto sp = current_player.lock();
 
-                if (sp)
+                if (is_multimedia_role() and sp->can_play())
                     sp->play();
 
                 Exported::bus->send(core::dbus::Message::make_method_return(msg));
@@ -571,13 +585,15 @@
 
             auto play_pause = [this](const core::dbus::Message::Ptr& msg)
             {
-                auto sp = current_player.lock();
+                const auto sp = current_player.lock();
 
-                if (sp)
+                if (is_multimedia_role())
                 {
-                    if (sp->playback_status() == media::Player::PlaybackStatus::playing)
+                    if (sp->playback_status() == media::Player::PlaybackStatus::playing
+                            and sp->can_pause())
                         sp->pause();
-                    else if (sp->playback_status() != media::Player::PlaybackStatus::null)
+                    else if (sp->playback_status() != media::Player::PlaybackStatus::null
+                                and sp->can_play())
                         sp->play();
                 }
 
@@ -586,94 +602,29 @@
             object->install_method_handler<mpris::Player::PlayPause>(play_pause);
         }
 
+        inline bool is_multimedia_role()
+        {
+            const auto sp = current_player.lock();
+
+            return (sp ? sp->audio_stream_role() == media::Player::AudioStreamRole::multimedia : false);
+        }
+
         void set_current_player(const std::shared_ptr<media::Player>& cp)
         {
-            unset_current_player();
-
+            std::cout << "*** " << __PRETTY_FUNCTION__ << std::endl;
             // We will not keep the object alive.
             current_player = cp;
 
             // And announce that we can be controlled again.
-            player.properties.can_control->set(false);
-
-            // We wire up player state changes
-            connections.seeked_to = cp->seeked_to().connect([this](std::uint64_t position)
-            {
-                player.signals.seeked_to->emit(position);
-            });
-
-            connections.duration_changed = cp->duration().changed().connect([this](std::uint64_t duration)
-            {
-                player.properties.duration->set(duration);
-            });
-
-            connections.position_changed = cp->position().changed().connect([this](std::uint64_t position)
-            {
-                player.properties.position->set(position);
-            });
-
-            connections.playback_status_changed = cp->playback_status().changed().connect([this](core::ubuntu::media::Player::PlaybackStatus status)
-            {
-                player.properties.playback_status->set(mpris::Player::PlaybackStatus::from(status));
-            });
-
-            connections.loop_status_changed = cp->loop_status().changed().connect([this](core::ubuntu::media::Player::LoopStatus status)
-            {
-                player.properties.loop_status->set(mpris::Player::LoopStatus::from(status));
-            });
-
-            connections.meta_data_changed = cp->meta_data_for_current_track().changed().connect([this](const core::ubuntu::media::Track::MetaData& md)
-            {
-                mpris::Player::Dictionary dict;
-
-                bool has_title = md.count(xesam::Title::name) > 0;
-                bool has_album_name = md.count(xesam::Album::name) > 0;
-                bool has_artist_name = md.count(xesam::Artist::name) > 0;
-
-                if (has_title)
-                    dict[xesam::Title::name] = dbus::types::Variant::encode(md.get(xesam::Title::name));
-                if (has_album_name)
-                    dict[xesam::Album::name] = dbus::types::Variant::encode(md.get(xesam::Album::name));
-                if (has_artist_name)
-                    dict[xesam::Artist::name] = dbus::types::Variant::encode(md.get(xesam::Artist::name));
-
-                dict[mpris::metadata::ArtUrl::name] = dbus::types::Variant::encode(
-                            cover_art_resolver(
-                                has_title ? md.get(xesam::Title::name) : "",
-                                has_album_name ? md.get(xesam::Album::name) : "",
-                                has_artist_name ? md.get(xesam::Artist::name) : ""));
-
-                mpris::Player::Dictionary wrap;
-                wrap[mpris::Player::Properties::Metadata::name()] = dbus::types::Variant::encode(dict);
-
-                player.signals.properties_changed->emit(
-                            std::make_tuple(
-                                dbus::traits::Service<mpris::Player::Properties::Metadata::Interface>::interface_name(),
-                                wrap,
-                                std::vector<std::string>()));
-            });
+            player.properties.can_control->set(true);
         }
 
-        void unset_current_player()
+        void reset_current_player()
         {
+            std::cout << __PRETTY_FUNCTION__ << std::endl;
+            // And announce that we can no longer be controlled.
+            player.properties.can_control->set(false);
             current_player.reset();
-
-            // We disconnect all previous event connections.
-            connections.seeked_to.disconnect();
-            connections.duration_changed.disconnect();
-            connections.position_changed.disconnect();
-            connections.playback_status_changed.disconnect();
-            connections.loop_status_changed.disconnect();
-            connections.meta_data_changed.disconnect();
-
-            // And announce that we cannot be controlled anymore.
-            player.properties.can_control->set(false);
-        }
-
-        void unset_if_current(const std::shared_ptr<media::Player>& cp)
-        {
-            if (cp == current_player.lock())
-                unset_current_player();
         }
 
         dbus::Bus::Ptr bus;
@@ -759,6 +710,14 @@
     return d->configuration.impl->resume_session(key);
 }
 
+void media::ServiceSkeleton::set_current_player(media::Player::PlayerKey key)
+{
+    const auto player = d->configuration.player_store->player_for_key(key);
+    // We only care to allow the MPRIS controls to apply to multimedia player (i.e. audio, video)
+    if (player->audio_stream_role() == media::Player::AudioStreamRole::multimedia)
+        d->exported.set_current_player(player);
+}
+
 void media::ServiceSkeleton::pause_other_sessions(media::Player::PlayerKey key)
 {
     d->configuration.impl->pause_other_sessions(key);

=== modified file 'src/core/media/service_skeleton.h'
--- src/core/media/service_skeleton.h	2015-04-16 15:19:35 +0000
+++ src/core/media/service_skeleton.h	2015-09-09 16:32:06 +0000
@@ -59,6 +59,7 @@
     void destroy_session(const std::string&, const media::Player::Configuration&);
     std::shared_ptr<Player> create_fixed_session(const std::string& name, const Player::Configuration&);
     std::shared_ptr<Player> resume_session(Player::PlayerKey);
+    void set_current_player(Player::PlayerKey key);
     void pause_other_sessions(Player::PlayerKey key);
 
     void run();

=== modified file 'src/core/media/service_stub.cpp'
--- src/core/media/service_stub.cpp	2015-04-17 16:44:30 +0000
+++ src/core/media/service_stub.cpp	2015-09-09 16:32:06 +0000
@@ -139,9 +139,17 @@
     });
 }
 
+void media::ServiceStub::set_current_player(Player::PlayerKey key)
+{
+    auto op = d->object->invoke_method_synchronously<mpris::Service::SetCurrentPlayer,
+         void>(key);
+
+    if (op.is_error())
+        throw std::runtime_error("Problem setting current player: " + op.error());
+}
+
 void media::ServiceStub::pause_other_sessions(media::Player::PlayerKey key)
 {
-    std::cout << __PRETTY_FUNCTION__ << std::endl;
     auto op = d->object->invoke_method_synchronously<mpris::Service::PauseOtherSessions,
          void>(key);
 

=== modified file 'src/core/media/service_stub.h'
--- src/core/media/service_stub.h	2015-04-10 16:13:55 +0000
+++ src/core/media/service_stub.h	2015-09-09 16:32:06 +0000
@@ -45,6 +45,7 @@
     void destroy_session(const std::string& uuid, const Player::Configuration&);
     std::shared_ptr<Player> create_fixed_session(const std::string& name, const Player::Configuration&);
     std::shared_ptr<Player> resume_session(Player::PlayerKey key);
+    void set_current_player(Player::PlayerKey key);
     void pause_other_sessions(Player::PlayerKey key);
 
   private:

=== modified file 'src/core/media/track_list_implementation.cpp'
--- src/core/media/track_list_implementation.cpp	2015-08-11 15:25:50 +0000
+++ src/core/media/track_list_implementation.cpp	2015-09-09 16:32:06 +0000
@@ -215,8 +215,6 @@
 
 void media::TrackListImplementation::reset()
 {
-    std::cout << __PRETTY_FUNCTION__ << std::endl;
-
     // Make sure playback stops
     on_end_of_tracklist()();
 

=== modified file 'src/core/media/track_list_skeleton.cpp'
--- src/core/media/track_list_skeleton.cpp	2015-08-11 15:25:50 +0000
+++ src/core/media/track_list_skeleton.cpp	2015-09-09 16:32:06 +0000
@@ -521,7 +521,6 @@
 
 void media::TrackListSkeleton::reset()
 {
-    std::cout << __PRETTY_FUNCTION__ << std::endl;
     d->current_track = d->empty_iterator;
 }