QUndoCommand Update UI

gsephelec

Hi,

What's the correct way to update the UI from a QUndoCommand?

I have a QImage which can be drawn on to using a custom QWidget class, it actually draws a 40x40 pixel rectangle but that's not important:

void assetEditor::drawPoint(const QPoint &scaledPoint)
{
    QPainter painter(&foregroundImage);

    /* Set the fill colour and no border */
    painter.setBrush(currentEditorState.paint.penColour);
    painter.setPen(QPen(currentEditorState.paint.penColour, currentEditorState.paint.penSize, Qt::NoPen, Qt::RoundCap, Qt::RoundJoin));

    /* Draw a 40x40 rectangle as the pixel */
    painter.drawRect( QRect( QPoint( (scaledPoint.x() * 40), (scaledPoint.y() * 40) ), QSize(40, 40) ) );
    update();
}

Then on the mouse release event, the undo command is called:

/* Scale the foreground down to the correct size to reduce storage on the undo stack */
QImage scaledDownForeground = scaleDownImage(foregroundImage);

undoStack->push(new assetEditorImageChange(currentEditorConfig.imageSize, &foregroundImage, scaledDownForeground, undoStoreImage, &currentEditorState, preUndoStoreEditorState, postUndoStoreEditorState));

Note the max size of an image will be 16x16 pixels, so I figured it won't kill the memory to store a copy of the image in the undo stack.

Undo command is then defined here:

assetEditorImageChange::assetEditorImageChange(
            int imageHeight,
            QImage *passForegroundImage,
            const QImage &newImageState,
            const QImage &oldImageState,
            assetEditor::editorState *passCurrentEditorConfig,
            const assetEditor::editorState &newConfigState,
            const assetEditor::editorState &oldConfigState,
            QUndoCommand *parent)
    : QUndoCommand(parent),
      editorImageHeight(imageHeight),
      foregroundImage(passForegroundImage),
      oldImage(oldImageState),
      newImage(newImageState),
      editorConfig(passCurrentEditorConfig),
      oldConfig(newConfigState),
      newConfig(oldConfigState),
{
}

QImage assetEditorImageChange::scaleUpImage(QImage originalImage) {

    QImage scaledUpImage;

    scaledUpImage = originalImage.scaledToHeight(( editorImageHeight * 40 ), Qt::FastTransformation);
    QPainter p(&scaledUpImage);
    p.drawImage(QPoint(0, 0), scaledUpImage);

    return scaledUpImage;
}

void assetEditorImageChange::undo()
{
    /* Clear the editor foreground */
    foregroundImage->fill(qRgba(0, 0, 0, 0));

    /* Scale up the image to restore */
    QImage scaledUp = scaleUpImage(oldImage);

    /* Restore the image */
    QPainter painter(foregroundImage);
    painter.drawImage(QPoint(0, 0), scaledUp);

    /* Revert the editor config back */
    memcpy((void*)editorConfig, (void*)&oldConfig, sizeof(assetEditor::editorState));

    /* Update the editor */
    qDebug() << "Call update here";
}

void assetEditorImageChange::redo()
{
    /* Clear the editor foreground */
    foregroundImage->fill(qRgba(0, 0, 0, 0));

    /* Scale up the image to restore */
    QImage scaledUp = scaleUpImage(newImage);

    /* Restore the image */
    QPainter painter(foregroundImage);
    painter.drawImage(QPoint(0, 0), scaledUp);

    /* Revert the editor config back */
    memcpy((void*)editorConfig, (void*)&newConfig, sizeof(assetEditor::editorState));

    /* Update the editor */
    qDebug() << "Call update here";
}

This all works fine if I create an undo action in mainWindow.cpp and call the undo function and the update function:

void MainWindow::on_actionUndo_triggered()
{
    /* Perform the undo commands */
    undoStack->undo();

    /* Update the UI */
    ui->imageEditor->update();
}

But the undo stack holds lots of commands that aren't related to the image editor, so it doesn't need to be updated for every undo/redo action.

What's the correct way to update the UI once an undo/redo command is called?

JonB

@gsephelec
If I understand you right: you have a custom QUndoCommand. That has members for a number of things you might need to save for undo/redo, including a "large" image. But not every undoable action needs to restore the image, and you would like to "save space" by not storing it when it won't be needed? (Really QImages are shared so space may not be as bad as you fear, but that's a different matter.)

So your QUndoCommand should only need to store those items it really needs to for its undo action. For example, you might store the QImage as a pointer to a newed object. Pass in nullptr where the image is not going to be needed. Test for whether the QUndoCommands QImage is nullptr before you attempt to restore it.

Another possibility is to subclass your assetEditorImageChange so that that it has derived classes to handle, say, image changes separately from config changes. Only the "image change" subclass holds a QImage. Push to the undo stack only whichever subclass is appropriate to the action in question. Now your stack holds subclassed objects which contain only whatever data they specifically need.

gsephelec

@JonB
The assetEditorImageChange command is only for changes to the image in the image editor, like drawing/erasing pixels. I have other QUndoCommands for different things as well, but the QUndoStack takes commands from the entire application, not just the image editor, so when you click the undo/redo button, you might be performing commands on the image editor, or somewhere else in the main window, in which case the image editor doesn't need to be updated - especially since in some situations, the image editor may not be shown, in which case it can't be updated.

JonB

@gsephelec
Then I can only guess you are asking to not always do ui->imageEditor->update(); after undoStack->undo();, is that the question? So, for example, make your undo() look at what it is undoing and return a result to the caller indicating whether the image editor still needs updating? Or, maybe, have the undo command which affects the image editor emit a signal/set a flag when it does so, so you will know the image needs updating?

gsephelec

@JonB

@JonB said in QUndoCommand Update UI:

@gsephelec
Then I can only guess you are asking to not always do ui->imageEditor->update(); after undoStack->undo();, is that the question?

Yes exactly :)

@JonB said in QUndoCommand Update UI:

@gsephelec
So, for example, make your undo() look at what it is undoing and return a result to the caller indicating whether the image editor still needs updating? Or, maybe, have the undo command which affects the image editor emit a signal/set a flag when it does so, so you will know the image needs updating?

Yes to both, which is the better practice and what would be the best way to implement it?

I did also look at passing an "update ui" function from within the assetEditor class to the QUndoCommand, but I couldn't get this to work properly and I wasn't sure if that was a messy way to get round my issue?