Sort elements and terminals by position with a strict weak ordering

Two position comparisons handed to std::sort were not strict weak
orderings, which std::sort requires; with the wrong kind of comparator
the sort is undefined behaviour (libstdc++ can read past the range,
MSVC debug builds assert "invalid comparator").

- comparPos(), used when renumbering the elements of a project, ended
  with "<=" on x and y, so two elements at the same position (pasted at
  the origin, placed by a script, stacked symbols) were each "before"
  the other.
- The terminal numbering dialog compared positions with a 1 px
  tolerance ("within 1 px counts as aligned, then compare the other
  axis"), which is not transitive: terminals at x = 2, 1.1 and 0.2 give
  a < b, b < c and c < a.

Move the comparisons into positionorder.h, a header-only helper:
xThenY()/yThenX() with "<", and roundedXThenY()/roundedYThenX(), which
round the positions to whole pixels first so that items a fraction of a
pixel apart still count as aligned, as the tolerance meant to, while
staying transitive. comparPos() keeps its folio and row-letter stages
and calls xThenY() for the last one.

No file-format change. Elements at distinct positions sort exactly as
before; the terminal order changes only for terminals less than a pixel
apart that straddle a half-pixel boundary.

Tests: tst_positionorder checks each order on a grid of awkward
positions by brute force (irreflexive, asymmetric, transitive, with a
transitive equivalence), the three-terminal cycle, twenty items at one
position, and that std::sort leaves the list sorted and intact. The
helper is new, so the test cannot fail on master; the call sites are
the two replacements in the diff.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Beat Hangartner <beat@hangartners.ch>

sources/positionorder.h

@@ -0,0 +1,69 @@
+/*
+	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/>.
+*/
+#ifndef POSITIONORDER_H
+#define POSITIONORDER_H
+
+#include <QPointF>
+#include <QtGlobal>
+
+/**
+	The order of items by their position on a folio, for std::sort: left to
+	right then top to bottom, or top to bottom then left to right. Each
+	function is a strict weak ordering, which std::sort requires: two items
+	at the same position come out in either order, but never "both before
+	each other", and the order is transitive. A comparison with "<=", or
+	with a tolerance ("within 1 px counts as aligned"), is neither, and
+	std::sort may then read past the range or assert.
+*/
+namespace PositionOrder
+{
+	/// a before b when a is left of b, or level with it and above it.
+	inline bool xThenY(const QPointF &a, const QPointF &b)
+	{
+		if (a.x() != b.x()) return a.x() < b.x();
+		return a.y() < b.y();
+	}
+
+	/// a before b when a is above b, or level with it and left of it.
+	inline bool yThenX(const QPointF &a, const QPointF &b)
+	{
+		if (a.y() != b.y()) return a.y() < b.y();
+		return a.x() < b.x();
+	}
+
+	/// The position rounded to whole pixels, so that items placed a
+	/// fraction of a pixel apart count as aligned.
+	inline QPointF rounded(const QPointF &p)
+	{
+		return QPointF(qRound(p.x()), qRound(p.y()));
+	}
+
+	/// xThenY() on the positions rounded to whole pixels.
+	inline bool roundedXThenY(const QPointF &a, const QPointF &b)
+	{
+		return xThenY(rounded(a), rounded(b));
+	}
+
+	/// yThenX() on the positions rounded to whole pixels.
+	inline bool roundedYThenX(const QPointF &a, const QPointF &b)
+	{
+		return yThenX(rounded(a), rounded(b));
+	}
+}
+
+#endif // POSITIONORDER_H

sources/qetgraphicsitem/element.cpp

@@ -53,6 +53,7 @@ static const QString plcTerminalKeys[] = {
 	QETInformation::ELMT_PLC_T4
 };
 #include "../qetxml.h"
+#include "../positionorder.h"
 #include "../qetversion.h"
 #include "qgraphicsitemutility.h"
 #include <QDebug>
@@ -1934,9 +1935,7 @@ bool comparPos(const Element *elmt1, const Element *elmt2)
 	if (a != b)
 		return a<b;
 	//In last compare the line, if line is egal, return sorted by row in real pos
-	if (elmt1->pos().x() == elmt2->pos().x())
-		return elmt1->y() <= elmt2->pos().y();
-	return elmt1->pos().x() <= elmt2->pos().x();
+	return PositionOrder::xThenY(elmt1->pos(), elmt2->pos());
 }
 
 /**

sources/ui/terminalnumberingdialog.cpp

@@ -4,6 +4,7 @@
 #include "../qetproject.h"
 #include "../diagram.h"
 #include "../qetgraphicsitem/element.h"
+#include "../positionorder.h"
 #include "../undocommand/changeelementinformationcommand.h"
 #include "../qet.h"
 #include <QUndoCommand>
@@ -210,14 +211,12 @@ QUndoCommand* TerminalNumberingDialog::getUndoCommand(QETProject *project) const
         // Then sort by folio (page) index
         if (a.folioIndex != b.folioIndex) return a.folioIndex < b.folioIndex;
 
-        // Finally sort by coordinates (with a 1.0px tolerance to handle slight misalignments)
-        if (axisX) {
-            if (qAbs(a.x - b.x) > 1.0) return a.x < b.x;
-            return a.y < b.y;
-        } else {
-            if (qAbs(a.y - b.y) > 1.0) return a.y < b.y;
-            return a.x < b.x;
-        }
+        // Finally sort by coordinates, rounded to whole pixels so that slight
+        // misalignments count as aligned (a tolerance test is not transitive,
+        // which std::sort requires)
+        const QPointF pa(a.x, a.y), pb(b.x, b.y);
+        return axisX ? PositionOrder::roundedXThenY(pa, pb)
+                     : PositionOrder::roundedYThenX(pa, pb);
     });
 
     // 4. Generate new numbering and create the undo command macro

tests/qttest/CMakeLists.txt

@@ -108,6 +108,13 @@ add_test(NAME tst_diagramsortkeys COMMAND tst_diagramsortkeys)
 target_include_directories(tst_diagramsortkeys PRIVATE ${QET_DIR}/sources)
 target_link_libraries(tst_diagramsortkeys PRIVATE Qt::Test)
 
+# positionorder.h is header-only: the position comparisons behind element
+# renumbering and terminal numbering, as strict weak orderings for std::sort.
+add_executable(tst_positionorder tst_positionorder.cpp)
+target_include_directories(tst_positionorder PRIVATE ${QET_DIR}/sources)
+target_link_libraries(tst_positionorder PRIVATE Qt::Test Qt::Core)
+add_test(NAME tst_positionorder COMMAND tst_positionorder)
+
 # bordercelllabels.h is header-only too.
 add_executable(tst_bordercelllabels tst_bordercelllabels.cpp)
 add_test(NAME tst_bordercelllabels COMMAND tst_bordercelllabels)

tests/qttest/tst_positionorder.cpp

@@ -0,0 +1,155 @@
+/*
+	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 "positionorder.h"
+
+#include <QtTest>
+#include <algorithm>
+#include <functional>
+
+/**
+	PositionOrder -- the position comparisons behind renumbering elements
+	(comparPos) and numbering terminals. std::sort needs a strict weak
+	ordering; the old comparisons used "<=" and a 1 px tolerance, which
+	are not one, so two elements at the same position (or three terminals
+	a fraction apart) were undefined behaviour.
+*/
+class tst_positionorder : public QObject
+{
+	Q_OBJECT
+
+	using Less = std::function<bool(const QPointF &, const QPointF &)>;
+
+	static QList<QPair<QString, Less>> orders()
+	{
+		return {{QStringLiteral("xThenY"), PositionOrder::xThenY},
+				{QStringLiteral("yThenX"), PositionOrder::yThenX},
+				{QStringLiteral("roundedXThenY"), PositionOrder::roundedXThenY},
+				{QStringLiteral("roundedYThenX"), PositionOrder::roundedYThenX}};
+	}
+
+private slots:
+	// Nothing is before itself, and equal points are before each other
+	// in neither direction: what "<=" broke.
+	void equalPointsAreNotOrdered()
+	{
+		const QPointF p(100, 200), q(100, 200);
+		for (const auto &order : orders()) {
+			QVERIFY2(!order.second(p, p), qPrintable(order.first));
+			QVERIFY2(!order.second(p, q), qPrintable(order.first));
+			QVERIFY2(!order.second(q, p), qPrintable(order.first));
+		}
+	}
+
+	void leftToRightThenTopToBottom()
+	{
+		QVERIFY(PositionOrder::xThenY(QPointF(10, 500), QPointF(20, 0)));
+		QVERIFY(!PositionOrder::xThenY(QPointF(20, 0), QPointF(10, 500)));
+		QVERIFY(PositionOrder::xThenY(QPointF(10, 0), QPointF(10, 5)));
+		QVERIFY(!PositionOrder::xThenY(QPointF(10, 5), QPointF(10, 0)));
+	}
+
+	void topToBottomThenLeftToRight()
+	{
+		QVERIFY(PositionOrder::yThenX(QPointF(500, 10), QPointF(0, 20)));
+		QVERIFY(!PositionOrder::yThenX(QPointF(0, 20), QPointF(500, 10)));
+		QVERIFY(PositionOrder::yThenX(QPointF(0, 10), QPointF(5, 10)));
+		QVERIFY(!PositionOrder::yThenX(QPointF(5, 10), QPointF(0, 10)));
+	}
+
+	// Items a fraction of a pixel apart (placed with Ctrl, or after a
+	// rotation) count as aligned, and are then ordered on the other axis.
+	void roundedTreatsFractionsAsAligned()
+	{
+		QVERIFY(PositionOrder::roundedXThenY(QPointF(10.2, 0), QPointF(9.8, 5)));
+		QVERIFY(!PositionOrder::roundedXThenY(QPointF(9.8, 5), QPointF(10.2, 0)));
+		QVERIFY(PositionOrder::roundedYThenX(QPointF(0, 10.2), QPointF(5, 9.8)));
+		QVERIFY(!PositionOrder::roundedYThenX(QPointF(5, 9.8), QPointF(0, 10.2)));
+	}
+
+	// The three terminals that broke the tolerance comparison: with
+	// "within 1 px counts as aligned", a < b (by y), b < c (by y) and
+	// c < a (by x), a cycle. Rounded, they are simply ordered by x.
+	void roundedIsTransitiveWhereToleranceWasNot()
+	{
+		const QPointF a(2, 0), b(1.1, 1), c(0.2, 2);
+		QVERIFY(PositionOrder::roundedXThenY(c, b));
+		QVERIFY(PositionOrder::roundedXThenY(b, a));
+		QVERIFY(PositionOrder::roundedXThenY(c, a));
+		QVERIFY(!PositionOrder::roundedXThenY(a, c));
+	}
+
+	// Every order is a strict weak ordering on a grid of awkward points:
+	// irreflexive, asymmetric, transitive, and with transitive
+	// equivalence. Checked by brute force on the whole set.
+	void strictWeakOrdering_data()
+	{
+		QTest::addColumn<QString>("name");
+		for (const auto &order : orders())
+			QTest::newRow(qPrintable(order.first)) << order.first;
+	}
+
+	void strictWeakOrdering()
+	{
+		QFETCH(QString, name);
+		Less less;
+		for (const auto &order : orders())
+			if (order.first == name) less = order.second;
+		QVERIFY(less);
+
+		QList<QPointF> points;
+		const QList<qreal> values{-1, -0.6, -0.4, 0, 0.2, 0.5, 0.9, 1, 1.1, 2, 10.5, 61.3, 61.7};
+		for (qreal x : values)
+			for (qreal y : values)
+				points << QPointF(x, y);
+		points << points.first() << QPointF(2, 0) << QPointF(1.1, 1) << QPointF(0.2, 2);
+
+		auto equiv = [&](const QPointF &p, const QPointF &q) { return !less(p, q) && !less(q, p); };
+		for (const QPointF &p : points) {
+			QVERIFY(!less(p, p));
+			for (const QPointF &q : points) {
+				if (less(p, q)) QVERIFY(!less(q, p));
+				for (const QPointF &r : points) {
+					if (less(p, q) && less(q, r)) QVERIFY2(less(p, r), "transitive");
+					if (equiv(p, q) && equiv(q, r)) QVERIFY2(equiv(p, r), "equivalence transitive");
+				}
+			}
+		}
+
+		// And std::sort on it ends sorted, with the list intact.
+		QList<QPointF> sorted = points;
+		std::sort(sorted.begin(), sorted.end(), less);
+		QCOMPARE(sorted.size(), points.size());
+		for (int i = 1; i < sorted.size(); ++i)
+			QVERIFY(!less(sorted.at(i), sorted.at(i - 1)));
+	}
+
+	// Twenty items at one position, as pasting at the origin leaves
+	// them: a list std::sort could scan past the end of with "<=".
+	void manyEqualPositionsSort()
+	{
+		QList<QPointF> points(20, QPointF(0, 0));
+		points << QPointF(-10, 0) << QPointF(10, 0);
+		std::sort(points.begin(), points.end(), PositionOrder::xThenY);
+		QCOMPARE(points.size(), 22);
+		QCOMPARE(points.first(), QPointF(-10, 0));
+		QCOMPARE(points.last(), QPointF(10, 0));
+	}
+};
+
+QTEST_GUILESS_MAIN(tst_positionorder)
+#include "tst_positionorder.moc"