Merge branch 'OpenCS-fix-modify' into 'master'

OpenCS - Re-implement the fix for Issue #6067.

See merge request OpenMW/openmw!1173

CHANGELOG.md

@@ -10,7 +10,6 @@
     Bug #4602: Robert's Bodies: crash inside createInstance()
     Bug #4700: Editor: Incorrect command implementation
     Bug #4744: Invisible particles must still be processed
-    Bug #4752: UpdateCellCommand doesn't undo properly
     Bug #5100: Persuasion doesn't always clamp the resulting disposition
     Bug #5120: Scripted object spawning updates physics system
     Bug #5379: Wandering NPCs falling through cantons

apps/opencs/model/doc/savingstages.cpp

@@ -285,8 +285,7 @@ void CSMDoc::WriteCellCollectionStage::writeReferences (const std::deque<int>& r
             {
                 refRecord.mRefNum.mIndex = newRefNum++;
             }
-
-            if ((refRecord.mOriginalCell.empty() ? refRecord.mCell : refRecord.mOriginalCell)
+            else if ((refRecord.mOriginalCell.empty() ? refRecord.mCell : refRecord.mOriginalCell)
                     != stream.str() && !interior)
             {
                 // An empty mOriginalCell is meant to indicate that it is the same as

apps/opencs/model/world/commanddispatcher.cpp

@@ -3,9 +3,6 @@
 #include <algorithm>
 #include <memory>
 
-#include <QAbstractItemModel>
-#include <QAbstractProxyModel>
-
 #include <components/misc/stringops.hpp>
 #include <components/misc/constants.hpp>
 
@@ -142,31 +139,12 @@ void CSMWorld::CommandDispatcher::executeModify (QAbstractItemModel *model, cons
     if (mLocked)
         return;
 
-    std::unique_ptr<CSMWorld::CloneCommand> clonedData;
-    std::unique_ptr<CSMWorld::DeleteCommand> deleteData;
-
-    std::string newId;
-
-    std::unique_ptr<CSMWorld::ModifyCommand> modifyData;
     std::unique_ptr<CSMWorld::UpdateCellCommand> modifyCell;
 
-    QAbstractItemModel* sourceModel = model;
-    if (IdTableProxyModel* proxy = dynamic_cast<IdTableProxyModel*> (model))
-        sourceModel = proxy->sourceModel();
-
-    CSMWorld::IdTable& table = dynamic_cast<CSMWorld::IdTable&>(*sourceModel); // for getId()
-    int stateColumn = table.findColumnIndex(Columns::ColumnId_Modification);
-    QModelIndex stateIndex = table.getModelIndex(table.getId(index.row()), stateColumn);
-    RecordBase::State state = static_cast<RecordBase::State> (sourceModel->data(stateIndex).toInt());
 
     int columnId = model->data (index, ColumnBase::Role_ColumnId).toInt();
 
-    // This is not guaranteed to be the same as \a model, since a proxy could be used.
-    IdTable& model2 = dynamic_cast<IdTable&> (*mDocument.getData().getTableModel(mId));
-
-    // DeleteCommand triggers a signal to the whole row from IdTable::setData(), so ignore the second call
-    if (state != RecordBase::State_Deleted &&
-        (columnId==Columns::ColumnId_PositionXPos || columnId==Columns::ColumnId_PositionYPos))
+    if (columnId==Columns::ColumnId_PositionXPos || columnId==Columns::ColumnId_PositionYPos)
     {
         const float oldPosition = model->data (index).toFloat();
 
@@ -178,9 +156,12 @@ void CSMWorld::CommandDispatcher::executeModify (QAbstractItemModel *model, cons
 
             int row = proxy ? proxy->mapToSource (index).row() : index.row();
 
+            // This is not guaranteed to be the same as \a model, since a proxy could be used.
+            IdTable& model2 = dynamic_cast<IdTable&> (*mDocument.getData().getTableModel (mId));
+
             int cellColumn = model2.searchColumnIndex (Columns::ColumnId_Cell);
 
-            if (cellColumn != -1)
+            if (cellColumn!=-1)
             {
                 QModelIndex cellIndex = model2.index (row, cellColumn);
 
@@ -188,46 +169,23 @@ void CSMWorld::CommandDispatcher::executeModify (QAbstractItemModel *model, cons
 
                 if (cellId.find ('#')!=std::string::npos)
                 {
-                    RefCollection& collection = mDocument.getData().getReferences();
-                    newId = collection.getNewId();
-
-                    clonedData.reset(new CloneCommand(table,
-                                table.getId(row),
-                                newId,
-                                CSMWorld::UniversalId::Type_Reference));
-
-                    deleteData.reset(new DeleteCommand(table,
-                                table.getId(row),
-                                CSMWorld::UniversalId::Type_Reference));
+                    // Need to recalculate the cell
+                    modifyCell.reset (new UpdateCellCommand (model2, row));
                 }
             }
         }
     }
 
-    if (!clonedData.get())
-    {
-        // DeleteCommand will trigger executeModify after setting the state to State_Deleted
-        // from CommandDelegate::setModelDataImp() - ignore
-        if (state != RecordBase::State_Deleted)
-            modifyData.reset(new CSMWorld::ModifyCommand(*model, index, new_));
-    }
+    std::unique_ptr<CSMWorld::ModifyCommand> modifyData (
+        new CSMWorld::ModifyCommand (*model, index, new_));
 
-    if (clonedData.get())
+    if (modifyCell.get())
     {
         CommandMacro macro (mDocument.getUndoStack());
-        macro.push(clonedData.release());
-        macro.push(deleteData.release());
-
-        // cannot do these earlier because newIndex is not available until CloneCommand is executed
-        QModelIndex newIndex = model2.getModelIndex (newId, index.column());
-        modifyData.reset (new CSMWorld::ModifyCommand (*model, newIndex, new_));
-        macro.push(modifyData.release());
-
-        // once the data is updated update the cell location
-        modifyCell.reset(new UpdateCellCommand(model2, newIndex.row()));
-        macro.push(modifyCell.release());
+        macro.push (modifyData.release());
+        macro.push (modifyCell.release());
     }
-    else if (!clonedData.get() && modifyData.get())
+    else
         mDocument.getUndoStack().push (modifyData.release());
 }
 

apps/opencs/model/world/refcollection.cpp

@@ -91,12 +91,48 @@ void CSMWorld::RefCollection::load (ESM::ESMReader& reader, int cellIndex, bool
         else
             ref.mCell = cell2.mId;
 
+        if (ref.mRefNum.mContentFile != -1 && !base)
+        {
+            ref.mRefNum.mContentFile = ref.mRefNum.mIndex >> 24;
+            ref.mRefNum.mIndex &= 0x00ffffff;
+        }
+
         unsigned int  refNum = (ref.mRefNum.mIndex & 0x00ffffff) |
             (ref.mRefNum.hasContentFile() ? ref.mRefNum.mContentFile : 0xff) << 24;
 
         std::map<unsigned int, unsigned int>::iterator iter = cache.find(refNum);
 
-        if (ref.mRefNum.mContentFile != -1 && !base) ref.mRefNum.mContentFile = ref.mRefNum.mIndex >> 24;
+        if (isMoved)
+        {
+            if (iter == cache.end())
+            {
+                CSMWorld::UniversalId id(CSMWorld::UniversalId::Type_Cell,
+                    mCells.getId(cellIndex));
+
+                messages.add(id, "Attempt to move a non-existent reference - RefNum index "
+                    + std::to_string(ref.mRefNum.mIndex) + ", refID " + ref.mRefID + ", content file index "
+                    + std::to_string(ref.mRefNum.mContentFile),
+                    /*hint*/"",
+                    CSMDoc::Message::Severity_Warning);
+                continue;
+            }
+
+            int index = getIntIndex(iter->second);
+
+            // ensure we have the same record id for setRecord()
+            ref.mId = getRecord(index).get().mId;
+            ref.mIdNum = extractIdNum(ref.mId);
+
+            std::unique_ptr<Record<CellRef> > record(new Record<CellRef>);
+            // TODO: check whether a base record be moved
+            record->mState = base ? RecordBase::State_BaseOnly : RecordBase::State_ModifiedOnly;
+            (base ? record->mBase : record->mModified) = std::move(ref);
+
+            // overwrite original record
+            setRecord(index, std::move(record));
+
+            continue; // NOTE: assumed moved references are not deleted at the same time
+        }
 
         if (isDeleted)
         {

apps/opencs/model/world/refcollection.hpp

@@ -26,7 +26,7 @@ namespace CSMWorld
     class RefCollection : public Collection<CellRef>
     {
             Collection<Cell>& mCells;
-            std::map<unsigned int, int> mRefIndex;
+            std::map<unsigned int, int> mRefIndex; // CellRef index keyed by CSMWorld::CellRef::mIdNum
 
             int mNextId;
 

apps/opencs/view/render/object.cpp

@@ -682,18 +682,20 @@ void CSVRender::Object::apply (CSMWorld::CommandMacro& commands)
 
             int cellColumn = collection.findColumnIndex (static_cast<CSMWorld::Columns::ColumnId> (
                 CSMWorld::Columns::ColumnId_Cell));
-            int refNumColumn = collection.findColumnIndex (static_cast<CSMWorld::Columns::ColumnId> (
-                CSMWorld::Columns::ColumnId_RefNum));
+            int origCellColumn = collection.findColumnIndex(static_cast<CSMWorld::Columns::ColumnId> (
+                CSMWorld::Columns::ColumnId_OriginalCell));
 
             if (cellIndex != originalIndex)
             {
                 /// \todo figure out worldspace (not important until multiple worldspaces are supported)
+                std::string origCellId = CSMWorld::CellCoordinates(originalIndex).getId("");
                 std::string cellId = CSMWorld::CellCoordinates (cellIndex).getId ("");
 
                 commands.push (new CSMWorld::ModifyCommand (*model,
-                    model->index (recordIndex, cellColumn), QString::fromUtf8 (cellId.c_str())));
-                commands.push (new CSMWorld::ModifyCommand( *model,
-                    model->index (recordIndex, refNumColumn), 0));
+                    model->index (recordIndex, origCellColumn), QString::fromUtf8 (origCellId.c_str())));
+                commands.push(new CSMWorld::ModifyCommand(*model,
+                    model->index(recordIndex, cellColumn), QString::fromUtf8(cellId.c_str())));
+                // NOTE: refnum is not modified for moving a reference to another cell
             }
         }