Skip to content
Permalink
Browse files

Fixed crash when resizing map causes objects to get removed

This bug sneaked in due to a QUndoCommand getting immediately executed
even when pushed as part of a macro, whereas the code was relying on it
getting executed later (in using a modern for-loop that assumed the
iterated container is not going to get modified).

I expected execution to be later, since the beginMacro/endMacro are
documented with a piece of "equivalent" code that would behave like
that. Turns out it's not exactly the same, and I was a fool for assuming
it was.
  • Loading branch information...
bjorn committed Jan 9, 2017
1 parent 6d3a36b commit ad3eb5b4d3e0036ec05b18a1c43c54910f17d87e
@@ -27,9 +27,11 @@
using namespace Tiled::Internal;

ChangeSelectedArea::ChangeSelectedArea(MapDocument *mapDocument,
const QRegion &newSelection)
const QRegion &newSelection,
QUndoCommand *parent)
: QUndoCommand(QCoreApplication::translate("Undo Commands",
"Change Selection"))
"Change Selection"),
parent)
, mMapDocument(mapDocument)
, mSelection(newSelection)
{
@@ -37,7 +37,8 @@ class ChangeSelectedArea: public QUndoCommand
* the given \a selection.
*/
ChangeSelectedArea(MapDocument *mapDocument,
const QRegion &selection);
const QRegion &selection,
QUndoCommand *parent = nullptr);

void undo() override;
void redo() override;
@@ -333,14 +333,15 @@ void MapDocument::resizeMap(const QSize &size, const QPoint &offset, bool remove
const QPointF pixelOffset = origin - newOrigin;

// Resize the map and each layer
mUndoStack->beginMacro(tr("Resize Map"));
QUndoCommand *command = new QUndoCommand(tr("Resize Map"));

for (int i = 0; i < mMap->layerCount(); ++i) {
Layer *layer = mMap->layerAt(i);

switch (layer->layerType()) {
case Layer::TileLayerType: {
TileLayer *tileLayer = static_cast<TileLayer*>(layer);
mUndoStack->push(new ResizeTileLayer(this, tileLayer, size, offset));
new ResizeTileLayer(this, tileLayer, size, offset, command);
break;
}
case Layer::ObjectGroupType: {
@@ -350,11 +351,11 @@ void MapDocument::resizeMap(const QSize &size, const QPoint &offset, bool remove
if (removeObjects) {
for (MapObject *o : objectGroup->objects()) {
if (!visibleIn(visibleArea, o, mRenderer)) {
mUndoStack->push(new RemoveMapObject(this, o));
new RemoveMapObject(this, o, command);
} else {
QPointF oldPos = o->position();
QPointF newPos = oldPos + pixelOffset;
mUndoStack->push(new MoveMapObject(this, o, newPos, oldPos));
new MoveMapObject(this, o, newPos, oldPos, command);
}
}
}
@@ -366,9 +367,10 @@ void MapDocument::resizeMap(const QSize &size, const QPoint &offset, bool remove
}
}

mUndoStack->push(new ResizeMap(this, size));
mUndoStack->push(new ChangeSelectedArea(this, movedSelection));
mUndoStack->endMacro();
new ResizeMap(this, size, command);
new ChangeSelectedArea(this, movedSelection, command);

mUndoStack->push(command);

// TODO: Handle layers that don't match the map size correctly
}
@@ -31,8 +31,10 @@ using namespace Tiled::Internal;

MoveMapObject::MoveMapObject(MapDocument *mapDocument,
MapObject *mapObject,
const QPointF &oldPos)
: mMapDocument(mapDocument)
const QPointF &oldPos,
QUndoCommand *parent)
: QUndoCommand(parent)
, mMapDocument(mapDocument)
, mMapObject(mapObject)
, mOldPos(oldPos)
, mNewPos(mapObject->position())
@@ -43,8 +45,10 @@ MoveMapObject::MoveMapObject(MapDocument *mapDocument,
MoveMapObject::MoveMapObject(MapDocument *mapDocument,
MapObject *mapObject,
const QPointF &newPos,
const QPointF &oldPos)
: mMapDocument(mapDocument)
const QPointF &oldPos,
QUndoCommand *parent)
: QUndoCommand(parent)
, mMapDocument(mapDocument)
, mMapObject(mapObject)
, mOldPos(oldPos)
, mNewPos(newPos)
@@ -37,12 +37,14 @@ class MoveMapObject : public QUndoCommand
public:
MoveMapObject(MapDocument *mapDocument,
MapObject *mapObject,
const QPointF &oldPos);
const QPointF &oldPos,
QUndoCommand *parent = nullptr);

MoveMapObject(MapDocument *mapDocument,
MapObject *mapObject,
const QPointF &newPos,
const QPointF &oldPos);
const QPointF &oldPos,
QUndoCommand *parent = nullptr);

void undo() override;
void redo() override;
@@ -28,8 +28,12 @@
namespace Tiled {
namespace Internal {

ResizeMap::ResizeMap(MapDocument *mapDocument, const QSize &size)
: QUndoCommand(QCoreApplication::translate("Undo Commands", "Resize Map"))
ResizeMap::ResizeMap(MapDocument *mapDocument,
const QSize &size,
QUndoCommand *parent)
: QUndoCommand(QCoreApplication::translate("Undo Commands",
"Resize Map"),
parent)
, mMapDocument(mapDocument)
, mSize(size)
{
@@ -36,7 +36,9 @@ class MapDocument;
class ResizeMap : public QUndoCommand
{
public:
ResizeMap(MapDocument *mapDocument, const QSize &size);
ResizeMap(MapDocument *mapDocument,
const QSize &size,
QUndoCommand *parent = nullptr);

void undo() override;
void redo() override;
@@ -33,9 +33,11 @@ using namespace Tiled::Internal;
ResizeTileLayer::ResizeTileLayer(MapDocument *mapDocument,
TileLayer *layer,
const QSize &size,
const QPoint &offset)
const QPoint &offset,
QUndoCommand *parent)
: QUndoCommand(QCoreApplication::translate("Undo Commands",
"Resize Layer"))
"Resize Layer"),
parent)
, mMapDocument(mapDocument)
, mIndex(mapDocument->map()->layers().indexOf(layer))
, mOriginalLayer(nullptr)
@@ -47,7 +47,8 @@ class ResizeTileLayer : public QUndoCommand
ResizeTileLayer(MapDocument *mapDocument,
TileLayer *layer,
const QSize &size,
const QPoint &offset);
const QPoint &offset,
QUndoCommand *parent = nullptr);

~ResizeTileLayer();

0 comments on commit ad3eb5b

Please sign in to comment.
You can’t perform that action at this time.