From 0513344b92533fe5690978cfa9fdbbed946ef317 Mon Sep 17 00:00:00 2001 From: Beat Hangartner Date: Mon, 5 Oct 2026 13:01:30 +0200 Subject: [PATCH] Address review: drop dummy undo children, applyCrop() returns bool - replaceImage(), mirror() and setTransparentColor() no longer add a dummy QUndoCommand child: the imageSource child already keeps QPropertyUndoCommand::mergeWith() from merging two of them. - The explanation of the position and pivot maths moves from crop() to applyCrop(), where that code now lives; the stale older doc block of crop() goes. - applyCrop() returns false when nothing was cropped, and qet.cropImage() passes that on. tst_imagecropundo checks it for the current crop, an empty rectangle and one outside the picture. Co-Authored-By: Claude Opus 5.5 --- sources/qetgraphicsitem/diagramimageitem.cpp | 94 ++++++-------------- sources/qetgraphicsitem/diagramimageitem.h | 2 +- sources/scripting/qetscriptapi.cpp | 6 +- tests/qttest/tst_imagecropundo.cpp | 8 ++ 4 files changed, 40 insertions(+), 70 deletions(-) diff --git a/sources/qetgraphicsitem/diagramimageitem.cpp b/sources/qetgraphicsitem/diagramimageitem.cpp index 4632e267c..00f8b030b 100644 --- a/sources/qetgraphicsitem/diagramimageitem.cpp +++ b/sources/qetgraphicsitem/diagramimageitem.cpp @@ -1845,15 +1845,6 @@ void DiagramImageItem::replaceImage() undo->setText(tr("Remplacer une image")); new QPropertyUndoCommand(this, "imageSource", QVariant::fromValue(oldSource), QVariant::fromValue(newSource), undo); - // Every call here is a separate, deliberate menu action with no - // compound child of its own (unlike crop(), which always chains a - // pos/rawPivot change and is naturally immune) -- two of them in a - // row would carry the exact same object, property, and text, which - // is indistinguishable from a legitimate merge to - // QPropertyUndoCommand::mergeWith(). A dummy child (already treated - // as "never merge" by that check) keeps each one its own, separate - // undo step regardless. - new QUndoCommand(undo); diagram()->undoStack().push(undo); } @@ -1901,11 +1892,6 @@ void DiagramImageItem::mirror(bool horizontal) undo->setText(horizontal ? tr("Miroir horizontal d'une image") : tr("Miroir vertical d'une image")); new QPropertyUndoCommand(this, "imageSource", QVariant::fromValue(oldSource), QVariant::fromValue(newSource), undo); - // See replaceImage()'s identical comment: a separate, deliberate - // action with no compound child of its own, so a dummy one is - // needed to stop two consecutive same-direction mirrors (identical - // object, property, and text) from silently merging into one. - new QUndoCommand(undo); diagram()->undoStack().push(undo); } @@ -1959,34 +1945,9 @@ void DiagramImageItem::setTransparentColor() undo->setText(tr("Définir une couleur transparente")); new QPropertyUndoCommand(this, "imageSource", QVariant::fromValue(oldSource), QVariant::fromValue(newSource), undo); - // See replaceImage()'s identical comment: a separate, deliberate - // action with no compound child of its own, so a dummy one is - // needed to stop two consecutive transparency edits (identical - // object, property, and text) from silently merging into one. - new QUndoCommand(undo); diagram()->undoStack().push(undo); } -/** - @brief DiagramImageItem::crop - Context-menu action: opens ImageCropDialog against pixmap_ (the - current, already colour-keyed display, so cropping is WYSIWYG - against whatever is actually visible), then applies the chosen - rectangle to both pixmap_ and m_base_pixmap together -- kept in - sync the same way mirror() keeps them in sync, since cropping is a - permanent, geometric change to the image's own content, unlike - setTransparentColor()'s non-destructive colour keying. - - pos() also needs adjusting, not just pixmap_: setPixmap() (called - via the "pixmap" undo command below) recomputes - transformOriginPoint() from the new, smaller boundingRect(), but - pos() itself is untouched by that -- without fixing it up here too, - the surviving content would visually jump to wherever local (0,0) - happens to land after shrinking, rather than staying exactly where - it already was. Chained into one undo step together with the pixmap - change, since undoing a crop has to restore both, or the restored - (larger) image ends up in the wrong place. -*/ /** @brief DiagramImageItem::crop Context-menu action: opens ImageCropDialog against m_base_pixmap @@ -1995,27 +1956,7 @@ void DiagramImageItem::setTransparentColor() destructive: nothing about the original content is ever discarded, only which region of it is currently being shown, exactly the same principle setTransparentColor() already follows for its own choices. - Recomputes pixmap_ via computeDisplayPixmap() so any already-picked - transparent colours are correctly re-applied to the newly-cropped - region, rather than lost (the crop dialog itself knows nothing - about them). - - pos() also needs adjusting, not just pixmap_: setPixmap() (called - via the "pixmap" undo command below) recomputes - transformOriginPoint() from the new boundingRect(), but pos() itself - is untouched by that -- without fixing it up here too, the - surviving content would visually jump to wherever local (0,0) ends - up after the crop rect changes, rather than staying exactly where - it already was. This has to work whether this is the first crop - ever applied or an adjustment of an existing one, so the position - math is always done relative to the CURRENT crop rect (m_crop_rect, - before it's updated below) -- when there's no previous crop, that's - simply the whole base, which is what the very first version of this - method assumed unconditionally. - - Chained into one undo step together with the pixmap change, since - undoing a crop has to restore both, or the restored (larger) image - ends up in the wrong place. + The crop itself is done by applyCrop(). */ void DiagramImageItem::crop() { @@ -2032,18 +1973,36 @@ void DiagramImageItem::crop() /** @brief DiagramImageItem::applyCrop - Show @a newCropRect of the original (in the original's own pixels), - keeping the centre of the kept region where it is on the folio. One - undo step, which restores the pixmap, the position, the pivot and the - crop rectangle together. + Show @a cropRect of the original (in the original's own pixels), + keeping the centre of the kept region where it is on the folio. + Recomputes pixmap_ via computeDisplayPixmap() so any already-picked + transparent colours are correctly re-applied to the newly-cropped + region, rather than lost. + + pos() also needs adjusting, not just pixmap_: setPixmap() (called + via the "pixmap" undo command below) recomputes + transformOriginPoint() from the new boundingRect(), but pos() itself + is untouched by that -- without fixing it up here too, the + surviving content would visually jump to wherever local (0,0) ends + up after the crop rect changes, rather than staying exactly where + it already was. This has to work whether this is the first crop + ever applied or an adjustment of an existing one, so the position + math is always done relative to the CURRENT crop rect (m_crop_rect, + before it's updated below) -- when there's no previous crop, that's + simply the whole base. + + One undo step, which restores the pixmap, the position, the pivot + and the crop rectangle together. + @return false if nothing was cropped: read-only folio, or a + rectangle that is empty, outside the original, or the current one. */ -void DiagramImageItem::applyCrop(const QRect &cropRect) +bool DiagramImageItem::applyCrop(const QRect &cropRect) { if (!diagram() || diagram()->isReadOnly()) - return; + return false; const QRect newCropRect = cropRect.intersected(m_base_pixmap.rect()); if (newCropRect.isEmpty() || newCropRect == m_crop_rect) - return; // nothing actually changed + return false; // nothing actually changed // newCropRect is in m_base_pixmap's own coordinates; converting its // center into the CURRENT local space (pixmap_'s own coordinates, @@ -2080,4 +2039,5 @@ void DiagramImageItem::applyCrop(const QRect &cropRect) QVariant::fromValue(newSource), undo); m_pivotIsCustom = false; diagram()->undoStack().push(undo); + return true; } diff --git a/sources/qetgraphicsitem/diagramimageitem.h b/sources/qetgraphicsitem/diagramimageitem.h index b75389c64..b3263f9b3 100644 --- a/sources/qetgraphicsitem/diagramimageitem.h +++ b/sources/qetgraphicsitem/diagramimageitem.h @@ -81,7 +81,7 @@ class DiagramImageItem : public QetGraphicsItem { QVariant imageSourceVariant() const; void setImageSourceVariant(const QVariant &source); QRect cropRect() const { return m_crop_rect; } - void applyCrop(const QRect &cropRect); + bool applyCrop(const QRect &cropRect); // attributes public: diff --git a/sources/scripting/qetscriptapi.cpp b/sources/scripting/qetscriptapi.cpp index 380463d93..7bfbaa344 100644 --- a/sources/scripting/qetscriptapi.cpp +++ b/sources/scripting/qetscriptapi.cpp @@ -3474,6 +3474,9 @@ bool QetScriptApi::setImageRotation(int folioIndex, int imageIndex, double angle Show only the rectangle (x, y, width, height) of the image's original, in the original's own pixels, as the crop tool does: one undo step, the kept region staying where it is on the folio. + @return false if nothing was cropped: no such image, read-only + project, or a rectangle that is empty, outside the original, or the + current crop. */ bool QetScriptApi::cropImage(int folioIndex, int imageIndex, int x, int y, int width, int height) { @@ -3488,8 +3491,7 @@ bool QetScriptApi::cropImage(int folioIndex, int imageIndex, int x, int y, int w .arg(folioIndex).arg(list.count()).arg(imageIndex)); return false; } - list.at(imageIndex)->applyCrop(QRect(x, y, width, height)); - return true; + return list.at(imageIndex)->applyCrop(QRect(x, y, width, height)); } /** diff --git a/tests/qttest/tst_imagecropundo.cpp b/tests/qttest/tst_imagecropundo.cpp index 403c3dd59..5ea009e82 100644 --- a/tests/qttest/tst_imagecropundo.cpp +++ b/tests/qttest/tst_imagecropundo.cpp @@ -97,6 +97,9 @@ var i = qet.addImage(0, '%1', 100, 100); var r = {full: qet.imageCrop(0, i)}; r.cropped_ok = qet.cropImage(0, i, 10, 5, 20, 10); r.cropped = qet.imageCrop(0, i); +r.same_ok = qet.cropImage(0, i, 10, 5, 20, 10); +r.empty_ok = qet.cropImage(0, i, 10, 5, 0, 10); +r.outside_ok = qet.cropImage(0, i, 100, 100, 20, 10); qet.undo(); r.undone = qet.imageCrop(0, i); qet.save('%2'); @@ -111,6 +114,11 @@ qet.log('PROBE ' + JSON.stringify(r)); QCOMPARE(r.value("full").toString(), QStringLiteral("0,0,40,30")); QVERIFY(r.value("cropped_ok").toBool()); QCOMPARE(r.value("cropped").toString(), QStringLiteral("10,5,20,10")); + // Crops that change nothing report it, and push no undo step: + // the undo below still undoes the real crop. + QCOMPARE(r.value("same_ok").toBool(true), false); + QCOMPARE(r.value("empty_ok").toBool(true), false); + QCOMPARE(r.value("outside_ok").toBool(true), false); QCOMPARE(r.value("undone").toString(), QStringLiteral("0,0,40,30")); QCOMPARE(r.value("redone").toString(), QStringLiteral("10,5,20,10"));