Merge pull request #1263 from ispyisail/fix/1238-empty-xrefpos

Fix #1238: cross-reference drawn over the contact's label

sources/properties/xrefproperties.cpp

@@ -22,6 +22,36 @@
 #include <QHash>
 #include <QMetaEnum>
 
+namespace {
+/**
+	@brief xrefPosFromKey
+	@param key : stored name of the position, e.g. "AlignBottom"
+	@return the position, or Qt::AlignBottom if key is empty or not one of
+	the positions the cross-reference settings offer. Older versions saved
+	an empty "xrefpos", which drew the cross-reference over the label.
+*/
+Qt::AlignmentFlag xrefPosFromKey(const QString &key)
+{
+	bool ok = false;
+	const int value = QMetaEnum::fromType<Qt::Alignment>()
+			.keyToValue(key.toStdString().data(), &ok);
+	if (!ok)
+		return Qt::AlignBottom;
+
+	switch (value) {
+		case Qt::AlignBottom:
+		case Qt::AlignTop:
+		case Qt::AlignLeft:
+		case Qt::AlignRight:
+		case Qt::AlignBaseline:
+		case Qt::AlignHCenter:
+			return Qt::AlignmentFlag(value);
+		default:
+			return Qt::AlignBottom;
+	}
+}
+}
+
 /**
 	@brief XRefProperties::XRefProperties
 	Default Constructor
@@ -96,8 +126,7 @@ void XRefProperties::fromSettings(const QSettings &settings,
 	m_master_label = settings.value(prefix % "master_label", "%f-%l%c").toString();
 	m_slave_label = settings.value(prefix % "slave_label", "(%f-%l%c)").toString();
 
-	QMetaEnum var = QMetaEnum::fromType<Qt::Alignment>();
-	m_xref_pos = Qt::AlignmentFlag(var.keyToValue((settings.value(prefix % "xrefpos", "AlignBottom").toString()).toStdString().data()));
+	m_xref_pos = xrefPosFromKey(settings.value(prefix % "xrefpos").toString());
 
 	for (QString key : m_prefix_keys) {
 		m_prefix.insert(key, settings.value(prefix + key % "prefix").toString());
@@ -157,14 +186,7 @@ bool XRefProperties::fromXml(const QDomElement &xml_element) {
 	QString snap = xml_element.attribute("snapto", "label");
 	snap == "bottom"? m_snap_to = Bottom : m_snap_to = Label;
 
-	QString xrefpos = xml_element.attribute("xrefpos","Left");
-
-	QMetaEnum var = QMetaEnum::fromType<Qt::Alignment>();
-
-	if(xml_element.hasAttribute("xrefpos"))
-		m_xref_pos = Qt::AlignmentFlag(var.keyToValue(xml_element.attribute("xrefpos").toStdString().data()));
-	else
-		m_xref_pos = Qt::AlignBottom;
+	m_xref_pos = xrefPosFromKey(xml_element.attribute("xrefpos"));
 
 	m_offset = xml_element.attribute("offset", "0").toInt();
 	m_slave_offset = xml_element.attribute("slave_offset", "0").toInt();

tests/qttest/CMakeLists.txt

@@ -668,3 +668,15 @@ add_executable(
 add_test(NAME tst_diagramcontext COMMAND tst_diagramcontext)
 target_include_directories(tst_diagramcontext PRIVATE ${QET_DIR}/sources)
 target_link_libraries(tst_diagramcontext PRIVATE Qt::Test Qt::Widgets Qt::Xml pugixml::pugixml)
+
+# XRefProperties: an empty or unknown cross-reference position, as older
+# versions saved it, reads as the default instead of drawing the
+# cross-reference over the element's label (issue #1238).
+add_executable(
+  tst_xrefpos
+  tst_xrefpos.cpp
+  ${QET_DIR}/sources/properties/xrefproperties.cpp
+  ${QET_DIR}/sources/properties/propertiesinterface.cpp)
+add_test(NAME tst_xrefpos COMMAND tst_xrefpos)
+target_include_directories(tst_xrefpos PRIVATE ${QET_DIR}/sources)
+target_link_libraries(tst_xrefpos PRIVATE Qt::Test Qt::Widgets Qt::Xml pugixml::pugixml)

tests/qttest/tst_xrefpos.cpp

@@ -0,0 +1,94 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+#include <QtTest>
+
+#include "properties/xrefproperties.h"
+
+/**
+	XRefProperties reads the cross-reference position from a project's
+	<xref xrefpos="..."> and from the settings file. Older versions saved an
+	empty value; read as-is it drew the cross-reference over the element's
+	label (issue #1238). An empty or unknown value must fall back to the
+	default, AlignBottom, and a valid one must be kept.
+*/
+class tst_xrefpos : public QObject
+{
+	Q_OBJECT
+
+	static Qt::AlignmentFlag fromXml(const QString &attribute, bool present = true)
+	{
+		QDomDocument doc;
+		QDomElement e = doc.createElement("xref");
+		e.setAttribute("type", "protection");
+		if (present)
+			e.setAttribute("xrefpos", attribute);
+		XRefProperties xrp;
+		xrp.fromXml(e);
+		return xrp.getXrefPos();
+	}
+
+	private slots:
+	void xml_data()
+	{
+		QTest::addColumn<QString>("stored");
+		QTest::addColumn<int>("expected");
+		QTest::newRow("empty")    << ""              << int(Qt::AlignBottom);
+		QTest::newRow("garbage")  << "Sideways"      << int(Qt::AlignBottom);
+		QTest::newRow("not offered") << "AlignJustify" << int(Qt::AlignBottom);
+		QTest::newRow("bottom")   << "AlignBottom"   << int(Qt::AlignBottom);
+		QTest::newRow("top")      << "AlignTop"      << int(Qt::AlignTop);
+		QTest::newRow("left")     << "AlignLeft"     << int(Qt::AlignLeft);
+		QTest::newRow("right")    << "AlignRight"    << int(Qt::AlignRight);
+		QTest::newRow("baseline") << "AlignBaseline" << int(Qt::AlignBaseline);
+		QTest::newRow("hcenter")  << "AlignHCenter"  << int(Qt::AlignHCenter);
+	}
+
+	void xml()
+	{
+		QFETCH(QString, stored);
+		QFETCH(int, expected);
+		QCOMPARE(int(fromXml(stored)), expected);
+	}
+
+	void xmlMissing()
+	{
+		QCOMPARE(fromXml(QString(), false), Qt::AlignBottom);
+	}
+
+	/**
+		An empty value is written back as "AlignBottom" on the next save,
+		so the file heals instead of carrying the empty value forward.
+	*/
+	void emptyIsRewritten()
+	{
+		QDomDocument doc;
+		QDomElement e = doc.createElement("xref");
+		e.setAttribute("xrefpos", "");
+		XRefProperties xrp;
+		xrp.fromXml(e);
+		QCOMPARE(xrp.toXml(doc).attribute("xrefpos"), QStringLiteral("AlignBottom"));
+	}
+
+	void settings()
+	{
+		QTemporaryDir dir;
+		QVERIFY(dir.isValid());
+		QSettings s(dir.filePath("qet.ini"), QSettings::IniFormat);
+		XRefProperties xrp;
+
+		s.setValue("xrefprotectionxrefpos", "");
+		xrp.fromSettings(s, "xrefprotection");
+		QCOMPARE(xrp.getXrefPos(), Qt::AlignBottom);
+
+		s.remove("xrefprotectionxrefpos");
+		xrp.setXrefPos(Qt::AlignTop);
+		xrp.fromSettings(s, "xrefprotection");
+		QCOMPARE(xrp.getXrefPos(), Qt::AlignBottom);
+
+		s.setValue("xrefprotectionxrefpos", "AlignRight");
+		xrp.fromSettings(s, "xrefprotection");
+		QCOMPARE(xrp.getXrefPos(), Qt::AlignRight);
+	}
+};
+
+QTEST_GUILESS_MAIN(tst_xrefpos)
+#include "tst_xrefpos.moc"