Undo drawing in QGraphicsScene - rate my solution

JeKK666

I was testing the Scribble example and i wanted to add a MSPaint-like undo functionality for each line drawn by the user.
So i jumped to reading about the Undo framework and the command pattern, while also skimming through the Undo example, and got a more or less clear idea of how things should have been done.

Thing is, when i went ahead and tried mixing the two things, i quite immediately hit a stop: while it's clear that the function performing the coloring of a pixel should become my redo() method, i understand i have to provide the code that will reverse such behavior, to create an appropriate undo() method.

I concluded (might be wrong, please confirm) that to properly undo a drawing operation, the correct way would be to save the pixel color before applying the new color on top of it, so when i call undo() said color will be restored.

At this point though, i felt the Undo framework to be more cumbersome than needed, so i did this: modified the drawLineTo() method to return the handle of the QGraphicsPath it creates, store it in a list of all the modified pixels, and for each new line drawn add that list to another list, like so:

QList<QList<QGraphicsPathItem*>> undoList;
QList<QGraphicsPathItem*> pixelList;
int32_t listIndex = 0; 
...
void ScribbleGraphView::mousePressEvent(QMouseEvent *event) {
    if (event->button() == Qt::LeftButton) {
        lastPoint = event->pos();
        scribbling = true;
        pixelList.clear();
    }
}

void ScribbleGraphView::mouseMoveEvent(QMouseEvent *event) {
    if ((event->buttons() & Qt::LeftButton) && scribbling)
        pixelList.append(drawLineTo(event->pos()));
}

void ScribbleGraphView::mouseReleaseEvent(QMouseEvent *event) {
    if (event->button() == Qt::LeftButton && scribbling) {
        pixelList.append(drawLineTo(event->pos()));
        scribbling = false;
        undoList.append(pixelList);
        listIndex++;
    }
}

At this point my undo function was trivial: just QGraphicsScene::remove() all the QGraphicsPath that make up the last element of undoList, then then delete said nested list and decrement the list index by one.

It works just fine, but i wonder, is it any good? Suggestions, improvements, memory optimization?
Did i reinvent the wheel while i should i have just used QUndo from the start?

JonB

@JeKK666 said in Undo drawing in QGraphicsScene - rate my solution:

Thing is, when i went ahead and tried mixing the two things, i quite immediately hit a stop: while it's clear that the function performing the coloring of a pixel should become my redo() method

This is indeed the case for Qt's Undo framework. Takes a bit of getting used to, but this is required.

i understand i have to provide the code that will reverse such behavior, to create an appropriate undo() method.

Indeed again.

The Undo framework is just a convenience. What you have done is OK. Just that the Undo framework does much the same, gives you a "formal" way to do, hooks to menu items, offers Undo and Redo items whose enablement is tied to Undo stack state etc.

You maintain your own pixelList. Undo framework essentially does the same, except that each item you keep in that list instead goes into an individual record separately and Undo framework maintains those in a list.

JeKK666

@JonB Thanks for your feedback :)