bamaer commented on code in PR #9204:
URL: https://github.com/apache/hop/pull/9204#discussion_r4439897085
##########
ui/src/main/java/org/apache/hop/ui/pipeline/transform/BaseTransformDialog.java:
##########
@@ -1235,20 +1236,14 @@ public static final void getFieldsFromPrevious(
try {
IRowMeta r = pipelineMeta.getPrevTransformFields(variables,
transformMeta);
selectedField = comboVar.getText();
- comboVar.removeAll();
-
+ String[] names = r != null && !r.isEmpty() ? r.getFieldNames() : new
String[0];
+ ComboItems.setItemsKeepingText(comboVar, names);
Review Comment:
`getFieldsFromPrevious` already called `getPrevTransformFields` first and
wrote `selectedField` back afterwards, so the old code kept the value on
failure. This change does not alter behaviour. `wCombo` in `ComboVar` is
editable, and `CCombo.setItems` keeps the text of an editable combo, so the
restore inside `ComboItems` does nothing here.
##########
plugins/transforms/pgp/src/main/java/org/apache/hop/pipeline/transforms/pgpencryptstream/PGPEncryptStreamDialog.java:
##########
@@ -338,8 +338,8 @@ private void getPreviousFields() {
String fieldValue = wStreamFieldName.getText();
wStreamFieldName.removeAll();
Review Comment:
`wStreamFieldName.removeAll()` still runs before the lookup. Only
`wKeyNameFieldName.removeAll()` was moved, so a failed lookup still empties the
stream field.
##########
ui/src/test/java/org/apache/hop/ui/core/widget/ComboItemsTest.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.ui.core.widget;
+
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import org.junit.jupiter.api.Test;
+
+class ComboItemsTest {
+
+ @Test
+ void replacingItemsWritesThePreviousTextBack() {
+ FakeCombo combo = new FakeCombo();
+ combo.text = "id_json";
+ combo.items = new String[] {"old"};
+
+ ComboItems.apply(combo::getText, combo::setItems, combo::setText, new
String[] {"other"});
+
+ assertArrayEquals(new String[] {"other"}, combo.items);
+ assertEquals("id_json", combo.text);
+ }
+
+ @Test
+ void nullItemsLeaveTheListAndStillRestoreTheText() {
+ FakeCombo combo = new FakeCombo();
+ combo.text = "id_json";
+ combo.items = new String[] {"kept"};
+
+ ComboItems.apply(combo::getText, combo::setItems, combo::setText, null);
+
+ assertArrayEquals(new String[] {"kept"}, combo.items);
+ assertEquals("id_json", combo.text);
+ }
+
+ @Test
+ void nullTextIsNotWrittenBack() {
+ FakeCombo combo = new FakeCombo();
+ combo.text = null;
+ combo.items = new String[] {"old"};
+
+ ComboItems.apply(combo::getText, combo::setItems, combo::setText, new
String[] {"other"});
+
+ assertArrayEquals(new String[] {"other"}, combo.items);
+ // setItems cleared the text, and a null previous value is not written
back.
+ assertEquals("", combo.text);
+ }
+
+ private static final class FakeCombo {
+ private String text;
+ private String[] items;
+
+ private String getText() {
+ return text;
+ }
+
+ private void setItems(String[] items) {
+ this.items = items;
+ // SWT clears the text when the items change.
+ this.text = "";
Review Comment:
The fake clears the text on every `setItems`. SWT's `CCombo` does that only
for read-only combos, so the test builds in the assumption it is meant to check.
##########
plugins/transforms/dynamicsqlrow/src/main/java/org/apache/hop/pipeline/transforms/dynamicsqlrow/DynamicSqlRowDialog.java:
##########
@@ -441,8 +441,8 @@ private void get() {
gotPreviousFields = true;
try {
String sqlfield = wSqlFieldName.getText();
- wSqlFieldName.removeAll();
IRowMeta r = pipelineMeta.getPrevTransformFields(variables,
transformName);
+ wSqlFieldName.removeAll();
if (r != null) {
wSqlFieldName.removeAll();
Review Comment:
Nit: after the move, `removeAll()` runs on line 445 and again here. One of
the two can go. The same applies to `ExecSqlRowDialog.java`, lines 407 and 409.
##########
ui/src/main/java/org/apache/hop/ui/core/widget/ComboItems.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.ui.core.widget;
+
+import java.util.function.Consumer;
+import java.util.function.Supplier;
+import org.eclipse.swt.custom.CCombo;
+import org.eclipse.swt.widgets.Combo;
+
+/**
+ * Replaces the items of a combo and writes the previous text back. {@code
setItems} clears the
+ * text, including a value that is not in the new list.
+ */
+public final class ComboItems {
+
+ private ComboItems() {}
+
+ public static void setItemsKeepingText(CCombo combo, String[] items) {
Review Comment:
The `CCombo`, `Combo` and `LabelCombo` overloads have no callers in this PR.
The `null`-items branch of `apply` (line 53) is not reached either, because the
only caller passes `new String[0]`. Either use the helper in the dialogs,
replacing their save/clear/restore code, or drop it.
##########
ui/src/main/java/org/apache/hop/ui/core/widget/ComboItems.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.ui.core.widget;
+
+import java.util.function.Consumer;
+import java.util.function.Supplier;
+import org.eclipse.swt.custom.CCombo;
+import org.eclipse.swt.widgets.Combo;
+
+/**
+ * Replaces the items of a combo and writes the previous text back. {@code
setItems} clears the
Review Comment:
`CCombo.setItems` clears the text only when the combo is read-only (`if
(!text.getEditable()) text.setText("")`). `removeAll()` is what always clears
it.
##########
plugins/transforms/xml/src/main/java/org/apache/hop/pipeline/transforms/xml/getxmldata/GetXmlDataDialog.java:
##########
@@ -1287,9 +1287,8 @@ public void widgetSelected(SelectionEvent e) {
private void setXMLStreamField() {
try {
- wXMLField.removeAll();
-
IRowMeta r = pipelineMeta.getPrevTransformFields(variables,
transformName);
+ wXMLField.removeAll();
Review Comment:
The catch block (lines 1305–1307) adds `<EMPTY>` to `wXMLField` and selects
it. `ok()` then saves it with `in.setXmlField(wXMLField.getText())`. A failed
lookup still replaces the configured field, now with `<EMPTY>` instead of an
empty string, which is the scenario in #5953. The catch block should keep the
previous value.
##########
plugins/transforms/ldap/src/main/java/org/apache/hop/pipeline/transforms/ldapinput/LdapInputDialog.java:
##########
@@ -1519,9 +1519,9 @@ private void setSearchBaseField() {
try {
String basefield = wSearchBaseField.getText();
String filterfield = wFilterField.getText();
Review Comment:
Older than this PR, in the same block: line 1533 calls
`wFilterField.setText(basefield)`, which should be `filterfield`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
