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()