Preserve group-by across database reloads.

Should resolve issue #95.

src/ashuffle.cc

@@ -137,6 +137,16 @@ struct MPDHost {
     }
 };
 
+std::optional<std::unique_ptr<Loader>> Reloader(mpd::MPD *mpd,
+                                                const Options &options) {
+    // Nothing we can do when `--file` is provided. The user is just stuck
+    // with whatever URIs we parsed the first time.
+    if (options.file_in != nullptr) {
+        return std::nullopt;
+    }
+    return std::make_unique<MPDLoader>(mpd, options.ruleset, options.group_by);
+}
+
 }  // namespace
 
 /* Keep adding songs when the queue runs out */
@@ -163,11 +173,13 @@ void Loop(mpd::MPD *mpd, ShuffleChain *songs, const Options &options,
         /* Only update the database if our original list was built from
          * MPD. */
         if (events.Has(MPD_IDLE_DATABASE) && options.file_in == nullptr) {
-            songs->Clear();
-            MPDLoader loader(mpd, options.ruleset);
-            loader.Load(songs);
-            std::cout << "Picking random songs out of a pool of "
-                      << songs->Len() << "." << std::endl;
+            std::optional<std::unique_ptr<Loader>> reloader =
+                Reloader(mpd, options);
+            if (reloader.has_value()) {
+                songs->Clear();
+                (*reloader)->Load(songs);
+                PrintChainLength(std::cout, *songs);
+            }
         } else if (events.Has(MPD_IDLE_QUEUE) || events.Has(MPD_IDLE_PLAYER)) {
             if (options.tweak.suspend_timeout != absl::ZeroDuration()) {
                 std::unique_ptr<mpd::Status> status = mpd->CurrentStatus();
@@ -189,10 +201,10 @@ std::unique_ptr<mpd::MPD> Connect(const mpd::Dialer &d, const Options &options,
                                   std::function<std::string()> &getpass_f) {
     /* Attempt to get host from command line if available. Otherwise use
      * MPD_HOST variable if available. Otherwise use 'localhost'. */
+    const char *env_host =
+        getenv("MPD_HOST") != nullptr ? getenv("MPD_HOST") : "localhost";
     std::string mpd_host_raw =
-        options.host.has_value()
-            ? *options.host
-            : getenv("MPD_HOST") ? getenv("MPD_HOST") : "localhost";
+        options.host.has_value() ? *options.host : env_host;
     MPDHost mpd_host(mpd_host_raw);
 
     /* Same thing for the port, use the command line defined port, environment
@@ -252,4 +264,22 @@ std::unique_ptr<mpd::MPD> Connect(const mpd::Dialer &d, const Options &options,
     return mpd;
 }
 
+// Print the size of the database to the given stream, accounting for grouping.
+void PrintChainLength(std::ostream &stream, const ShuffleChain &songs) {
+    if (songs.Len() == 0) {
+        stream << "Song pool is empty." << std::endl;
+        return;
+    }
+
+    // If we're grouping.
+    if (songs.Len() != songs.LenURIs()) {
+        stream << absl::StrFormat("Picking from %u groups (%u songs).",
+                                  songs.Len(), songs.LenURIs())
+               << std::endl;
+    } else {
+        stream << "Picking random songs out of a pool of " << songs.Len() << "."
+               << std::endl;
+    }
+}
+
 }  // namespace ashuffle

src/ashuffle.h

@@ -36,6 +36,9 @@ struct TestDelegate {
 void Loop(mpd::MPD* mpd, ShuffleChain* songs, const Options& options,
           TestDelegate d = TestDelegate());
 
+// Print the size of the database to the given stream, accounting for grouping.
+void PrintChainLength(std::ostream& stream, const ShuffleChain& chain);
+
 }  // namespace ashuffle
 
 #endif

src/main.cc

@@ -100,18 +100,11 @@ int main(int argc, const char* argv[]) {
     }
 
     if (songs.Len() == 0) {
-        std::cerr << "Song pool is empty." << std::endl;
+        PrintChainLength(std::cerr, songs);
         exit(EXIT_FAILURE);
     }
 
-    if (!options.group_by.empty()) {
-        std::cout << absl::StrFormat("Picking from %u groups (%u songs).",
-                                     songs.Len(), songs.LenURIs())
-                  << std::endl;
-    } else {
-        std::cout << "Picking random songs out of a pool of " << songs.Len()
-                  << "." << std::endl;
-    }
+    PrintChainLength(std::cout, songs);
 
     if (options.queue_only) {
         size_t number_of_songs = 0;

src/shuffle.cc

@@ -20,8 +20,8 @@ void ShuffleChain::Add(ShuffleItem item) {
     _pool.push_back(_items.size() - 1);
 }
 
-size_t ShuffleChain::Len() { return _items.size(); }
-size_t ShuffleChain::LenURIs() {
+size_t ShuffleChain::Len() const { return _items.size(); }
+size_t ShuffleChain::LenURIs() const {
     size_t sum = 0;
     for (auto& group : _items) {
         sum += group._uris.size();

src/shuffle.h

@@ -46,10 +46,10 @@ class ShuffleChain {
     void Add(ShuffleItem i);
 
     // Return the total number of Items (groups) in this chain.
-    size_t Len();
+    size_t Len() const;
 
     // Return the total number of URIs in this chain, in all items.
-    size_t LenURIs();
+    size_t LenURIs() const;
 
     // Pick a group of songs out of this chain.
     const std::vector<std::string>& Pick();

t/ashuffle_test.cc

@@ -8,6 +8,7 @@
 #include <iostream>
 #include <memory>
 #include <optional>
+#include <sstream>
 #include <string>
 #include <string_view>
 #include <variant>
@@ -293,6 +294,63 @@ TEST_F(LoopTest, Suspend) {
     EXPECT_THAT(mpd.Playing(), Optional(song_a));
 }
 
+TEST(MPDUpdateTest, GroupByPersistsAcrossUpdate) {
+    std::vector<fake::Song> songs = {
+        fake::Song("first", {{MPD_TAG_ALBUM, "album_a"}}),
+        fake::Song("second", {{MPD_TAG_ALBUM, "album_a"}}),
+    };
+
+    fake::MPD mpd;
+    for (auto &song : songs) {
+        mpd.db.push_back(song);
+    }
+
+    // Assuming we are shuffling by group, so shuffle items are grouped by
+    // album.
+    ShuffleChain chain;
+    {
+        std::vector<std::string> group = {songs[0].URI(), songs[1].URI()};
+        chain.Add(group);
+    }
+
+    // Group by album.
+    Options opts;
+    opts.group_by = {MPD_TAG_ALBUM};
+
+    // Do an initialization.
+    Loop(&mpd, &chain, opts, init_only_d);
+
+    // Make sure that ashuffle starts by enquing the first two songs, and then
+    // playing the first song.
+    ASSERT_THAT(mpd.queue, ElementsAre(songs[0], songs[1]));
+    ASSERT_THAT(mpd.Playing(), Optional(songs[0]));
+
+    // Only run the inner loop for the rest of the test.
+    opts.tweak.play_on_startup = false;
+
+    // Trigger a database update.
+    mpd.idle_f = [] { return mpd::IdleEventSet(MPD_IDLE_DATABASE); };
+    Loop(&mpd, &chain, opts, loop_once_d);
+
+    // No queue modifications should take place, just a database reload.
+    ASSERT_THAT(mpd.queue, ElementsAre(songs[0], songs[1]));
+    ASSERT_THAT(mpd.Playing(), Optional(songs[0]));
+
+    // Simulate that we've finished the queue.
+    mpd.state.playing = false;
+    mpd.state.song_position = std::nullopt;
+    mpd.idle_f = [] { return mpd::IdleEventSet(MPD_IDLE_QUEUE); };
+
+    Loop(&mpd, &chain, opts, loop_once_d);
+
+    // Finally assert that we've re-grouped correctly, and re-enqueued the full
+    // item. This may fail if we did not re-group when we received the
+    // MPD_IDLE_DATABASE event.
+    EXPECT_THAT(mpd.queue, ElementsAre(songs[0], songs[1], songs[0], songs[1]));
+    EXPECT_THAT(mpd.Playing(), Optional(songs[0]));
+    EXPECT_THAT(mpd.state.song_position, Optional(2));
+}
+
 struct ConnectTestCase {
     // Want is used to set the actual server host/port.
     mpd::Address want;
@@ -594,3 +652,33 @@ TEST(ConnectDeathTest, BadPermsBadPrompt) {
     EXPECT_EXIT((void)Connect(dialer, Options(), pass_f), ExitedWithCode(1),
                 HasSubstr("required command still not allowed"));
 }
+
+TEST(PrintChainLengthTest, Empty) {
+    ShuffleChain chain;
+    std::stringstream out;
+
+    PrintChainLength(out, chain);
+    EXPECT_THAT(out.str(), HasSubstr("empty"));
+}
+
+TEST(PrintChainLengthTest, SingleURINoGroups) {
+    ShuffleChain chain;
+    chain.Add("first");
+
+    std::stringstream out;
+
+    PrintChainLength(out, chain);
+    EXPECT_THAT(out.str(), HasSubstr("pool of 1."));
+}
+
+TEST(PrintChainLengthTest, Groups) {
+    ShuffleChain chain;
+    chain.Add("first");
+    std::vector<std::string> group = {"second", "third"};
+    chain.Add(group);
+
+    std::stringstream out;
+
+    PrintChainLength(out, chain);
+    EXPECT_THAT(out.str(), HasSubstr("2 groups (3 songs)"));
+}