Removed the inventory delay mode for now.

I said I wouldn't do it, but I change my mind. Here is why:
The delay mode is used in the trading code, and in the inventory
code for lua only.

In the lua part, the delay mode is useless as it is used only
when the script is requesting removal of more items than owned.
This is something that can't be handled well anyway,
as for the script part, even when the character was still
keeping its items.
Plus, it's not actually an error, IMHO.

In the trading code, it made sense. But the next commit
will bring design changes making the inventory items
and the equipment items not linked anymore. Hence, making
it impossible to trade equipped items, for instance,
which is a good thing IMHO. It will also remove the need
of a delayed inventory mode in that case.

The two cases where it is used will disappear, making it
useless.
Last but no least, the part watching for equipment changes
in the commit() function is not something easy to debug
without first having a proper and working base.
(checking of two multimaps changes, for instance).

We can add it back once the code has been fully debugged
for the most skilled among us.

And yes, I am sure of what I'm currently doing.

For now the whole inventory and trading code is in need of review
but it already was, anyway.

4 files changed, 90 insertions, 282 deletions

diff --git a/src/game-server/inventory.cpp b/src/game-server/inventory.cpp
index d7baedf2..e218a916 100644
--- a/src/game-server/inventory.cpp
+++ b/src/game-server/inventory.cpp
@@ -28,190 +28,11 @@
 #include "net/messageout.h"
 #include "utils/logger.h"
 
-Inventory::Inventory(Character *p, bool d):
-    mPoss(&p->getPossessions()), mInvMsg(GPMSG_INVENTORY),
-    mEqmMsg(GPMSG_EQUIP), mCharacter(p), mDelayed(d)
+Inventory::Inventory(Character *p):
+    mPoss(&p->getPossessions()), mCharacter(p)
 {
 }
 
-Inventory::~Inventory()
-{
-    commit(false);
-}
-
-void Inventory::restart()
-{
-    mInvMsg.clear();
-    mInvMsg.writeInt16(GPMSG_INVENTORY);
-}
-
-void Inventory::cancel()
-{
-    assert(mDelayed);
-    Possessions &poss = mCharacter->getPossessions();
-    if (mPoss != &poss)
-    {
-        delete mPoss;
-        mPoss = &poss;
-    }
-    restart();
-}
-
-void Inventory::commit(bool doRestart)
-{
-    Possessions &poss = mCharacter->getPossessions();
-    /* Sends changes, whether delayed or not. */
-    if (mInvMsg.getLength() > 2)
-    {
-        /* Send the message to the client directly. Perhaps this should be
-           done through an update flag, too? */
-        gameHandler->sendTo(mCharacter, mInvMsg);
-    }
-    if (mPoss != &poss)
-    {
-        if (mDelayed)
-        {
-            /*
-             * Search for any and all changes to equipment.
-             * Search through equipment for changes between
-             * old and new equipment.
-             * Send changes directly when there is a change.
-             * Even when equipment references to invy slots are the same,
-             * it still needs to be searched for changes
-             * to the internal equiment slot usage.
-             * This is probably the worst part of doing this in delayed mode.
-             */
-            IdSlotMap oldEquip, newEquip;
-            {
-                EquipData::const_iterator it1, it2, it1_end, it2_end;
-                for (it1 = mPoss->equipSlots.begin(),
-                     it1_end = mPoss->equipSlots.end();
-                    it1 != it1_end;
-                    ++it1)
-                {
-#ifdef INV_CONST_BOUND_DEBUG
-                        IdSlotMap::const_iterator temp2, temp =
-#endif
-                        newEquip.insert(
-                                newEquip.upper_bound(it1->second),
-                                std::make_pair(it1->second, it1->first));
-#ifdef INV_CONST_BOUND_DEBUG
-                        if (temp !=
-                            --(temp2 = newEquip.upper_bound(it1->second)))
-                            throw;
-#endif
-                }
-                for (it2 = poss.equipSlots.begin(),
-                     it2_end = poss.equipSlots.end();
-                    it2 != it2_end;
-                    ++it2)
-                    oldEquip.insert(
-                            oldEquip.upper_bound(it2->second),
-                            std::make_pair(it2->second, it2->first));
-            }
-            {
-                IdSlotMap::const_iterator it1     = newEquip.begin(),
-                                          it2     = oldEquip.begin(),
-                                          it1_end = newEquip.end(),
-                                          it2_end = oldEquip.end(),
-                                          temp1, temp2;
-                while (it1 != it1_end || it2 != it2_end)
-                {
-                    if (it1 == it1_end)
-                    {
-                        if (it2 == it2_end)
-                            break;
-                        equip_sub(0, it1);
-                    }
-                    else if (it2 == it2_end)
-                        equip_sub(newEquip.count(it2->first), it2);
-                    else if (it1->first == it2->first)
-                    {
-                        double invSlot = it1->first;
-                        while ((it1 != it1_end && it1->first == invSlot) ||
-                               (it2 != it2_end && it2->first == invSlot))
-                        {
-                            /*
-                             * Item is still equipped, but need to check
-                             *      that the slots didn't change.
-                             */
-                            if (it1->second == it2->second)
-                            {
-                                // No change.
-                                ++it1;
-                                ++it2;
-                                continue;
-                            }
-                            unsigned int itemId =
-                                    mPoss->inventory.at(it1->first).itemId;
-                            changeEquipment(itemId, itemId);
-                            break;
-                        }
-                    }
-                    else if (it1->first > it2->first)
-                        equip_sub(newEquip.count(it2->first), it2);
-                    else // it1->first < it2->first
-                        equip_sub(0, it1);
-                }
-            }
-        }
-        poss = *mPoss;
-        delete mPoss;
-        mPoss = &poss;
-    }
-
-    /* Update server sided states if in delayed mode. If we are not in
-       delayed mode, the server sided states already reflect the changes
-       that have just been sent to the client. */
-
-    if (mEqmMsg.getLength() > 2)
-        gameHandler->sendTo(mCharacter, mEqmMsg);
-
-    if (doRestart)
-        restart();
-}
-
-void Inventory::equip_sub(unsigned int newCount, IdSlotMap::const_iterator &it)
-{
-    const unsigned int invSlot = it->first;
-    unsigned int count = 0, eqSlot = it->second;
-    mEqmMsg.writeInt16(invSlot);
-    mEqmMsg.writeInt8(newCount);
-    do {
-        if (newCount)
-        {
-            if (it->second != eqSlot)
-            {
-                mEqmMsg.writeInt8(eqSlot);
-                mEqmMsg.writeInt8(count);
-                count = 1;
-                eqSlot = it->second;
-            }
-            ++count;
-        }
-        if (itemManager->isEquipSlotVisible(it->second))
-            mCharacter->raiseUpdateFlags(UPDATEFLAG_LOOKSCHANGE);
-    } while ((++it)->first == invSlot);
-    if (count)
-    {
-        mEqmMsg.writeInt8(eqSlot);
-        mEqmMsg.writeInt8(count);
-    }
-    mEqmMsg.writeInt16(invSlot);
-    changeEquipment(newCount ? 0 : mPoss->inventory.at(invSlot).itemId,
-                    newCount ? mPoss->inventory.at(invSlot).itemId : 0);
-}
-
-void Inventory::prepare()
-{
-    if (!mDelayed)
-        return;
-
-    Possessions *poss = &mCharacter->getPossessions();
-    if (mPoss == poss)
-        mPoss = new Possessions(*poss);
-}
-
 void Inventory::sendFull() const
 {
     /* Sends all the information needed to construct inventory
@@ -242,8 +63,6 @@ void Inventory::sendFull() const
 
 void Inventory::initialize()
 {
-    assert(!mDelayed);
-
     InventoryData::iterator it1;
     EquipData::const_iterator it2, it2_end = mPoss->equipSlots.end();
     /*
@@ -343,10 +162,11 @@ unsigned int Inventory::getItem(unsigned int slot) const
 
 unsigned int Inventory::insert(unsigned int itemId, unsigned int amount)
 {
-    unsigned int maxPerSlot = itemManager->getItem(itemId)->getMaxPerSlot();
     if (!itemId || !amount)
         return 0;
-    prepare();
+
+    MessageOut invMsg(GPMSG_INVENTORY);
+    unsigned int maxPerSlot = itemManager->getItem(itemId)->getMaxPerSlot();
     InventoryData::iterator it, it_end = mPoss->inventory.end();
     // Add to slots with existing items of this type first.
     for (it = mPoss->inventory.begin(); it != it_end; ++it)
@@ -369,11 +189,11 @@ unsigned int Inventory::insert(unsigned int itemId, unsigned int amount)
                 amount -= spaceleft;
             }
 
-            mInvMsg.writeInt16(it->first);
-            mInvMsg.writeInt16(itemId);
-            mInvMsg.writeInt16(it->second.amount);
+            invMsg.writeInt16(it->first);
+            invMsg.writeInt16(itemId);
+            invMsg.writeInt16(it->second.amount);
             if (!amount)
-                return 0;
+                break;
         }
 
     int slot = 0;
@@ -381,7 +201,7 @@ unsigned int Inventory::insert(unsigned int itemId, unsigned int amount)
     for (it = mPoss->inventory.begin();; ++it)
     {
         if (!amount)
-            return 0;
+            break;
         int lim = (it == it_end) ? INVENTORY_SLOTS : it->first;
         while (amount && slot < lim)
         {
@@ -389,14 +209,18 @@ unsigned int Inventory::insert(unsigned int itemId, unsigned int amount)
             mPoss->inventory[slot].itemId = itemId;
             mPoss->inventory[slot].amount = additions;
             amount -= additions;
-            mInvMsg.writeInt16(slot++); // Last read, so also increment
-            mInvMsg.writeInt16(itemId);
-            mInvMsg.writeInt16(additions);
+            invMsg.writeInt16(slot++); // Last read, so also increment
+            invMsg.writeInt16(itemId);
+            invMsg.writeInt16(additions);
         }
         ++slot; // Skip the slot that the iterator points to
         if (it == it_end) break;
     }
 
+    // Send that first, before checking potential removals
+    if (invMsg.getLength() > 2)
+        gameHandler->sendTo(mCharacter, invMsg);
+
     checkInventorySize();
 
     return amount;
@@ -415,9 +239,11 @@ unsigned int Inventory::count(unsigned int itemId) const
 
 unsigned int Inventory::remove(unsigned int itemId, unsigned int amount, bool force)
 {
-    prepare();
     bool inv = false,
          eq = !itemManager->getItem(itemId)->getItemEquipData().empty();
+
+    MessageOut invMsg(GPMSG_INVENTORY);
+    bool triggerLeaveInventory = true;
     for (InventoryData::iterator it = mPoss->inventory.begin(),
                                  it_end = mPoss->inventory.end();
          it != it_end; ++it)
@@ -448,29 +274,34 @@ unsigned int Inventory::remove(unsigned int itemId, unsigned int amount, bool fo
                 unsigned int sub = std::min(amount, it->second.amount);
                 amount -= sub;
                 it->second.amount -= sub;
-                mInvMsg.writeInt16(it->first);
+                invMsg.writeInt16(it->first);
                 if (it->second.amount)
                 {
-                    mInvMsg.writeInt16(it->second.itemId);
-                    mInvMsg.writeInt16(it->second.amount);
+                    invMsg.writeInt16(it->second.itemId);
+                    invMsg.writeInt16(it->second.amount);
                     // Some still exist, and we have none left to remove, so
                     // no need to run leave invy triggers.
                     if (!amount)
-                        return 0;
+                        triggerLeaveInventory = false;
                 }
                 else
                 {
-                    mInvMsg.writeInt16(0);
+                    invMsg.writeInt16(0);
                     mPoss->inventory.erase(it);
                 }
             }
             else
                 // We found an instance of them existing and have none left to
                 // remove, so no need to run leave invy triggers.
-                return 0;
+                triggerLeaveInventory = false;
         }
-    if (force)
+
+    if (triggerLeaveInventory)
         itemManager->getItem(itemId)->useTrigger(mCharacter, ITT_LEAVE_INVY);
+
+    if (invMsg.getLength() > 2)
+        gameHandler->sendTo(mCharacter, invMsg);
+
     // Rather inefficient, but still usable for now assuming small invy size.
     // FIXME
     return inv && !force ? remove(itemId, amount, true) : amount;
@@ -481,7 +312,7 @@ unsigned int Inventory::move(unsigned int slot1, unsigned int slot2,
 {
     if (!amount || slot1 == slot2 || slot2 >= INVENTORY_SLOTS)
         return amount;
-    prepare();
+
     InventoryData::iterator it1 = mPoss->inventory.find(slot1),
                             it2 = mPoss->inventory.find(slot2),
                             inv_end = mPoss->inventory.end();
@@ -498,6 +329,8 @@ unsigned int Inventory::move(unsigned int slot1, unsigned int slot2,
             //     items in the same slot anyway.
             it->second =  slot2;
 
+    MessageOut invMsg(GPMSG_INVENTORY);
+
     unsigned int nb = std::min(amount, it1->second.amount);
     if (it2 == inv_end)
     {
@@ -510,20 +343,20 @@ unsigned int Inventory::move(unsigned int slot1, unsigned int slot2,
         it1->second.amount -= nb;
         amount -= nb;
 
-        mInvMsg.writeInt16(slot1);                  // Slot
+        invMsg.writeInt16(slot1);                  // Slot
         if (it1->second.amount)
         {
-            mInvMsg.writeInt16(it1->second.itemId); // Item Id
-            mInvMsg.writeInt16(it1->second.amount); // Amount
+            invMsg.writeInt16(it1->second.itemId); // Item Id
+            invMsg.writeInt16(it1->second.amount); // Amount
         }
         else
         {
-            mInvMsg.writeInt16(0);
+            invMsg.writeInt16(0);
             mPoss->inventory.erase(it1);
         }
-        mInvMsg.writeInt16(slot2);                  // Slot
-        mInvMsg.writeInt16(it1->second.itemId);     // Item Id (same as slot 1)
-        mInvMsg.writeInt16(nb);                     // Amount
+        invMsg.writeInt16(slot2);                  // Slot
+        invMsg.writeInt16(it1->second.itemId);     // Item Id (same as slot 1)
+        invMsg.writeInt16(nb);                     // Amount
     }
     else
     {
@@ -538,34 +371,37 @@ unsigned int Inventory::move(unsigned int slot1, unsigned int slot2,
         it2->second.amount += nb;
         amount -= nb;
 
-        mInvMsg.writeInt16(slot1);                  // Slot
+        invMsg.writeInt16(slot1);                  // Slot
         if (it1->second.amount)
         {
-            mInvMsg.writeInt16(it1->second.itemId); // Item Id
-            mInvMsg.writeInt16(it1->second.amount); // Amount
+            invMsg.writeInt16(it1->second.itemId); // Item Id
+            invMsg.writeInt16(it1->second.amount); // Amount
         }
         else
         {
-            mInvMsg.writeInt16(0);
+            invMsg.writeInt16(0);
             mPoss->inventory.erase(it1);
         }
-        mInvMsg.writeInt16(slot2);                  // Slot
-        mInvMsg.writeInt16(it2->second.itemId);     // Item Id
-        mInvMsg.writeInt16(it2->second.amount);     // Amount
+        invMsg.writeInt16(slot2);                  // Slot
+        invMsg.writeInt16(it2->second.itemId);     // Item Id
+        invMsg.writeInt16(it2->second.amount);     // Amount
     }
+
+    if (invMsg.getLength() > 2)
+        gameHandler->sendTo(mCharacter, invMsg);
+
     return amount;
 }
 
 unsigned int Inventory::removeFromSlot(unsigned int slot, unsigned int amount)
 {
-    prepare();
-
     InventoryData::iterator it = mPoss->inventory.find(slot);
 
     // When the given slot doesn't exist, we can't remove anything
     if (it == mPoss->inventory.end())
         return amount;
 
+    MessageOut invMsg(GPMSG_INVENTORY);
     // Check if an item of the same class exists elsewhere in the inventory
     bool exists = false;
     for (InventoryData::const_iterator it2 = mPoss->inventory.begin(),
@@ -587,17 +423,21 @@ unsigned int Inventory::removeFromSlot(unsigned int slot, unsigned int amount)
     unsigned int sub = std::min(amount, it->second.amount);
     amount -= sub;
     it->second.amount -= sub;
-    mInvMsg.writeInt16(it->first);
+    invMsg.writeInt16(it->first);
     if (it->second.amount)
     {
-        mInvMsg.writeInt16(it->second.itemId);
-        mInvMsg.writeInt16(it->second.amount);
+        invMsg.writeInt16(it->second.itemId);
+        invMsg.writeInt16(it->second.amount);
     }
     else
     {
-        mInvMsg.writeInt16(0);
+        invMsg.writeInt16(0);
         mPoss->inventory.erase(it);
     }
+
+    if (invMsg.getLength() > 2)
+        gameHandler->sendTo(mCharacter, invMsg);
+
     return amount;
 }
 
@@ -636,6 +476,8 @@ bool Inventory::equip(int slot, bool override)
     if (eq.empty())
         return false;
     ItemEquipInfo const *ovd = 0;
+
+    MessageOut equipMsg(GPMSG_EQUIP);
     // Iterate through all possible combinations of slots
     for (ItemEquipsInfo::const_iterator it2 = eq.begin(),
          it2_end = eq.end(); it2 != it2_end; ++it2)
@@ -679,19 +521,15 @@ bool Inventory::equip(int slot, bool override)
             /*
              * Clean fit. Equip and apply immediately.
              */
-            if (!mDelayed) {
-                mEqmMsg.writeInt16(slot);           // Inventory slot
-                mEqmMsg.writeInt8(it2->size());     // Equip slot type count
-            }
+            equipMsg.writeInt16(slot);           // Inventory slot
+            equipMsg.writeInt8(it2->size());     // Equip slot type count
             for (it3 = it2->begin(),
                  it3_end = it2->end();
                  it3 != it3_end;
                  ++it3)
             {
-                if (!mDelayed) {
-                    mEqmMsg.writeInt8(it3->first);  // Equip slot
-                    mEqmMsg.writeInt8(it3->second); // How many are used
-                }
+                equipMsg.writeInt8(it3->first);  // Equip slot
+                equipMsg.writeInt8(it3->second); // How many are used
                 /*
                  * This bit can be somewhat inefficient, but is far better for
                  *  average case assuming most equip use one slot max for each
@@ -704,8 +542,8 @@ bool Inventory::equip(int slot, bool override)
                     mPoss->equipSlots.insert(
                             std::make_pair(it3->first, slot));
             }
-            if (!mDelayed)
-                changeEquipment(0, it->second.itemId);
+
+            changeEquipment(0, it->second.itemId);
             return true;
             case 1:
             /*
@@ -731,6 +569,9 @@ bool Inventory::equip(int slot, bool override)
             break;
         }
     }
+
+    if (equipMsg.getLength() > 2)
+        gameHandler->sendTo(mCharacter, equipMsg);
     // We didn't find a clean equip.
     if (ovd)
     {
@@ -762,11 +603,11 @@ bool Inventory::unequip(EquipData::iterator it)
 
 bool Inventory::unequip(unsigned int slot, EquipData::iterator *itp)
 {
-    prepare();
     EquipData::iterator it = itp ? *itp : mPoss->equipSlots.begin(),
                         it_end = mPoss->equipSlots.end();
     bool changed = false;
 
+    MessageOut equipMsg(GPMSG_EQUIP);
     // Erase all equip entries that point to the given inventory slot
     while (it != it_end)
     {
@@ -781,12 +622,15 @@ bool Inventory::unequip(unsigned int slot, EquipData::iterator *itp)
         }
     }
 
-    if (changed && !mDelayed)
+    if (changed)
     {
         changeEquipment(mPoss->inventory.at(slot).itemId, 0);
-        mEqmMsg.writeInt16(slot);
-        mEqmMsg.writeInt8(0);
+        equipMsg.writeInt16(slot);
+        equipMsg.writeInt8(0);
     }
 
+    if (equipMsg.getLength() > 2)
+        gameHandler->sendTo(mCharacter, equipMsg);
+
     return changed;
 }
diff --git a/src/game-server/inventory.h b/src/game-server/inventory.h
index 4a77ad93..3f8403ae 100644
--- a/src/game-server/inventory.h
+++ b/src/game-server/inventory.h
@@ -35,30 +35,15 @@ class Inventory
 
         /**
          * Creates a view on the possessions of a character.
-         * @param delayed If the changes need to be cancelable.
          */
-        Inventory(Character *, bool delayed = false);
+        Inventory(Character *);
 
         /**
          * Commits delayed changes if applicable.
          * Sends the update message to the client.
          */
-        ~Inventory();
-
-        /**
-         * Commits changes.
-         * Exclusive to delayed mode.
-         * @param doRestart Whether to prepare the inventory for more changes
-                   after this. If you are unsure, it is safe (though not
-                   terribly efficient) to leave this as true.
-         */
-        void commit(bool doRestart = true);
-
-        /**
-         * Cancels changes.
-         * Exclusive to delayed mode.
-         */
-        void cancel();
+        ~Inventory()
+        {}
 
         /**
          * Sends complete inventory status to the client.
@@ -129,19 +114,6 @@ class Inventory
         unsigned int getItem(unsigned int slot) const;
 
     private:
-
-        /**
-         * Make sure that changes are being done on a copy, not directly.
-         * No effect when not in delayed mode.
-         */
-        void prepare();
-
-        /**
-         * Starts a new notification message.
-         */
-        void restart();
-
-
         /**
          * Check the inventory is within the slot limit and capacity.
          * Forcibly delete items from the end if it is not.
@@ -164,15 +136,8 @@ class Inventory
         void changeEquipment(ItemClass *oldI, ItemClass *newI);
 
         Possessions *mPoss; /**< Pointer to the modified possessions. */
-        /**
-         * Update message containing inventory changes.
-         * Note that in sendFull(), this is reused to send all full changes
-         * (for both inventory and equipment)
-         */
-        MessageOut mInvMsg;
-        MessageOut mEqmMsg; /**< Update message containing equipment changes */
+
         Character *mCharacter; /**< Character to notify. */
-        bool mDelayed;      /**< Delayed changes. */
 };
 
 #endif
diff --git a/src/game-server/trade.cpp b/src/game-server/trade.cpp
index 51509307..e1779d2f 100644
--- a/src/game-server/trade.cpp
+++ b/src/game-server/trade.cpp
@@ -130,7 +130,7 @@ void Trade::agree(Character *c)
 
     // Check if both player has the objects in their inventories
     // and enouth money, then swap them.
-    Inventory v1(mChar1, true), v2(mChar2, true);
+    Inventory v1(mChar1), v2(mChar2);
     if (mChar1->getAttribute(mCurrencyId) >= mMoney1 - mMoney2 &&
         mChar2->getAttribute(mCurrencyId) >= mMoney2 - mMoney1 &&
         perform(mItems1, v1, v2) &&
@@ -143,8 +143,6 @@ void Trade::agree(Character *c)
     }
     else
     {
-        v1.cancel();
-        v2.cancel();
         cancel();
         return;
     }
diff --git a/src/scripting/lua.cpp b/src/scripting/lua.cpp
index d461bf44..b422fc3e 100644
--- a/src/scripting/lua.cpp
+++ b/src/scripting/lua.cpp
@@ -359,7 +359,7 @@ static int chr_inv_change(lua_State *s)
         return 0;
     }
     int nb_items = (lua_gettop(s) - 1) / 2;
-    Inventory inv(q, true);
+    Inventory inv(q);
     for (int i = 0; i < nb_items; ++i)
     {
         if (!lua_isnumber(s, i * 2 + 2) || !lua_isnumber(s, i * 2 + 3))
@@ -377,12 +377,13 @@ static int chr_inv_change(lua_State *s)
         }
         else if (nb < 0)
         {
+            // Removing too much item is a success as for the scripter's
+            // point of view. We log it anyway.
             nb = inv.remove(id, -nb);
             if (nb)
             {
-                inv.cancel();
-                lua_pushboolean(s, 0);
-                return 1;
+                LOG_WARN("mana.chr_inv_change() removed more items than owned: "
+                     << "character: " << q->getName() << " item id: " << id);
             }
         }
         else