addUndo and combineWithList looks treacherous

[m-7761]

I just notice it seems that passing "false" to sendUndo sets m_listCombine, but it's one way so as soon as it's set to false no undo objects can combine for the rest of the operation.

Also combineWithList doesn't just try to combine with the last sendUndo but iterates through all of them, and combines with any compatible.

Unless I'm misunderstanding this (I'm not intimately familiar with it) this strategy 1) shoots you in the foot for combining huge lists, and 2) makes the items sent to sendUndo execute out-of-order, which seems like eventually something's going to break and be hard to debug and it's too chaotic to even reason about.

I think this is why usually "false" was sent to sendUndo even though it could obviously benefit from combining many things.

void UndoManager::addUndo(Undo *u, bool listCombine)
{
    clearRedo();

    if(!listCombine)
    {
        //FIX ME!
        //Why is this one way? See combineWithList for more concerns!

        m_listCombine = false;
    }

    if(m_currentUndo)
    {
        // Try to combine these undo items
        if(combineWithList(u))
        {
            // Combined, release new one
            u->release();
        }
        else
        {
            // Push the current undo onto the current list and make this undo current
            pushUndoToList(m_currentUndo);
            m_currentUndo = u;
        }
    }
    else
    {
        // No undo yet, this is our first
        m_currentUndo = u;
    }
}

BTW: MU_MoveFrameVertex::combine doesn't check m_frame and m_anim. But it's not used with combine anyway, and neither does MU_MoveFramePoint::combine, but I've removed this one from my code.

The sane alternative is to aggressively combine to the end of the list, but assume that if you're mixing undo objects that in-order execution is more important than combining, i.e. consolidating. In some cases undo objects are generated without a clear "operation" structure so there's no way to know how they will interact in those cases.

[m-7761]

Actually, the problem that led me to this was solved by fixing MU_MoveFrameVertex.

It turns out combineWithList always tries to combine with the back of the list, so the "sendUndo" bool parameter just controls the combining with deeper in the operation.

Not foreseeing this I changed a lot of sendUndo calls to use "true" in my code. I think this ability needs to be removed period because the boundaries of an "operation" typically extend to an entire modal dialog, during which a lot can change and the meaning of the undo variables change to the point the "combine" analysis isn't even accurate.

Edited: Of course the first instance of "false" kills this feature for the rest of the operation, but it seems error prone to me to work like this and not worth any benefits to risk a potential disaster.