fix: variable name changes when element name/label changes

[abdul99ahad]

Closes #863

Proposed Changes

• When creating or updating the label in Decision box, the variable name automatically updates
• Setting the element name automatically sets variable name

[barmac]

I don't see any changes regarding the decision table. Is it already present in that editor? I know that we don't support variable adding or modification in the decision table, but it could be present in the XML brought by user.

[abdul99ahad]

I don't see any changes regarding the decision table. Is it already present in that editor? I know that we don't support variable adding or modification in the decision table, but it could be present in the XML brought by user.

Since variables weren't available in the modeling interface, I initially assumed they weren't part of the decision tables. However, I've now fixed the issue for all elements to ensure everything is covered.

[barmac]

bo.variable.name = newLabel;

This is a technique called monkey-patching. If you add tests for commandStack.undo, you will notice that the change cannot be undone. We need to dispatch an updateProperties call which updates the variable name.
@philippfromme perhaps it makes sense you look into this together with @abdul99ahad

[abdul99ahad]

Thanks, @barmac, for pointing this out. I discussed the monkey patching technique with @philippfromme earlier and I didn't realize that my implementation went against the recommended approach. I apologize for the oversight. I've updated the implementation to align with the correct approach.

[barmac]

Notice the comment I left. Let's add tests for undo, and fix the code. As I am off for the next two weeks, please ask @philippfromme or other team member for support.

[abdul99ahad]

Notice the comment I left. Let's add tests for undo, and fix the code. As I am off for the next two weeks, please ask @philippfromme or other team member for support.

I have fixed the code and added the test cases that support undo/redo

[abdul99ahad]

@barmac @philippfromme We could move the NameChangeBehavior from dmn-boxed-expression to a shared NameChangeBehavior. The key difference is that the shared version only syncs the element name with the variable name, not the other way around, while the boxed-expression version supports both directions. Should we keep this distinction, or do we need to refactor?

[philippfromme]

@barmac @philippfromme We could move the NameChangeBehavior from dmn-boxed-expression to a shared NameChangeBehavior. The key difference is that the shared version only syncs the element name with the variable name, not the other way around, while the boxed-expression version supports both directions. Should we keep this distinction, or do we need to refactor?

Is there anything keeping us from handling both cases in a shared component?

[abdul99ahad]

@barmac @philippfromme We could move the NameChangeBehavior from dmn-boxed-expression to a shared NameChangeBehavior. The key difference is that the shared version only syncs the element name with the variable name, not the other way around, while the boxed-expression version supports both directions. Should we keep this distinction, or do we need to refactor?

Is there anything keeping us from handling both cases in a shared component?

We can cover that case as well. I previously discussed this with @barmac, who mentioned that only a one-way sync is required as of now.

[barmac]

I will look into this today again.

[barmac]

Note that the variable name change is not reflected in the literal expression editor:

Screen.Recording.2024-09-13.at.10.40.49.mov

[barmac]

Looks that it's an already existing issue. LiteralExpressionPropertiesEditorComponent does not update on external changes.

[barmac]

@barmac @philippfromme We could move the NameChangeBehavior from dmn-boxed-expression to a shared NameChangeBehavior. The key difference is that the shared version only syncs the element name with the variable name, not the other way around, while the boxed-expression version supports both directions. Should we keep this distinction, or do we need to refactor?

Is there anything keeping us from handling both cases in a shared component?

We can cover that case as well. I previously discussed this with @barmac, who mentioned that only a one-way sync is required as of now.

The thing about this is that in the future, I expect that we replace dmn-js-literal-expression with dmn-js-boxed-expression entirely where you cannot change the variable name by design. Still, I think that, contrary to what I said before, for the external changes it makes sense to sync it two-way.

[abdul99ahad]

Note that the variable name change is not reflected in the literal expression editor:

Screen.Recording.2024-09-13.at.10.40.49.mov

Yes, the variable name is changing but the state is not updating. Hence, when you're on the same screen (lieral expression editor), the change doesn't reflect although its value has been updated.

For variable name changing directly, the element name won't change since it's currently not supported.

[barmac]

Yes, the variable name is changing but the state is not updating. Hence, when you're on the same screen (lieral expression editor), the change doesn't reflect although its value has been updated.

IMO the update should happen as it does in boxed expression. Let the field display the current value.

[abdul99ahad]

Yes, the variable name is changing but the state is not updating. Hence, when you're on the same screen (lieral expression editor), the change doesn't reflect although its value has been updated.

IMO the update should happen as it does in boxed expression. Let the field display the current value.

@barmac need your support in updating the state externally as I'm stuck and can only update the state within the component. We might need to use Wrapper with ChangeSupport just like in boxed-expression.

[barmac]

Suggested change

console.log('componentDidUpdate',);

[barmac]

The CI failure seems to be unrelated. I'll give it a second try.

[barmac]

🚀

[barmac]

One optional improvement suggestion, but other than that it looks good.