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.

sources/SearchAndReplace/searchandreplaceworker.cpp

@@ -226,9 +226,12 @@ void SearchAndReplaceWorker::replaceConductor(QList<Conductor *> 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 <Conductor *> conductors_list = c->relatedPotentialConductors(true);
 			conductors_list << c;
@@ -241,12 +244,16 @@ void SearchAndReplaceWorker::replaceConductor(QList<Conductor *> 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;
 

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 <typename T>
@@ -75,19 +82,38 @@ namespace ConductorMultiEdit
 	{
 		QList<T *> list;
 		QSet<T *> seen;
+			//The explicitly picked ones, so a separation met while walking
+			//the list cannot demote a wire the user asked to edit.
+		QSet<T *> 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<T *> 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<T *> 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<T *> 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;
 	}
 

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
 */

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<Conductor *> 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;
 

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;

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<FakeWire *> separated;
+				const Wires t = ConductorMultiEdit::targets(selection, true,
+								      &separated);
+					//c is picked, so it is not "the far side" of anything
+				QCOMPARE(separated, (QSet<FakeWire *> {}));
+
+				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<FakeWire *> separated;
+			const Wires t = ConductorMultiEdit::targets(Wires {&a}, true,
+								      &separated);
+			QCOMPARE(QSet<FakeWire *>(t.begin(), t.end()),
+				 (QSet<FakeWire *> {&a, &b, &c}));
+			QCOMPARE(separated, (QSet<FakeWire *> {}));
+
+			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<FakeWire *> 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()