From 123c56bb130340b6daf2df9fb6e88f99509e7efb Mon Sep 17 00:00:00 2001 From: Kellermorph Date: Mon, 19 Oct 2026 20:48:06 +0200 Subject: [PATCH] Sort terminal strips into the plan's own order The strip sorted by label: X1:1, X1:2 ... with X1:N and X1:PE at the end whatever the plan looks like, so a strip drawn across pages came out in an order no cable follows. Sort it the way the plan is drawn instead -- folio by folio, then left to right -- in both places that order a strip: the auto import's own sort of the terminals it groups, and SortTerminalStripCommand, which the import pushes for a strip it just creates and which the editor's sort button and qet.sortTerminalStrip() push as well. The two share one comparator (SortTerminalStripCommand::planOrderLessThan), so they cannot fight each other. A spare stands on no folio, has no place in the plan order and goes last, by label; the label breaks the remaining ties, so two terminals at the very same spot keep a stable order. --- .../UndoCommand/sortterminalstripcommand.cpp | 70 ++++- .../UndoCommand/sortterminalstripcommand.h | 21 ++ .../TerminalStrip/terminalstripautoimport.cpp | 21 +- sources/scripting/qetscriptapi.cpp | 5 +- tests/qttest/CMakeLists.txt | 6 + tests/qttest/tst_terminalstriporder.cpp | 272 ++++++++++++++++++ 6 files changed, 381 insertions(+), 14 deletions(-) create mode 100644 tests/qttest/tst_terminalstriporder.cpp diff --git a/sources/TerminalStrip/UndoCommand/sortterminalstripcommand.cpp b/sources/TerminalStrip/UndoCommand/sortterminalstripcommand.cpp index 898ec8973..ce54bfb94 100644 --- a/sources/TerminalStrip/UndoCommand/sortterminalstripcommand.cpp +++ b/sources/TerminalStrip/UndoCommand/sortterminalstripcommand.cpp @@ -19,6 +19,9 @@ #include "../terminalstrip.h" #include "../physicalterminal.h" #include "../realterminal.h" +#include "../../diagram.h" +#include "../../positionorder.h" +#include "../../qetgraphicsitem/element.h" #include "../../utils/qetutils.h" SortTerminalStripCommand::SortTerminalStripCommand(TerminalStrip *strip, QUndoCommand *parent) : @@ -47,16 +50,73 @@ void SortTerminalStripCommand::redo() void SortTerminalStripCommand::sort() { - auto label_of = [](const QSharedPointer &t) -> QString + //The plan position of a physical terminal is the position of the + //level that sits on a folio; a terminal whose levels are all + //spares has no position and is ordered by label only. + auto position_of = [](const QSharedPointer &t_) -> QSharedPointer { - return t->realTerminalCount() ? t->realTerminals().constLast()->label() - : QString(); + if (!t_) { + return {}; + } + for (const auto &real_ : t_->realTerminals()) { + if (real_ && real_->element()) { + return real_; + } + } + return t_->realTerminalCount() ? t_->realTerminals().constFirst() + : QSharedPointer(); }; std::stable_sort(m_new_order.begin(), m_new_order.end(), - [&label_of](const QSharedPointer &arg1, + [&position_of](const QSharedPointer &arg1, const QSharedPointer &arg2) { - return QETUtils::naturalLessThan(label_of(arg1), label_of(arg2)); + return planOrderLessThan(position_of(arg1), position_of(arg2)); }); } + +/** + @brief SortTerminalStripCommand::planOrderLessThan + Order two real terminals the way the plan is drawn : folio by folio, + then left to right. Terminals with no folio position (spares) come + last, and the label only breaks ties, so the result stays stable and + predictable whatever the labels look like ("X1:N" and "X1:PE" stand + where they are drawn, not at the end). + Both the auto import and sort() use this, so the two orders of the + same strip can never fight each other. + @a a_ terminal to compare + @a b_ other terminal to compare + @return true if @a a_ comes before @a b_ +*/ +bool SortTerminalStripCommand::planOrderLessThan( + const QSharedPointer &a_, + const QSharedPointer &b_) +{ + if (!a_ || !b_) { + return a_ && !b_; //a missing terminal goes last + } + + const Element *ea = a_->element(); + const Element *eb = b_->element(); + auto folio_of = [](const Element *e_) -> int + { + return (e_ && e_->diagram()) ? e_->diagram()->folioIndex() : -1; + }; + + const int fa = folio_of(ea); + const int fb = folio_of(eb); + if (fa != fb) { + //A terminal with no folio has no place in the plan order + if (fa < 0) { + return false; + } + if (fb < 0) { + return true; + } + return fa < fb; + } + if (fa >= 0 && ea->pos() != eb->pos()) { + return PositionOrder::roundedXThenY(ea->pos(), eb->pos()); + } + return QETUtils::naturalLessThan(a_->label(), b_->label()); +} diff --git a/sources/TerminalStrip/UndoCommand/sortterminalstripcommand.h b/sources/TerminalStrip/UndoCommand/sortterminalstripcommand.h index eae51e782..514fa90bc 100644 --- a/sources/TerminalStrip/UndoCommand/sortterminalstripcommand.h +++ b/sources/TerminalStrip/UndoCommand/sortterminalstripcommand.h @@ -20,10 +20,12 @@ #include #include +#include #include class TerminalStrip; class PhysicalTerminal; +class RealTerminal; /** * @brief The SortTerminalStripCommand class @@ -38,6 +40,25 @@ class SortTerminalStripCommand : public QUndoCommand void undo() override; void redo() override; + /** + * @brief planOrderLessThan + * Order two real terminals the way the plan is drawn : folio by + * folio, then left to right (top to bottom only breaks ties + * within a column). A spare has no folio and comes last, by + * label, and the label breaks every remaining tie (two + * terminals at the very same spot), which keeps the result + * stable. + * + * The auto import sorts with it too, so the order it works out + * cannot be rewritten by the SortTerminalStripCommand it then + * pushes for the strip it just created. + * @param a_ : first terminal + * @param b_ : second terminal + * @return true if @a a_ comes before @a b_ + */ + static bool planOrderLessThan(const QSharedPointer &a_, + const QSharedPointer &b_); + private: void sort(); diff --git a/sources/TerminalStrip/terminalstripautoimport.cpp b/sources/TerminalStrip/terminalstripautoimport.cpp index 6e9508dda..808c7b2d8 100644 --- a/sources/TerminalStrip/terminalstripautoimport.cpp +++ b/sources/TerminalStrip/terminalstripautoimport.cpp @@ -128,7 +128,9 @@ namespace * whose label prefix or plant and location no longer match its strip. * * In both cases the order the user gave to a strip is never changed, - * only a strip created by the run itself is sorted. + * only a strip created by the run itself is sorted -- the way the plan + * is drawn : folio by folio, then left to right (see + * SortTerminalStripCommand::planOrderLessThan). */ TerminalStripAutoImport::ImportResult TerminalStripAutoImport::importFreeTerminals( QETProject *project, bool move_placed) @@ -187,16 +189,21 @@ TerminalStripAutoImport::ImportResult TerminalStripAutoImport::importFreeTermina for (auto it_ = groups_.begin(); it_ != groups_.end(); ++it_) { - auto by_label_ = [](const QSharedPointer &a_, - const QSharedPointer &b_) { - return QETUtils::naturalLessThan(a_->label(), b_->label()); + //The plan order : folio by folio, then left to right, so the + //strip comes out the way the plan is drawn instead of label + //by label ("N" and "PE" stay where they are on the sheet). + //SortTerminalStripCommand, pushed below for a strip this run + //creates, sorts with the same rule and cannot undo it. + auto by_plan_ = [](const QSharedPointer &a_, + const QSharedPointer &b_) { + return SortTerminalStripCommand::planOrderLessThan(a_, b_); }; std::sort(it_.value().free_terminals.begin(), - it_.value().free_terminals.end(), by_label_); + it_.value().free_terminals.end(), by_plan_); std::sort(it_.value().placed_terminals.begin(), it_.value().placed_terminals.end(), - [&by_label_](const auto &a_, const auto &b_) { - return by_label_(a_.first, b_.first); + [&by_plan_](const auto &a_, const auto &b_) { + return by_plan_(a_.first, b_.first); }); } diff --git a/sources/scripting/qetscriptapi.cpp b/sources/scripting/qetscriptapi.cpp index cde86eef6..13774c6a7 100644 --- a/sources/scripting/qetscriptapi.cpp +++ b/sources/scripting/qetscriptapi.cpp @@ -3314,8 +3314,9 @@ bool QetScriptApi::bridgeTerminals(int stripIndex, const QVariantList &realTermi /** @brief QetScriptApi::sortTerminalStrip - Reorder a strip's physical positions into the canonical order the - editor's own "sort" button computes, through SortTerminalStripCommand. + Reorder a strip's physical positions into the order the plan is + drawn in -- folio by folio, then left to right -- the same + SortTerminalStripCommand the editor's own "sort" button pushes. */ bool QetScriptApi::sortTerminalStrip(int stripIndex) { diff --git a/tests/qttest/CMakeLists.txt b/tests/qttest/CMakeLists.txt index 644cf4725..097bd3533 100644 --- a/tests/qttest/CMakeLists.txt +++ b/tests/qttest/CMakeLists.txt @@ -332,6 +332,12 @@ target_link_libraries(tst_textresizehandles PRIVATE Qt::Test Qt::Widgets) # rectangle's does; a pivot placed by hand stays. qet_add_app_test(tst_linepivotresize tst_linepivotresize.cpp) +# The terminal strip sorts into the plan's own order -- folio by folio, then +# left to right -- instead of by label; the auto import and +# SortTerminalStripCommand share one comparator for it, so a strip the import +# creates cannot come out in another order than the sort it then pushes. +qet_add_app_test(tst_terminalstriporder tst_terminalstriporder.cpp) + # legacyelementtypes.h is header-only: the rewrite of element type names in # nomenclature queries saved before June 2022. add_executable(tst_legacyelementtypes tst_legacyelementtypes.cpp) diff --git a/tests/qttest/tst_terminalstriporder.cpp b/tests/qttest/tst_terminalstriporder.cpp new file mode 100644 index 000000000..1303f5b53 --- /dev/null +++ b/tests/qttest/tst_terminalstriporder.cpp @@ -0,0 +1,272 @@ +/* + 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 "qetapptest.h" + +#include "TerminalStrip/UndoCommand/sortterminalstripcommand.h" +#include "TerminalStrip/physicalterminal.h" +#include "TerminalStrip/realterminal.h" +#include "TerminalStrip/terminalstrip.h" +#include "diagram.h" +#include "properties/elementdata.h" +#include "qetgraphicsitem/element.h" +#include "qetproject.h" +#include "utils/qetutils.h" + +#include + +/** + The terminal strip sorts into the plan's own order -- folio by folio, + then left to right -- instead of by label, so "X1:N" and "X1:PE" + stand where they are drawn rather than at the end of the strip. The + auto import and SortTerminalStripCommand share the one comparator + (SortTerminalStripCommand::planOrderLessThan), so the order the + import works out cannot be rewritten by the sort it then pushes for + the strip it just created. + + fixtures/every_feature.qet has the strip X1 with element 891b70f2 + (x=300) left of 4861e268 (x=360) on the first folio, and 4861e268 + holds an empty label where 891b70f2 has a non-empty one: a label + sort would put 4861e268 first, which is the very swap this test + pins down. +*/ +class tst_terminalstriporder : public QObject +{ + Q_OBJECT + + QETProject *m_project = nullptr; + + Element *element(const QString &uuid) const + { + for (Diagram *d_ : m_project->diagrams()) { + for (Element *e_ : d_->elements()) { + if (e_->uuid().toString() == uuid) { + return e_; + } + } + } + return nullptr; + } + + TerminalStrip *strip() const + { + const auto strips_ = m_project->terminalStrip(); + return strips_.isEmpty() ? nullptr : strips_.first(); + } + + QSharedPointer terminalInStrip(const QString &element_uuid) const + { + auto *strip_ = strip(); + if (!strip_) { + return {}; + } + for (const auto &real_ : strip_->realTerminals()) { + if (real_ && real_->element() + && real_->element()->uuid().toString() == element_uuid) { + return real_; + } + } + return {}; + } + + /// The element uuids of the strip's physical positions, in strip order. + QStringList physicalOrder() const + { + QStringList order_; + auto *strip_ = strip(); + if (!strip_) { + return order_; + } + for (const auto &physical_ : strip_->physicalTerminal()) { + if (physical_ && physical_->realTerminalCount() + && physical_->realTerminals().first()->element()) { + order_ << physical_->realTerminals().first()->element()->uuid().toString(); + } + } + return order_; + } + +private slots: + void init(); + void cleanup(); + + void positionBeatsLabel(); + void folioComesBeforePosition(); + void spareWithoutFolioGoesLast(); + void sortKeepsThePlanOrder(); + void sortFollowsMovedElements(); +}; + +void tst_terminalstriporder::init() +{ + const QString fixture_ = QFINDTESTDATA("fixtures/every_feature.qet"); + QVERIFY(!fixture_.isEmpty()); + m_project = new QETProject(fixture_); + QCOMPARE(m_project->state(), QETProject::Ok); +} + +void tst_terminalstriporder::cleanup() +{ + delete m_project; + m_project = nullptr; +} + +/** + Two terminals of one folio, 891b70f2 left of 4861e268: the position + decides, and the label says the opposite (4861e268's is empty), so a + comparator by label would order them the wrong way round. +*/ +void tst_terminalstriporder::positionBeatsLabel() +{ + const auto a_ = terminalInStrip(QStringLiteral("{891b70f2-2822-41c3-9f94-28efb448b5c4}")); + const auto b_ = terminalInStrip(QStringLiteral("{4861e268-d344-413e-a86f-034ea5ada16c}")); + QVERIFY(a_); + QVERIFY(b_); + QCOMPARE(a_->element()->pos().x() < b_->element()->pos().x(), true); + + QVERIFY(SortTerminalStripCommand::planOrderLessThan(a_, b_)); + QVERIFY(!SortTerminalStripCommand::planOrderLessThan(b_, a_)); + + //The labels would sort the other way round (b's is empty), which + //is exactly the order the plan must NOT follow + QVERIFY(QETUtils::naturalLessThan(b_->label(), a_->label())); +} + +/** + The page comes before the position ("page by page, then left to + right"): a terminal moved to the second folio sorts after every + terminal of the first one, even when it stands further left than + them. The element is put back on its own folio before the test + ends, so the project is left as it was. +*/ +void tst_terminalstriporder::folioComesBeforePosition() +{ + const auto a_ = terminalInStrip(QStringLiteral("{891b70f2-2822-41c3-9f94-28efb448b5c4}")); + const auto b_ = terminalInStrip(QStringLiteral("{4861e268-d344-413e-a86f-034ea5ada16c}")); + QVERIFY(a_); + QVERIFY(b_); + Element *moved_ = a_->element(); + QVERIFY(moved_); + const QList folios_ = m_project->diagrams(); + QVERIFY(folios_.size() > 1); + Diagram *own_folio_ = moved_->diagram(); + Diagram *later_folio_ = folios_.at(1); + QVERIFY(later_folio_ != own_folio_); + const QPointF original_pos_ = moved_->pos(); + + //Put a on the second folio, left of where b stands on the first + own_folio_->removeItem(moved_); + later_folio_->addItem(moved_); + moved_->setPos(QPointF(50, 600)); + QVERIFY(moved_->diagram() == later_folio_); + + const bool folio_later_ = later_folio_->folioIndex() > own_folio_->folioIndex(); + const bool a_left_of_b_ = moved_->pos().x() < b_->element()->pos().x(); + const bool b_first_ = SortTerminalStripCommand::planOrderLessThan(b_, a_); + const bool a_first_ = SortTerminalStripCommand::planOrderLessThan(a_, b_); + + //Back to where it was, so the conductors drawn to it and the + //project's cleanup see the folio they were loaded from + later_folio_->removeItem(moved_); + own_folio_->addItem(moved_); + moved_->setPos(original_pos_); + QVERIFY(moved_->diagram() == own_folio_); + + QVERIFY(folio_later_); + QVERIFY(a_left_of_b_); //the position alone would put a first + QVERIFY(b_first_); //the page decides: b (first folio) first + QVERIFY(!a_first_); +} + +/** + A spare terminal stands on no folio: it has no place in the plan + order and goes last, whatever its label would say. +*/ +void tst_terminalstriporder::spareWithoutFolioGoesLast() +{ + ElementData data_; + data_.m_informations.addValue(QStringLiteral("label"), + QStringLiteral("AAA")); + const auto spare_ = RealTerminal::createSpare(data_); + QVERIFY(spare_); + QVERIFY(spare_->isSpare()); + + const auto a_ = terminalInStrip(QStringLiteral("{891b70f2-2822-41c3-9f94-28efb448b5c4}")); + QVERIFY(a_); + //"AAA" would sort first by label alone + QVERIFY(QETUtils::naturalLessThan(spare_->label(), a_->label())); + + QVERIFY(SortTerminalStripCommand::planOrderLessThan(a_, spare_)); + QVERIFY(!SortTerminalStripCommand::planOrderLessThan(spare_, a_)); +} + +/** + The sort command, run on the fixture as it stands, keeps the strip + the way the plan is drawn: 891b70f2 (x=300) before 4861e268 + (x=360). The old label sort swapped them, since 4861e268's label is + the empty one. +*/ +void tst_terminalstriporder::sortKeepsThePlanOrder() +{ + auto *strip_ = strip(); + QVERIFY(strip_); + QCOMPARE(strip_->physicalTerminalCount(), 2); + + SortTerminalStripCommand command_(strip_); + command_.redo(); + + QCOMPARE(physicalOrder(), + QStringList({ + QStringLiteral("{891b70f2-2822-41c3-9f94-28efb448b5c4}"), + QStringLiteral("{4861e268-d344-413e-a86f-034ea5ada16c}"), + })); +} + +/** + The order follows the plan, not the strip's history: once the two + elements are swapped on the folio, sorting puts the strip the other + way round too. +*/ +void tst_terminalstriporder::sortFollowsMovedElements() +{ + auto *strip_ = strip(); + QVERIFY(strip_); + Element *a_ = element(QStringLiteral("{891b70f2-2822-41c3-9f94-28efb448b5c4}")); + Element *b_ = element(QStringLiteral("{4861e268-d344-413e-a86f-034ea5ada16c}")); + QVERIFY(a_); + QVERIFY(b_); + + b_->setPos(100, 400); + a_->setPos(700, 400); + + SortTerminalStripCommand command_(strip_); + command_.redo(); + + QCOMPARE(physicalOrder(), + QStringList({ + QStringLiteral("{4861e268-d344-413e-a86f-034ea5ada16c}"), + QStringLiteral("{891b70f2-2822-41c3-9f94-28efb448b5c4}"), + })); +} + +int main(int argc, char **argv) +{ + QStandardPaths::setTestModeEnabled(true); + return QetAppTest::run(argc, argv); +} + +#include "tst_terminalstriporder.moc"