From a8699f6f6228503f1612c7d3ed7f42ce68550bed Mon Sep 17 00:00:00 2001 From: ispyisail Date: Thu, 1 Oct 2026 23:10:15 +1300 Subject: [PATCH] Fix the wire properties panel changing properties nobody edited With "Show the properties of a selected conductor in the Selection properties panel" switched on, editing a wire there could change things the user never touched: - Enter in a field (Function, Section...) is not used by the line edit, so it reaches the checkable "Multifilaire" group box around it, which takes it as a click. The wire was switched to single-line, gaining ground, neutral and phase symbols. The modal dialog never shows this because its OK button takes Enter first. - Every edit wrote back the whole set of properties as the widget holds them, so a value the widget cannot show exactly was rewritten: a dash size of 1 became 2. The panel now swallows Enter at its checkable group boxes, and writes only the fields that differ from what it showed, through the new ConductorProperties::applyChanges(). "Apply to all conductors of this potential" uses the same rule, so the rest of the potential keeps its own values too. tst_conductorapplychanges checks each field on its own: change that one field, and a wire whose every field differs takes it and keeps the rest. Co-Authored-By: Claude Opus 5.5 --- sources/conductorproperties.cpp | 52 ++++++ sources/conductorproperties.h | 2 + .../ui/conductorpropertieseditorwidget.cpp | 101 +++++++---- sources/ui/conductorpropertieseditorwidget.h | 7 +- tests/qttest/CMakeLists.txt | 13 ++ tests/qttest/tst_conductorapplychanges.cpp | 167 ++++++++++++++++++ 6 files changed, 307 insertions(+), 35 deletions(-) create mode 100644 tests/qttest/tst_conductorapplychanges.cpp diff --git a/sources/conductorproperties.cpp b/sources/conductorproperties.cpp index 585839b57..12a55539c 100644 --- a/sources/conductorproperties.cpp +++ b/sources/conductorproperties.cpp @@ -757,6 +757,58 @@ void ConductorProperties::applyForEqualAttributes(QList lis equal = true; } +/** + @brief ConductorProperties::applyChanges + Copy into this every attribute that differs between before and after, + and leave every other attribute as it is. The Selection properties + panel applies an edit this way, so a conductor keeps everything the + user did not change: its own text, function, cable... and any value + the panel cannot show exactly. + @param before : the properties as they were shown to the user + @param after : the properties after the user's edit +*/ +void ConductorProperties::applyChanges(const ConductorProperties &before, + const ConductorProperties &after) +{ + const auto take = [](auto &mine, const auto &was, const auto &now) { + if (was != now) + mine = now; + }; + + take(type, before.type, after.type); + take(color, before.color, after.color); + take(m_bicolor, before.m_bicolor, after.m_bicolor); + take(m_color_2, before.m_color_2, after.m_color_2); + take(m_dash_size, before.m_dash_size, after.m_dash_size); + take(style, before.style, after.style); + take(text, before.text, after.text); + take(text_color, before.text_color, after.text_color); + take(m_formula, before.m_formula, after.m_formula); + take(m_cable, before.m_cable, after.m_cable); + take(m_bus, before.m_bus, after.m_bus); + take(m_function, before.m_function, after.m_function); + take(m_tension_protocol, before.m_tension_protocol, after.m_tension_protocol); + take(m_wire_color, before.m_wire_color, after.m_wire_color); + take(m_wire_section, before.m_wire_section, after.m_wire_section); + take(m_show_text, before.m_show_text, after.m_show_text); + take(text_size, before.text_size, after.text_size); + take(cond_size, before.cond_size, after.cond_size); + take(verti_rotate_text, before.verti_rotate_text, after.verti_rotate_text); + take(horiz_rotate_text, before.horiz_rotate_text, after.horiz_rotate_text); + take(m_one_text_per_folio, before.m_one_text_per_folio, after.m_one_text_per_folio); + take(m_horizontal_alignment, before.m_horizontal_alignment, after.m_horizontal_alignment); + take(m_vertical_alignment, before.m_vertical_alignment, after.m_vertical_alignment); + + //Single-line symbols, one by one + SingleLineProperties slp_before = before.singleLineProperties; + SingleLineProperties slp_after = after.singleLineProperties; + take(singleLineProperties.hasGround, slp_before.hasGround, slp_after.hasGround); + take(singleLineProperties.hasNeutral, slp_before.hasNeutral, slp_after.hasNeutral); + take(singleLineProperties.is_pen, slp_before.is_pen, slp_after.is_pen); + if (slp_before.phasesCount() != slp_after.phasesCount()) + singleLineProperties.setPhasesCount(slp_after.phasesCount()); +} + /** @brief ConductorProperties::defaultProperties @return the default properties stored in the setting file diff --git a/sources/conductorproperties.h b/sources/conductorproperties.h index 9f86576d6..d29bbdbaa 100644 --- a/sources/conductorproperties.h +++ b/sources/conductorproperties.h @@ -126,6 +126,8 @@ class ConductorProperties void fromSettings(QSettings &, const QString & = QString()); static QString typeToString(ConductorType); void applyForEqualAttributes(QList list); + void applyChanges(const ConductorProperties &before, + const ConductorProperties &after); static ConductorProperties defaultProperties(); diff --git a/sources/ui/conductorpropertieseditorwidget.cpp b/sources/ui/conductorpropertieseditorwidget.cpp index b54e53ae0..8b50fa02d 100644 --- a/sources/ui/conductorpropertieseditorwidget.cpp +++ b/sources/ui/conductorpropertieseditorwidget.cpp @@ -34,6 +34,7 @@ #include #include #include +#include #include #include #include @@ -89,10 +90,35 @@ ConductorPropertiesEditorWidget::ConductorPropertiesEditorWidget( // while keeping a minimum height so it stays usable when the dock is short. setSizePolicy(QSizePolicy::Preferred, QSizePolicy::Expanding); setMinimumHeight(200); + //Enter in a field (Function, Section...) is not used by the field: it + //goes on to the checkable "Multifilaire" or "Unifilaire" group box + //around it, which takes it as a click and switches the wire between + //multi-line and single-line. The modal dialog never shows this, its + //OK button takes Enter; here nothing does, so stop it at the box. + for (auto *gb : m_cpw->findChildren()) + if (gb->isCheckable()) + gb->installEventFilter(this); + setDisabled(true); setConductor(conductor); } +/** + @brief ConductorPropertiesEditorWidget::eventFilter + Swallow Enter on the checkable group boxes, see the constructor. +*/ +bool ConductorPropertiesEditorWidget::eventFilter(QObject *watched, QEvent *event) +{ + if (event->type() == QEvent::KeyPress + || event->type() == QEvent::KeyRelease) + { + const int key = static_cast(event)->key(); + if (key == Qt::Key_Return || key == Qt::Key_Enter) + return true; + } + return PropertiesEditorWidget::eventFilter(watched, event); +} + ConductorPropertiesEditorWidget::~ConductorPropertiesEditorWidget() {} @@ -130,7 +156,7 @@ void ConductorPropertiesEditorWidget::apply() if (!m_conductor || !m_conductor->diagram()) return; if (QUndoCommand *undo = associatedUndo()) m_conductor->diagram()->undoStack().push(undo); - m_initial = m_conductor->properties(); + m_shown = m_cpw->properties(); } /** @@ -212,13 +238,13 @@ void ConductorPropertiesEditorWidget::disconnectChangeSignals() /** @brief ConductorPropertiesEditorWidget::reset - Discard the in-progress edit, restoring the conductor's current properties. + Discard the in-progress edit, restoring what the widget showed. */ void ConductorPropertiesEditorWidget::reset() { if (!m_conductor) return; m_updating = true; - m_cpw->setProperties(m_initial); + m_cpw->setProperties(m_shown); m_updating = false; } @@ -230,54 +256,61 @@ void ConductorPropertiesEditorWidget::updateUi() { if (!m_conductor) return; m_updating = true; - m_initial = m_conductor->properties(); - m_cpw->setProperties(m_initial); + m_cpw->setProperties(m_conductor->properties()); + //Read back rather than keep the conductor's own values: a value the + //widget cannot show exactly must not count as an edit. + m_shown = m_cpw->properties(); m_updating = false; } /** @brief ConductorPropertiesEditorWidget::associatedUndo - @return the edit as a QPropertyUndoCommand, or nullptr if unchanged. + @return the edit as one undo step, or nullptr if nothing changes. + + Only the fields the user changed are written: the conductor keeps every + other value, including one the widget cannot show exactly. When "apply to all" is ticked, every conductor on the same potential is - updated in the same undo step (one undo reverts them all), exactly as the - modal dialog does (ConductorPropertiesDialog::PropertiesDialog). Otherwise - only the selected conductor is changed. + updated too, in the same undo step (one undo reverts them all), as the + modal dialog does (ConductorPropertiesDialog::PropertiesDialog). */ QUndoCommand *ConductorPropertiesEditorWidget::associatedUndo() const { if (!m_conductor) return nullptr; const ConductorProperties new_properties = m_cpw->properties(); - if (new_properties == m_conductor->properties()) return nullptr; + if (new_properties == m_shown) return nullptr; - QVariant old_value, new_value; - old_value.setValue(m_conductor->properties()); - new_value.setValue(new_properties); - - auto *undo = new QPropertyUndoCommand( - m_conductor, "properties", old_value, new_value); - undo->setText(tr("Modifier les propriétés d'un conducteur", "undo caption")); - - // Propagate to every conductor on the same potential, as the modal dialog - // does: each related conductor becomes a child command of the same undo - // step, set to the same target properties. + QList targets {m_conductor}; if (m_apply_all_cb && m_apply_all_cb->isChecked()) + for (Conductor *potential_conductor : m_conductor->relatedPotentialConductors()) + if (!targets.contains(potential_conductor)) + targets << potential_conductor; + + auto *undo = new QUndoCommand(); + int changed = 0; + for (Conductor *conductor : std::as_const(targets)) { - const auto potential = m_conductor->relatedPotentialConductors(); - if (!potential.isEmpty()) - { - undo->setText(tr("Modifier les propriétés de plusieurs conducteurs", - "undo caption")); - for (Conductor *potential_conductor : potential) - { - QVariant old_v; - old_v.setValue(potential_conductor->properties()); - new QPropertyUndoCommand( - potential_conductor, "properties", old_v, new_value, undo); - } - } + const ConductorProperties old_properties = conductor->properties(); + ConductorProperties properties = old_properties; + properties.applyChanges(m_shown, new_properties); + if (properties == old_properties) continue; + + QVariant old_value, new_value; + old_value.setValue(old_properties); + new_value.setValue(properties); + new QPropertyUndoCommand(conductor, "properties", old_value, new_value, undo); + ++changed; } + + if (!changed) + { + delete undo; + return nullptr; + } + undo->setText(changed == 1 + ? tr("Modifier les propriétés d'un conducteur", "undo caption") + : tr("Modifier les propriétés de plusieurs conducteurs", "undo caption")); return undo; } diff --git a/sources/ui/conductorpropertieseditorwidget.h b/sources/ui/conductorpropertieseditorwidget.h index 0479a6bee..a5f517738 100644 --- a/sources/ui/conductorpropertieseditorwidget.h +++ b/sources/ui/conductorpropertieseditorwidget.h @@ -54,6 +54,9 @@ class ConductorPropertiesEditorWidget : public PropertiesEditorWidget QString title() const override; bool setLiveEdit(bool live_edit) override; + protected: + bool eventFilter(QObject *watched, QEvent *event) override; + private: void connectChangeSignals(); void disconnectChangeSignals(); @@ -62,7 +65,9 @@ class ConductorPropertiesEditorWidget : public PropertiesEditorWidget ConductorPropertiesWidget *m_cpw = nullptr; QCheckBox *m_apply_all_cb = nullptr; Conductor *m_conductor = nullptr; - ConductorProperties m_initial; + //What the widget showed before the edit: the fields that + //differ from it are the ones the user changed. + ConductorProperties m_shown; QList m_live_connections; bool m_updating = false; }; diff --git a/tests/qttest/CMakeLists.txt b/tests/qttest/CMakeLists.txt index e428b6d49..e339f3721 100644 --- a/tests/qttest/CMakeLists.txt +++ b/tests/qttest/CMakeLists.txt @@ -510,6 +510,19 @@ target_compile_definitions(tst_terminaluuids PRIVATE "QET_TEST_BINARY_PATH=\"$\"" "QET_EXAMPLES_DIR=\"${QET_DIR}/examples\"") +# ConductorProperties::applyChanges() -- the Selection properties panel +# writes only the fields the user edited. +add_executable( + tst_conductorapplychanges + tst_conductorapplychanges.cpp + ${QET_DIR}/sources/conductorproperties.cpp + ${QET_DIR}/sources/qet.cpp + ${QET_DIR}/sources/qeticons.cpp + ${QET_DIR}/sources/shortcutmanager.cpp) +add_test(NAME tst_conductorapplychanges COMMAND tst_conductorapplychanges) +target_include_directories(tst_conductorapplychanges PRIVATE ${QET_DIR}/sources) +target_link_libraries(tst_conductorapplychanges PRIVATE Qt::Test Qt::Widgets Qt::Xml pugixml::pugixml) + # DiagramContext::fromXml() -- the two readers (QDom for projects, pugixml # for element definitions in the collection) give the same values: stray # spaces trimmed, accents kept. diff --git a/tests/qttest/tst_conductorapplychanges.cpp b/tests/qttest/tst_conductorapplychanges.cpp new file mode 100644 index 000000000..aa286b0a0 --- /dev/null +++ b/tests/qttest/tst_conductorapplychanges.cpp @@ -0,0 +1,167 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +#include + +#include + +#include "conductorproperties.h" +#include "qetapp.h" + + // qet.cpp needs it; the application is not linked +QString QETApp::m_interface_language; + +// ConductorProperties::applyChanges(): the Selection properties panel +// applies to each wire only the fields the user changed. Every field is +// checked on its own: change that one field, and a wire whose every field +// differs must take that field and keep all the others. +class tst_conductorapplychanges : public QObject +{ + Q_OBJECT + + using Setter = std::function; + + // One setter per field. Variant 1 always differs from 0 and 2, so + // a field that is not copied leaves the result visibly wrong; + // two-valued fields reuse variant 0's value for 2. + static QList> fields() + { + return { + {"type", [](ConductorProperties &p, int v) { + p.type = v == 1 ? ConductorProperties::Single : ConductorProperties::Multi; }}, + {"color", [](ConductorProperties &p, int v) { + p.color = QColor::fromRgb(10 + v, 0, 0); }}, + {"bicolor", [](ConductorProperties &p, int v) { + p.m_bicolor = v == 1; }}, + {"color_2", [](ConductorProperties &p, int v) { + p.m_color_2 = QColor::fromRgb(0, 10 + v, 0); }}, + {"dash_size", [](ConductorProperties &p, int v) { + p.m_dash_size = 2 + v; }}, + {"style", [](ConductorProperties &p, int v) { + p.style = Qt::PenStyle(Qt::SolidLine + v); }}, + {"text", [](ConductorProperties &p, int v) { + p.text = QStringLiteral("text%1").arg(v); }}, + {"text_color", [](ConductorProperties &p, int v) { + p.text_color = QColor::fromRgb(0, 0, 10 + v); }}, + {"formula", [](ConductorProperties &p, int v) { + p.m_formula = QStringLiteral("formula%1").arg(v); }}, + {"cable", [](ConductorProperties &p, int v) { + p.m_cable = QStringLiteral("cable%1").arg(v); }}, + {"bus", [](ConductorProperties &p, int v) { + p.m_bus = QStringLiteral("bus%1").arg(v); }}, + {"function", [](ConductorProperties &p, int v) { + p.m_function = QStringLiteral("function%1").arg(v); }}, + {"tension_protocol", [](ConductorProperties &p, int v) { + p.m_tension_protocol = QStringLiteral("24V%1").arg(v); }}, + {"wire_color", [](ConductorProperties &p, int v) { + p.m_wire_color = QStringLiteral("BU%1").arg(v); }}, + {"wire_section", [](ConductorProperties &p, int v) { + p.m_wire_section = QStringLiteral("1.%1").arg(v); }}, + {"show_text", [](ConductorProperties &p, int v) { + p.m_show_text = v == 1; }}, + {"text_size", [](ConductorProperties &p, int v) { + p.text_size = 7 + v; }}, + {"cond_size", [](ConductorProperties &p, int v) { + p.cond_size = 1.5 + v; }}, + {"verti_rotate_text", [](ConductorProperties &p, int v) { + p.verti_rotate_text = 90.0 * v; }}, + {"horiz_rotate_text", [](ConductorProperties &p, int v) { + p.horiz_rotate_text = 45.0 * v; }}, + {"one_text_per_folio", [](ConductorProperties &p, int v) { + p.m_one_text_per_folio = v == 1; }}, + {"horizontal_alignment", [](ConductorProperties &p, int v) { + p.m_horizontal_alignment = v == 0 ? Qt::AlignTop + : v == 1 ? Qt::AlignBottom : Qt::AlignVCenter; }}, + {"vertical_alignment", [](ConductorProperties &p, int v) { + p.m_vertical_alignment = v == 0 ? Qt::AlignLeft + : v == 1 ? Qt::AlignRight : Qt::AlignHCenter; }}, + {"single_line_ground", [](ConductorProperties &p, int v) { + p.singleLineProperties.hasGround = v == 1; }}, + {"single_line_neutral", [](ConductorProperties &p, int v) { + p.singleLineProperties.hasNeutral = v == 1; }}, + {"single_line_pen", [](ConductorProperties &p, int v) { + p.singleLineProperties.is_pen = v == 1; }}, + {"single_line_phases", [](ConductorProperties &p, int v) { + p.singleLineProperties.setPhasesCount(v + 1); }}, + }; + } + + // Every field set to the given variant + static ConductorProperties all(int variant) + { + ConductorProperties p; + for (const auto &f : fields()) + f.second(p, variant); + return p; + } + + private slots: + void eachFieldAlone_data() + { + QTest::addColumn("index"); + const auto list = fields(); + for (int i = 0; i < list.size(); ++i) + QTest::newRow(list.at(i).first) << i; + } + + // The shown wire is all variant 0, the user changes one field + // to variant 1, another selected wire is all variant 2: it + // takes that one field and keeps every other one. + void eachFieldAlone() + { + QFETCH(int, index); + const Setter set = fields().at(index).second; + + const ConductorProperties before = all(0); + ConductorProperties after = before; + set(after, 1); + QVERIFY(after != before); + + ConductorProperties other = all(2); + ConductorProperties expected = other; + set(expected, 1); + + other.applyChanges(before, after); + QVERIFY(other == expected); + } + + // Nothing edited: nothing copied, whatever the wire holds + void noEditChangesNothing() + { + const ConductorProperties shown = all(0); + ConductorProperties other = all(2); + const ConductorProperties kept = other; + other.applyChanges(shown, shown); + QVERIFY(other == kept); + } + + // Every field edited: the wire ends up exactly as edited, so + // no field compared by operator== is left out of applyChanges + void everyFieldEdited() + { + ConductorProperties other = all(2); + other.applyChanges(all(0), all(1)); + QVERIFY(other == all(1)); + } + + // Set the function on a wire with its own number and cable + void functionOnTwoWires() + { + ConductorProperties shown; + shown.text = QStringLiteral("101"); + shown.m_cable = QStringLiteral("W1"); + ConductorProperties edited = shown; + edited.m_function = QStringLiteral("24V DC"); + + ConductorProperties second; + second.text = QStringLiteral("102"); + second.m_cable = QStringLiteral("W2"); + second.m_function = QStringLiteral("old"); + second.applyChanges(shown, edited); + + QCOMPARE(second.m_function, QStringLiteral("24V DC")); + QCOMPARE(second.text, QStringLiteral("102")); + QCOMPARE(second.m_cable, QStringLiteral("W2")); + } +}; + +QTEST_GUILESS_MAIN(tst_conductorapplychanges) +#include "tst_conductorapplychanges.moc"