Check if metadata key exists and if so "Remove" before "add"

[aymanhab]

Fixes issue #3589

Brief summary of changes

Actually it's possible to addMetaData for units but only if it doesn't exist. I modified the test case and the method in bindings to exercise this workflow. Will update the c3dExport file once we agree on what to include.

Testing I've completed

Updated Java test case to change units to Meters

Looking for feedback on...

Whether update the .i file considering that the name of the method starting with "add", or leave alone and update the test code/sample accordingly basically leaving door for future confusion!

CHANGELOG.md (choose one)

• no need to update yet

---

This change is 

[aymanhab]

@nickbianco for your review/feedback

[nickbianco]

@aymanhab, this change seems reasonable, but is there no way to simply update existing metadata values given a key, rather than removing then adding?

[nickbianco]

Is the logic in this test necessary given the changes in common.i?

Edit: nevermind, I see that you implemented both possible changes for comparison.

[nickbianco]

I think the ideal solution is to add a method like setTableMetaDataForKey to DataTable. It would update the table metadata object, which is a typedef'd ValueArrayDictionary, which already has the method setValueForKey. In other words, setTableMetaDataForKey would be a wrapper for setValueForKey.

[aymanhab]

Unfortunately the documentation of the method in ValueArrayDictionary.h states
/** Set the value corresponding to a given key. Value is set only if the key
does not already exist. */

[nickbianco]

Oh, weird. That's a poorly named method. Okay, I'm mostly fine with the change, it's just odd to have remove then add to just change a value.

One more suggestion: ValueArrayDictionary has updValueForKey which you can use to modify values. We could add a new method to AbstractDataTable called setTableMetaData (templated, in scripting it would be setTableMetaDataString) which modifies the meta data using updValueForKey. There is a mis-match in usages set and upd between the table and the meta data array here, but considering that there's already a mis-match in the current implementation (add versus set), I think this would be fine.

[aymanhab]

@nickbianco I agree about the wierdness ;)
Actually there's no updValueForKey there's only updValueArrayForKey, I was going to add the method but considering the unexpected naming (and cloning behavior underneath,) I'd lean to leave it as is and open an issue for future revamp. Please let me know what you think.

Another option is to add new method clearly named e.g. setValueForKeyEvenIfExists?

[nickbianco]

@aymanhab, I see, updValueArrayForKey is not exactly what we want. In that case, I think your change to common.i is fine, especially since it's just on the bindings side. We can make a proper change sometime in the future.

[aymanhab]

Sounds good, thanks @nickbianco 👍 I'll update changelog and then we can merge.

[nickbianco]

@aymanhab you should update the test to remove the extra code if you're keeping the common.i changes.

[aymanhab]

@nickbianco test failure is another intermittent Moco failure on osx.

[nickbianco]

One quick change in the CHANGELOG, then LGTM

[aymanhab]

Thanks @nickbianco