Fix Do not mark a pipeline or workflow dirty when a dialog is OK without edits

[leehaut]

Opening a transform or action, changing nothing, and clicking OK used to mark the pipeline or workflow as needing save. Cancel or closing the dialog without OK did not. This was easy to hit on every component, because it came from the shared OK path rather than from one plugin.

How it works

Two separate false positives were stacked:

1. SWT widgets turn null into "". XmlMetadataUtil omits a null field and writes <tag/> for an empty string. Dialog getInfo() / ok() always writes widget text back, so OK without edits produced XML that failed a string equals. Comparison now uses XmlHandler.sameContentIgnoringEmptyValues, which treats an omitted element as the same as an empty one. Undo snapshot compare uses the same helper. Disk format is unchanged: null is still omitted, "" is still a tag.

2. The dirty flag was restored from a clone. After OK, the delegate did setChanged(before.hasChanged()) when XML matched. Copying a transform or action goes through setLocation / setRowDistribution, and those setters flip wrapperChanged. A clone of a clean object therefore looked dirty, so OK without edits still marked the file changed. The live hasChanged() flag from before the dialog is used instead. Copy factories also clear that induced flag when the source is unchanged.

Workflow actions had no XML compare at all. Several dialogs call setChanged() unconditionally on OK (Start is the obvious case). They now use the same omitted-vs-empty comparison, then restore the live flag when content did not change.

Entry points:

• HopGuiPipelineTransformDelegate (transform dialog, partitioning, error handling)
• HopGuiWorkflowActionDelegate
• DefaultTransformMetaCopyFactory / DefaultActionCopyFactory

Tests

• XmlHandlerUnitTest (omitted vs <tag/> / <tag></tag>, nested empty shells, real text still detected, trailing empty list item)
• XmlMetadataUtilTest.nullAndEmptyStringSerializeDifferentlyButCompareEqualForChangeDetection
• XmlSnapshotUndoTest.sameXmlContentTreatsOmittedAndEmptyElementsAsEqual
• DefaultTransformMetaCopyFactoryTest.copyOfUnchangedTransformIsNotMarkedChanged
• DefaultActionCopyFactoryTest.copyOfUnchangedActionMetaIsNotMarkedChanged

[mattcasters]

Hi @leehaut,

Thank you for investigating this issue! You did great detective work identifying the two root causes behind why pipelines and workflows were getting marked dirty after simply clicking OK:

1. The copy factories inadvertently flipping wrapperChanged via setter calls like setLocation.
2. The asymmetry between null and "" across dialog round-trips.

The fixes to DefaultTransformMetaCopyFactory, DefaultActionCopyFactory, and preserving the live alreadyChanged flag in the delegates are spot-on. Those should definitely be merged.

However, I have some strong reservations regarding the additions to XmlHandler (sameContentIgnoringEmptyValues) and its usage in XmlSnapshotUndo:

---

1. Performance Impact on the UI Thread

In XmlSnapshotUndo.sameXmlContent, this comparison is run on commitDialogUndo for the entire pipeline or workflow XML.

Calling XmlHandler.sameContentIgnoringEmptyValues means:

• Decompressing the entire pipeline snapshot twice.
• Parsing both complete pipeline XML documents into full W3C DOM Document trees (wrapLoadXmlString) via DocumentBuilder.parse().
• Recursively walking every single node, collecting child lists, and trimming text on the SWT UI thread.
• applyDirtyFlag then repeats this check comparing against lastSavedSnapshot.

For large pipelines (dozens or hundreds of transforms, hops, notes, and connections), running full DOM tree builds and recursive comparisons on the UI thread whenever a dialog closes introduces significant latency and heavy GC churn. The snapshot undo system was specifically designed to use fast, single-pass string comparison: decompress(left).equals(decompress(right)).

---

2. Edge Cases and Potential Data Loss in the XML Comparator

Implementing a custom XML equivalence crawler introduces subtle bugs:

• Mixed content / text dropped: In sameElementIgnoringEmptyValues, if an element has child elements (!leftChildren.isEmpty()), directText(left) is never checked. Any direct text on that parent element is ignored in comparison.
• False equality on list / table rows: In extraEmptyListItemIsIgnored, <fields><field><name>a</name></field></fields> is considered equal to <fields><field><name>a</name></field><field/></fields>. If a user adds a new blank row in a table/grid, or clears all values in a row, the comparator considers it unchanged. The file will not be marked dirty, no undo point will be created, and the edit won't be saved.
• Layering: XmlHandler is a low-level XML utility class. It shouldn't contain domain-specific heuristics where omitted elements ≡ empty elements. In standard XML, an omitted element and an empty element can have very different meanings.

---

3. Suggested Alternative Approach

The core reason before.getXml().equals(after.getXml()) failed is:

• An uninitialized field was null → XmlMetadataUtil omitted the tag.
• The dialog opened, the text widget showed "", and on OK the dialog set the field to "".
• XmlMetadataUtil serialized "" as <tag/>.

In Hop, empty strings and null for text properties are semantically the same (empty/unset). Hop XML files generally don't need or want empty tags like <tag/> cluttering the document.

Rather than parsing DOM trees after the fact to paper over this difference:

1. In XmlMetadataUtil: If we don't serialize empty strings (treating "" like null in serializeFieldValueToXml):

   if (value != null && !(value instanceof String s && s.isEmpty()))

   (or alternatively in GuiCompositeWidgets / dialog binding by not overwriting an existing null value with "" if the control is empty)
2. Then before.getXml() and after.getXml() will produce byte-for-byte identical XML.
3. XmlSnapshotUndo can revert back to the simple, fast decompress(left).equals(decompress(right)).

What do you think about separating the copy-factory fixes from the XML comparison, and handling the empty-string serialization at the source instead?

[mattcasters]

See the previous comment with details of the requested changes.

[leehaut]

Thanks @mattcasters — I agree with the split, and with keeping the copy-factory / live alreadyChanged changes.

On XmlSnapshotUndo: you are right. Comparing the entire pipeline or workflow with a DOM walk on the UI thread is too heavy. I will revert sameXmlContent to decompress(left).equals(decompress(right)).

On treating empty strings like null in XmlMetadataUtil (not writing the tag): that may well be the cleaner long-term fix, and I am not ruling it out. It does change what we persist, though, so I would like to hear from @hansva before we go that way.

On #8173 the first attempt omitted empty tags the same as null, and the concern then was that, on transform/pipeline upgrades, a missing field and an explicit blank are not always the same thing. I do not want to reopen that without checking.

@hansva, would you be OK with skipping empty string tags at serialization time, or should we keep null (omitted) and "" (<tag/>) distinct on disk?

In the meantime I can land the copy-factory / alreadyChanged part and take the full-document XML compare back out, so the undo path stays a simple string equals.