From 408ef5114d2e1274f657f6d2fb36846e106705d9 Mon Sep 17 00:00:00 2001 From: Kellermorph Date: Tue, 13 Oct 2026 20:13:18 +0200 Subject: [PATCH] Terminal: pass the voltage across a separation only when it was edited The three code paths that copied the voltage / protocol to the far side of a function separated terminal -- the conductor properties dialog, search/replace and the script API -- compared it against the far wire's own properties instead of against the edit itself. The two sides differ on that field just as often as not, so the comparison held no matter what was edited and every change overwrote the far side's voltage: setting a colour on a 24 V side dragged the 0 V one along, where master left it alone. Compare the edit -- what the dialog was filled with against what the user made of it -- so that a change of the voltage / protocol is what crosses, which is what the selection panel multi-edit already did. The values are read before the undo commands are pushed, since QPropertyUndoCommand applies on push. An explicitly selected wire is never the far side of anything: picking one wire on each side of a separation and editing them edits them both, in full, however the selection happens to be walked. Nor is a wire that belongs to a potential on any path, so reaching it once normally and once across a separation gives it the full edit. --- .../searchandreplaceworker.cpp | 22 +++-- sources/conductormultiedit.h | 42 +++++++-- sources/conductorproperties.cpp | 7 ++ sources/scripting/qetscriptapi.cpp | 16 +++- sources/ui/conductorpropertiesdialog.cpp | 9 +- tests/qttest/tst_conductormultiedit.cpp | 92 +++++++++++++++++++ 6 files changed, 170 insertions(+), 18 deletions(-) diff --git a/sources/SearchAndReplace/searchandreplaceworker.cpp b/sources/SearchAndReplace/searchandreplaceworker.cpp index 8af0ad5bc..5f4b55998 100644 --- a/sources/SearchAndReplace/searchandreplaceworker.cpp +++ b/sources/SearchAndReplace/searchandreplaceworker.cpp @@ -226,9 +226,12 @@ void SearchAndReplaceWorker::replaceConductor(QList list) project_->undoStack()->beginMacro(QObject::tr("Search / replace conductors properties.")); for (Conductor *c : list) { - ConductorProperties cp = applyChange(c->properties(), m_conductor_properties); + //Captured before the pushes below: QPropertyUndoCommand applies + //on push, so reading it afterwards would give back @a cp. + const ConductorProperties old_properties = c->properties(); + ConductorProperties cp = applyChange(old_properties, m_conductor_properties); - if (cp != c->properties()) + if (cp != old_properties) { QSet conductors_list = c->relatedPotentialConductors(true); conductors_list << c; @@ -241,12 +244,16 @@ void SearchAndReplaceWorker::replaceConductor(QList list) } //The other side of a function separated terminal element - //only receives the voltage / protocol (@see + //only receives the voltage / protocol, and only when this + //replacement really changed it: the comparison is this + //replacement's own before / after (@a old_properties vs + //@a cp), never the far wire's own value, which would make + //every replacement overwrite it (@see //Conductor::functionSeparatedConductors). for (Conductor *cc : c->functionSeparatedConductors(true)) { ConductorProperties separated_cp = cc->properties(); - separated_cp.applyTensionProtocolChange(cc->properties(), cp); + separated_cp.applyTensionProtocolChange(old_properties, cp); if (separated_cp == cc->properties()) continue; @@ -369,12 +376,15 @@ void SearchAndReplaceWorker::replaceAdvanced( } //The other side of a function separated terminal element - //only receives the voltage / protocol (@see + //only receives the voltage / protocol, and only when + //this replacement really changed it: the comparison is + //this replacement's own before / after, never the far + //wire's own value (@see //Conductor::functionSeparatedConductors). for (Conductor *c : conductor->functionSeparatedConductors(true)) { ConductorProperties separated_properties = c->properties(); - separated_properties.applyTensionProtocolChange(c->properties(), new_properties); + separated_properties.applyTensionProtocolChange(old_properties, new_properties); if (separated_properties == c->properties()) continue; diff --git a/sources/conductormultiedit.h b/sources/conductormultiedit.h index e580c1074..baf6898e8 100644 --- a/sources/conductormultiedit.h +++ b/sources/conductormultiedit.h @@ -67,6 +67,13 @@ namespace ConductorMultiEdit terminal element -- conductors undo() then gives the voltage / protocol only, and no other property. nullptr, the default, leaves those conductors out of the returned list entirely. + + A conductor the user picked themselves is never part of it: the + selection is the edit reaching that conductor, so it takes the + full edit however remote it is from the other selected ones -- + picking a wire on each side of a separation and editing them is + exactly that. Nor is a conductor that belongs to a potential, on + any path, however far that potential is reached. @return each conductor an edit reaches, once */ template @@ -75,19 +82,38 @@ namespace ConductorMultiEdit { QList list; QSet seen; + //The explicitly picked ones, so a separation met while walking + //the list cannot demote a wire the user asked to edit. + QSet chosen; + for (T *conductor : selected) + if (conductor) + chosen.insert(conductor); + //What the potentials reach, and what only a function separation + //reaches. A conductor can be in both -- wired normally on one + //path and across a separation on another -- and then belongs to + //the potential. + QSet potential_reached, separated_reached; + for (T *conductor : selected) { - //Already reached through an earlier conductor's potential: - //its own potential is the same one. - if (!conductor || seen.contains(conductor)) + if (!conductor) continue; - seen.insert(conductor); - list << conductor; - if (!apply_all) + //Already reached through an earlier conductor's potential or + //separation: its own potential adds nothing new to walk over + //-- unless the user picked it, because a separation may have + //put it in the list before its potential was ever reached. + const bool fresh = !seen.contains(conductor); + if (fresh) + { + seen.insert(conductor); + list << conductor; + } + if (!apply_all || (!fresh && !chosen.contains(conductor))) continue; const QSet potential = conductor->relatedPotentialConductors(); for (T *other : potential) { + potential_reached.insert(other); if (!seen.contains(other)) { seen.insert(other); @@ -99,15 +125,17 @@ namespace ConductorMultiEdit const QSet separated = conductor->functionSeparatedConductors(); for (T *other : separated) { + separated_reached.insert(other); if (!seen.contains(other)) { seen.insert(other); list << other; - function_separated->insert(other); } } } } + if (function_separated) + *function_separated = separated_reached - potential_reached - chosen; return list; } diff --git a/sources/conductorproperties.cpp b/sources/conductorproperties.cpp index 962334d8f..6fc5564e8 100644 --- a/sources/conductorproperties.cpp +++ b/sources/conductorproperties.cpp @@ -832,6 +832,13 @@ void ConductorProperties::applyChanges(const ConductorProperties &before, protocol describes the electrical node itself, while the text, the function, the wire color... describe one side of the separation and stay local to it. + + Both arguments describe one edit -- what was shown to the user, and + what the user made of it -- so that only an edit which really + changed the voltage / protocol passes it on. Passing the far wire's + own properties as @a before instead would make every edit overwrite + its voltage: the two sides differ on that field just as often as + not, so the comparison would hold no matter what was edited. @param before : the properties as they were shown to the user @param after : the properties after the user's edit */ diff --git a/sources/scripting/qetscriptapi.cpp b/sources/scripting/qetscriptapi.cpp index 2d19b4408..d56b84046 100644 --- a/sources/scripting/qetscriptapi.cpp +++ b/sources/scripting/qetscriptapi.cpp @@ -1211,13 +1211,17 @@ bool QetScriptApi::setConductorProperty(int folioIndex, const QString &elementUu Conductor *conductor = findConductor(folioIndex, elementUuid, terminalIndex, caller); if (!conductor) return false; - ConductorProperties properties = conductor->properties(); + //Captured before the pushes below: QPropertyUndoCommand applies on + //push, so reading the properties afterwards would already give back + //@a properties. + const ConductorProperties before = conductor->properties(); + ConductorProperties properties = before; if (!setConductorPropertyValue(properties, property, value)) { log(QStringLiteral("qet.%1: '%2' is not a valid value for %3") .arg(caller, value, property)); return false; } - if (properties == conductor->properties()) return true; // already so + if (properties == before) return true; // already so QSet potential = conductor->relatedPotentialConductors(true); potential << conductor; @@ -1232,11 +1236,15 @@ bool QetScriptApi::setConductorProperty(int folioIndex, const QString &elementUu } //The other side of a function separated terminal element gets the //voltage / protocol only: it is the one property that still - //describes the electrical node rather than one side of it. + //describes the electrical node rather than one side of it. And it + //only arrives when this call really changed it -- before / after + //is the edit, not the far wire's own value, which differs from it + //just as often as not and would then be overwritten by every call, + //colour included. for (Conductor *c : conductor->functionSeparatedConductors(true)) { ConductorProperties separated_properties = c->properties(); - separated_properties.applyTensionProtocolChange(c->properties(), properties); + separated_properties.applyTensionProtocolChange(before, properties); if (separated_properties == c->properties()) continue; diff --git a/sources/ui/conductorpropertiesdialog.cpp b/sources/ui/conductorpropertiesdialog.cpp index f271d52d6..c539bdf7c 100644 --- a/sources/ui/conductorpropertiesdialog.cpp +++ b/sources/ui/conductorpropertiesdialog.cpp @@ -95,11 +95,18 @@ void ConductorPropertiesDialog::PropertiesDialog(Conductor *conductor, //electrical node and not one side of it. if (cpd.applyAll()) { + //Only an edit that really changed the voltage / protocol passes + //it on, so the comparison is the edit itself -- what the dialog + //was filled with, against what the user left in it -- and never + //the far wire's own value, which differs from it just as often + //as not (a 24 V side against a 0 V one, say) and would then be + //overwritten by every edit, voltage included. + const ConductorProperties shown = conductor->properties(); foreach (Conductor *separated_conductor, conductor->functionSeparatedConductors()) { ConductorProperties separated_properties = separated_conductor->properties(); separated_properties.applyTensionProtocolChange( - separated_conductor->properties(), + shown, cpd.properties()); if (separated_properties == separated_conductor->properties()) continue; diff --git a/tests/qttest/tst_conductormultiedit.cpp b/tests/qttest/tst_conductormultiedit.cpp index 859271f9d..757bc2544 100644 --- a/tests/qttest/tst_conductormultiedit.cpp +++ b/tests/qttest/tst_conductormultiedit.cpp @@ -165,6 +165,98 @@ class tst_conductormultiedit : public QObject QCOMPARE(c.m_properties.m_function, QStringLiteral("fnC")); } + // The explicitly picked side of a separation: selecting one + // wire on each side and editing them edits them both, in + // full -- however the selection happens to be walked (#review) + void selectedWireIsNeverSeparated() + { + FakeWire a("1", "fnA", "", {}), c("3", "fnC", "", {}); + FakeWire::separate({&a}, {&c}); + + const ConductorProperties original_a = a.properties(); + const ConductorProperties original_c = c.properties(); + const ConductorProperties after = edited(original_a, + &ConductorProperties::m_function, "PWR"); + + for (const Wires &selection : {Wires {&a, &c}, Wires {&c, &a}}) + { + //Each order starts from the same, unedited state + a.setProperties(original_a); + c.setProperties(original_c); + + QSet separated; + const Wires t = ConductorMultiEdit::targets(selection, true, + &separated); + //c is picked, so it is not "the far side" of anything + QCOMPARE(separated, (QSet {})); + + QUndoStack stack; + QUndoCommand *undo = ConductorMultiEdit::undo(t, original_a, + after, separated); + QVERIFY(undo); + stack.push(undo); + QCOMPARE(a.m_properties.m_function, QStringLiteral("PWR")); + QCOMPARE(c.m_properties.m_function, QStringLiteral("PWR")); + } + } + + // A wire that belongs to the potential as well -- on any path, + // however far that path runs -- takes the full edit, even when a + // separation also reaches it. The fake's potential is one hop + // deep, unlike the real one, so the shared potential is stated + // in full here. + void wireReachableBothWaysIsNotSeparated() + { + FakeWire a("1", "fnA", "", {}), b("2", "fnB", "", {}), + c("3", "fnC", "", {}); + FakeWire::join({&a, &b, &c}); + FakeWire::separate({&a}, {&c}); + + QSet separated; + const Wires t = ConductorMultiEdit::targets(Wires {&a}, true, + &separated); + QCOMPARE(QSet(t.begin(), t.end()), + (QSet {&a, &b, &c})); + QCOMPARE(separated, (QSet {})); + + const ConductorProperties shown = a.properties(); + QUndoStack stack; + stack.push(ConductorMultiEdit::undo(t, shown, edited(shown, + &ConductorProperties::m_function, "PWR"), separated)); + QCOMPARE(c.m_properties.m_function, QStringLiteral("PWR")); + } + + // The far side only takes the voltage when the edit changed it: + // editing the function of a 24 V wire must leave the 0 V one + // at 0 V, not drag it along (#review) + void separatedWireKeepsVoltageWhenNotEdited() + { + FakeWire a("1", "fnA", "", {}), c("3", "fnC", "", {}); + FakeWire::separate({&a}, {&c}); + a.m_properties.m_tension_protocol = "24V"; + c.m_properties.m_tension_protocol = "0V"; + + QSet separated; + const Wires t = ConductorMultiEdit::targets(Wires {&a}, true, + &separated); + const ConductorProperties shown = a.properties(); + QUndoStack stack; + + //Only the function changes: the far side changes nothing + stack.push(ConductorMultiEdit::undo(t, shown, edited(shown, + &ConductorProperties::m_function, "PWR"), separated)); + QCOMPARE(c.m_properties.m_function, QStringLiteral("fnC")); + QCOMPARE(c.m_properties.m_tension_protocol, QStringLiteral("0V")); + + //Now the voltage changes too: it crosses, and only it + const ConductorProperties shown_now = a.properties(); + stack.push(ConductorMultiEdit::undo(t, shown_now, edited(shown_now, + &ConductorProperties::m_tension_protocol, "30V"), separated)); + QCOMPARE(c.m_properties.m_tension_protocol, QStringLiteral("30V")); + QCOMPARE(c.m_properties.m_function, QStringLiteral("fnC")); + QCOMPARE(a.m_properties.m_tension_protocol, QStringLiteral("30V")); + } + // One edit of three wires: each takes the function, keeps its // own number and cable; one undo restores all three exactly void undoRedoThreeWires()