Commit 5ea31706 authored by Dmitry Kazakov's avatar Dmitry Kazakov

Fixed a really tricky bug in walkers

https://bugs.kde.org/show_bug.cgi?id=293314
He-he ;)

It happened due to the following:
1) Undo of removeLayer command added a layer and issued setDirty
   for this node
2) The scheduler calculated the walker for this update and put it
   into the queue
3) Undo of addLayer command removed one of the layers of the stack,
   leaving the queued walker in a very interesting state.

The point is that the walker stored a shared pointer to the removed
node, so it still had access to it and there were no crashes, but its
behavior was not obvious (read "unpredictable").

So I extended the meaning of the walker's checksum. Now it depends on
the graph sequence number as well, so the walker is recalculated when
he gets to know its data is outdated.

About Graph Sequence Number.

Now KisNodeGraphListener maintains an integer which shows current "version"
of the graph or its "sequence number". This integer is incremented on every
change made to the graph. So if you have some information about the graph
which was acquired while the sequence number was X and now you see the number
equals to Y, you should know your information is outdated and someone has
changed the graph since then.

BUG:293314
parent 5ddea2ad
......@@ -144,6 +144,7 @@ set(kritaimage_LIB_SRCS
kis_count_visitor.cpp
kis_histogram.cc
kis_image_interfaces.cpp
kis_node_graph_listener.cpp
kis_image.cc
kis_image_signal_router.cpp
kis_image_config.cpp
......
......@@ -242,6 +242,15 @@ void KisAsyncMerger::startMerge(KisBaseRectsWalker &walker, bool notifyClones) {
if(notifyClones) {
doNotifyClones(walker);
}
if(m_currentProjection) {
warnImage << "BUG: The walker hasn't reached the root layer!";
warnImage << " Start node:" << walker.startNode() << "Requested rect:" << walker.requestedRect();
warnImage << " There must be an inconsistency in the walkers happened!";
warnImage << " Please report a bug describing how you got this message.";
// reset projection to avoid artefacts in next merges and allow people to work further
resetProjection();
}
}
bool KisAsyncMerger::isRootNode(KisNodeSP node) {
......@@ -264,7 +273,6 @@ void KisAsyncMerger::setupProjection(KisNodeSP currentNode, const QRect& rect, b
if (useTempProjection) {
if(!m_cachedPaintDevice)
m_cachedPaintDevice = new KisPaintDevice(parentOriginal->colorSpace());
m_currentProjection = m_cachedPaintDevice;
m_currentProjection->prepareClone(parentOriginal);
m_finalProjection = parentOriginal;
......
......@@ -102,6 +102,7 @@ public:
void collectRects(KisNodeSP node, const QRect& requestedRect) {
clear();
m_nodeChecksum = calculateChecksum(node, requestedRect);
m_graphChecksum = node->graphSequenceNumber();
m_resultChangeRect = requestedRect;
m_resultUncroppedChangeRect = requestedRect;
m_requestedRect = requestedRect;
......@@ -116,7 +117,9 @@ public:
bool checksumValid() {
Q_ASSERT(m_startNode);
return m_nodeChecksum == calculateChecksum(m_startNode, m_requestedRect);
return
m_nodeChecksum == calculateChecksum(m_startNode, m_requestedRect) &&
m_graphChecksum == m_startNode->graphSequenceNumber();
}
inline void setCropRect(QRect cropRect) {
......@@ -399,6 +402,13 @@ private:
*/
qint32 m_nodeChecksum;
/**
* Used for getting know whether the structure of
* the graph has changed since the walker was
* calculated
*/
qint32 m_graphChecksum;
/**
* Temporary variables
*/
......
......@@ -170,56 +170,26 @@ KisImage::~KisImage()
disconnect(); // in case Qt gets confused
}
void KisImage::aboutToAddANode(KisNode *parent, int index)
{
Q_UNUSED(parent);
Q_UNUSED(index);
}
void KisImage::nodeHasBeenAdded(KisNode *parent, int index)
{
KisNodeGraphListener::nodeHasBeenAdded(parent, index);
SANITY_CHECK_LOCKED("nodeHasBeenAdded");
m_d->signalRouter->emitNodeHasBeenAdded(parent, index);
}
void KisImage::aboutToRemoveANode(KisNode *parent, int index)
{
KisNodeGraphListener::aboutToRemoveANode(parent, index);
SANITY_CHECK_LOCKED("aboutToRemoveANode");
m_d->signalRouter->emitAboutToRemoveANode(parent, index);
}
void KisImage::nodeHasBeenRemoved(KisNode *parent, int index)
{
Q_UNUSED(parent);
Q_UNUSED(index);
}
void KisImage::aboutToMoveNode(KisNode *node, int oldIndex, int newIndex)
{
Q_UNUSED(node);
Q_UNUSED(oldIndex);
Q_UNUSED(newIndex);
/**
* We do not use move signals. A node is first deleted from the
* stack and then added back. So please catch remove/insert signals
*/
}
void KisImage::nodeHasBeenMoved(KisNode *node, int oldIndex, int newIndex)
{
Q_UNUSED(node);
Q_UNUSED(oldIndex);
Q_UNUSED(newIndex);
/**
* We do not use move signals. A node is first deleted from the
* stack and then added back. So please catch remove/insert signals
*/
}
void KisImage::nodeChanged(KisNode* node)
{
KisNodeGraphListener::nodeChanged(node);
m_d->signalRouter->emitNodeChanged(node);
}
......@@ -994,15 +964,8 @@ KisLayerSP KisImage::flattenLayer(KisLayerSP layer)
undoAdapter()->beginMacro(i18n("Flatten Layer"));
undoAdapter()->addCommand(new KisImageLayerAddCommand(this, newLayer, layer->parent(), layer));
KisNodeSP node = layer->firstChild();
while (node) {
undoAdapter()->addCommand(new KisImageLayerRemoveCommand(this, node));
node = node->nextSibling();
}
undoAdapter()->addCommand(new KisImageLayerRemoveCommand(this, layer));
QList<const KisMetaData::Store*> srcs;
srcs.append(layer->metaData());
......@@ -1210,8 +1173,10 @@ KisActionRecorder* KisImage::actionRecorder() const
void KisImage::setRootLayer(KisGroupLayerSP rootLayer)
{
if(m_d->rootLayer)
if(m_d->rootLayer) {
m_d->rootLayer->setGraphListener(0);
m_d->rootLayer->disconnect();
}
m_d->rootLayer = rootLayer;
m_d->rootLayer->disconnect();
......@@ -1410,6 +1375,8 @@ void KisImage::notifyProjectionUpdated(const QRect &rc)
void KisImage::requestProjectionUpdate(KisNode *node, const QRect& rect)
{
KisNodeGraphListener::requestProjectionUpdate(node, rect);
if (m_d->scheduler) {
m_d->scheduler->updateProjection(node, rect, bounds());
}
......
......@@ -82,12 +82,8 @@ public:
public: // KisNodeGraphListener implementation
void aboutToAddANode(KisNode *parent, int index);
void nodeHasBeenAdded(KisNode *parent, int index);
void aboutToRemoveANode(KisNode *parent, int index);
void nodeHasBeenRemoved(KisNode *parent, int index);
void aboutToMoveNode(KisNode * node, int oldIndex, int newIndex);
void nodeHasBeenMoved(KisNode * node, int oldIndex, int newIndex);
void nodeChanged(KisNode * node);
void requestProjectionUpdate(KisNode *node, const QRect& rect);
......
......@@ -124,6 +124,11 @@ void KisNode::accept(KisProcessingVisitor &visitor, KisUndoAdapter *undoAdapter)
return visitor.visit(this, undoAdapter);
}
int KisNode::graphSequenceNumber() const
{
return m_d->graphListener ? m_d->graphListener->graphSequenceNumber() : -1;
}
KisNodeGraphListener *KisNode::graphListener() const
{
return m_d->graphListener;
......
......@@ -165,6 +165,13 @@ public:
public: // Graph methods
/**
* @return the graph sequence number calculated by the associated
* graph listener. You can use it for checking for changes in the
* graph.
*/
int graphSequenceNumber() const;
/**
* @return the graph listener this node belongs to. 0 if the node
* does not belong to a grap listener.
......
/*
* Copyright (c) 2007 Boudewijn Rempt <boud@valdyas.org>
* Copyright (c) 2012 Dmitry Kazakov <dimula73@gmail.com>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
#include "kis_node_graph_listener.h"
struct KisNodeGraphListener::Private
{
Private() : sequenceNumber(0) {}
int sequenceNumber;
};
KisNodeGraphListener::KisNodeGraphListener()
: m_d(new Private())
{
}
KisNodeGraphListener::~KisNodeGraphListener()
{
delete m_d;
}
void KisNodeGraphListener::aboutToAddANode(KisNode *parent, int index)
{
m_d->sequenceNumber++;
}
void KisNodeGraphListener::nodeHasBeenAdded(KisNode *parent, int index)
{
m_d->sequenceNumber++;
}
void KisNodeGraphListener::aboutToRemoveANode(KisNode *parent, int index)
{
m_d->sequenceNumber++;
}
void KisNodeGraphListener::nodeHasBeenRemoved(KisNode *parent, int index)
{
m_d->sequenceNumber++;
}
void KisNodeGraphListener::aboutToMoveNode(KisNode * node, int oldIndex, int newIndex)
{
m_d->sequenceNumber++;
}
void KisNodeGraphListener::nodeHasBeenMoved(KisNode * node, int oldIndex, int newIndex)
{
m_d->sequenceNumber++;
}
int KisNodeGraphListener::graphSequenceNumber() const
{
return m_d->sequenceNumber;
}
void KisNodeGraphListener::nodeChanged(KisNode * node)
{
}
void KisNodeGraphListener::requestProjectionUpdate(KisNode * node, const QRect& rect)
{
}
......@@ -20,6 +20,7 @@
#include "krita_export.h"
class KisNode;
class QRect;
/**
* Implementations of this class are called by nodes whenever the node
......@@ -30,52 +31,75 @@ class KisNode;
* The reason for this go-between is that we don't want our nodes to
* be QObjects, nor to have sig-slot connections between every node
* and every mode.
*
* It also manages the sequence number of the graph. This is a number
* which can be used as a checksum for whether the graph has chenged
* from some period of time or not. \see graphSequenceNumber()
*/
class KRITAIMAGE_EXPORT KisNodeGraphListener
{
public:
KisNodeGraphListener();
virtual ~KisNodeGraphListener() {}
virtual ~KisNodeGraphListener();
/**
* Inform the model that we're going to add a node.
*/
virtual void aboutToAddANode(KisNode *parent, int index) = 0;
virtual void aboutToAddANode(KisNode *parent, int index);
/**
* Inform the model we're done adding a node.
*/
virtual void nodeHasBeenAdded(KisNode *parent, int index) = 0;
virtual void nodeHasBeenAdded(KisNode *parent, int index);
/**
* Inform the model we're going to remove a node.
*/
virtual void aboutToRemoveANode(KisNode *parent, int index) = 0;
virtual void aboutToRemoveANode(KisNode *parent, int index);
/**
* Inform the model we're done removing a node.
*/
virtual void nodeHasBeenRemoved(KisNode *parent, int index) = 0;
virtual void nodeHasBeenRemoved(KisNode *parent, int index);
/**
* Inform the model we're about to start moving a node (which
* includes removing and adding the same node)
*/
virtual void aboutToMoveNode(KisNode * node, int oldIndex, int newIndex) = 0;
virtual void aboutToMoveNode(KisNode * node, int oldIndex, int newIndex);
/**
* Inform the model we're done moving the node: it has been
* removed and added successfully
*/
virtual void nodeHasBeenMoved(KisNode * node, int oldIndex, int newIndex) = 0;
virtual void nodeHasBeenMoved(KisNode * node, int oldIndex, int newIndex);
virtual void nodeChanged(KisNode * node) = 0;
virtual void nodeChanged(KisNode * node);
/**
* Inform the model that a node has been changed (setDirty)
*/
virtual void requestProjectionUpdate(KisNode * node, const QRect& rect) = 0;
virtual void requestProjectionUpdate(KisNode * node, const QRect& rect);
/**
* Returns the sequence of the graph.
*
* Every time some operation performed, which might change the
* hierarchy of the nodes, the sequence number grows by one. So
* if you have any information about the graph which was acquired
* when the sequence number was X and now it has become Y, it
* means your information is outdated.
*
* It is used in the scheduler for checking whether queued walkers
* should be regenerated.
*/
int graphSequenceNumber() const;
private:
struct Private;
Private * const m_d;
};
#endif
......@@ -73,5 +73,40 @@ void KisNodeGraphListenerTest::testRecursiveUpdateOfListener()
QCOMPARE(child2->graphListener(), &listener);
}
void KisNodeGraphListenerTest::testSequenceNumber()
{
KisNodeFacade nodeFacade;
TestUtil::TestGraphListener listener;
KisNodeSP rootNode = new TestNode();
KisNodeSP child1 = new TestNode();
KisNodeSP child2 = new TestNode();
nodeFacade.setRoot(rootNode);
rootNode->setGraphListener(&listener);
int seqno = 0;
seqno = listener.graphSequenceNumber();
nodeFacade.addNode(child1, rootNode);
QVERIFY(seqno != listener.graphSequenceNumber());
seqno = listener.graphSequenceNumber();
nodeFacade.addNode(child2, rootNode);
QVERIFY(seqno != listener.graphSequenceNumber());
seqno = listener.graphSequenceNumber();
nodeFacade.moveNode(child1, rootNode, child2);
QVERIFY(seqno != listener.graphSequenceNumber());
seqno = listener.graphSequenceNumber();
nodeFacade.removeNode(child1);
QVERIFY(seqno != listener.graphSequenceNumber());
seqno = listener.graphSequenceNumber();
nodeFacade.removeNode(child2);
QVERIFY(seqno != listener.graphSequenceNumber());
}
QTEST_KDEMAIN(KisNodeGraphListenerTest, NoGUI)
#include "kis_node_graph_listener_test.moc"
......@@ -49,6 +49,7 @@ private slots:
void testUpdateOfListener();
void testRecursiveUpdateOfListener();
void testSequenceNumber();
};
#endif
......@@ -1050,7 +1050,7 @@ void KisWalkersTest::testMasksOverlapping()
+----------+
*/
void KisWalkersTest::testChecksum()
void KisWalkersTest::testRectsChecksum()
{
QRect imageRect(0,0,512,512);
QRect dirtyRect(100,100,100,100);
......@@ -1098,6 +1098,50 @@ void KisWalkersTest::testChecksum()
}
void KisWalkersTest::testGraphStructureChecksum()
{
QRect imageRect(0,0,512,512);
QRect dirtyRect(100,100,100,100);
const KoColorSpace * colorSpace = KoColorSpaceRegistry::instance()->rgb8();
KisImageSP image = new KisImage(0, imageRect.width(), imageRect.height(), colorSpace, "walker test");
KisLayerSP paintLayer1 = new KisPaintLayer(image, "paint1", OPACITY_OPAQUE_U8);
KisLayerSP paintLayer2 = new KisPaintLayer(image, "paint2", OPACITY_OPAQUE_U8);
image->lock();
image->addNode(paintLayer1, image->rootLayer());
image->unlock();
KisMergeWalker walker(imageRect);
walker.collectRects(paintLayer1, dirtyRect);
QCOMPARE(walker.checksumValid(), true);
image->lock();
image->addNode(paintLayer2, image->rootLayer());
image->unlock();
QCOMPARE(walker.checksumValid(), false);
walker.recalculate(dirtyRect);
QCOMPARE(walker.checksumValid(), true);
image->lock();
image->moveNode(paintLayer1, image->rootLayer(), paintLayer2);
image->unlock();
QCOMPARE(walker.checksumValid(), false);
walker.recalculate(dirtyRect);
QCOMPARE(walker.checksumValid(), true);
image->lock();
image->removeNode(paintLayer1);
image->unlock();
QCOMPARE(walker.checksumValid(), false);
walker.recalculate(dirtyRect);
QCOMPARE(walker.checksumValid(), true);
}
QTEST_KDEMAIN(KisWalkersTest, NoGUI)
#include "kis_walkers_test.moc"
......@@ -244,7 +244,8 @@ private slots:
void testCachedVisiting();
void testMasksVisiting();
void testMasksOverlapping();
void testChecksum();
void testRectsChecksum();
void testGraphStructureChecksum();
private:
void verifyResult(KisBaseRectsWalker &walker, QStringList reference,
......
......@@ -235,40 +235,37 @@ class TestGraphListener : public KisNodeGraphListener
{
public:
virtual void aboutToAddANode(KisNode *, int) {
virtual void aboutToAddANode(KisNode *parent, int index) {
KisNodeGraphListener::aboutToAddANode(parent, index);
beforeInsertRow = true;
}
virtual void nodeHasBeenAdded(KisNode *, int) {
virtual void nodeHasBeenAdded(KisNode *parent, int index) {
KisNodeGraphListener::nodeHasBeenAdded(parent, index);
afterInsertRow = true;
}
virtual void aboutToRemoveANode(KisNode *, int) {
virtual void aboutToRemoveANode(KisNode *parent, int index) {
KisNodeGraphListener::aboutToRemoveANode(parent, index);
beforeRemoveRow = true;
}
virtual void nodeHasBeenRemoved(KisNode *, int) {
virtual void nodeHasBeenRemoved(KisNode *parent, int index) {
KisNodeGraphListener::nodeHasBeenRemoved(parent, index);
afterRemoveRow = true;
}
virtual void aboutToMoveNode(KisNode *, int, int) {
virtual void aboutToMoveNode(KisNode *parent, int oldIndex, int newIndex) {
KisNodeGraphListener::aboutToMoveNode(parent, oldIndex, newIndex);
beforeMove = true;
}
virtual void nodeHasBeenMoved(KisNode *, int, int) {
virtual void nodeHasBeenMoved(KisNode *parent, int oldIndex, int newIndex) {
KisNodeGraphListener::nodeHasBeenMoved(parent, oldIndex, newIndex);
afterMove = true;
}
virtual void nodeChanged(KisNode*) {
}
virtual void requestProjectionUpdate(KisNode *node, const QRect& rect) {
}
bool beforeInsertRow;
bool afterInsertRow;
bool beforeRemoveRow;
......
......@@ -56,16 +56,8 @@ public:
{
}
void aboutToAddANode(KisNode *, int) {}
void nodeHasBeenAdded(KisNode *, int) {}
void aboutToRemoveANode(KisNode *, int) {}
void nodeHasBeenRemoved(KisNode *, int) {}
void aboutToMoveNode(KisNode *, int, int) {}
void nodeHasBeenMoved(KisNode *, int, int) {}
void nodeChanged(KisNode*) {}
void requestProjectionUpdate(KisNode *node, const QRect& rect) {
Q_UNUSED(node);
KisNodeGraphListener::requestProjectionUpdate(node, rect);
QMutexLocker locker(&m_lock);
m_scratchPad->imageUpdated(rect);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment