Merge pull request #1368 from bhangart/refactor/image-pixmap-from-source

Compute a picture's displayed pixmap from its source

sources/qetgraphicsitem/diagramimageitem.cpp

@@ -206,14 +206,16 @@ void DiagramImageItem::setPixmap(const QPixmap &pixmap) {
 /**
 	@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
+	colours -- and show the picture computed from it. The displayed
-	own, set by the same undo command.
+	pixmap is never set on its own by an edit: an action that changed
+	one and not the other would be undone half-way.
 */
 void DiagramImageItem::setImageSource(const ImageSource &source)
 {
 	m_base_pixmap = source.base;
 	m_crop_rect = source.crop;
 	m_transparent_colors = source.colors;
+	setPixmap(computeDisplayPixmap(m_base_pixmap, m_crop_rect, m_transparent_colors));
 }
 
 QVariant DiagramImageItem::imageSourceVariant() const
@@ -1841,7 +1843,6 @@ void DiagramImageItem::replaceImage()
 		return;
 	}
 
-	const QPixmap oldPixmap = pixmap_;
 	const QPixmap newPixmap = QPixmap::fromImage(image);
 
 	// A wholesale replacement, not an edit of the existing image -- the
@@ -1852,11 +1853,7 @@ void DiagramImageItem::replaceImage()
 	const ImageSource oldSource = imageSource();
 	const ImageSource newSource{newPixmap, newPixmap.rect(), {}};
 
-	auto *undo = new QPropertyUndoCommand(this, "pixmap", oldPixmap, newPixmap);
+	pushImageSourceChange(tr("Remplacer une image"), oldSource, newSource);
-	undo->setText(tr("Remplacer une image"));
-	new QPropertyUndoCommand(this, "imageSource", QVariant::fromValue(oldSource),
-							 QVariant::fromValue(newSource), undo);
-	diagram()->undoStack().push(undo);
 }
 
 /**
@@ -1872,13 +1869,11 @@ void DiagramImageItem::mirror(bool horizontal)
 	if (!diagram() || diagram()->isReadOnly())
 		return;
 
-	const QPixmap oldPixmap = pixmap_;
 	const QTransform flip = horizontal ? QTransform(-1, 0, 0, 1, 0, 0) : QTransform(1, 0, 0, -1, 0, 0);
-	const QPixmap newPixmap = pixmap_.transformed(flip);
 
-	// The base is flipped the same way, to stay in sync with pixmap_ --
+	// The base is flipped; the displayed pixmap follows from it. The
-	// but the picked-colours list itself is left untouched: the actual
+	// picked-colours list itself is left untouched: the actual colour
-	// colour values don't change when the image is mirrored, only their
+	// 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.
 	const ImageSource oldSource = imageSource();
@@ -1899,27 +1894,10 @@ void DiagramImageItem::mirror(bool horizontal)
 		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);
+	pushImageSourceChange(horizontal ? tr("Miroir horizontal d'une image") : tr("Miroir vertical d'une image"),
-	undo->setText(horizontal ? tr("Miroir horizontal d'une image") : tr("Miroir vertical d'une image"));
+						  oldSource, newSource);
-	new QPropertyUndoCommand(this, "imageSource", QVariant::fromValue(oldSource),
-							 QVariant::fromValue(newSource), undo);
-	diagram()->undoStack().push(undo);
 }
 
-/**
-	@brief DiagramImageItem::setTransparentColor
-	Context-menu action: opens ImageTransparentColorDialog against
-	m_base_pixmap (the pristine source), pre-populated with whatever
-	colours and tolerance were remembered from a previous session --
-	both problems fixed together, since they had the same root cause:
-	passing pixmap_ (the already colour-keyed result) as if it were the
-	source, with nowhere to remember which colours produced it. Applies
-	the result the same way replaceImage() and mirror() do, through the
-	"pixmap" property, so undo/redo stays consistent across all three;
-	m_base_pixmap itself is deliberately left untouched here, since this
-	action only ever changes which colours are keyed out of it, not the
-	source those colours are keyed out of.
-*/
 /**
 	@brief DiagramImageItem::setTransparentColor
 	Context-menu action: opens ImageTransparentColorDialog against the
@@ -1927,12 +1905,12 @@ void DiagramImageItem::mirror(bool horizontal)
 	uncropped original -- picking a colour from a region that's already
 	been permanently cropped away would be picking a colour that isn't
 	even part of the image anymore. Pre-populated with whatever colours
-	and tolerance were remembered from a previous session. Applies the
+	and tolerance were remembered from a previous session. Only the
-	result the same way replaceImage() and mirror() do, through the
+	colour list changes; m_base_pixmap and m_crop_rect are deliberately
-	"pixmap" property, so undo/redo stays consistent across all three;
+	left untouched here, since this action only ever changes which
-	m_base_pixmap and m_crop_rect are deliberately left untouched here,
+	colours are keyed out, never the source region they're keyed out of.
-	since this action only ever changes which colours are keyed out,
+	The displayed pixmap is computed from the new list by
-	never the source region they're keyed out of.
+	setImageSource(), with the same colour keying the dialog previewed.
 */
 void DiagramImageItem::setTransparentColor()
 {
@@ -1949,11 +1927,22 @@ void DiagramImageItem::setTransparentColor()
 	ImageSource newSource = oldSource;
 	newSource.colors = dialog.pickedColors();
 
-	const QPixmap oldPixmap = pixmap_;
+	pushImageSourceChange(tr("Définir une couleur transparente"), oldSource, newSource);
-	const QPixmap newPixmap = dialog.resultPixmap();
+}
 
-	auto *undo = new QPropertyUndoCommand(this, "pixmap", oldPixmap, newPixmap);
+/**
-	undo->setText(tr("Définir une couleur transparente"));
+	@brief DiagramImageItem::pushImageSourceChange
+	One undo step, named @a text, that takes the picture from
+	@a oldSource to @a newSource. A plain QUndoCommand holds the property
+	change so that two identical actions in a row (two horizontal
+	mirrors, say) stay two steps: QPropertyUndoCommand::mergeWith() would
+	merge them, and that is right for a slider's ticks, not for two
+	separate menu actions.
+*/
+void DiagramImageItem::pushImageSourceChange(const QString &text, const ImageSource &oldSource,
+											 const ImageSource &newSource)
+{
+	auto *undo = new QUndoCommand(text);
 	new QPropertyUndoCommand(this, "imageSource", QVariant::fromValue(oldSource),
 							 QVariant::fromValue(newSource), undo);
 	diagram()->undoStack().push(undo);
@@ -1986,13 +1975,12 @@ void DiagramImageItem::crop()
 	@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
+	setImageSource() recomputes pixmap_ 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
+	pos() also needs adjusting, not just pixmap_: the new pixmap has a
-	via the "pixmap" undo command below) recomputes
+	new boundingRect(), but pos() itself
-	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
@@ -2024,30 +2012,26 @@ bool DiagramImageItem::applyCrop(const QRect &cropRect)
 	const QPointF cropCenterScene = mapToScene(newCropCenterInCurrentLocal);
 	const QPointF oldPos = pos();
 
-	const QPixmap oldPixmap = pixmap_;
-	const QPixmap newPixmap = computeDisplayPixmap(m_base_pixmap, newCropRect, m_transparent_colors);
 	const ImageSource oldSource = imageSource();
 	ImageSource newSource = oldSource;
 	newSource.crop = newCropRect;
 
-	// boundingRect() is exactly QRectF(pixmap_.rect()) (confirmed by
+	// The new pixmap is the kept region, so its rectangle is known
-	// reading the actual implementation, not assumed) -- so the new
+	// before it is computed: imageRect() will be newCropRect's size at
-	// origin point can be derived directly from newPixmap here. Unlike
+	// (0,0). Unlike before this class gained a proper transform,
-	// before this class gained a proper transform, setPixmap() no
+	// setPixmap() no longer manages the pivot automatically at all (see
-	// longer manages the pivot automatically at all (see its own
+	// its own comment for why) -- the crop has to set it explicitly
-	// comment for why) -- crop() now has to set it explicitly itself,
+	// itself, chained into the same undo step as the source and position
-	// chained into the same undo step as the pixmap and position
 	// changes, since all three genuinely change together here.
 	const QPointF oldPivot = m_transform.pivot;
-	const QPointF newOriginPoint = QRectF(newPixmap.rect()).center();
+	const QPointF newOriginPoint = QRectF(QPointF(), QSizeF(newCropRect.size())).center();
 	const QPointF newPos = cropCenterScene - newOriginPoint;
 
-	auto *undo = new QPropertyUndoCommand(this, "pixmap", oldPixmap, newPixmap);
+	auto *undo = new QUndoCommand(tr("Rogner une image"));
-	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);
+	new QPropertyUndoCommand(this, "pos", oldPos, newPos, undo);
+	new QPropertyUndoCommand(this, "rawPivot", oldPivot, newOriginPoint, undo);
 	m_pivotIsCustom = false;
 	diagram()->undoStack().push(undo);
 	return true;

sources/qetgraphicsitem/diagramimageitem.h

@@ -50,8 +50,9 @@ class DiagramImageItem : public QetGraphicsItem {
 	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
+	// colours -- as one value. The displayed pixmap is computed from it,
-	// key, mirror, replace) is undone together with the displayed pixmap.
+	// so every edit of it (crop, colour key, mirror, replace) is one
+	// undo step on this property alone.
 	Q_PROPERTY(QVariant imageSource READ imageSourceVariant WRITE setImageSourceVariant)
 	Q_PROPERTY(bool adaptToDarkTheme READ adaptToDarkTheme WRITE setAdaptToDarkTheme NOTIFY adaptToDarkThemeChanged)
 	// A second, deliberately non-compensating property on the SAME
@@ -174,6 +175,7 @@ class DiagramImageItem : public QetGraphicsItem {
 	void mirror(bool horizontal);
 	void setTransparentColor();
 	void crop();
+	void pushImageSourceChange(const QString &text, const ImageSource &oldSource, const ImageSource &newSource);
 	void restoreAspectRatio();
 	void saveImageAs();
 	void saveOriginalImageAs();

tests/qttest/tst_imagecropundo.cpp

@@ -25,6 +25,7 @@
 #include <QProcess>
 #include <QProcessEnvironment>
 #include <QRegularExpression>
+#include <QSize>
 #include <QTemporaryDir>
 
 /**
@@ -77,6 +78,16 @@ class tst_imagecropundo : public QObject
 		QFile f(path);
 		return f.open(QIODevice::ReadOnly) ? f.readAll() : QByteArray();
 	}
+	// The size of the picture as shown, from the <image> element's own
+	// text (its first text node: a cropped picture carries <image_base>
+	// as a child too).
+	static QSize shownSize(const QByteArray &xml)
+	{
+		const QRegularExpression image(QStringLiteral("<image\\b[^>]*>\\s*([A-Za-z0-9+/=]+)"));
+		const QRegularExpressionMatch m = image.match(QString::fromUtf8(xml));
+		if (!m.hasMatch()) return {};
+		return QImage::fromData(QByteArray::fromBase64(m.captured(1).toLatin1())).size();
+	}
 
 private slots:
 	void initTestCase()
@@ -125,6 +136,9 @@ qet.log('PROBE ' + JSON.stringify(r));
 		const QByteArray undone_xml = read(undone);
 		QVERIFY(undone_xml.contains("<image "));
 		QVERIFY2(!undone_xml.contains("<crop "), "an undone crop was saved");
+		// The picture shown follows: whole after undo, the kept region after redo.
+		QCOMPARE(shownSize(undone_xml), QSize(40, 30));
+		QCOMPARE(shownSize(read(redone)), QSize(20, 10));
 		const QRegularExpressionMatch crop =
 				QRegularExpression(QStringLiteral("<crop ([^>]*)/>")).match(QString::fromUtf8(read(redone)));
 		QVERIFY2(crop.hasMatch(), "the redone crop was not saved");