Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

• linux: https://make.mudlet.org/snapshots/9723a6/Mudlet-4.10.1-testing-pr4722-f997b5ad-linux-x64.AppImage.tar
• osx: https://make.mudlet.org/snapshots/e7b3c4/Mudlet-4.10.1-testing-pr4722-f997b5ad.dmg
• windows: https://make.mudlet.org/snapshots/b09bfa/Mudlet-4.10.1-testing-pr4722-f997b5ad-windows.zip

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

This kind of bug would have been prevented if we could do for (auto [key, value]: with QMap - but we can't, see https://www.kdab.com/qt-range-based-for-loops-and-structured-bindings. It brings a case against using Qt containers and for using std:: ones which are, now, more modern.

... and for using std:: ones which are, now, more modern.

Which have their own peculiarities to trap the unwary.

What caused this bug to slip through was multiple compilers not spotting that I was assigning an int to a QString - my best guess is that there are so many QString constructors that possibly the one that took a single int and converted it to a QChar was being used. I expect a std::string constructor would probably do the same. So whilst the error was an oversight of mine I do not believe that using a Qt class was the cause of this. Fortunately someone spotted it and reported it and we fixed it before it got into a release version.

It was bad semantics, I believe, which the std:: containers help solve. The compiler should be the last line of defense, not the first one.

If you look at this diff, you have no idea what the key and value change is:

Because the names mean absolutely nothing. You have to dig further to find their meaning.

Meanwhile, if we gave them meaningful names, it will be much easier to work with it so consider pseudocode:

        for (auto [exit, roomid] : getSpecialExits())) {
            if (pSourceR->hasSpecialExitLock(exit)) {
                continue; // Is a locked exit so forget it...
            }

Instead of:

        QMapIterator<QString, int> itSpecialExit(pSourceR->getSpecialExits());
        while (itSpecialExit.hasNext()) {
            itSpecialExit.next();
            if (pSourceR->hasSpecialExitLock(itSpecialExit.key())) {
                continue; // Is a locked exit so forget it...
            }

I believe the former is far more understandable and less bugs will happen.

The bug was no less likely to happen IMNSHO if, when the changes (from getOtherMap() to getSpecialExits()) was done one did not also remember to swap around the two variable names:

-        for (auto [roomid, exit] : getOtherMap())) {
+        for (auto [exit, roomid] : getSpecialExits())) {

and I cannot see that I would have immediate been more likely to have spotted the issue there than in the existing code. Probably less so because of the lesser exposure to using auto and range-based loops.

To be perfectly frank it would have been equally valid to have had a local reference/copy of the value to the element concerned like I already had for the room id (as target):

-target = itSpecialExit.key();
-const QString exitName = itSpecialExit.value();
+target = itSpecialExit.value();
+const QString exitName = itSpecialExit.key();

which was indeed done in some other places and made the change over much simpler in those cases - however with only two uses of that item it was marginal as to whether it would have been worth it.

Now we know that it was, since a bug was caused without it.