From d68431499bc0f498282cfb8a2731c7852ac1663d Mon Sep 17 00:00:00 2001
From: Salanto <62221668+Salanto@users.noreply.github.com>
Date: Sat, 9 Jul 2022 06:15:17 -0700
Subject: [PATCH] Fix musiclist duplication (#314)

* Fix musiclist duplication

* Move regression test into own test
---
 core/src/config_manager.cpp                   | 10 +++++--
 core/src/server.cpp                           |  1 -
 .../tst_unittest_config_manager.cpp           | 27 +++++++++++++++++++
 3 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/core/src/config_manager.cpp b/core/src/config_manager.cpp
index 07595fa..1b08f4e 100644
--- a/core/src/config_manager.cpp
+++ b/core/src/config_manager.cpp
@@ -145,11 +145,17 @@ MusicList ConfigManager::musiclist()
         return QMap<QString, QPair<QString, int>>{}; // Server can still run without music.
     }
 
+    // Make sure the list is empty before appending new data.
+    if (!m_ordered_list->empty()) {
+        m_ordered_list->clear();
+    }
+
     // Akashi expects the musiclist to be contained in a JSON array, even if its only a single category.
     QJsonArray l_Json_root_array = l_music_list_json.array();
     QJsonObject l_child_obj;
     QJsonArray l_child_array;
-    for (int i = 0; i <= l_Json_root_array.size() - 1; i++) { // Iterate trough entire JSON file to assemble musiclist
+
+    for (int i = 0; i < l_Json_root_array.size(); i++) { // Iterate trough entire JSON file to assemble musiclist
         l_child_obj = l_Json_root_array.at(i).toObject();
 
         // Technically not a requirement, but neat for organisation.
@@ -163,7 +169,7 @@ MusicList ConfigManager::musiclist()
         }
 
         l_child_array = l_child_obj["songs"].toArray();
-        for (int i = 0; i <= l_child_array.size() - 1; i++) { // Inner for loop because a category can contain multiple songs.
+        for (int i = 0; i < l_child_array.size(); i++) { // Inner for loop because a category can contain multiple songs.
             QJsonObject l_song_obj = l_child_array.at(i).toObject();
             QString l_song_name = l_song_obj["name"].toString();
             QString l_real_name = l_song_obj["realname"].toString();
diff --git a/core/src/server.cpp b/core/src/server.cpp
index e8b56d4..4ef9946 100644
--- a/core/src/server.cpp
+++ b/core/src/server.cpp
@@ -114,7 +114,6 @@ void Server::start()
     m_backgrounds = ConfigManager::backgrounds();
 
     // Build our music manager.
-    ConfigManager::musiclist();
     music_manager = new MusicManager(ConfigManager::ordered_songs(), ConfigManager::cdnList(), ConfigManager::musiclist(), this);
     connect(music_manager, &MusicManager::sendFMPacket, this, &Server::unicast);
     connect(music_manager, &MusicManager::sendAreaFMPacket, this, QOverload<AOPacket *, int>::of(&Server::broadcast));
diff --git a/tests/unittest_config_manager/tst_unittest_config_manager.cpp b/tests/unittest_config_manager/tst_unittest_config_manager.cpp
index b1a3554..a4730fd 100644
--- a/tests/unittest_config_manager/tst_unittest_config_manager.cpp
+++ b/tests/unittest_config_manager/tst_unittest_config_manager.cpp
@@ -33,6 +33,15 @@ class tst_ConfigManager : public QObject
 
     void ordered_songs();
 
+    /**
+     * @brief Tests the loading routine for the musiclist.
+     *
+     * @details Handles regression testing for a bug fixed in PR#314 in which a
+     * musiclist would be loaded twice and the old root instance was not cleared,
+     * causing each reload to append the musiclist to the old list.
+     */
+    void regression_pr_314();
+
     void CommandInfo();
 
     void iprangeBans();
@@ -210,6 +219,24 @@ void tst_ConfigManager::ordered_songs()
     QCOMPARE(l_ordered_musiclist.at(3), "Announce The Truth (JFA).opus");
 }
 
+void tst_ConfigManager::regression_pr_314()
+{
+    // Populate songlist.
+    Q_UNUSED(ConfigManager::musiclist());
+
+    // Tests for regression where a reload of the songlist would cause the list to duplicate.
+    QStringList l_list = ConfigManager::ordered_songs();
+
+    // We have a populated ordered list and it has a valid size.
+    QCOMPARE(l_list.isEmpty(), false);
+    QCOMPARE(l_list.size(), 4);
+
+    // We are reloading the songlist. The size should be the same and the list has not changed.
+    Q_UNUSED(ConfigManager::musiclist());
+    QCOMPARE(l_list.size(), ConfigManager::ordered_songs().size());
+    QCOMPARE(l_list, ConfigManager::ordered_songs());
+}
+
 void tst_ConfigManager::CommandInfo()
 {
     // Prepare command help cache.