This is an automated email from the ASF dual-hosted git repository.
hansva pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/hop.git
The following commit(s) were added to refs/heads/main by this push:
new 4f5df5e4ad Fix Do not mark a pipeline or workflow dirty when a dialog
is OK without edits (#8247)
4f5df5e4ad is described below
commit 4f5df5e4ad2dbc90005db8d1385430708322b6cb
Author: Lance <[email protected]>
AuthorDate: Fri Sep 18 00:44:15 2026 +0800
Fix Do not mark a pipeline or workflow dirty when a dialog is OK without
edits (#8247)
* Do not mark a pipeline or workflow dirty when a dialog is OK without edits
Signed-off-by: lance <[email protected]>
* Keep null and empty string distinct on disk for dialog OK detection
Signed-off-by: lance <[email protected]>
---------
Signed-off-by: lance <[email protected]>
---
.../metadata/serializer/xml/DialogOkContent.java | 248 +++++++++++++++++++++
.../serializer/xml/DialogOkContentTest.java | 156 +++++++++++++
.../serializer/xml/XmlMetadataUtilTest.java | 6 +-
.../copy/DefaultTransformMetaCopyFactory.java | 4 +
.../action/copy/DefaultActionCopyFactory.java | 9 +-
.../copy/DefaultTransformMetaCopyFactoryTest.java | 14 ++
.../action/copy/DefaultActionCopyFactoryTest.java | 11 +
.../ui/core/gui/GuiCompositeWidgetsGroupTest.java | 42 +++-
.../hop/ui/core/gui/GuiCompositeWidgets.java | 9 +
.../delegates/HopGuiPipelineTransformDelegate.java | 32 ++-
.../delegates/HopGuiWorkflowActionDelegate.java | 17 ++
11 files changed, 524 insertions(+), 24 deletions(-)
diff --git
a/core/src/main/java/org/apache/hop/metadata/serializer/xml/DialogOkContent.java
b/core/src/main/java/org/apache/hop/metadata/serializer/xml/DialogOkContent.java
new file mode 100644
index 0000000000..35691f2430
--- /dev/null
+++
b/core/src/main/java/org/apache/hop/metadata/serializer/xml/DialogOkContent.java
@@ -0,0 +1,248 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hop.metadata.serializer.xml;
+
+import java.lang.reflect.Array;
+import java.lang.reflect.Field;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.IdentityHashMap;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import org.apache.hop.metadata.api.HopMetadataProperty;
+import org.apache.hop.metadata.util.ReflectionUtil;
+
+/**
+ * Dialog-OK change detection that keeps {@code null} and {@code ""} distinct
on disk.
+ *
+ * <p>SWT text widgets cannot hold {@code null}, so an untouched empty control
round-trips as {@code
+ * ""}. For String fields those two values are treated as the same content.
List size still counts:
+ * an extra blank table row is a real edit. Serialization is unchanged: {@code
null} is omitted,
+ * {@code ""} is written as an empty tag.
+ */
+public final class DialogOkContent {
+ private DialogOkContent() {}
+
+ /**
+ * True when a widget read-back of {@code ""} should not overwrite a field
that is still {@code
+ * null}.
+ */
+ public static boolean widgetEmptyLeavesNull(Object currentValue, Object
widgetValue) {
+ return currentValue == null && widgetValue instanceof String text &&
text.isEmpty();
+ }
+
+ /**
+ * True when two objects represent the same persisted content for dialog OK,
treating String
+ * {@code null} and {@code ""} as equal.
+ */
+ public static boolean same(Object left, Object right) {
+ return same(left, right, Collections.newSetFromMap(new
IdentityHashMap<>()));
+ }
+
+ private static boolean same(Object left, Object right, Set<Object> visiting)
{
+ if (left == right || (isNullOrEmptyString(left) &&
isNullOrEmptyString(right))) {
+ return true;
+ }
+ if (left == null) {
+ return isEmptyContainer(right);
+ }
+ if (right == null) {
+ return isEmptyContainer(left);
+ }
+ return sameStructured(left, right, visiting);
+ }
+
+ private static boolean sameStructured(Object left, Object right, Set<Object>
visiting) {
+ if (left instanceof String || right instanceof String) {
+ return Objects.equals(left, right);
+ }
+ if (left instanceof Collection<?> leftItems && right instanceof
Collection<?> rightItems) {
+ return sameCollections(leftItems, rightItems, visiting);
+ }
+ if (left instanceof Map<?, ?> leftMap && right instanceof Map<?, ?>
rightMap) {
+ return sameMaps(leftMap, rightMap, visiting);
+ }
+ if (left.getClass().isArray() && right.getClass().isArray()) {
+ return sameArrays(left, right, visiting);
+ }
+ if (hasMetadataProperties(left) || hasMetadataProperties(right)) {
+ return sameMetadataObjects(left, right, visiting);
+ }
+ return Objects.equals(left, right) || sameGetXml(left, right);
+ }
+
+ private static boolean hasMetadataProperties(Object value) {
+ return
XmlMetadataUtil.hasHopMetadataSerializableProperties(value.getClass());
+ }
+
+ private static boolean sameMetadataObjects(Object left, Object right,
Set<Object> visiting) {
+ if (visiting.contains(left) && visiting.contains(right)) {
+ return true;
+ }
+ visiting.add(left);
+ visiting.add(right);
+ try {
+ return sameMetadata(left, right, visiting);
+ } finally {
+ visiting.remove(left);
+ visiting.remove(right);
+ }
+ }
+
+ private static boolean sameCollections(
+ Collection<?> leftItems, Collection<?> rightItems, Set<Object> visiting)
{
+ if (leftItems.size() != rightItems.size()) {
+ return false;
+ }
+ Iterator<?> leftIterator = leftItems.iterator();
+ Iterator<?> rightIterator = rightItems.iterator();
+ while (leftIterator.hasNext()) {
+ if (!same(leftIterator.next(), rightIterator.next(), visiting)) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ private static boolean sameMaps(Map<?, ?> leftMap, Map<?, ?> rightMap,
Set<Object> visiting) {
+ if (leftMap.size() != rightMap.size()) {
+ return false;
+ }
+ for (Map.Entry<?, ?> entry : leftMap.entrySet()) {
+ if (!rightMap.containsKey(entry.getKey())) {
+ return false;
+ }
+ if (!same(entry.getValue(), rightMap.get(entry.getKey()), visiting)) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ private static boolean sameArrays(Object left, Object right, Set<Object>
visiting) {
+ int length = Array.getLength(left);
+ if (length != Array.getLength(right)) {
+ return false;
+ }
+ for (int i = 0; i < length; i++) {
+ if (!same(Array.get(left, i), Array.get(right, i), visiting)) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ private static boolean sameMetadata(Object left, Object right, Set<Object>
visiting) {
+ Class<?> leftClass = left.getClass();
+ Class<?> rightClass = right.getClass();
+ if (leftClass != rightClass) {
+ return false;
+ }
+ List<Field> fields =
+ ReflectionUtil.findAllFields(leftClass, new
MetadataPropertyKeyFunction(), false);
+ for (Field field : fields) {
+ if (!isComparableProperty(field)) {
+ continue;
+ }
+ try {
+ boolean isBoolean = field.getType() == boolean.class ||
field.getType() == Boolean.class;
+ Object leftValue = ReflectionUtil.getFieldValue(left, field.getName(),
isBoolean);
+ Object rightValue = ReflectionUtil.getFieldValue(right,
field.getName(), isBoolean);
+ if (!same(leftValue, rightValue, visiting)) {
+ return false;
+ }
+ } catch (Exception e) {
+ return false;
+ }
+ }
+ try {
+ for (Method getter : ReflectionUtil.findAllMethods(leftClass, "get")) {
+ if (!isComparableGetter(getter)) {
+ continue;
+ }
+ Object leftValue = getter.invoke(left);
+ Method rightGetter = rightClass.getMethod(getter.getName());
+ Object rightValue = rightGetter.invoke(right);
+ if (!same(leftValue, rightValue, visiting)) {
+ return false;
+ }
+ }
+ } catch (Exception e) {
+ return false;
+ }
+ return true;
+ }
+
+ private static boolean isComparableProperty(Field field) {
+ int modifiers = field.getModifiers();
+ if (Modifier.isTransient(modifiers)
+ || Modifier.isVolatile(modifiers)
+ || Modifier.isStatic(modifiers)) {
+ return false;
+ }
+ HopMetadataProperty property =
field.getAnnotation(HopMetadataProperty.class);
+ return property != null && !property.isExcludedFromSerialization();
+ }
+
+ private static boolean isComparableGetter(Method getter) {
+ HopMetadataProperty property =
getter.getAnnotation(HopMetadataProperty.class);
+ return property != null
+ && !property.isExcludedFromSerialization()
+ && getter.getParameterCount() == 0;
+ }
+
+ private static boolean isNullOrEmptyString(Object value) {
+ return value == null || (value instanceof String text && text.isEmpty());
+ }
+
+ private static boolean isEmptyContainer(Object value) {
+ if (value instanceof Collection<?> items) {
+ return items.isEmpty();
+ }
+ if (value instanceof Map<?, ?> map) {
+ return map.isEmpty();
+ }
+ return value != null && value.getClass().isArray() &&
Array.getLength(value) == 0;
+ }
+
+ /**
+ * Fallback for objects that persist with a custom {@code getXml()} and no
{@code
+ * HopMetadataProperty} fields. Empty-string round-trips are not rewritten
here; those classes
+ * typically already write null and {@code ""} the same way.
+ */
+ private static boolean sameGetXml(Object left, Object right) {
+ try {
+ Method leftXml = left.getClass().getMethod("getXml");
+ Method rightXml = right.getClass().getMethod("getXml");
+ if (leftXml.getParameterCount() != 0
+ || rightXml.getParameterCount() != 0
+ || leftXml.getReturnType() != String.class
+ || rightXml.getReturnType() != String.class) {
+ return false;
+ }
+ return Objects.equals(leftXml.invoke(left), rightXml.invoke(right));
+ } catch (Exception e) {
+ return false;
+ }
+ }
+}
diff --git
a/core/src/test/java/org/apache/hop/metadata/serializer/xml/DialogOkContentTest.java
b/core/src/test/java/org/apache/hop/metadata/serializer/xml/DialogOkContentTest.java
new file mode 100644
index 0000000000..69160ab84e
--- /dev/null
+++
b/core/src/test/java/org/apache/hop/metadata/serializer/xml/DialogOkContentTest.java
@@ -0,0 +1,156 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hop.metadata.serializer.xml;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.hop.metadata.serializer.xml.classes.Field;
+import org.apache.hop.metadata.serializer.xml.classes.Info;
+import org.apache.hop.metadata.serializer.xml.classes.MetaData;
+import org.apache.hop.metadata.serializer.xml.classes.TestEnum;
+import org.junit.jupiter.api.Test;
+
+/** Unit test for {@link DialogOkContent} */
+class DialogOkContentTest {
+
+ @Test
+ void widgetEmptyLeavesNullOnlyWhenCurrentIsNull() {
+ assertTrue(DialogOkContent.widgetEmptyLeavesNull(null, ""));
+ assertFalse(DialogOkContent.widgetEmptyLeavesNull("", ""));
+ assertFalse(DialogOkContent.widgetEmptyLeavesNull(null, "typed"));
+ assertFalse(DialogOkContent.widgetEmptyLeavesNull("hello", ""));
+ assertFalse(DialogOkContent.widgetEmptyLeavesNull(null, null));
+ }
+
+ @Test
+ void nullAndEmptyStringAreTheSameContent() {
+ assertTrue(DialogOkContent.same(null, ""));
+ assertTrue(DialogOkContent.same("", null));
+ assertTrue(DialogOkContent.same("", ""));
+ assertTrue(DialogOkContent.same(null, null));
+ }
+
+ @Test
+ void realStringChangeIsDetected() {
+ assertFalse(DialogOkContent.same(null, "stop"));
+ assertFalse(DialogOkContent.same("", "stop"));
+ assertFalse(DialogOkContent.same("old", "new"));
+ assertFalse(DialogOkContent.same("old", ""));
+ }
+
+ @Test
+ void whitespaceIsNotTreatedAsEmpty() {
+ assertFalse(DialogOkContent.same(null, " "));
+ assertFalse(DialogOkContent.same("", " "));
+ }
+
+ @Test
+ void fieldNullFormatEqualsEmptyFormat() {
+ Field withNull = new Field("a", "String", 50, -1, null, TestEnum.ONE);
+ Field withEmpty = new Field("a", "String", 50, -1, "", TestEnum.ONE);
+ assertTrue(DialogOkContent.same(withNull, withEmpty));
+ }
+
+ @Test
+ void fieldFormatTextChangeIsDetected() {
+ Field before = new Field("a", "String", 50, -1, null, TestEnum.ONE);
+ Field after = new Field("a", "String", 50, -1, "yyyy", TestEnum.ONE);
+ assertFalse(DialogOkContent.same(before, after));
+ }
+
+ @Test
+ void extraEmptyListItemIsAChange() {
+ MetaData oneField = new MetaData();
+ oneField.getFields().add(new Field("a", "String", 50, -1, null,
TestEnum.ONE));
+
+ MetaData trailingEmpty = new MetaData();
+ trailingEmpty.getFields().add(new Field("a", "String", 50, -1, null,
TestEnum.ONE));
+ trailingEmpty.getFields().add(new Field());
+
+ assertFalse(DialogOkContent.same(oneField, trailingEmpty));
+ }
+
+ @Test
+ void emptyListAndNullListAreTheSame() {
+ MetaData withList = new MetaData();
+ MetaData withNullList = new MetaData();
+ withNullList.setFields(null);
+ assertTrue(DialogOkContent.same(withList, withNullList));
+ }
+
+ @Test
+ void nestedNullStringEqualsEmptyString() {
+ MetaData left = new MetaData();
+ left.setInfo(new Info(null, "b"));
+ MetaData right = new MetaData();
+ right.setInfo(new Info("", "b"));
+ assertTrue(DialogOkContent.same(left, right));
+ }
+
+ @Test
+ void filenameChangeIsDetected() {
+ MetaData left = new MetaData();
+ left.setFilename(null);
+ MetaData right = new MetaData();
+ right.setFilename("file.csv");
+ assertFalse(DialogOkContent.same(left, right));
+ }
+
+ @Test
+ void enumChangeIsDetected() {
+ Field left = new Field("a", "String", 50, -1, null, TestEnum.ONE);
+ Field right = new Field("a", "String", 50, -1, null, TestEnum.TWO);
+ assertFalse(DialogOkContent.same(left, right));
+ }
+
+ @Test
+ void integerChangeIsDetected() {
+ Field left = new Field("a", "String", 50, -1, null, TestEnum.ONE);
+ Field right = new Field("a", "String", 80, -1, null, TestEnum.ONE);
+ assertFalse(DialogOkContent.same(left, right));
+ }
+
+ @Test
+ void stringListExtraEmptyItemIsAChange() {
+ MetaData left = new MetaData();
+ left.setValues(new ArrayList<>(List.of("v1")));
+ MetaData right = new MetaData();
+ right.setValues(new ArrayList<>(List.of("v1", "")));
+ assertFalse(DialogOkContent.same(left, right));
+ }
+
+ @Test
+ void nullAndEmptyStillSerializeDifferently() throws Exception {
+ Field withNullFormat = new Field("a", "String", 50, -1, null,
TestEnum.ONE);
+ Field withEmptyFormat = new Field("a", "String", 50, -1, "", TestEnum.ONE);
+
+ String nullXml = XmlMetadataUtil.serializeObjectToXml(withNullFormat);
+ String emptyXml = XmlMetadataUtil.serializeObjectToXml(withEmptyFormat);
+
+ assertFalse(nullXml.contains("<format"), "null is omitted from XML");
+ assertTrue(emptyXml.contains("<format"), "empty string is written as a
tag");
+ assertNotEquals(nullXml, emptyXml, "disk format still distinguishes null
from empty string");
+ assertTrue(
+ DialogOkContent.same(withNullFormat, withEmptyFormat),
+ "dialog OK that turns null into empty string is not a content change");
+ }
+}
diff --git
a/core/src/test/java/org/apache/hop/metadata/serializer/xml/XmlMetadataUtilTest.java
b/core/src/test/java/org/apache/hop/metadata/serializer/xml/XmlMetadataUtilTest.java
index 48d5d25613..3442b33f66 100644
---
a/core/src/test/java/org/apache/hop/metadata/serializer/xml/XmlMetadataUtilTest.java
+++
b/core/src/test/java/org/apache/hop/metadata/serializer/xml/XmlMetadataUtilTest.java
@@ -95,7 +95,7 @@ class XmlMetadataUtilTest {
Node fieldsNode = XmlHandler.getSubNode(node, "fields");
List<Node> fieldNodes = XmlHandler.getNodes(fieldsNode, "field");
assertEquals(3, fieldNodes.size());
- Node fieldNode = fieldNodes.get(0);
+ Node fieldNode = fieldNodes.getFirst();
assertEquals("a", XmlHandler.getTagValue(fieldNode, "name"));
assertEquals("String", XmlHandler.getTagValue(fieldNode, "type"));
assertEquals("50", XmlHandler.getTagValue(fieldNode, "length"));
@@ -239,7 +239,7 @@ class XmlMetadataUtilTest {
assertEquals(1, withCopy.getSteps().size());
assertEquals(1, withCopy.getHops().size());
- assertEquals("S1", withCopy.getHops().get(0).getFrom().getName());
- assertNull(withCopy.getHops().get(0).getTo());
+ assertEquals("S1", withCopy.getHops().getFirst().getFrom().getName());
+ assertNull(withCopy.getHops().getFirst().getTo());
}
}
diff --git
a/engine/src/main/java/org/apache/hop/pipeline/transform/copy/DefaultTransformMetaCopyFactory.java
b/engine/src/main/java/org/apache/hop/pipeline/transform/copy/DefaultTransformMetaCopyFactory.java
index 0e8a051469..58f3d07b39 100644
---
a/engine/src/main/java/org/apache/hop/pipeline/transform/copy/DefaultTransformMetaCopyFactory.java
+++
b/engine/src/main/java/org/apache/hop/pipeline/transform/copy/DefaultTransformMetaCopyFactory.java
@@ -127,6 +127,10 @@ public class DefaultTransformMetaCopyFactory implements
ITransformMetaCopyFactor
if (copy.getTransform() != null) {
copy.getTransform().setChanged();
}
+ } else {
+ // Copying location / row distribution uses setters that flip
wrapperChanged. A snapshot of
+ // an unchanged transform must stay unchanged so dialog
OK-without-edits is not dirty.
+ copy.setChanged(false);
}
} else {
// Explicitly clear changed state if not preserving it
diff --git
a/engine/src/main/java/org/apache/hop/workflow/action/copy/DefaultActionCopyFactory.java
b/engine/src/main/java/org/apache/hop/workflow/action/copy/DefaultActionCopyFactory.java
index d584cd9994..a40fd11aee 100644
---
a/engine/src/main/java/org/apache/hop/workflow/action/copy/DefaultActionCopyFactory.java
+++
b/engine/src/main/java/org/apache/hop/workflow/action/copy/DefaultActionCopyFactory.java
@@ -202,11 +202,14 @@ public class DefaultActionCopyFactory implements
IActionCopyFactory {
copy.setParentWorkflowMeta(source.getParentWorkflowMeta());
}
- // Apply changed state based on context - this fixes the race condition
- if (context.isPreserveChangedState()) {
- // This will call setChanged() which sets the changed state on the
underlying action
+ // Apply changed state based on context. Copying location uses setters
that flip
+ // wrapperChanged, so a snapshot of an unchanged action must be cleared
when the source is
+ // clean. Always marking the copy changed made dialog OK-without-edits
look like a real edit.
+ if (context.isPreserveChangedState() && source.hasChanged()) {
copy.setChanged();
log.logRowlevel("Set changed state on copied ActionMeta");
+ } else {
+ copy.setChanged(false);
}
log.logRowlevel("Successfully copied ActionMeta");
diff --git
a/engine/src/test/java/org/apache/hop/pipeline/transform/copy/DefaultTransformMetaCopyFactoryTest.java
b/engine/src/test/java/org/apache/hop/pipeline/transform/copy/DefaultTransformMetaCopyFactoryTest.java
index 54fa97b14b..8d1525d257 100644
---
a/engine/src/test/java/org/apache/hop/pipeline/transform/copy/DefaultTransformMetaCopyFactoryTest.java
+++
b/engine/src/test/java/org/apache/hop/pipeline/transform/copy/DefaultTransformMetaCopyFactoryTest.java
@@ -182,4 +182,18 @@ class DefaultTransformMetaCopyFactoryTest {
assertTrue(
cloned.getTransform().hasChanged(), "Clone inner transform should
preserve changed state");
}
+
+ @Test
+ void copyOfUnchangedTransformIsNotMarkedChanged() {
+ sourceTransformMeta.setLocation(50, 80);
+ sourceTransformMeta.setChanged(false);
+ dummyMeta.setChanged(false);
+
+ TransformMeta copy = factory.copy(sourceTransformMeta,
CopyContext.DEFAULT);
+
+ assertFalse(sourceTransformMeta.hasChanged(), "Source should stay
unchanged");
+ assertFalse(
+ copy.hasChanged(),
+ "Copying a clean transform must not mark the copy changed
(location/row-distribution setters used to flip wrapperChanged)");
+ }
}
diff --git
a/engine/src/test/java/org/apache/hop/workflow/action/copy/DefaultActionCopyFactoryTest.java
b/engine/src/test/java/org/apache/hop/workflow/action/copy/DefaultActionCopyFactoryTest.java
index 07d3e4cd04..8b3af3bf3c 100644
---
a/engine/src/test/java/org/apache/hop/workflow/action/copy/DefaultActionCopyFactoryTest.java
+++
b/engine/src/test/java/org/apache/hop/workflow/action/copy/DefaultActionCopyFactoryTest.java
@@ -196,6 +196,17 @@ class DefaultActionCopyFactoryTest {
assertTrue(copy.hasChanged(), "Copy should preserve changed state");
}
+ @Test
+ void copyOfUnchangedActionMetaIsNotMarkedChanged() {
+ sourceActionMeta.setLocation(40, 60);
+ sourceActionMeta.setChanged(false);
+
+ ActionMeta copy = factory.copy(sourceActionMeta, CopyContext.DEFAULT);
+
+ assertFalse(sourceActionMeta.hasChanged(), "Source should stay unchanged");
+ assertFalse(copy.hasChanged(), "Copying a clean action must not mark the
copy changed");
+ }
+
@Test
void testIntegrationWithReplaceMeta() {
// Test that replaceMeta now uses the copy factory
diff --git
a/rcp/src/test/java/org/apache/hop/ui/core/gui/GuiCompositeWidgetsGroupTest.java
b/rcp/src/test/java/org/apache/hop/ui/core/gui/GuiCompositeWidgetsGroupTest.java
index 9864165fdb..775636d5f7 100644
---
a/rcp/src/test/java/org/apache/hop/ui/core/gui/GuiCompositeWidgetsGroupTest.java
+++
b/rcp/src/test/java/org/apache/hop/ui/core/gui/GuiCompositeWidgetsGroupTest.java
@@ -19,8 +19,10 @@ package org.apache.hop.ui.core.gui;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
import static org.junit.jupiter.api.Assertions.assertNotEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import java.lang.reflect.Field;
@@ -33,6 +35,7 @@ import org.apache.hop.core.gui.plugin.GuiRegistry;
import org.apache.hop.core.gui.plugin.GuiWidgetElement;
import org.apache.hop.core.gui.plugin.GuiWidgetGroupType;
import org.apache.hop.core.variables.Variables;
+import org.apache.hop.ui.core.widget.TextVar;
import org.apache.hop.ui.testing.SwtBotTestBase;
import org.eclipse.swt.custom.CTabFolder;
import org.eclipse.swt.layout.FormData;
@@ -75,6 +78,36 @@ class GuiCompositeWidgetsGroupTest extends SwtBotTestBase {
widgets.setWidgetsContents(source, shell, FLAT_PARENT);
widgets.getWidgetsContents(source, FLAT_PARENT);
assertEquals("beta", source.getName());
+ assertNull(source.getNote());
+ } finally {
+ shell.dispose();
+ }
+ }
+
+ @Test
+ void emptyTextWidgetLeavesNullFieldNull() {
+ Shell shell = new Shell(display);
+ shell.setLayout(new FormLayout());
+ try {
+ FlatSample source = new FlatSample();
+ source.setName("alpha");
+ assertNull(source.getNote());
+
+ GuiCompositeWidgets widgets = new GuiCompositeWidgets(new Variables());
+ widgets.createCompositeWidgets(source, null, shell, FLAT_PARENT, null);
+ widgets.setWidgetsContents(source, shell, FLAT_PARENT);
+ widgets.getWidgetsContents(source, FLAT_PARENT);
+ assertNull(source.getNote());
+
+ TextVar note = (TextVar) widgets.getWidgetsMap().get("note");
+ assertNotNull(note);
+ note.setText("typed");
+ widgets.getWidgetsContents(source, FLAT_PARENT);
+ assertEquals("typed", source.getNote());
+
+ note.setText("");
+ widgets.getWidgetsContents(source, FLAT_PARENT);
+ assertEquals("", source.getNote());
} finally {
shell.dispose();
}
@@ -179,7 +212,7 @@ class GuiCompositeWidgetsGroupTest extends SwtBotTestBase {
assertFalse(first.getVisible());
assertTrue(second.getVisible());
- assertTrue(second.getLayoutData() instanceof FormData);
+ assertInstanceOf(FormData.class, second.getLayoutData());
FormData secondData = (FormData) second.getLayoutData();
assertTrue(secondData.height == -1 || secondData.height > 0);
@@ -242,6 +275,13 @@ class GuiCompositeWidgetsGroupTest extends SwtBotTestBase {
type = GuiElementType.TEXT,
label = "Name")
private String name;
+
+ @GuiWidgetElement(
+ id = "note",
+ parentId = FLAT_PARENT,
+ type = GuiElementType.TEXT,
+ label = "Note")
+ private String note;
}
@GuiPlugin
diff --git
a/ui/src/main/java/org/apache/hop/ui/core/gui/GuiCompositeWidgets.java
b/ui/src/main/java/org/apache/hop/ui/core/gui/GuiCompositeWidgets.java
index 32dabfe654..f52d1e0231 100644
--- a/ui/src/main/java/org/apache/hop/ui/core/gui/GuiCompositeWidgets.java
+++ b/ui/src/main/java/org/apache/hop/ui/core/gui/GuiCompositeWidgets.java
@@ -50,6 +50,7 @@ import org.apache.hop.core.vfs.HopVfs;
import org.apache.hop.i18n.BaseMessages;
import org.apache.hop.metadata.api.IHopMetadata;
import org.apache.hop.metadata.api.IHopMetadataProvider;
+import org.apache.hop.metadata.serializer.xml.DialogOkContent;
import org.apache.hop.ui.core.ConstUi;
import org.apache.hop.ui.core.PropsUi;
import org.apache.hop.ui.core.dialog.BaseDialog;
@@ -1737,6 +1738,14 @@ public class GuiCompositeWidgets {
return;
}
+ // SWT text widgets cannot hold null. An empty read-back of a field
that is still null
+ // is not an edit: writing "" would serialize as <tag/> instead of
omitting the field.
+ if (parameterType == String.class
+ && DialogOkContent.widgetEmptyLeavesNull(
+ readFieldValue(sourceData, guiElements), value)) {
+ return;
+ }
+
setter.invoke(sourceData, value);
} catch (Exception e) {
diff --git
a/ui/src/main/java/org/apache/hop/ui/hopgui/file/pipeline/delegates/HopGuiPipelineTransformDelegate.java
b/ui/src/main/java/org/apache/hop/ui/hopgui/file/pipeline/delegates/HopGuiPipelineTransformDelegate.java
index 035a622850..8f23b0cc45 100644
---
a/ui/src/main/java/org/apache/hop/ui/hopgui/file/pipeline/delegates/HopGuiPipelineTransformDelegate.java
+++
b/ui/src/main/java/org/apache/hop/ui/hopgui/file/pipeline/delegates/HopGuiPipelineTransformDelegate.java
@@ -26,7 +26,6 @@ import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
-import org.apache.hop.core.Const;
import org.apache.hop.core.exception.HopException;
import org.apache.hop.core.extension.ExtensionPointHandler;
import org.apache.hop.core.extension.HopExtensionPoint;
@@ -41,6 +40,7 @@ import org.apache.hop.core.util.Utils;
import org.apache.hop.core.variables.IVariables;
import org.apache.hop.history.AuditManager;
import org.apache.hop.i18n.BaseMessages;
+import org.apache.hop.metadata.serializer.xml.DialogOkContent;
import org.apache.hop.pipeline.IPartitioner;
import org.apache.hop.pipeline.PipelineHopMeta;
import org.apache.hop.pipeline.PipelineMeta;
@@ -200,6 +200,9 @@ public class HopGuiPipelineTransformDelegate {
dialog = getTransformDialog(transformMeta.getTransform(), pipelineMeta,
name);
TransformMeta before = null;
byte[] beforeSnapshot = null;
+ // Capture from the live object. A clone is often already "changed"
because copying location
+ // or row distribution goes through setters that flip wrapperChanged.
+ boolean alreadyChanged = transformMeta.hasChanged();
if (dialog != null) {
dialogs.put(name, dialog);
@@ -269,7 +272,7 @@ public class HopGuiPipelineTransformDelegate {
if (hasTransformMetaChanged(before, after)) {
transformMeta.setChanged();
} else {
- transformMeta.setChanged(before.hasChanged());
+ transformMeta.setChanged(alreadyChanged);
}
}
pipelineGraph.updateGui();
@@ -600,6 +603,7 @@ public class HopGuiPipelineTransformDelegate {
}
public void editTransformPartitioning(PipelineMeta pipelineMeta,
TransformMeta transformMeta) {
+ boolean alreadyChanged = transformMeta.hasChanged();
byte[] beforeSnapshot = pipelineGraph.captureUndoSnapshot();
String[] schemaNames;
try {
@@ -663,7 +667,7 @@ public class HopGuiPipelineTransformDelegate {
if (hasTransformMetaChanged(partitionBefore, partitionAfter)) {
transformMeta.setChanged();
} else {
- transformMeta.setChanged(partitionBefore.hasChanged());
+ transformMeta.setChanged(alreadyChanged);
}
pipelineGraph.redraw();
pipelineGraph.updateGui();
@@ -730,6 +734,7 @@ public class HopGuiPipelineTransformDelegate {
List<TransformMeta> targetTransforms =
pipelineMeta.findNextTransforms(transformMeta, true);
// now edit this transformErrorMeta object:
+ boolean alreadyChanged = transformMeta.hasChanged();
TransformMeta before = (TransformMeta) transformMeta.clone();
byte[] beforeSnapshot = pipelineGraph.captureUndoSnapshot();
TransformErrorMetaDialog dialog =
@@ -747,7 +752,7 @@ public class HopGuiPipelineTransformDelegate {
if (hasTransformMetaChanged(before, after)) {
transformMeta.setChanged();
} else {
- transformMeta.setChanged(before.hasChanged());
+ transformMeta.setChanged(alreadyChanged);
}
pipelineGraph.redraw();
}
@@ -757,23 +762,16 @@ public class HopGuiPipelineTransformDelegate {
/**
* Returns {@code true} if two transform snapshots differ in persisted
configuration (transform
* body, partitioning, GUI placement, and error handling).
+ *
+ * <p>String {@code null} and {@code ""} are treated as the same so a dialog
OK that only
+ * round-trips widget text does not mark the pipeline dirty. An extra empty
table row is still a
+ * change. Serialization still writes null and empty string differently.
*/
private static boolean hasTransformMetaChanged(TransformMeta before,
TransformMeta after) {
- try {
- if (!before.getXml().equals(after.getXml())) {
- return true;
- }
-
- return !getErrorMetaXml(before).equals(getErrorMetaXml(after));
- } catch (HopException e) {
- // If comparison fails, treat as changed to avoid losing edits.
+ if (!DialogOkContent.same(before, after)) {
return true;
}
- }
-
- private static String getErrorMetaXml(TransformMeta transformMeta) throws
HopException {
- TransformErrorMeta errorMeta = transformMeta.getTransformErrorMeta();
- return errorMeta == null ? Const.EMPTY_STRING : errorMeta.getXml();
+ return !DialogOkContent.same(before.getTransformErrorMeta(),
after.getTransformErrorMeta());
}
public void delTransforms(PipelineMeta pipelineMeta, List<TransformMeta>
transforms) {
diff --git
a/ui/src/main/java/org/apache/hop/ui/hopgui/file/workflow/delegates/HopGuiWorkflowActionDelegate.java
b/ui/src/main/java/org/apache/hop/ui/hopgui/file/workflow/delegates/HopGuiWorkflowActionDelegate.java
index 1979eb1e37..4f42ba343f 100644
---
a/ui/src/main/java/org/apache/hop/ui/hopgui/file/workflow/delegates/HopGuiWorkflowActionDelegate.java
+++
b/ui/src/main/java/org/apache/hop/ui/hopgui/file/workflow/delegates/HopGuiWorkflowActionDelegate.java
@@ -29,6 +29,7 @@ import org.apache.hop.core.plugins.PluginRegistry;
import org.apache.hop.core.security.Permission;
import org.apache.hop.core.variables.IVariables;
import org.apache.hop.i18n.BaseMessages;
+import org.apache.hop.metadata.serializer.xml.DialogOkContent;
import org.apache.hop.ui.core.PropsUi;
import org.apache.hop.ui.core.dialog.BaseDialog;
import org.apache.hop.ui.core.dialog.ErrorDialog;
@@ -292,6 +293,8 @@ public class HopGuiWorkflowActionDelegate {
}
byte[] beforeSnapshot = workflowGraph.captureUndoSnapshot();
+ boolean alreadyChanged = action.hasChanged();
+ ActionMeta before = action.clone();
IAction jei = action.getAction();
@@ -305,6 +308,11 @@ public class HopGuiWorkflowActionDelegate {
//
workflowMeta.renameActionIfNameCollides(action);
workflowGraph.commitDialogUndo(beforeSnapshot);
+ if (hasActionMetaChanged(before, action)) {
+ action.setChanged();
+ } else {
+ action.setChanged(alreadyChanged);
+ }
}
workflowGraph.updateGui();
} else {
@@ -404,4 +412,13 @@ public class HopGuiWorkflowActionDelegate {
workflowGraph.updateGui();
}
+
+ /**
+ * Returns {@code true} if two action snapshots differ in persisted
configuration. String {@code
+ * null} and {@code ""} are treated as the same so a dialog OK that only
round-trips widget text
+ * does not mark the workflow dirty. An extra empty table row is still a
change.
+ */
+ private static boolean hasActionMetaChanged(ActionMeta before, ActionMeta
after) {
+ return !DialogOkContent.same(before, after);
+ }
}
