kotschak/qelectrotech-source-mirror
0
0
Fork 0
mirror of https://github.com/qelectrotech/qelectrotech-source-mirror.git synced 2026-10-07 20:34:14 +02:00
Code Issues Actions Packages Projects Releases Wiki Activity

Compute a picture's displayed pixmap from its source

Browse Source 
The displayed pixmap and imageSource (original, crop rectangle,
transparent colours) were two undo values that had to change together,
although the pixmap follows from the source. An action that updated
one and not the other would bring back the bug fixed in #1310.

setImageSource() now recomputes the displayed pixmap, and crop, colour
key, mirror and replace push one undo step on imageSource alone. A
plain QUndoCommand holds the property change, so two identical actions
in a row stay two steps. Loading a project still shows the saved pixmap
as before.

tst_imagecropundo also checks the size of the saved picture after undo
and after redo.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
Beat Hangartner
2026-10-07 06:25:14 +02:00
parent 0708338bce
commit 1b0ab6d26a
3 changed files with 62 additions and 62 deletions
Download Patch File Download Diff File Expand all files Collapse all files
sources/qetgraphicsitem/diagramimageitem.cpp
+44 -60
View File
@@ -197,14 +197,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
own, set by the same undo command.
colours -- and show the picture computed from it. The displayed
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
@@ -1830,7 +1832,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
@@ -1841,11 +1842,7 @@ void DiagramImageItem::replaceImage()
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);
diagram()->undoStack().push(undo);
pushImageSourceChange(tr("Remplacer une image"), oldSource, newSource);
}
/**
@@ -1861,13 +1858,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_ --
// but the picked-colours list itself is left untouched: the actual
// colour values don't change when the image is mirrored, only their
// The base is flipped; the displayed pixmap follows from it. The
// picked-colours list itself is left untouched: the actual 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.
const ImageSource oldSource = imageSource();
@@ -1888,27 +1883,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);
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);
diagram()->undoStack().push(undo);
pushImageSourceChange(horizontal ? tr("Miroir horizontal d'une image") : tr("Miroir vertical d'une image"),
oldSource, newSource);
}
/**
@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
@@ -1916,12 +1894,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
result the same way replaceImage() and mirror() do, through the
"pixmap" property, so undo/redo stays consistent across all three;
m_base_pixmap and m_crop_rect are deliberately left untouched here,
since this action only ever changes which colours are keyed out,
never the source region they're keyed out of.
and tolerance were remembered from a previous session. Only the
colour list changes; m_base_pixmap and m_crop_rect are deliberately
left untouched here, since this action only ever changes which
colours are keyed out, never the source region they're keyed out of.
The displayed pixmap is computed from the new list by
setImageSource(), with the same colour keying the dialog previewed.
*/
void DiagramImageItem::setTransparentColor()
{
@@ -1938,11 +1916,22 @@ void DiagramImageItem::setTransparentColor()
ImageSource newSource = oldSource;
newSource.colors = dialog.pickedColors();
const QPixmap oldPixmap = pixmap_;
const QPixmap newPixmap = dialog.resultPixmap();
pushImageSourceChange(tr("Définir une couleur transparente"), oldSource, newSource);
}
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);
@@ -1975,13 +1964,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
via the "pixmap" undo command below) recomputes
transformOriginPoint() from the new boundingRect(), but pos() itself
pos() also needs adjusting, not just pixmap_: the new pixmap has a
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
@@ -2013,30 +2001,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
// reading the actual implementation, not assumed) -- so the new
// origin point can be derived directly from newPixmap here. Unlike
// before this class gained a proper transform, setPixmap() no
// longer manages the pivot automatically at all (see its own
// comment for why) -- crop() now has to set it explicitly itself,
// chained into the same undo step as the pixmap and position
// The new pixmap is the kept region, so its rectangle is known
// before it is computed: imageRect() will be newCropRect's size at
// (0,0). Unlike before this class gained a proper transform,
// setPixmap() no longer manages the pivot automatically at all (see
// its own comment for why) -- the crop has to set it explicitly
// itself, chained into the same undo step as the source 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);
undo->setText(tr("Rogner une image"));
new QPropertyUndoCommand(this, "pos", oldPos, newPos, undo);
new QPropertyUndoCommand(this, "rawPivot", oldPivot, newOriginPoint, undo);
auto *undo = new QUndoCommand(tr("Rogner une image"));
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
+4 -2
View File
@@ -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
// key, mirror, replace) is undone together with the displayed pixmap.
// colours -- as one value. The displayed pixmap is computed from it,
// 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)
// A second, deliberately non-compensating property on the SAME
// underlying value -- setPivot() (above) intentionally adjusts
@@ -170,6 +171,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
+14
View File
@@ -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");