Improve: Preserve addMapMenu order on new additions

src/T2DMap.cpp

@@ -3711,23 +3711,27 @@ void T2DMap::slot_toggleMapViewOnly()
 void T2DMap::populateUserContextMenus(QMenu& menu)
 {
     QMap<QString, QMenu*> userMenus;
-    QMapIterator<QString, QStringList> menuIterator(mUserMenus);
 
-    while (menuIterator.hasNext()) {
-        menuIterator.next();
-        const QStringList menuInfo = menuIterator.value();
+    // First pass: create all menus in insertion order
+    for (const QString& uniqueName : mUserMenusOrder) {
+        if (!mUserMenus.contains(uniqueName)) {
+            continue; // Skip if menu was removed
+        }
+        const QStringList menuInfo = mUserMenus.value(uniqueName);
         const QString displayName = menuInfo.value(1);
         auto* userMenu = new QMenu(displayName, &menu);
-        userMenus.insert(menuIterator.key(), userMenu);
+        userMenus.insert(uniqueName, userMenu);
     }
 
-    menuIterator.toFront();
-    while (menuIterator.hasNext()) {
-        menuIterator.next();
-        const QStringList menuInfo = menuIterator.value();
+    // Second pass: add menus to their parents in insertion order
+    for (const QString& uniqueName : mUserMenusOrder) {
+        if (!mUserMenus.contains(uniqueName)) {
+            continue; // Skip if menu was removed
+        }
+        const QStringList menuInfo = mUserMenus.value(uniqueName);
         const QString menuParent = menuInfo.value(0);
 
-        if (auto* childMenu = userMenus.value(menuIterator.key(), nullptr)) {
+        if (auto* childMenu = userMenus.value(uniqueName, nullptr)) {
             if (menuParent.isEmpty()) {
                 menu.addMenu(childMenu);
             } else if (auto* parentMenu = userMenus.value(menuParent, nullptr)) {

src/T2DMap.h

@@ -217,6 +217,8 @@ class T2DMap : public QWidget
 
     // unique name, List:parent name ("" if null), display name
     QMap<QString, QStringList> mUserMenus;
+    // Tracks insertion order of menu unique names (preserves creation order)
+    QList<QString> mUserMenusOrder;
 
     bool mRoomBeingMoved = false;
     QPointF mRoomMoveLastMapPoint;

src/TLuaInterpreterMapper.cpp

@@ -562,7 +562,12 @@ int TLuaInterpreter::addMapMenu(lua_State* L)
     if (host.mpMap) {
         if (host.mpMap->mpMapper) {
             if (host.mpMap->mpMapper->mp2dMap) {
-                host.mpMap->mpMapper->mp2dMap->mUserMenus.insert(uniqueName, menuList);
+                auto* mp2dMap = host.mpMap->mpMapper->mp2dMap;
+                mp2dMap->mUserMenus.insert(uniqueName, menuList);
+                // Track insertion order - only add if not already present
+                if (!mp2dMap->mUserMenusOrder.contains(uniqueName)) {
+                    mp2dMap->mUserMenusOrder.append(uniqueName);
+                }
             }
         }
     }
@@ -1844,18 +1849,40 @@ int TLuaInterpreter::getMapMenus(lua_State* L)
         return warnArgumentValue(L, __func__, "you haven't opened a map yet");
     }
 
+    bool returnDetailed = false;
+    if (lua_gettop(L) > 0) {
+        returnDetailed = getVerifiedBool(L, __func__, 1, "return detailed information", true);
+    }
+
+    auto* mp2dMap = host.mpMap->mpMapper->mp2dMap;
     lua_newtable(L);
-    QMapIterator<QString, QStringList> it(host.mpMap->mpMapper->mp2dMap->mUserMenus);
-    while (it.hasNext()) {
-        it.next();
-        QString parent, display;
-        QStringList menuInfo = it.value();
-        parent = menuInfo[0];
-        display = menuInfo[1];
-        qDebug() << it.key() << parent << display;
-        lua_pushstring(L, display.toUtf8().constData());
-        lua_pushstring(L, parent.isEmpty() ? "top-level" : parent.toUtf8().constData());
-        lua_settable(L, -3);
+
+    // Iterate in insertion order
+    int index = 1;
+    for (const QString& uniqueName : mp2dMap->mUserMenusOrder) {
+        if (!mp2dMap->mUserMenus.contains(uniqueName)) {
+            continue; // Skip if menu was removed but order list wasn't cleaned up
+        }
+        const QStringList& menuInfo = mp2dMap->mUserMenus[uniqueName];
+        const QString& parent = menuInfo[0];
+        const QString& display = menuInfo[1];
+
+        if (returnDetailed) {
+            // Build an integer-indexed array so consumers can use ipairs() and get the correct order
+            lua_createtable(L, 0, 3);
+            lua_pushstring(L, uniqueName.toUtf8().constData());
+            lua_setfield(L, -2, "uniquename");
+            lua_pushstring(L, display.toUtf8().constData());
+            lua_setfield(L, -2, "name");
+            lua_pushstring(L, parent.isEmpty() ? "top-level" : parent.toUtf8().constData());
+            lua_setfield(L, -2, "parent");
+            lua_rawseti(L, -2, index++);
+        } else {
+            // Return original format for backward compatibility
+            lua_pushstring(L, display.toUtf8().constData());
+            lua_pushstring(L, parent.isEmpty() ? "top-level" : parent.toUtf8().constData());
+            lua_settable(L, -3);
+        }
     }
 
     return 1;
@@ -2899,36 +2926,41 @@ int TLuaInterpreter::removeMapMenu(lua_State* L)
     if (host.mpMap) {
         if (host.mpMap->mpMapper) {
             if (host.mpMap->mpMapper->mp2dMap) {
-                host.mpMap->mpMapper->mp2dMap->mUserMenus.remove(uniqueName);
+                auto* mp2dMap = host.mpMap->mpMapper->mp2dMap;
+                mp2dMap->mUserMenus.remove(uniqueName);
                 //remove all entries with this as parent
                 QStringList removeList;
                 removeList.append(uniqueName);
                 bool newElement = true;
                 while (newElement) {
                     newElement = false;
-                    QMapIterator<QString, QStringList> it(host.mpMap->mpMapper->mp2dMap->mUserMenus);
+                    QMapIterator<QString, QStringList> it(mp2dMap->mUserMenus);
                     while (it.hasNext()) {
                         it.next();
                         QStringList menuInfo = it.value();
                         const QString parent = menuInfo[0];
                         if (removeList.contains(parent)) {
-                            host.mpMap->mpMapper->mp2dMap->mUserMenus.remove(it.key());
+                            mp2dMap->mUserMenus.remove(it.key());
                             if (it.key() != "" && !removeList.contains(it.key())) {
-                                host.mpMap->mpMapper->mp2dMap->mUserMenus.remove(it.key());
+                                mp2dMap->mUserMenus.remove(it.key());
                                 removeList.append(it.key());
                                 newElement = true;
                             }
                         }
                     }
                 }
-                QMapIterator<QString, QStringList> it2(host.mpMap->mpMapper->mp2dMap->mUserActions);
+                QMapIterator<QString, QStringList> it2(mp2dMap->mUserActions);
                 while (it2.hasNext()) {
                     it2.next();
                     const QString actParent = it2.value()[1];
                     if (removeList.contains(actParent)) {
-                        host.mpMap->mpMapper->mp2dMap->mUserActions.remove(it2.key());
+                        mp2dMap->mUserActions.remove(it2.key());
                     }
                 }
+                // Remove all deleted menus from the order list
+                for (const QString& menuToRemove : removeList) {
+                    mp2dMap->mUserMenusOrder.removeAll(menuToRemove);
+                }
             }
         }
     }

src/mudlet-lua/tests/MapMenu_spec.lua

@@ -0,0 +1,136 @@
+describe("Tests map menu functions", function()
+
+  setup(function()
+    -- createMapper initializes mp2dMap which is required by addMapMenu/getMapMenus
+    createMapper(0, 0, 400, 300)
+  end)
+
+  describe("Tests getMapMenus detailed mode", function()
+
+    before_each(function()
+      addMapMenu("_test_detail_A", "", "Detail Menu A")
+      addMapMenu("_test_detail_B", "_test_detail_A", "Detail Menu B")
+    end)
+
+    after_each(function()
+      removeMapMenu("_test_detail_A")
+      removeMapMenu("_test_detail_B")
+    end)
+
+    it("should return an integer-indexed array usable with ipairs", function()
+      local menus = getMapMenus(true)
+      assert.is_table(menus)
+      assert.is_not_nil(menus[1])
+      assert.is_nil(menus[0])
+    end)
+
+    it("should include uniquename, name, and parent fields for each entry", function()
+      local menus = getMapMenus(true)
+      local found
+      for _, m in ipairs(menus) do
+        if m.uniquename == "_test_detail_A" then
+          found = m
+          break
+        end
+      end
+      assert.is_not_nil(found, "test menu _test_detail_A not found in results")
+      assert.are.equal("_test_detail_A", found.uniquename)
+      assert.are.equal("Detail Menu A", found.name)
+      assert.are.equal("top-level", found.parent)
+    end)
+
+    it("should report the parent uniquename for child menus", function()
+      local menus = getMapMenus(true)
+      local found
+      for _, m in ipairs(menus) do
+        if m.uniquename == "_test_detail_B" then
+          found = m
+          break
+        end
+      end
+      assert.is_not_nil(found, "child menu _test_detail_B not found in results")
+      assert.are.equal("_test_detail_A", found.parent)
+    end)
+
+    it("should report 'top-level' for menus with no parent", function()
+      local menus = getMapMenus(true)
+      for _, m in ipairs(menus) do
+        if m.uniquename == "_test_detail_A" then
+          assert.are.equal("top-level", m.parent)
+          return
+        end
+      end
+      assert.is_true(false, "_test_detail_A not found in results")
+    end)
+
+  end)
+
+  describe("Tests getMapMenus order preservation", function()
+
+    after_each(function()
+      removeMapMenu("_test_order_X")
+      removeMapMenu("_test_order_Y")
+      removeMapMenu("_test_order_Z")
+    end)
+
+    it("should return menus in insertion order", function()
+      addMapMenu("_test_order_X", "", "Order X")
+      addMapMenu("_test_order_Y", "", "Order Y")
+      addMapMenu("_test_order_Z", "", "Order Z")
+
+      local menus = getMapMenus(true)
+      local posX, posY, posZ
+      for i, m in ipairs(menus) do
+        if m.uniquename == "_test_order_X" then posX = i end
+        if m.uniquename == "_test_order_Y" then posY = i end
+        if m.uniquename == "_test_order_Z" then posZ = i end
+      end
+      assert.is_not_nil(posX, "_test_order_X not found")
+      assert.is_not_nil(posY, "_test_order_Y not found")
+      assert.is_not_nil(posZ, "_test_order_Z not found")
+      assert.is_true(posX < posY, "X should precede Y in insertion order")
+      assert.is_true(posY < posZ, "Y should precede Z in insertion order")
+    end)
+
+    it("should not produce a duplicate entry when a menu is re-added", function()
+      addMapMenu("_test_order_X", "", "Order X")
+      addMapMenu("_test_order_Y", "", "Order Y")
+      addMapMenu("_test_order_X", "", "Order X Updated")
+
+      local menus = getMapMenus(true)
+      local count = 0
+      for _, m in ipairs(menus) do
+        if m.uniquename == "_test_order_X" then
+          count = count + 1
+        end
+      end
+      assert.are.equal(1, count)
+    end)
+
+  end)
+
+  describe("Tests getMapMenus non-detailed mode", function()
+
+    before_each(function()
+      addMapMenu("_test_basic_A", "", "Basic Menu A")
+    end)
+
+    after_each(function()
+      removeMapMenu("_test_basic_A")
+    end)
+
+    it("should return a table keyed by display name with parent as value", function()
+      local menus = getMapMenus()
+      assert.is_table(menus)
+      assert.are.equal("top-level", menus["Basic Menu A"])
+    end)
+
+    it("should behave identically when called with explicit false", function()
+      local menus = getMapMenus(false)
+      assert.is_table(menus)
+      assert.are.equal("top-level", menus["Basic Menu A"])
+    end)
+
+  end)
+
+end)