#46 Protocol parameters overwritten by RMIR

[Leonardo Brondani Schenkel]

I independently have encountered the same problem reported in https://www.hifi-remote.com/forums/viewtopic.php?t=103894:

It manifests in the following forms:

• once you submit a device upgrade, even if it's the only one, you can no longer edit the protocol parameters (they revert back to previous values when the editor closes)
• if you have multiple upgrades using the same protocol, the parameters are always overwritten with the values from the first one
• if you edit the file manually instead, and open it, the values stay as in the file until you attempt to edit the upgrade, then the problems happen again

Strangely enough, I also stumbled upon a workaround: if Options/Advanced/Non-model editor is turned on, the problems above do not manifest. Turn if on again, and they come back.

After struggling a lot with the above I decided to run RMIR under a debugger and diagnose what was happening (I am a Java developer). And I found out what the issue was:

The problem is that protocol parameters while they're being edited are both live and global state, they're components from the protocol device parameters being shown in the UI that are per-protocol and protocols are global. And when new instances of the class DeviceUpgrade are created, they try to clone the protocol parameter values of the upgrade by using the protocol device parameters, and that overwrites the values in those components.

One of the code paths that creates new device upgrades is the renderer of the device upgrade table in the Devices tab. When the table needs to be refreshed, it will clone the upgrades from the model in order to update the UI.

When non-modal editor is OFF (the default), and you launch the device upgrade editor, it will disable the parent window (containing the device upgrade table), open the editor, and when you press OK one of the first things it will do is to re-enable the parent window.

When the parent window is re-enabled, Swing will trigger a refresh/re-render of the table component. That will invoke the code path trying to clone the existing device upgrades, and the first entry in the table that has the same protocol will result in that code path cloning the previous values (the ones in the model, in the pre-edited state) and overwriting the values entered in the device upgrade editor. This happens very early, before the values are actually processed and the model is updated, so any changes the user have made are reverted back.

When non-modal editor is ON, then the parent window is never re-enabled and that does not trigger any table refresh at that point. The problematic code path is not invoked, the values are actually processed and the data models are updated, and then the table is refreshed, with the problematic code path being invoked, but now the models are actually containing the post-edited state so that is fine.

I am attaching a patch that addresses this problem. It changes the DeviceUpgrade cloning code so it never attempts to use the protocol device parameters (and their components):

diff --git a/src/main/java/com/hifiremote/jp1/Value.java b/src/main/java/com/hifiremote/jp1/Value.java
--- a/src/main/java/com/hifiremote/jp1/Value.java   (revision 2240)
+++ b/src/main/java/com/hifiremote/jp1/Value.java   (date 1777961033694)
@@ -1,6 +1,9 @@
 package com.hifiremote.jp1;

 // TODO: Auto-generated Javadoc
+
+import java.util.Arrays;
+
 /**

  * The Class Value.
  */
@@ -39,6 +42,18 @@
     this.defaultValue = defaultValue;
   }


+  public Value copy()
+  {
+    return new Value( userValue, defaultValue );
+  }
+
+  public static Value[] copy( Value[] values )
+  {
+    return Arrays.stream( values )
+        .map( Value::copy )
+        .toArray( Value[]::new );
+  }
+
   /**
    * Gets the user value.
    * 

diff --git a/src/main/java/com/hifiremote/jp1/DeviceUpgrade.java b/src/main/java/com/hifiremote/jp1/DeviceUpgrade.java
--- a/src/main/java/com/hifiremote/jp1/DeviceUpgrade.java   (revision 2240)
+++ b/src/main/java/com/hifiremote/jp1/DeviceUpgrade.java   (date 1777621985560)
@@ -139,8 +139,17 @@
     // copy the device parameter values
     if ( base.protocol != null )
     {
-      protocol.setDeviceParms( base.parmValues );
-      parmValues = protocol.getDeviceParmValues();
+      // *DO NOT* call protocol.setDeviceParams() here to clone parmValues!
+      // Protocol device params are global state, and may be:
+      // - live: currently rendered in the UI; and/or
+      // - dirty: containing user-entered input which has not been saved yet.
+      //
+      // Violation of this rule will result in this code path overwriting
+      // user-entered protocol parameters. For example, if the table showing
+      // device upgrades is re-rendered while the upgrade dialog is open,
+      // the old values in the table data will overwrite any changes the user
+      // has typed.
+      parmValues = Value.copy( parmValues );
     }

     // Copy the functions and their assignments

This completely addresses the problems described. I kindly ask for this fix (or an equivalent one) to be added to RMIR. Thank you.

[Graham Dixon]

This fix is now included in RMIR v3.2.16, posted yesterday.