Merge lp:~phablet-team/media-hub/bg-playlists-vivid into lp:media-hub/stable

[Jim Hodapp]

* Connected the TrackList signals on the stub side
* Make has_next() more reliable
* Make next() and previous() work better
* Fix various bugs that arose from testing the TrackList code

* Connected the TrackList signals on the stub side
* Make has_next() more reliable
* Make next() and previous() work better
* Fix various bugs that arose from testing the TrackList code

[lp:~phablet-team/media-hub/bg-playlists-vivid]

165. By Jim Hodapp on 2015-08-07

Bump version to release on vivid+overlay

166. By Jim Hodapp on 2015-08-07

Bump version to release on vivid+overlay take 2

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

Overall looks good, although I have some comments.

review: Needs Fixing

[lp:~phablet-team/media-hub/bg-playlists-vivid]

167. By Jim Hodapp on 2015-08-11

Address code review comments.

168. By Jim Hodapp on 2015-08-11

Make sure media-hub debian/control version is UNRELEASED

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

LGTM

review: Approve

[lp:~phablet-team/media-hub/bg-playlists-vivid]

169. By Jim Hodapp on 2015-08-11

Don't export MPRIS MediaPlayer2 interface on dbus until we have a client that can take advantage of it

=== modified file 'debian/changelog'
--- debian/changelog	2015-07-10 18:12:22 +0000
+++ debian/changelog	2015-08-11 16:10:30 +0000
@@ -1,4 +1,10 @@
-media-hub (3.1.0+15.10.20150710-0ubuntu1) wily; urgency=medium
+media-hub (3.1.0+15.04.20150710-0ubuntu2) UNRELEASED; urgency=medium
+
+  * Change version to vivid+overlay
+
+ -- Jim Hodapp <[e-mail]>  Fri, 07 Aug 2015 10:46:42 -0400
+
+media-hub (3.1.0+15.04.20150710-0ubuntu1) vivid; urgency=medium
 
   [ CI Train Bot ]
   * New rebuild forced.
@@ -9,7 +15,7 @@
 
  -- CI Train Bot <[e-mail]>  Fri, 10 Jul 2015 18:12:21 +0000
 
-media-hub (3.1.0+15.10.20150604-0ubuntu1) wily; urgency=medium
+media-hub (3.1.0+15.04.20150604-0ubuntu1) vivid; urgency=medium
 
   [ CI Train Bot ]
   * New rebuild forced.
@@ -20,7 +26,7 @@
 
  -- CI Train Bot <[e-mail]>  Thu, 04 Jun 2015 14:18:22 +0000
 
-media-hub (3.1.0+15.10.20150601-0ubuntu1) wily; urgency=medium
+media-hub (3.1.0+15.04.20150601-0ubuntu1) vivid; urgency=medium
 
   [ CI Train Bot ]
   * New rebuild forced.
@@ -31,7 +37,7 @@
 
  -- CI Train Bot <[e-mail]>  Mon, 01 Jun 2015 16:31:53 +0000
 
-media-hub (3.1.0+15.10.20150527.1-0ubuntu1) wily; urgency=medium
+media-hub (3.1.0+15.04.20150527.1-0ubuntu1) vivid; urgency=medium
 
   [ CI Train Bot ]
   * New rebuild forced.
@@ -42,7 +48,7 @@
 
  -- CI Train Bot <[e-mail]>  Wed, 27 May 2015 18:38:16 +0000
 
-media-hub (3.1.0+15.10.20150522-0ubuntu1) wily; urgency=medium
+media-hub (3.1.0+15.04.20150522-0ubuntu1) vivid; urgency=medium
 
   [ Jim Hodapp ]
   * Fix issues with not reporting failed decoding error to the client.

=== modified file 'include/core/media/track.h'
--- include/core/media/track.h	2014-02-12 16:02:48 +0000
+++ include/core/media/track.h	2015-08-11 16:10:30 +0000
@@ -107,6 +107,7 @@
     bool operator==(const Track&) const;
 
     virtual const Id& id() const;
+    virtual const UriType& uri() const;
     /*
     class MetaData
     {

=== modified file 'include/core/media/track_list.h'
--- include/core/media/track_list.h	2015-04-29 21:18:20 +0000
+++ include/core/media/track_list.h	2015-08-11 16:10:30 +0000
@@ -25,8 +25,9 @@
 
 #include <functional>
 #include <iosfwd>
+#include <memory>
+#include <string>
 #include <vector>
-#include <memory>
 
 namespace core
 {
@@ -61,6 +62,9 @@
     /** Gets all the metadata available for a given Track. */
     virtual Track::MetaData query_meta_data_for_track(const Track::Id& id) = 0;
 
+    /** Gets the URI for a given Track. */
+    virtual Track::UriType query_uri_for_track(const Track::Id& id) = 0;
+
     /** Adds a URI in the TrackList. */
     virtual void add_track_with_uri_at(const Track::UriType& uri, const Track::Id& position, bool make_current) = 0;
 
@@ -108,6 +112,9 @@
     /** Used to notify the Player of when the client requested that the Player should immediately play a new track. */
     virtual const core::Signal<std::pair<Track::Id, bool>>& on_go_to_track() const = 0;
 
+    /** Used to notify the Player of when the end of the tracklist has been reached. */
+    virtual const core::Signal<void>& on_end_of_tracklist() const = 0;
+
 protected:
     TrackList();
 };

=== modified file 'src/core/media/CMakeLists.txt'
--- src/core/media/CMakeLists.txt	2015-02-24 15:22:19 +0000
+++ src/core/media/CMakeLists.txt	2015-08-11 16:10:30 +0000
@@ -88,7 +88,7 @@
 
     client_death_observer.cpp
     hashed_keyed_player_store.cpp
-    hybris_client_death_observer.cpp    
+    hybris_client_death_observer.cpp
     cover_art_resolver.cpp
     engine.cpp
 

=== modified file 'src/core/media/gstreamer/engine.cpp'
--- src/core/media/gstreamer/engine.cpp	2015-06-01 16:30:59 +0000
+++ src/core/media/gstreamer/engine.cpp	2015-08-11 16:10:30 +0000
@@ -374,7 +374,6 @@
 
 gstreamer::Engine::Engine() : d(new Private{})
 {
-    cout << "Creating a new Engine instance in " << __PRETTY_FUNCTION__ << endl;
     d->state = media::Engine::State::no_media;
 }
 
@@ -413,7 +412,7 @@
 
 bool gstreamer::Engine::play()
 {
-    auto result = d->playbin.set_state_and_wait(GST_STATE_PLAYING);
+    const auto result = d->playbin.set_state_and_wait(GST_STATE_PLAYING);
 
     if (result)
     {
@@ -430,10 +429,12 @@
 {
     // No need to wait, and we can immediately return.
     if (d->state == media::Engine::State::stopped)
+    {
+        std::cerr << "Current player state is already stopped - no need to change state to stopped" << std::endl;
         return true;
-
-    auto result = d->playbin.set_state_and_wait(GST_STATE_NULL);
-
+    }
+
+    const auto result = d->playbin.set_state_and_wait(GST_STATE_NULL);
     if (result)
     {
         d->state = media::Engine::State::stopped;
@@ -446,7 +447,7 @@
 
 bool gstreamer::Engine::pause()
 {
-    auto result = d->playbin.set_state_and_wait(GST_STATE_PAUSED);
+    const auto result = d->playbin.set_state_and_wait(GST_STATE_PAUSED);
 
     if (result)
     {

=== modified file 'src/core/media/mpris/track_list.h'
--- src/core/media/mpris/track_list.h	2015-04-14 17:02:09 +0000
+++ src/core/media/mpris/track_list.h	2015-08-11 16:10:30 +0000
@@ -49,9 +49,11 @@
     }
 
     DBUS_CPP_METHOD_DEF(GetTracksMetadata, TrackList)
+    DBUS_CPP_METHOD_DEF(GetTracksUri, TrackList)
     DBUS_CPP_METHOD_DEF(AddTrack, TrackList)
     DBUS_CPP_METHOD_DEF(RemoveTrack, TrackList)
     DBUS_CPP_METHOD_DEF(GoTo, TrackList)
+    DBUS_CPP_METHOD_DEF(Reset, TrackList)
 
     struct Signals
     {
@@ -80,6 +82,13 @@
 
         DBUS_CPP_SIGNAL_DEF
         (
+            TrackChanged,
+            TrackList,
+            core::ubuntu::media::Track::Id
+        )
+
+        DBUS_CPP_SIGNAL_DEF
+        (
             TrackMetadataChanged,
             TrackList,
             BOOST_IDENTITY_TYPE((std::tuple<std::map<std::string, dbus::types::Variant>, dbus::types::ObjectPath>))
@@ -118,15 +127,16 @@
             : configuration(configuration),
               properties
               {
-                  configuration.object->get_property<Properties::Tracks>(),
-                  configuration.object->get_property<Properties::CanEditTracks>(),
+                  configuration.object->template get_property<Properties::Tracks>(),
+                  configuration.object->template get_property<Properties::CanEditTracks>(),
               },
               signals
               {
-                  configuration.object->get_signal<Signals::TrackListReplaced>(),
-                  configuration.object->get_signal<Signals::TrackAdded>(),
-                  configuration.object->get_signal<Signals::TrackRemoved>(),
-                  configuration.object->get_signal<Signals::TrackMetadataChanged>(),
+                  configuration.object->template get_signal<Signals::TrackListReplaced>(),
+                  configuration.object->template get_signal<Signals::TrackAdded>(),
+                  configuration.object->template get_signal<Signals::TrackRemoved>(),
+                  configuration.object->template get_signal<Signals::TrackChanged>(),
+                  configuration.object->template get_signal<Signals::TrackMetadataChanged>(),
                   configuration.object->template get_signal<core::dbus::interfaces::Properties::Signals::PropertiesChanged>()
               }
         {
@@ -169,6 +179,7 @@
             core::dbus::Signal<Signals::TrackListReplaced, Signals::TrackListReplaced::ArgumentType>::Ptr tracklist_replaced;
             core::dbus::Signal<Signals::TrackAdded, Signals::TrackAdded::ArgumentType>::Ptr track_added;
             core::dbus::Signal<Signals::TrackRemoved, Signals::TrackRemoved::ArgumentType>::Ptr track_removed;
+            core::dbus::Signal<Signals::TrackChanged, Signals::TrackChanged::ArgumentType>::Ptr track_changed;
             core::dbus::Signal<Signals::TrackMetadataChanged, Signals::TrackMetadataChanged::ArgumentType>::Ptr track_metadata_changed;
 
             dbus::Signal <core::dbus::interfaces::Properties::Signals::PropertiesChanged,

=== removed file 'src/core/media/null_track_list.h'
--- src/core/media/null_track_list.h	2014-12-15 21:02:14 +0000
+++ src/core/media/null_track_list.h	1970-01-01 00:00:00 +0000
@@ -1,114 +0,0 @@
-/*
- *
- * This program is free software: you can redistribute it and/or modify it
- * under the terms of the GNU Lesser General Public License version 3,
- * as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU Lesser General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public License
- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
- *
- * Authored by: Thomas Voß <[e-mail]>
- */
-
-#ifndef CORE_MEDIA_NULL_TRACK_LIST_H_
-#define CORE_MEDIA_NULL_TRACK_LIST_H_
-
-#include <core/media/track.h>
-#include <core/media/track_list.h>
-
-namespace core
-{
-namespace ubuntu
-{
-namespace media
-{
-// A helper type to replace the playlist implementation below.
-// Please note that this type is only a temporary manner. Ideally,
-// the actual implementation should be injected as a dependency from the
-// outside.
-struct NullTrackList : public media::TrackList
-{
-    NullTrackList() = default;
-
-    bool has_next()
-    {
-        return false;
-    }
-
-    media::Track::Id next()
-    {
-        return media::Track::Id{};
-    }
-
-    media::Track::UriType query_uri_for_track(const media::Track::Id&)
-    {
-        return media::Track::UriType{};
-    }
-
-    const core::Property<bool>& can_edit_tracks() const override
-    {
-        return props_and_sigs.can_edit_tracks;
-    }
-
-    const core::Property<Container>& tracks() const override
-    {
-        return props_and_sigs.tracks;
-    }
-
-    virtual media::Track::MetaData query_meta_data_for_track(const media::Track::Id&) override
-    {
-        return media::Track::MetaData{};
-    }
-
-    void add_track_with_uri_at(const media::Track::UriType&, const media::Track::Id&, bool) override
-    {
-    }
-
-    void remove_track(const media::Track::Id&) override
-    {
-    }
-
-    void go_to(const media::Track::Id&) override
-    {
-    }
-
-    const core::Signal<void>& on_track_list_replaced() const override
-    {
-        return props_and_sigs.on_track_list_replaced;
-    }
-
-    const core::Signal<media::Track::Id>& on_track_added() const override
-    {
-        return props_and_sigs.on_track_added;
-    }
-
-    const core::Signal<media::Track::Id>& on_track_removed() const override
-    {
-        return props_and_sigs.on_track_removed;
-    }
-
-    const core::Signal<media::Track::Id>& on_track_changed() const override
-    {
-        return props_and_sigs.on_track_changed;
-    }
-
-    struct
-    {
-        core::Property<bool> can_edit_tracks;
-        core::Property<TrackList::Container> tracks;
-        core::Signal<void> on_track_list_replaced;
-        core::Signal<media::Track::Id> on_track_added;
-        core::Signal<media::Track::Id> on_track_removed;
-        core::Signal<media::Track::Id> on_track_changed;
-    } props_and_sigs;
-};
-}
-}
-}
-
-#endif // CORE_MEDIA_NULL_TRACK_LIST_H_

=== modified file 'src/core/media/player_implementation.cpp'
--- src/core/media/player_implementation.cpp	2015-06-01 16:31:50 +0000
+++ src/core/media/player_implementation.cpp	2015-08-11 16:10:30 +0000
@@ -287,6 +287,20 @@
         engine->reset();
     }
 
+    void open_first_track_from_tracklist(const media::Track::Id& id)
+    {
+        const Track::UriType uri = track_list->query_uri_for_track(id);
+        if (!uri.empty())
+        {
+            // Using a TrackList for playback, added tracks via add_track(), but open_uri hasn't been called yet
+            // to load a media resource
+            std::cout << "Calling d->engine->open_resource_for_uri() for first track added only: " << uri << std::endl;
+            std::cout << "\twith a Track::Id: " << id << std::endl;
+            static const bool do_pipeline_reset = false;
+            engine->open_resource_for_uri(uri, do_pipeline_reset);
+        }
+    }
+
     // Our link back to our parent.
     media::PlayerImplementation<Parent>* parent;
     // We just store the parameters passed on construction.
@@ -375,6 +389,7 @@
     // When the client changes the loop status, make sure to update the TrackList
     Parent::loop_status().changed().connect([this](media::Player::LoopStatus loop_status)
     {
+        std::cout << "LoopStatus: " << loop_status << std::endl;
         d->track_list->on_loop_status_changed(loop_status);
     });
 
@@ -408,18 +423,20 @@
         if (d->doing_abandon)
             return;
 
+        // Prevent on_go_to_track from executing as it's not needed in this case. on_go_to_track
+        // (see the lambda below) is only needed when the client explicitly calls next() not during
+        // the about_to_finish condition
+        d->doing_go_to_track.lock();
+
         Parent::about_to_finish()();
 
-        // This lambda needs to be mutually exclusive with the on_go_to_track lambda below
-        d->doing_go_to_track.lock();
-
         const media::Track::Id prev_track_id = d->track_list->current();
         // Make sure that the TrackList keeps advancing. The logic for what gets played next,
         // if anything at all, occurs in TrackListSkeleton::next()
         const Track::UriType uri = d->track_list->query_uri_for_track(d->track_list->next());
         if (prev_track_id != d->track_list->current() && !uri.empty())
         {
-            std::cout << "Setting next track on playbin: " << uri << std::endl;
+            std::cout << "Advancing to next track on playbin: " << uri << std::endl;
             static const bool do_pipeline_reset = false;
             d->engine->open_resource_for_uri(uri, do_pipeline_reset);
         }
@@ -457,12 +474,25 @@
         Parent::error()(e);
     });
 
+    d->track_list->on_end_of_tracklist().connect([this]()
+    {
+        if (d->engine->state() != gstreamer::Engine::State::ready
+                && d->engine->state() != gstreamer::Engine::State::stopped)
+        {
+            std::cout << "End of tracklist reached, stopping playback" << std::endl;
+            d->engine->stop();
+        }
+    });
+
+
     d->track_list->on_go_to_track().connect([this](std::pair<const media::Track::Id, bool> p)
     {
-        // This prevents the TrackList from auto advancing in other areas such as the about_to_finish signal
-        // handler.
         // This lambda needs to be mutually exclusive with the about_to_finish lambda above
-        d->doing_go_to_track.lock();
+        const bool locked = d->doing_go_to_track.try_lock();
+        // If the try_lock fails, it means that about_to_finish lambda above has it locked and it will
+        // call d->engine->open_resource_for_uri()
+        if (!locked)
+            return;
 
         const media::Track::Id id = p.first;
         const bool toggle_player_state = p.second;
@@ -483,7 +513,13 @@
             d->engine->play();
 
         d->doing_go_to_track.unlock();
+    });
 
+    d->track_list->on_track_added().connect([this](const media::Track::Id& id)
+    {
+        std::cout << "** Track was added, handling in PlayerImplementation" << std::endl;
+        if (d->track_list->tracks()->size() == 1)
+            d->open_first_track_from_tracklist(id);
     });
 
     // Everything is setup, we now subscribe to death notifications.
@@ -614,16 +650,6 @@
 template<typename Parent>
 void media::PlayerImplementation<Parent>::play()
 {
-    if (d->track_list != nullptr && d->track_list->tracks()->size() > 0 && d->engine->state() == media::Engine::State::no_media)
-    {
-        // Using a TrackList for playback, added tracks via add_track(), but open_uri hasn't been called yet
-        // to load a media resource
-        std::cout << "No media loaded yet, calling open_uri on first track in track_list" << std::endl;
-        static const bool do_pipeline_reset = true;
-        d->engine->open_resource_for_uri(d->track_list->query_uri_for_track(d->track_list->current()), do_pipeline_reset);
-        std::cout << *d->track_list << endl;
-    }
-
     d->engine->play();
 }
 

=== modified file 'src/core/media/service_skeleton.cpp'
--- src/core/media/service_skeleton.cpp	2015-04-27 21:25:03 +0000
+++ src/core/media/service_skeleton.cpp	2015-08-11 16:10:30 +0000
@@ -139,7 +139,7 @@
                 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())));
             });
-            
+
             auto reply = dbus::Message::make_method_return(msg);
             reply->writer() << std::make_tuple(op, uuid);
 
@@ -161,20 +161,24 @@
             std::string uuid;
             msg->reader() >> uuid;
 
-            auto key = uuid_player_map.at(uuid);
+            // Make sure we don't try to do a lookup if the map is empty
+            if (!uuid_player_map.empty())
+            {
+                const auto key = uuid_player_map.at(uuid);
 
-            if (player_owner_map.count(key) != 0) {
-                auto info = player_owner_map.at(key);
-                // Check if session is attached(1) and that the detachment
-                // request comes from the same peer(2) that created the session.
-                if (std::get<1>(info) && (std::get<2>(info) == msg->sender())) { // Player is attached
-                    std::get<1>(info) = false; // Detached
-                    std::get<2>(info).clear(); // Clear registered sender/peer
-                    auto player = configuration.player_store->player_for_key(key);
-                    player->lifetime().set(media::Player::Lifetime::resumable);
+                if (player_owner_map.count(key) != 0) {
+                    auto info = player_owner_map.at(key);
+                    // Check if session is attached(1) and that the detachment
+                    // request comes from the same peer(2) that created the session.
+                    if (std::get<1>(info) && (std::get<2>(info) == msg->sender())) { // Player is attached
+                        std::get<1>(info) = false; // Detached
+                        std::get<2>(info).clear(); // Clear registered sender/peer
+                        auto player = configuration.player_store->player_for_key(key);
+                        player->lifetime().set(media::Player::Lifetime::resumable);
+                    }
                 }
             }
-            
+
             auto reply = dbus::Message::make_method_return(msg);
             impl->access_bus()->send(reply);
 
@@ -256,7 +260,6 @@
 
     void handle_destroy_session(const core::dbus::Message::Ptr& msg)
     {
-     
         try
         {
             std::string uuid;
@@ -445,9 +448,9 @@
     // We keep a list of keys and their respective owners and states.
     // value: (owner context, attached state, attached dbus name)
     std::map<media::Player::PlayerKey, std::tuple<std::string, bool, std::string>> player_owner_map;
-    
+
     boost::uuids::random_generator gen;
-     
+
     // We expose the entire service as an MPRIS player.
     struct Exported
     {

=== modified file 'src/core/media/track.cpp'
--- src/core/media/track.cpp	2014-02-12 15:53:57 +0000
+++ src/core/media/track.cpp	2015-08-11 16:10:30 +0000
@@ -25,9 +25,10 @@
 struct media::Track::Private
 {
     media::Track::Id id;
+    media::Track::UriType uri;
 };
 
-media::Track::Track(const media::Track::Id& id) : d(new Private{id})
+media::Track::Track(const media::Track::Id& id) : d(new Private{id, std::string{}})
 {
 }
 
@@ -39,3 +40,8 @@
 {
     return d->id;
 }
+
+const media::Track::UriType& media::Track::uri() const
+{
+    return d->uri;
+}

=== modified file 'src/core/media/track_list_implementation.cpp'
--- src/core/media/track_list_implementation.cpp	2015-05-22 21:19:28 +0000
+++ src/core/media/track_list_implementation.cpp	2015-08-11 16:10:30 +0000
@@ -59,7 +59,7 @@
 
 media::Track::UriType media::TrackListImplementation::query_uri_for_track(const media::Track::Id& id)
 {
-    auto it = d->meta_data_cache.find(id);
+    const auto it = d->meta_data_cache.find(id);
 
     if (it == d->meta_data_cache.end())
         return Track::UriType{};
@@ -69,7 +69,7 @@
 
 media::Track::MetaData media::TrackListImplementation::query_meta_data_for_track(const media::Track::Id& id)
 {
-    auto it = d->meta_data_cache.find(id);
+    const auto it = d->meta_data_cache.find(id);
 
     if (it == d->meta_data_cache.end())
         return Track::MetaData{};
@@ -87,10 +87,14 @@
     std::stringstream ss; ss << d->object->path().as_string() << "/" << d->track_counter++;
     Track::Id id{ss.str()};
 
+    std::cout << "Adding Track::Id: " << id << std::endl;
+    std::cout << "\tURI: " << uri << std::endl;
+
     auto result = tracks().update([this, id, position, make_current](TrackList::Container& container)
     {
         auto it = std::find(container.begin(), container.end(), position);
         container.insert(it, id);
+        std::cout << "container.size(): " << container.size() << std::endl;
 
         return true;
     });
@@ -99,6 +103,8 @@
     {
         if (d->meta_data_cache.count(id) == 0)
         {
+            // FIXME: This code seems to conflict badly when called multiple times in a row: causes segfaults
+#if 0
             try {
                 d->meta_data_cache[id] = std::make_tuple(
                             uri,
@@ -106,6 +112,11 @@
             } catch (const std::runtime_error &e) {
                 std::cerr << "Failed to retrieve metadata for track '" << uri << "' (" << e.what() << ")" << std::endl;
             }
+#else
+            d->meta_data_cache[id] = std::make_tuple(
+                    uri,
+                    core::ubuntu::media::Track::MetaData{});
+#endif
         } else
         {
             std::get<0>(d->meta_data_cache[id]) = uri;
@@ -120,8 +131,14 @@
             go_to(id, toggle_player_state);
         }
 
+        // 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 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;
     }
 }
 
@@ -133,6 +150,8 @@
         return true;
     });
 
+    reset_current_iterator_if_needed();
+
     if (result)
     {
         d->meta_data_cache.erase(id);
@@ -155,8 +174,12 @@
 {
     std::cout << __PRETTY_FUNCTION__ << std::endl;
 
+    if (tracks().get().empty())
+        return;
+
     auto result = tracks().update([this](TrackList::Container& container)
     {
+        // Save off the original TrackList ordering
         d->original_tracklist.assign(container.begin(), container.end());
         std::random_shuffle(container.begin(), container.end());
         return true;
@@ -173,8 +196,12 @@
 {
     std::cout << __PRETTY_FUNCTION__ << std::endl;
 
+    if (tracks().get().empty() or d->original_tracklist.empty())
+        return;
+
     auto result = tracks().update([this](TrackList::Container& container)
     {
+        // Restore the original TrackList ordering
         container.assign(d->original_tracklist.begin(), d->original_tracklist.end());
         return true;
     });
@@ -190,6 +217,9 @@
 {
     std::cout << __PRETTY_FUNCTION__ << std::endl;
 
+    // Make sure playback stops
+    on_end_of_tracklist()();
+
     auto result = tracks().update([this](TrackList::Container& container)
     {
         container.clear();
@@ -198,5 +228,7 @@
         return true;
     });
 
+    media::TrackListSkeleton::reset();
+
     (void) result;
 }

=== modified file 'src/core/media/track_list_skeleton.cpp'
--- src/core/media/track_list_skeleton.cpp	2015-04-29 21:18:20 +0000
+++ src/core/media/track_list_skeleton.cpp	2015-08-11 16:10:30 +0000
@@ -60,6 +60,7 @@
           {
               skeleton.signals.track_added,
               skeleton.signals.track_removed,
+              skeleton.signals.track_changed,
               skeleton.signals.tracklist_replaced
           }
     {
@@ -70,15 +71,28 @@
         media::Track::Id track;
         msg->reader() >> track;
 
-        auto meta_data = impl->query_meta_data_for_track(track);
+        const auto meta_data = impl->query_meta_data_for_track(track);
 
-        auto reply = dbus::Message::make_method_return(msg);
+        const auto reply = dbus::Message::make_method_return(msg);
         reply->writer() << *meta_data;
         bus->send(reply);
     }
 
+    void handle_get_tracks_uri(const core::dbus::Message::Ptr& msg)
+    {
+        media::Track::Id track;
+        msg->reader() >> track;
+
+        const auto uri = impl->query_uri_for_track(track);
+
+        const auto reply = dbus::Message::make_method_return(msg);
+        reply->writer() << uri;
+        bus->send(reply);
+    }
+
     void handle_add_track_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)
         {
             Track::UriType uri; media::Track::Id after; bool make_current;
@@ -123,6 +137,14 @@
         bus->send(reply);
     }
 
+    void handle_reset(const core::dbus::Message::Ptr& msg)
+    {
+        impl->reset();
+
+        auto reply = dbus::Message::make_method_return(msg);
+        bus->send(reply);
+    }
+
     media::TrackListSkeleton* impl;
     dbus::Bus::Ptr bus;
     dbus::Object::Ptr object;
@@ -138,10 +160,12 @@
     {
         typedef core::dbus::Signal<mpris::TrackList::Signals::TrackAdded, mpris::TrackList::Signals::TrackAdded::ArgumentType> DBusTrackAddedSignal;
         typedef core::dbus::Signal<mpris::TrackList::Signals::TrackRemoved, mpris::TrackList::Signals::TrackRemoved::ArgumentType> DBusTrackRemovedSignal;
+        typedef core::dbus::Signal<mpris::TrackList::Signals::TrackChanged, mpris::TrackList::Signals::TrackChanged::ArgumentType> DBusTrackChangedSignal;
         typedef core::dbus::Signal<mpris::TrackList::Signals::TrackListReplaced, mpris::TrackList::Signals::TrackListReplaced::ArgumentType> DBusTrackListReplacedSignal;
 
         Signals(const std::shared_ptr<DBusTrackAddedSignal>& remote_track_added,
                 const std::shared_ptr<DBusTrackRemovedSignal>& remote_track_removed,
+                const std::shared_ptr<DBusTrackChangedSignal>& remote_track_changed,
                 const std::shared_ptr<DBusTrackListReplacedSignal>& remote_track_list_replaced)
         {
             // Connect all of the MPRIS interface signals to be emitted over dbus
@@ -155,6 +179,11 @@
                 remote_track_removed->emit(id);
             });
 
+            on_track_changed.connect([remote_track_changed](const media::Track::Id &id)
+            {
+                remote_track_changed->emit(id);
+            });
+
             on_track_list_replaced.connect([remote_track_list_replaced](const media::TrackList::ContainerTrackIdTuple &tltuple)
             {
                 remote_track_list_replaced->emit(tltuple);
@@ -163,9 +192,10 @@
 
         core::Signal<Track::Id> on_track_added;
         core::Signal<Track::Id> on_track_removed;
+        core::Signal<Track::Id> on_track_changed;
         core::Signal<TrackList::ContainerTrackIdTuple> on_track_list_replaced;
-        core::Signal<Track::Id> on_track_changed;
         core::Signal<std::pair<Track::Id, bool>> on_go_to_track;
+        core::Signal<void> on_end_of_tracklist;
     } signals;
 };
 
@@ -179,6 +209,11 @@
                   std::ref(d),
                   std::placeholders::_1));
 
+    d->object->install_method_handler<mpris::TrackList::GetTracksUri>(
+        std::bind(&Private::handle_get_tracks_uri,
+                  std::ref(d),
+                  std::placeholders::_1));
+
     d->object->install_method_handler<mpris::TrackList::AddTrack>(
         std::bind(&Private::handle_add_track_with_uri_at,
                   std::ref(d),
@@ -193,74 +228,184 @@
         std::bind(&Private::handle_go_to,
                   std::ref(d),
                   std::placeholders::_1));
+
+    d->object->install_method_handler<mpris::TrackList::Reset>(
+        std::bind(&Private::handle_reset,
+                  std::ref(d),
+                  std::placeholders::_1));
 }
 
 media::TrackListSkeleton::~TrackListSkeleton()
 {
 }
 
-bool media::TrackListSkeleton::has_next() const
+bool media::TrackListSkeleton::has_next()
 {
-    const auto next_track = std::next(d->current_track);
-    std::cout << "has_next track? " << (next_track != tracks().get().end() ? "yes" : "no") << std::endl;
-    return next_track != tracks().get().end();
+    if (tracks().get().empty())
+        return false;
+
+    const auto next_track = std::next(current_iterator());
+    return !is_last_track(next_track);
 }
 
-bool media::TrackListSkeleton::has_previous() const
+bool media::TrackListSkeleton::has_previous()
 {
+    if (tracks().get().empty())
+        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;
 
-    std::cout << "has_previous track? " << (d->current_track != tracks().get().begin() ? "yes" : "no") << std::endl;
-    return d->current_track != tracks().get().begin();
+    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());
 }
 
 media::Track::Id media::TrackListSkeleton::next()
 {
     std::cout << __PRETTY_FUNCTION__ << std::endl;
     if (tracks().get().empty())
-        return *(d->current_track);
+        return *(d->empty_iterator);
+
+    const auto next_track = std::next(current_iterator());
+    bool do_go_to_next_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;
+    }
+    // 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;
+    }
+    else
+    {
+        // Next track is not the last track
+        if (not is_last_track(next_track))
+        {
+            std::cout << "Advancing to next track: " << *(next_track) << std::endl;
+            d->current_track = next_track;
+            do_go_to_next_track = true;
+        }
+        // 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()();
+        }
+    }
+
+    if (do_go_to_next_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);
+        // Signal the PlayerImplementation to play the next track
+        on_go_to_track()(p);
+    }
+
+    return *(current_iterator());
+}
+
+media::Track::Id media::TrackListSkeleton::previous()
+{
+    std::cout << __PRETTY_FUNCTION__ << std::endl;
+    if (tracks().get().empty())
+        return *(d->empty_iterator);
+
+    bool do_go_to_previous_track = false;
 
     // Loop on the current track forever
     if (d->loop_status == media::Player::LoopStatus::track)
     {
         std::cout << "Looping on the current track..." << std::endl;
-        return *(d->current_track);
+        do_go_to_previous_track = true;
     }
     // Loop over the whole playlist and repeat
-    else if (d->loop_status == media::Player::LoopStatus::playlist && !has_next())
+    else if (d->loop_status == media::Player::LoopStatus::playlist && is_first_track(current_iterator()))
     {
         std::cout << "Looping on the entire TrackList..." << std::endl;
-        d->current_track = tracks().get().begin();
-        return *(d->current_track);
-    }
-    else if (has_next())
-    {
-        // Keep returning the next track until the last track is reached
-        d->current_track = std::next(d->current_track);
-        std::cout << *this << std::endl;
-    }
-
-    return *(d->current_track);
-}
-
-media::Track::Id media::TrackListSkeleton::previous()
-{
-    // TODO: Add logic to calculate the previous track
-    return *(d->current_track);
+        d->current_track = std::prev(tracks().get().end());
+        do_go_to_previous_track = true;
+    }
+    else
+    {
+        // Current track is not the first track
+        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()();
+        }
+    }
+
+    if (do_go_to_previous_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);
+    }
+
+    return *(current_iterator());
 }
 
 const media::Track::Id& media::TrackListSkeleton::current()
 {
+    return *(current_iterator());
+}
+
+const media::TrackList::ConstIterator& media::TrackListSkeleton::current_iterator()
+{
     // Prevent the TrackList from sitting at the end which will cause
     // a segfault when calling current()
     if (tracks().get().size() && (d->current_track == d->empty_iterator))
+    {
+        std::cout << "Wrapping d->current_track back to begin()" << std::endl;
         d->current_track = d->skeleton.properties.tracks->get().begin();
+    }
     else if (tracks().get().empty())
+    {
         std::cerr << "TrackList is empty therefore there is no valid current track" << std::endl;
-
-    return *(d->current_track);
+    }
+
+    return d->current_track;
+}
+
+void media::TrackListSkeleton::reset_current_iterator_if_needed()
+{
+    // If all tracks got removed then we need to keep a sane current
+    // iterator for further use.
+    if (tracks().get().empty())
+        d->current_track = d->empty_iterator;
 }
 
 const core::Property<bool>& media::TrackListSkeleton::can_edit_tracks() const
@@ -293,7 +438,18 @@
     if (shuffle)
         shuffle_tracks();
     else
+    {
+        // Save the current Track::Id of what's currently playing to restore after unshuffle
+        const media::Track::Id current_id = *(current_iterator());
+
         unshuffle_tracks();
+
+        // Since we use assign() in unshuffle_tracks, which invalidates existing iterators, we need
+        // to make sure that current is pointing to the right place
+        auto it = std::find(tracks().get().begin(), tracks().get().end(), current_id);
+        if (it != tracks().get().end())
+            d->current_track = it;
+    }
 }
 
 const core::Property<media::TrackList::Container>& media::TrackListSkeleton::tracks() const
@@ -328,6 +484,11 @@
     return d->signals.on_go_to_track;
 }
 
+const core::Signal<void>& media::TrackListSkeleton::on_end_of_tracklist() const
+{
+    return d->signals.on_end_of_tracklist;
+}
+
 core::Signal<media::TrackList::ContainerTrackIdTuple>& media::TrackListSkeleton::on_track_list_replaced()
 {
     return d->signals.on_track_list_replaced;
@@ -353,6 +514,17 @@
     return d->signals.on_go_to_track;
 }
 
+core::Signal<void>& media::TrackListSkeleton::on_end_of_tracklist()
+{
+    return d->signals.on_end_of_tracklist;
+}
+
+void media::TrackListSkeleton::reset()
+{
+    std::cout << __PRETTY_FUNCTION__ << std::endl;
+    d->current_track = d->empty_iterator;
+}
+
 // operator<< pretty prints the given TrackList to the given output stream.
 inline std::ostream& media::operator<<(std::ostream& out, const media::TrackList& tracklist)
 {

=== modified file 'src/core/media/track_list_skeleton.h'
--- src/core/media/track_list_skeleton.h	2015-04-29 21:18:20 +0000
+++ src/core/media/track_list_skeleton.h	2015-08-11 16:10:30 +0000
@@ -41,8 +41,8 @@
         const core::ubuntu::media::apparmor::ubuntu::RequestAuthenticator::Ptr& request_authenticator);
     ~TrackListSkeleton();
 
-    bool has_next() const;
-    bool has_previous() const;
+    bool has_next();
+    bool has_previous();
     Track::Id next();
     Track::Id previous();
     const Track::Id& current();
@@ -52,10 +52,13 @@
 
     const core::Signal<ContainerTrackIdTuple>& on_track_list_replaced() const;
     const core::Signal<Track::Id>& on_track_added() const;
+    core::Signal<Track::Id>& on_track_added();
     const core::Signal<Track::Id>& on_track_removed() const;
     const core::Signal<Track::Id>& on_track_changed() const;
     const core::Signal<std::pair<Track::Id, bool>>& on_go_to_track() const;
     core::Signal<std::pair<Track::Id, bool>>& on_go_to_track();
+    const core::Signal<void>& on_end_of_tracklist() const;
+    core::Signal<void>& on_end_of_tracklist();
     core::Signal<Track::Id>& on_track_removed();
 
     core::Property<Container>& tracks();
@@ -67,12 +70,18 @@
     void on_shuffle_changed(bool shuffle);
 
 protected:
+    inline bool is_first_track(const ConstIterator &it);
+    inline bool is_last_track(const ConstIterator &it);
+    inline const TrackList::ConstIterator& current_iterator();
+    void reset_current_iterator_if_needed();
+
     core::Property<bool>& can_edit_tracks();
 
     core::Signal<ContainerTrackIdTuple>& on_track_list_replaced();
-    core::Signal<Track::Id>& on_track_added();
     core::Signal<Track::Id>& on_track_changed();
 
+    void reset();
+
 private:
     struct Private;
     std::unique_ptr<Private> d;

=== modified file 'src/core/media/track_list_stub.cpp'
--- src/core/media/track_list_stub.cpp	2015-04-29 21:18:20 +0000
+++ src/core/media/track_list_stub.cpp	2015-08-11 16:10:30 +0000
@@ -48,7 +48,14 @@
           parent(parent),
           object(object),
           can_edit_tracks(object->get_property<mpris::TrackList::Properties::CanEditTracks>()),
-          tracks(object->get_property<mpris::TrackList::Properties::Tracks>())
+          tracks(object->get_property<mpris::TrackList::Properties::Tracks>()),
+          signals
+          {
+              object->get_signal<mpris::TrackList::Signals::TrackAdded>(),
+              object->get_signal<mpris::TrackList::Signals::TrackRemoved>(),
+              object->get_signal<mpris::TrackList::Signals::TrackListReplaced>(),
+              object->get_signal<mpris::TrackList::Signals::TrackChanged>()
+          }
     {
     }
 
@@ -59,11 +66,70 @@
     std::shared_ptr<core::dbus::Property<mpris::TrackList::Properties::CanEditTracks>> can_edit_tracks;
     std::shared_ptr<core::dbus::Property<mpris::TrackList::Properties::Tracks>> tracks;
 
-    core::Signal<media::TrackList::ContainerTrackIdTuple> on_track_list_replaced;
-    core::Signal<Track::Id> on_track_added;
-    core::Signal<Track::Id> on_track_removed;
-    core::Signal<Track::Id> on_track_changed;
-    core::Signal<std::pair<Track::Id, bool>> on_go_to_track;
+
+    struct Signals
+    {
+        typedef core::dbus::Signal<mpris::TrackList::Signals::TrackAdded, mpris::TrackList::Signals::TrackAdded::ArgumentType> DBusTrackAddedSignal;
+        typedef core::dbus::Signal<mpris::TrackList::Signals::TrackRemoved, mpris::TrackList::Signals::TrackRemoved::ArgumentType> DBusTrackRemovedSignal;
+        typedef core::dbus::Signal<mpris::TrackList::Signals::TrackListReplaced, mpris::TrackList::Signals::TrackListReplaced::ArgumentType> DBusTrackListReplacedSignal;
+        typedef core::dbus::Signal<mpris::TrackList::Signals::TrackChanged, mpris::TrackList::Signals::TrackChanged::ArgumentType> DBusTrackChangedSignal;
+
+        Signals(const std::shared_ptr<DBusTrackAddedSignal>& track_added,
+                const std::shared_ptr<DBusTrackRemovedSignal>& track_removed,
+                const std::shared_ptr<DBusTrackListReplacedSignal>& track_list_replaced,
+                const std::shared_ptr<DBusTrackChangedSignal>& track_changed)
+            : on_track_added(),
+              on_track_removed(),
+              on_track_list_replaced(),
+              on_track_changed(),
+              dbus
+              {
+                  track_added,
+                  track_removed,
+                  track_list_replaced,
+                  track_changed,
+              }
+        {
+            dbus.on_track_added->connect([this](const Track::Id& id)
+            {
+                std::cout << "OnTrackAdded signal arrived via the bus." << std::endl;
+                on_track_added(id);
+            });
+
+            dbus.on_track_removed->connect([this](const Track::Id& id)
+            {
+                std::cout << "OnTrackRemoved signal arrived via the bus." << std::endl;
+                on_track_removed(id);
+            });
+
+            dbus.on_track_list_replaced->connect([this](const media::TrackList::ContainerTrackIdTuple& list)
+            {
+                std::cout << "OnTrackListRemoved signal arrived via the bus." << std::endl;
+                on_track_list_replaced(list);
+            });
+
+            dbus.on_track_changed->connect([this](const Track::Id& id)
+            {
+                std::cout << "OnTrackChanged signal arrived via the bus." << std::endl;
+                on_track_changed(id);
+            });
+        }
+
+        core::Signal<Track::Id> on_track_added;
+        core::Signal<Track::Id> on_track_removed;
+        core::Signal<media::TrackList::ContainerTrackIdTuple> on_track_list_replaced;
+        core::Signal<Track::Id> on_track_changed;
+        core::Signal<std::pair<Track::Id, bool>> on_go_to_track;
+        core::Signal<void> on_end_of_tracklist;
+
+        struct DBus
+        {
+            std::shared_ptr<DBusTrackAddedSignal> on_track_added;
+            std::shared_ptr<DBusTrackRemovedSignal> on_track_removed;
+            std::shared_ptr<DBusTrackListReplacedSignal> on_track_list_replaced;
+            std::shared_ptr<DBusTrackChangedSignal> on_track_changed;
+        } dbus;
+    } signals;
 };
 
 media::TrackListStub::TrackListStub(
@@ -104,6 +170,18 @@
     return md;
 }
 
+media::Track::UriType media::TrackListStub::query_uri_for_track(const media::Track::Id& id)
+{
+    auto op = d->object->invoke_method_synchronously<
+                mpris::TrackList::GetTracksUri,
+                std::string>(id);
+
+    if (op.is_error())
+        throw std::runtime_error("Problem querying track for uri: " + op.error());
+
+    return op.value();
+}
+
 void media::TrackListStub::add_track_with_uri_at(
         const media::Track::UriType& uri,
         const media::Track::Id& id,
@@ -161,33 +239,38 @@
 
 void media::TrackListStub::reset()
 {
-    std::cerr << "reset() does nothing from the client side" << std::endl;
+    auto op = d->object->invoke_method_synchronously<mpris::TrackList::Reset, void>();
+
+    if (op.is_error())
+        throw std::runtime_error("Problem resetting tracklist: " + op.error());
 }
 
 const core::Signal<media::TrackList::ContainerTrackIdTuple>& media::TrackListStub::on_track_list_replaced() const
 {
-    std::cout << "Signal on_track_list_replaced arrived via the bus" << std::endl;
-    return d->on_track_list_replaced;
+    return d->signals.on_track_list_replaced;
 }
 
 const core::Signal<media::Track::Id>& media::TrackListStub::on_track_added() const
 {
-    std::cout << "Signal on_track_added arrived via the bus" << std::endl;
-    return d->on_track_added;
+    return d->signals.on_track_added;
 }
 
 const core::Signal<media::Track::Id>& media::TrackListStub::on_track_removed() const
 {
-    std::cout << "Signal on_track_removed arrived via the bus" << std::endl;
-    return d->on_track_removed;
+    return d->signals.on_track_removed;
 }
 
 const core::Signal<media::Track::Id>& media::TrackListStub::on_track_changed() const
 {
-    return d->on_track_changed;
+    return d->signals.on_track_changed;
 }
 
 const core::Signal<std::pair<media::Track::Id, bool>>& media::TrackListStub::on_go_to_track() const
 {
-    return d->on_go_to_track;
+    return d->signals.on_go_to_track;
+}
+
+const core::Signal<void>& media::TrackListStub::on_end_of_tracklist() const
+{
+    return d->signals.on_end_of_tracklist;
 }

=== modified file 'src/core/media/track_list_stub.h'
--- src/core/media/track_list_stub.h	2015-04-29 21:18:20 +0000
+++ src/core/media/track_list_stub.h	2015-08-11 16:10:30 +0000
@@ -45,6 +45,7 @@
     const core::Property<Container>& tracks() const;
 
     Track::MetaData query_meta_data_for_track(const Track::Id& id);
+    Track::UriType query_uri_for_track(const Track::Id& id);
 
     void add_track_with_uri_at(const Track::UriType& uri, const Track::Id& position, bool make_current);
     void remove_track(const Track::Id& id);
@@ -64,6 +65,7 @@
     const core::Signal<Track::Id>& on_track_removed() const;
     const core::Signal<Track::Id>& on_track_changed() const;
     const core::Signal<std::pair<Track::Id, bool>>& on_go_to_track() const;
+    const core::Signal<void>& on_end_of_tracklist() const;
 
 private:
     struct Private;