Don't clear history each time individual items are added or removed.

This will prevent the playback history from vanishing unexpectedly in case a preset can't be loaded.

Also fixed a few typos in the Doxygen comments and clarified how the history is changed when changing the playlist.

Author: kblaschke

src/playlist/Playlist.cpp

@@ -65,13 +65,22 @@ bool Playlist::AddItem(const std::string& filename, uint32_t index, bool allowDu
         }
     }
 
-    m_presetHistory.clear();
     if (index >= m_items.size())
     {
         m_items.emplace_back(filename);
     }
     else
     {
+        // Increment indices of items equal or grater than the newly added index.
+        for (auto& historyItem : m_presetHistory)
+        {
+            if (historyItem >= index)
+            {
+                historyItem++;
+            }
+        }
+
+
         m_items.emplace(m_items.cbegin() + index, filename);
     }
 
@@ -83,7 +92,6 @@ auto Playlist::AddPath(const std::string& path, uint32_t index, bool recursive,
 {
     uint32_t presetsAdded{0};
 
-    m_presetHistory.clear();
     if (recursive)
     {
         try
@@ -129,6 +137,15 @@ auto Playlist::AddPath(const std::string& path, uint32_t index, bool recursive,
         }
     }
 
+    // Increment indices of items in playback history equal or grater than the newly added index.
+    for (auto& historyItem : m_presetHistory)
+    {
+        if (historyItem >= index)
+        {
+            historyItem += presetsAdded;
+        }
+    }
+
     return presetsAdded;
 }
 
@@ -140,7 +157,23 @@ auto Playlist::RemoveItem(uint32_t index) -> bool
         return false;
     }
 
-    m_presetHistory.clear();
+    // Remove item from history and decrement indices of items after the removed index.
+    for (auto it = begin(m_presetHistory); it != end(m_presetHistory);)
+    {
+        if (*it == index)
+        {
+            it = m_presetHistory.erase(it);
+            continue;
+        }
+
+        if (*it > index)
+        {
+            (*it)--;
+        }
+
+        ++it;
+    }
+
     m_items.erase(m_items.cbegin() + index);
 
     return true;
@@ -333,6 +366,15 @@ void Playlist::RemoveLastHistoryEntry()
 }
 
 
+auto Playlist::HistoryItems() const -> std::vector<uint32_t>
+{
+    std::vector<uint32_t> items;
+    std::copy(begin(m_presetHistory), end(m_presetHistory), std::back_inserter(items));
+
+    return items;
+}
+
+
 auto Playlist::Filter() -> class Filter&
 {
     return m_filter;

src/playlist/Playlist.hpp

@@ -37,7 +37,7 @@ class Playlist
 {
 public:
     /**
-     * Short-hand constant which can be used in AddItem() to add new presets at the end of the playlist.
+     * Shorthand constant which can be used in AddItem() to add new presets at the end of the playlist.
      */
     static constexpr auto InsertAtEnd = std::numeric_limits<uint32_t>::max();
 
@@ -89,21 +89,23 @@ class Playlist
     virtual bool Empty() const;
 
     /**
-     * @brief Clears the current playlist.
+     * @brief Clears the current playlist and playback history.
      */
     virtual void Clear();
 
     /**
      * @brief Returns the playlist items.
      * @return A vector with items in the current playlist.
      */
-    virtual const std::vector<Item>& Items() const;
+    virtual auto Items() const -> const std::vector<Item>&;
 
     /**
      * @brief Adds a preset file to the playlist.
      *
      * Use Playlist::InsertAtEnd as index to always insert an item at the end of the playlist.
      *
+     * The playback history will be kept, and indices are updated accordingly.
+     *
      * @param filename The file path and name to add.
      * @param index The index to insert the preset at. If larger than the playlist size, it's added
      *              to the end of the playlist.
@@ -117,7 +119,9 @@ class Playlist
      * @brief Adds presets (recursively) from the given path.
      *
      * The function will scan the given path (and possible subdirs) for files with a .milk extension
-     * and and the to the playlist, starting at the given index.
+     * and add them to the playlist, starting at the given index.
+     *
+     * The playback history will be kept, and indices are updated accordingly.
      *
      * The order of the added files is unspecified. Use the Sort() method to sort the playlist or
      * the newly added range.
@@ -135,7 +139,8 @@ class Playlist
                          bool allowDuplicates) -> uint32_t;
 
     /**
-     * @brief Removed a playlist item at the given playlist index.
+     * @brief Removes a playlist item at the given playlist index.
+     * The playback history will be kept, and indices are updated accordingly.
      * @param index The index to remove.
      * @return True if an item was removed, false if the index was out of bounds and no item was
      *         removed..
@@ -159,6 +164,8 @@ class Playlist
      *
      * Sorting is case-sensitive.
      *
+     * The playback history is cleared when calling this function.
+     *
      * @param startIndex The index to start sorting at. If the index is larger than the last
      *                   item index, the playlist will remain unchanged.
      * @param count The number of items to sort. If the value exceeds the playlist length, only
@@ -226,6 +233,12 @@ class Playlist
      */
     virtual void RemoveLastHistoryEntry();
 
+    /**
+     * @brief Returns a vector with the playlist indices of the current playback history.
+     * @return A vector of indices with the last played presets.
+     */
+    virtual auto HistoryItems() const -> std::vector<uint32_t>;
+
     /**
      * @brief Returns the current playlist filter list.
      * @return The filter list for the current playlist.

tests/playlist/PlaylistTest.cpp

@@ -153,6 +153,38 @@ TEST(projectMPlaylistPlaylist, AddItemNoDuplicates)
 }
 
 
+TEST(projectMPlaylistPlaylist, AddItemWithHistory)
+{
+    Playlist playlist;
+    EXPECT_TRUE(playlist.AddItem("/some/file", 0, false));
+    EXPECT_TRUE(playlist.AddItem("/some/other/file", Playlist::InsertAtEnd, false));
+    EXPECT_TRUE(playlist.AddItem("/and/another/file", Playlist::InsertAtEnd, false));
+
+    ASSERT_EQ(playlist.Size(), 3);
+
+    playlist.SetPresetIndex(1);
+    playlist.SetPresetIndex(2);
+    playlist.SetPresetIndex(0);
+
+    auto historyItemsBefore = playlist.HistoryItems();
+    ASSERT_EQ(historyItemsBefore.size(), 3);
+    EXPECT_EQ(historyItemsBefore.at(0), 0); // Playback started with index 0
+    EXPECT_EQ(historyItemsBefore.at(1), 1);
+    EXPECT_EQ(historyItemsBefore.at(2), 2);
+
+    EXPECT_TRUE(playlist.AddItem("/yet/another/file", 1, false));
+
+    ASSERT_EQ(playlist.Size(), 4);
+
+    auto historyItemsAfter = playlist.HistoryItems();
+    ASSERT_EQ(historyItemsAfter.size(), 3);
+    EXPECT_EQ(historyItemsAfter.at(0), 0);
+    EXPECT_EQ(historyItemsAfter.at(1), 2);
+    EXPECT_EQ(historyItemsAfter.at(2), 3);
+}
+
+
+
 TEST(projectMPlaylistPlaylist, AddPathRecursively)
 {
     Playlist playlist;
@@ -218,7 +250,7 @@ TEST(projectMPlaylistPlaylist, AddPathNonRecursively)
 }
 
 
-TEST(projectMPlaylistPlaylist, AddPathnonRecursivelyNoDuplicates)
+TEST(projectMPlaylistPlaylist, AddPathNonRecursivelyNoDuplicates)
 {
     Playlist playlist;
 
@@ -289,6 +321,36 @@ TEST(projectMPlaylistPlaylist, RemoveItemFromMiddle)
 }
 
 
+TEST(projectMPlaylistPlaylist, RemoveItemWithHistory)
+{
+    Playlist playlist;
+    EXPECT_TRUE(playlist.AddItem("/some/file", Playlist::InsertAtEnd, false));
+    EXPECT_TRUE(playlist.AddItem("/some/other/file", Playlist::InsertAtEnd, false));
+    EXPECT_TRUE(playlist.AddItem("/yet/another/file", Playlist::InsertAtEnd, false));
+
+    ASSERT_EQ(playlist.Size(), 3);
+
+    playlist.SetPresetIndex(1);
+    playlist.SetPresetIndex(2);
+    playlist.SetPresetIndex(0);
+
+    auto historyItemsBefore = playlist.HistoryItems();
+    ASSERT_EQ(historyItemsBefore.size(), 3);
+    EXPECT_EQ(historyItemsBefore.at(0), 0); // Playback started with index 0
+    EXPECT_EQ(historyItemsBefore.at(1), 1);
+    EXPECT_EQ(historyItemsBefore.at(2), 2);
+
+    EXPECT_TRUE(playlist.RemoveItem(1));
+
+    ASSERT_EQ(playlist.Size(), 2);
+
+    auto historyItemsAfter = playlist.HistoryItems();
+    ASSERT_EQ(historyItemsAfter.size(), 2);
+    EXPECT_EQ(historyItemsAfter.at(0), 0);
+    EXPECT_EQ(historyItemsAfter.at(1), 1);
+}
+
+
 TEST(projectMPlaylistPlaylist, RemoveItemIndexOutOfBounds)
 {
     Playlist playlist;