From 5cbd65c1d43d741bf703de3a454f450d1e80f9f9 Mon Sep 17 00:00:00 2001 From: Beat Hangartner Date: Sun, 4 Oct 2026 19:45:03 +0200 Subject: [PATCH 1/3] Undo the crop, colours and original of a picture with its pixels Cropping, keying out a colour, mirroring and replacing a picture set its crop rectangle, transparent colours and original directly and put only the displayed pixmap into the undo command. After Ctrl+Z the picture looked right, but a save still wrote the undone crop and colours, and the picture came back cropped once reopened; a later crop or colour dialog also started from the undone values. The three are now one property, imageSource, changed in the same undo step as the pixmap. The crop itself moves out of the dialog into applyCrop(), so that it can be applied without one. Co-Authored-By: Claude Opus 5.5 --- sources/qetgraphicsitem/diagramimageitem.cpp | 69 +++++++++++++++++--- sources/qetgraphicsitem/diagramimageitem.h | 19 ++++++ 2 files changed, 79 insertions(+), 9 deletions(-) diff --git a/sources/qetgraphicsitem/diagramimageitem.cpp b/sources/qetgraphicsitem/diagramimageitem.cpp index ac82aa5bd..4632e267c 100644 --- a/sources/qetgraphicsitem/diagramimageitem.cpp +++ b/sources/qetgraphicsitem/diagramimageitem.cpp @@ -194,6 +194,30 @@ void DiagramImageItem::setPixmap(const QPixmap &pixmap) { emit pixmapChanged(); } +/** + @brief DiagramImageItem::setImageSource + Set the picture's source -- original, crop rectangle and transparent + colours. Only stores them: the displayed pixmap is a property of its + own, set by the same undo command. +*/ +void DiagramImageItem::setImageSource(const ImageSource &source) +{ + m_base_pixmap = source.base; + m_crop_rect = source.crop; + m_transparent_colors = source.colors; +} + +QVariant DiagramImageItem::imageSourceVariant() const +{ + return QVariant::fromValue(imageSource()); +} + +void DiagramImageItem::setImageSourceVariant(const QVariant &source) +{ + if (source.canConvert()) + setImageSource(source.value()); +} + /** @brief DiagramImageItem::setScaleFactorX / setScaleFactorY / setRotationAngle Matching QetShapeItem's own setters for the identical fields -- @@ -1814,12 +1838,13 @@ void DiagramImageItem::replaceImage() // or region was cropped for the old one, so this starts that // memory fresh rather than carrying over choices that would no // longer make sense. - m_base_pixmap = newPixmap; - m_crop_rect = newPixmap.rect(); - m_transparent_colors.clear(); + const ImageSource oldSource = imageSource(); + const ImageSource newSource{newPixmap, newPixmap.rect(), {}}; auto *undo = new QPropertyUndoCommand(this, "pixmap", oldPixmap, newPixmap); 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 @@ -1854,7 +1879,9 @@ void DiagramImageItem::mirror(bool horizontal) // colour values don't change when the image is mirrored, only their // positions, so whatever was already keyed transparent should stay // remembered and still apply correctly to the flipped version. - m_base_pixmap = m_base_pixmap.transformed(flip); + const ImageSource oldSource = imageSource(); + ImageSource newSource = oldSource; + newSource.base = m_base_pixmap.transformed(flip); // m_crop_rect, unlike the colour list, DOES need to change: it's // defined in terms of positions within the base, and those @@ -1864,14 +1891,16 @@ void DiagramImageItem::mirror(bool horizontal) // whether this reads m_base_pixmap's size from before or after the // assignment above). if (horizontal) - m_crop_rect = QRect(m_base_pixmap.width() - m_crop_rect.left() - m_crop_rect.width(), + newSource.crop = QRect(m_base_pixmap.width() - m_crop_rect.left() - m_crop_rect.width(), m_crop_rect.top(), m_crop_rect.width(), m_crop_rect.height()); else - m_crop_rect = QRect(m_crop_rect.left(), m_base_pixmap.height() - m_crop_rect.top() - m_crop_rect.height(), + newSource.crop = QRect(m_crop_rect.left(), m_base_pixmap.height() - m_crop_rect.top() - m_crop_rect.height(), m_crop_rect.width(), m_crop_rect.height()); auto *undo = new QPropertyUndoCommand(this, "pixmap", oldPixmap, newPixmap); 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 @@ -1919,13 +1948,17 @@ void DiagramImageItem::setTransparentColor() if (dialog.exec() != QDialog::Accepted) return; - m_transparent_colors = dialog.pickedColors(); + const ImageSource oldSource = imageSource(); + ImageSource newSource = oldSource; + newSource.colors = dialog.pickedColors(); const QPixmap oldPixmap = pixmap_; const QPixmap newPixmap = dialog.resultPixmap(); auto *undo = new QPropertyUndoCommand(this, "pixmap", oldPixmap, newPixmap); 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 @@ -1994,7 +2027,21 @@ void DiagramImageItem::crop() if (dialog.exec() != QDialog::Accepted) return; - const QRect newCropRect = dialog.cropRect(); + applyCrop(dialog.cropRect()); +} + +/** + @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. +*/ +void DiagramImageItem::applyCrop(const QRect &cropRect) +{ + if (!diagram() || diagram()->isReadOnly()) + return; + const QRect newCropRect = cropRect.intersected(m_base_pixmap.rect()); if (newCropRect.isEmpty() || newCropRect == m_crop_rect) return; // nothing actually changed @@ -2009,7 +2056,9 @@ void DiagramImageItem::crop() const QPixmap oldPixmap = pixmap_; const QPixmap newPixmap = computeDisplayPixmap(m_base_pixmap, newCropRect, m_transparent_colors); - m_crop_rect = newCropRect; + const ImageSource oldSource = imageSource(); + ImageSource newSource = oldSource; + newSource.crop = newCropRect; // boundingRect() is exactly QRectF(pixmap_.rect()) (confirmed by // reading the actual implementation, not assumed) -- so the new @@ -2027,6 +2076,8 @@ void DiagramImageItem::crop() undo->setText(tr("Rogner une image")); new QPropertyUndoCommand(this, "pos", oldPos, newPos, undo); new QPropertyUndoCommand(this, "rawPivot", oldPivot, newOriginPoint, undo); + new QPropertyUndoCommand(this, "imageSource", QVariant::fromValue(oldSource), + QVariant::fromValue(newSource), undo); m_pivotIsCustom = false; diagram()->undoStack().push(undo); } diff --git a/sources/qetgraphicsitem/diagramimageitem.h b/sources/qetgraphicsitem/diagramimageitem.h index 570d06770..b75389c64 100644 --- a/sources/qetgraphicsitem/diagramimageitem.h +++ b/sources/qetgraphicsitem/diagramimageitem.h @@ -49,6 +49,10 @@ class DiagramImageItem : public QetGraphicsItem { Q_PROPERTY(qreal skewY READ skewY WRITE setSkewY NOTIFY transformChanged) Q_PROPERTY(QPointF pivot READ pivot WRITE setPivot NOTIFY transformChanged) Q_PROPERTY(QString label READ label WRITE setLabel NOTIFY labelChanged) + // The picture's source -- original, crop rectangle, transparent + // colours -- as one value, so that every edit of it (crop, colour + // key, mirror, replace) is undone together with the displayed pixmap. + Q_PROPERTY(QVariant imageSource READ imageSourceVariant WRITE setImageSourceVariant) // A second, deliberately non-compensating property on the SAME // underlying value -- setPivot() (above) intentionally adjusts // pos() to keep the image visually in place, which is exactly @@ -65,6 +69,19 @@ class DiagramImageItem : public QetGraphicsItem { DiagramImageItem(QetGraphicsItem * = nullptr); DiagramImageItem(const QPixmap &pixmap, QetGraphicsItem * = nullptr); ~DiagramImageItem() override; + + struct ImageSource + { + QPixmap base; + QRect crop; + QList colors; + }; + ImageSource imageSource() const { return {m_base_pixmap, m_crop_rect, m_transparent_colors}; } + void setImageSource(const ImageSource &source); + QVariant imageSourceVariant() const; + void setImageSourceVariant(const QVariant &source); + QRect cropRect() const { return m_crop_rect; } + void applyCrop(const QRect &cropRect); // attributes public: @@ -246,4 +263,6 @@ class DiagramImageItem : public QetGraphicsItem { QPointF m_label_scale{1.0, 1.0}; // scale the label rect was last computed for -- see updateLabelScale() bool m_resizeCenterAnchored = false; // decided once, at press time -- see handlerMousePressEvent()'s comment for why, mirroring the identical fix already made for shape creation }; +Q_DECLARE_METATYPE(DiagramImageItem::ImageSource) + #endif From 05b71e07a08d13b45a32323214d388d099030070 Mon Sep 17 00:00:00 2001 From: Beat Hangartner Date: Sun, 4 Oct 2026 19:45:03 +0200 Subject: [PATCH 2/3] Add qet.cropImage() and qet.imageCrop() to the script API Crop a picture as the crop tool does, in one undo step, and read its crop rectangle back. tst_imagecropundo uses them on the real binary: a crop that was undone is not saved, and redoing it saves it again. Co-Authored-By: Claude Opus 5.5 --- sources/scripting/qetscriptapi.cpp | 36 ++++++++ sources/scripting/qetscriptapi.h | 2 + tests/qttest/CMakeLists.txt | 12 +++ tests/qttest/tst_imagecropundo.cpp | 129 +++++++++++++++++++++++++++++ 4 files changed, 179 insertions(+) create mode 100644 tests/qttest/tst_imagecropundo.cpp diff --git a/sources/scripting/qetscriptapi.cpp b/sources/scripting/qetscriptapi.cpp index 0de2b7497..380463d93 100644 --- a/sources/scripting/qetscriptapi.cpp +++ b/sources/scripting/qetscriptapi.cpp @@ -3469,6 +3469,42 @@ bool QetScriptApi::setImageRotation(int folioIndex, int imageIndex, double angle return true; } +/** + @brief QetScriptApi::cropImage + 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. +*/ +bool QetScriptApi::cropImage(int folioIndex, int imageIndex, int x, int y, int width, int height) +{ + if (!m_project) return false; + if (m_project->isReadOnly()) { + log(QStringLiteral("qet.cropImage: project is read-only")); + return false; + } + const QList list = sortedImages(folioIndex); + if (imageIndex < 0 || imageIndex >= list.count()) { + log(QStringLiteral("qet.cropImage: folio %1 has %2 image(s), no index %3") + .arg(folioIndex).arg(list.count()).arg(imageIndex)); + return false; + } + list.at(imageIndex)->applyCrop(QRect(x, y, width, height)); + return true; +} + +/** + @brief QetScriptApi::imageCrop + @return the image's crop rectangle in its original's pixels, as + "x,y,width,height", or an empty string for no such image. +*/ +QString QetScriptApi::imageCrop(int folioIndex, int imageIndex) const +{ + const QList list = sortedImages(folioIndex); + if (imageIndex < 0 || imageIndex >= list.count()) return QString(); + const QRect r = list.at(imageIndex)->cropRect(); + return QStringLiteral("%1,%2,%3,%4").arg(r.x()).arg(r.y()).arg(r.width()).arg(r.height()); +} + bool QetScriptApi::deleteImage(int folioIndex, int imageIndex) { if (!m_project) return false; diff --git a/sources/scripting/qetscriptapi.h b/sources/scripting/qetscriptapi.h index 572168a0f..6a75b1b24 100644 --- a/sources/scripting/qetscriptapi.h +++ b/sources/scripting/qetscriptapi.h @@ -539,6 +539,8 @@ class QetScriptApi : public QObject Q_INVOKABLE int addImage(int folioIndex, const QString &filePath, double x, double y); Q_INVOKABLE bool setImageScale(int folioIndex, int imageIndex, double factor); Q_INVOKABLE bool setImageRotation(int folioIndex, int imageIndex, double angle); + Q_INVOKABLE bool cropImage(int folioIndex, int imageIndex, int x, int y, int width, int height); + Q_INVOKABLE QString imageCrop(int folioIndex, int imageIndex) const; Q_INVOKABLE bool deleteImage(int folioIndex, int imageIndex); Q_INVOKABLE int addPdfPage(int folioIndex, const QString &pdfPath, int pageNumber, int dpi, double x, double y); diff --git a/tests/qttest/CMakeLists.txt b/tests/qttest/CMakeLists.txt index 3b7e8c2b5..ec5c53a8b 100644 --- a/tests/qttest/CMakeLists.txt +++ b/tests/qttest/CMakeLists.txt @@ -593,6 +593,18 @@ if(QET_HAS_SCRIPTING) "QET_TEST_BINARY_PATH=\"$\"" "QET_EXAMPLES_DIR=\"${QET_DIR}/examples\"") + # Undoing a crop of a picture restores its crop rectangle too, so a + # project saved after the undo does not record the undone crop. + add_executable( + tst_imagecropundo + tst_imagecropundo.cpp) + add_test(NAME tst_imagecropundo COMMAND tst_imagecropundo) + add_dependencies(tst_imagecropundo qelectrotech) + target_link_libraries(tst_imagecropundo PRIVATE Qt::Test Qt::Gui) + target_compile_definitions(tst_imagecropundo PRIVATE + "QET_TEST_BINARY_PATH=\"$\"" + "QET_EXAMPLES_DIR=\"${QET_DIR}/examples\"") + # The same symbol placed twice in a project whose collection does not # start with its "import" category (examples/lmdg.qet). add_executable( diff --git a/tests/qttest/tst_imagecropundo.cpp b/tests/qttest/tst_imagecropundo.cpp new file mode 100644 index 000000000..403c3dd59 --- /dev/null +++ b/tests/qttest/tst_imagecropundo.cpp @@ -0,0 +1,129 @@ +/* + Copyright 2006-2026 The QElectroTech Team + This file is part of QElectroTech. + + QElectroTech 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. + + QElectroTech 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 QElectroTech. If not, see . +*/ +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +/** + Undoing a crop restores the crop rectangle too, not only the pixels + shown: before, a project saved after Ctrl+Z still recorded the undone + crop, and the picture came back cropped once reopened. Runs the real + binary on a script: add a picture, crop it, undo, save; redo, save. +*/ +class tst_imagecropundo : public QObject +{ + Q_OBJECT + + QTemporaryDir m_dir; + + QJsonObject run(const QString &script, const QString &project) + { + const QString path = m_dir.filePath(QStringLiteral("probe.js")); + const QString home = m_dir.filePath(QStringLiteral("home")); + QDir().mkpath(home); + QFile f(path); + if (!f.open(QIODevice::WriteOnly)) return {}; + f.write(script.toUtf8()); + f.close(); + + QProcessEnvironment env = QProcessEnvironment::systemEnvironment(); + env.insert(QStringLiteral("QT_QPA_PLATFORM"), QStringLiteral("offscreen")); + env.insert(QStringLiteral("QET_ENABLE_SCRIPTING"), QStringLiteral("1")); + env.insert(QStringLiteral("HOME"), home); + env.insert(QStringLiteral("XDG_CONFIG_HOME"), home + QStringLiteral("/config")); + env.insert(QStringLiteral("XDG_DATA_HOME"), home + QStringLiteral("/data")); + env.insert(QStringLiteral("TMPDIR"), m_dir.path()); + QProcess proc; + proc.setProcessEnvironment(env); + proc.start(QStringLiteral(QET_TEST_BINARY_PATH), + {QStringLiteral("--run"), path, project}); + if (!proc.waitForFinished(120000)) return {}; + const QString out = QString::fromUtf8(proc.readAllStandardOutput() + + proc.readAllStandardError()); + const QString mark = QStringLiteral("PROBE "); + for (const QString &line : out.split(QLatin1Char('\n'))) { + const int i = line.indexOf(mark); + if (i >= 0) + return QJsonDocument::fromJson(line.mid(i + mark.size()).toUtf8()).object(); + } + return {}; + } + + static QByteArray read(const QString &path) + { + QFile f(path); + return f.open(QIODevice::ReadOnly) ? f.readAll() : QByteArray(); + } + +private slots: + void initTestCase() + { + QVERIFY(m_dir.isValid()); + QVERIFY(QFile::exists(QStringLiteral(QET_TEST_BINARY_PATH))); + QImage img(40, 30, QImage::Format_RGB32); + img.fill(Qt::darkGreen); + QVERIFY(img.save(m_dir.filePath(QStringLiteral("pic.png")))); + } + + void undoneCropIsNotSaved() + { + const QString undone = m_dir.filePath(QStringLiteral("undone.qet")); + const QString redone = m_dir.filePath(QStringLiteral("redone.qet")); + const QString script = QStringLiteral(R"JS( +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); +qet.undo(); +r.undone = qet.imageCrop(0, i); +qet.save('%2'); +qet.redo(); +r.redone = qet.imageCrop(0, i); +qet.save('%3'); +qet.log('PROBE ' + JSON.stringify(r)); +)JS").arg(m_dir.filePath(QStringLiteral("pic.png")), undone, redone); + + const QJsonObject r = run(script, QStringLiteral(QET_EXAMPLES_DIR "/741.qet")); + QVERIFY2(!r.isEmpty(), "the script logged nothing"); + 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")); + QCOMPARE(r.value("undone").toString(), QStringLiteral("0,0,40,30")); + QCOMPARE(r.value("redone").toString(), QStringLiteral("10,5,20,10")); + + const QByteArray undone_xml = read(undone); + QVERIFY(undone_xml.contains("]*)/>")).match(QString::fromUtf8(read(redone))); + QVERIFY2(crop.hasMatch(), "the redone crop was not saved"); + for (const char *attribute : {R"(x="10")", R"(y="5")", R"(w="20")", R"(h="10")"}) + QVERIFY2(crop.captured(1).contains(QLatin1String(attribute)), attribute); + } +}; + +QTEST_GUILESS_MAIN(tst_imagecropundo) +#include "tst_imagecropundo.moc" From 0513344b92533fe5690978cfa9fdbbed946ef317 Mon Sep 17 00:00:00 2001 From: Beat Hangartner Date: Mon, 5 Oct 2026 13:01:30 +0200 Subject: [PATCH 3/3] 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"));