Merge pull request #1310 from bhangart/fix/image-crop-undo

Undo the crop, colours and original of a picture together with its pixels

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<ImageSource>())
+		setImageSource(source.value<ImageSource>());
+}
+
 /**
 	@brief DiagramImageItem::setScaleFactorX / setScaleFactorY / setRotationAngle
 	Matching QetShapeItem's own setters for the identical fields --
@@ -1814,21 +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"));
-	// 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);
+	new QPropertyUndoCommand(this, "imageSource", QVariant::fromValue(oldSource),
+							 QVariant::fromValue(newSource), undo);
 	diagram()->undoStack().push(undo);
 }
 
@@ -1854,7 +1870,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,19 +1882,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"));
-	// 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);
+	new QPropertyUndoCommand(this, "imageSource", QVariant::fromValue(oldSource),
+							 QVariant::fromValue(newSource), undo);
 	diagram()->undoStack().push(undo);
 }
 
@@ -1919,41 +1934,20 @@ 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"));
-	// 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);
+	new QPropertyUndoCommand(this, "imageSource", QVariant::fromValue(oldSource),
+							 QVariant::fromValue(newSource), 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
@@ -1962,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()
 {
@@ -1994,9 +1968,41 @@ void DiagramImageItem::crop()
 	if (dialog.exec() != QDialog::Accepted)
 		return;
 
-	const QRect newCropRect = dialog.cropRect();
+	applyCrop(dialog.cropRect());
+}
+
+/**
+	@brief DiagramImageItem::applyCrop
+	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.
+*/
+bool DiagramImageItem::applyCrop(const QRect &cropRect)
+{
+	if (!diagram() || diagram()->isReadOnly())
+		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,
@@ -2009,7 +2015,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 +2035,9 @@ 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);
+	return true;
 }

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<ImageTransparentColorDialog::PickedColor> 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; }
+	bool 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

sources/scripting/qetscriptapi.cpp

@@ -3475,6 +3475,44 @@ 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.
+	@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)
+{
+	if (!m_project) return false;
+	if (m_project->isReadOnly()) {
+		log(QStringLiteral("qet.cropImage: project is read-only"));
+		return false;
+	}
+	const QList<DiagramImageItem *> 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;
+	}
+	return list.at(imageIndex)->applyCrop(QRect(x, y, width, height));
+}
+
+/**
+	@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<DiagramImageItem *> 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;

sources/scripting/qetscriptapi.h

@@ -540,6 +540,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);

tests/qttest/CMakeLists.txt

@@ -593,6 +593,18 @@ if(QET_HAS_SCRIPTING)
     "QET_TEST_BINARY_PATH=\"$<TARGET_FILE:qelectrotech>\""
     "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=\"$<TARGET_FILE:qelectrotech>\""
+    "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(

tests/qttest/tst_imagecropundo.cpp

@@ -0,0 +1,137 @@
+/*
+	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 <http://www.gnu.org/licenses/>.
+*/
+#include <QtTest>
+
+#include <QDir>
+#include <QFile>
+#include <QImage>
+#include <QJsonDocument>
+#include <QJsonObject>
+#include <QProcess>
+#include <QProcessEnvironment>
+#include <QRegularExpression>
+#include <QTemporaryDir>
+
+/**
+	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);
+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');
+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"));
+		// 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"));
+
+		const QByteArray undone_xml = read(undone);
+		QVERIFY(undone_xml.contains("<image "));
+		QVERIFY2(!undone_xml.contains("<crop "), "an undone crop was saved");
+		const QRegularExpressionMatch crop =
+				QRegularExpression(QStringLiteral("<crop ([^>]*)/>")).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"