From 1023b38f8e9c4289c8639cca67fe3f2e3448c9a9 Mon Sep 17 00:00:00 2001 From: Beat Hangartner Date: Thu, 8 Oct 2026 06:03:31 +0200 Subject: [PATCH] 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 Signed-off-by: Beat Hangartner --- sources/positionorder.h | 69 +++++++++++ sources/qetgraphicsitem/element.cpp | 5 +- sources/ui/terminalnumberingdialog.cpp | 15 ++- tests/qttest/CMakeLists.txt | 7 ++ tests/qttest/tst_positionorder.cpp | 155 +++++++++++++++++++++++++ 5 files changed, 240 insertions(+), 11 deletions(-) create mode 100644 sources/positionorder.h create mode 100644 tests/qttest/tst_positionorder.cpp diff --git a/sources/positionorder.h b/sources/positionorder.h new file mode 100644 index 000000000..4d895f419 --- /dev/null +++ b/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 . +*/ +#ifndef POSITIONORDER_H +#define POSITIONORDER_H + +#include +#include + +/** + 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 diff --git a/sources/qetgraphicsitem/element.cpp b/sources/qetgraphicsitem/element.cpp index 80804145e..54c328ca5 100644 --- a/sources/qetgraphicsitem/element.cpp +++ b/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 @@ -1934,9 +1935,7 @@ bool comparPos(const Element *elmt1, const Element *elmt2) if (a != b) return apos().x() == elmt2->pos().x()) - return elmt1->y() <= elmt2->pos().y(); - return elmt1->pos().x() <= elmt2->pos().x(); + return PositionOrder::xThenY(elmt1->pos(), elmt2->pos()); } /** diff --git a/sources/ui/terminalnumberingdialog.cpp b/sources/ui/terminalnumberingdialog.cpp index fde1df6da..6e30d32cc 100644 --- a/sources/ui/terminalnumberingdialog.cpp +++ b/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 @@ -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 diff --git a/tests/qttest/CMakeLists.txt b/tests/qttest/CMakeLists.txt index 7e10b46fa..53a6ab620 100644 --- a/tests/qttest/CMakeLists.txt +++ b/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) diff --git a/tests/qttest/tst_positionorder.cpp b/tests/qttest/tst_positionorder.cpp new file mode 100644 index 000000000..1ee9de8b1 --- /dev/null +++ b/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 . +*/ +#include "positionorder.h" + +#include +#include +#include + +/** + 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; + + static QList> 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("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 points; + const QList 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 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 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"