Update business object so undo / redo works?

[Adrian]

Hi everyone,

I ran into an issue with undo / redo in a custom properties panel with extension elements. I saw this thread which suggests to use commands. I also found this example which uses modeling.updateProperties to set the extension elements and has working undo / redo.

However, when I tried to use updateProperties to set the extension elements I noticed that undo / redo do not work, although the details are confusing. Unfortunately, I am not at liberty to share most of our code, so I will try to show the relevant parts and explain what is happening. Maybe someone has an idea.

Behaviour
Undo is generally working

1. Create an element that has custom properties → element is created
2. Press Ctrl+Z → element is removed

But with my custom properties things get weird

1. Create an element that has custom properties → element is created
2. Write a custom property → property is correctly set in the bpmn when downloaded
3. Press Ctrl+Z → nothing happens, property is incorrectly still set in the bpmn when downloaded
4. Press Ctrl+Z again → the element is removed

It seems to me as if the Write-Property-Action is correctly added to the command (event?) stack. It also is popped off the stack when undoing. But the actual writing is not undone.

Code (sort of)
As mentioned I cannot share much of our code, but this is the code I use to write the extension elements as well as the objects it uses.

Code

const modeling = useService('modeling');
console.log(JSON.stringify(element))
console.log(JSON.stringify(extensionElements))
return modeling.updateProperties(element, {extensionElements});

element

{
   "labels":[
      
   ],
   "children":[
      
   ],
   "id":"Activity_0s9re6u",
   "width":105,
   "height":55,
   "name":"test",
   "type":"bpmn:ServiceTask",
   "di":{
      "$type":"bpmndi:BPMNShape",
      "bounds":{
         "$type":"dc:Bounds",
         "x":588,
         "y":183,
         "width":105,
         "height":55
      },
      "id":"Activity_0s9re6u_di"
   },
   "x":588,
   "y":183,
   "order":{
      "level":5
   }
}

extensionElements

{
   "$type":"bpmn:ExtensionElements",
   "values":[
      {
         "$type":"activiti:Field",
         "name":"to",
         "string":{
            "$type":"activiti:String",
            "text":"asd"
         }
      },
      {
         "$type":"activiti:Field",
         "name":"from",
         "string":{
            "$type":"activiti:String",
            "text":"writingProperty"
         }
      },
      {
         "$type":"activiti:Field",
         "name":"subject",
         "string":{
            "$type":"activiti:String",
            "text":""
         }
      },
      {
         "$type":"activiti:Field",
         "name":"cc",
         "string":{
            "$type":"activiti:String",
            "text":""
         }
      },
      {
         "$type":"activiti:Field",
         "name":"bcc",
         "string":{
            "$type":"activiti:String",
            "text":""
         }
      },
      {
         "$type":"activiti:Field",
         "name":"html",
         "string":{
            "$type":"activiti:String",
            "text":""
         }
      },
      {
         "$type":"activiti:Field",
         "name":"attachmentName",
         "string":{
            "$type":"activiti:String",
            "text":""
         }
      },
      {
         "$type":"activiti:Field",
         "name":"attachmentMimeType",
         "string":{
            "$type":"activiti:String",
            "text":"text/plain"
         }
      },
      {
         "$type":"activiti:Field",
         "name":"attachmentContent",
         "string":{
            "$type":"activiti:String",
            "text":""
         }
      }
   ]
}

[nikku]

Generally you’ll be able to verify your logic without the properties panel in a focused unit test. I’d suggest you to do it, including undo + redo and verify that this works properly. If it does not, try to extract the scenario into something that we can inspect end-to-end, and then we’ll be able to help.

We use modeling#updateProperties at the core of the properties panel for Camunda extensions, not operating on activity but camunda extensions, of course. So I believe this is a userland bug.

[Adrian]

I’ve tried to extract it as best as possible in this repository.

The part mentioned above, where I actually invoke updateProperties is found here.

You can reproduce the problem by:

1. npm install
2. npm run build
3. Host the contents of the public folder via a web server of your choice.
4. Navigate to public/index.html
5. Drag the “E-Mail verschicken” module into the diagram
6. Edit a few fields in the “Module” section of the properties panel.
7. Try to undo your edits with Ctrl+Z

You’ll notice that you have to undo several times (depending on how many fields you edited) before the module actually disappears.

My interpretation is that the changes are correctly added to the stack, but when undoing are not actually reverted. But I don’t know where I’m going wrong.

[Adrian]

@nikku Just wanted to ask if you might have the time to look at my example repository. I know it’s still quite large, but I’m not sure how to reduce it further. I’d really appreciate even a direction of inquiry, right now I’m at a complete loss.

[nikku]

@Adrian We’ll have a look.

[barmac]

I had a look at your code, and noticed that before calling modeling.updateProperties you mutate the extensionElements directly: undo-redo-example/app/provider/properties/parts/Util.js at master · shade-belisar/undo-redo-example · GitHub

What you could do instead is to copy the existing extension elements:

const extensionElements = businessObject.get('extensionElements');
const preexistingValues = extensionElements ? extensionElements.get('values') : [];
const newExtensionElements = bpmnFactory.create('bpmn:ExtensionElements', { values: preexistingValues.concat(listener) });

modeling.updateProperties(element, { extensionElements: newExtensionElements });

[Adrian]

That unfortunately changes nothing. I’ve added copying of extensionElements to all set methods (here) but the resulting behaviour is the same as before.

[barmac]

Yeah, but you still reuse the values array. Notice that array.push mutates the array, and this is exactly what is happening in undo-redo-example/app/provider/properties/parts/Util.js at bugfix/copy-elements-attempt · shade-belisar/undo-redo-example · GitHub

I suggested using concat for a specific reason, i.e. to have a copy of the initial array.

For your code snippet, try this:

export const getExtensionElements = (bpmnFactory, businessObject) => {
    const extensionElements = businessObject.extensionElements;
    const preexistingValues = extensionElements ? extensionElements.get('values') : [];
-    return bpmnFactory.create('bpmn:ExtensionElements', { values: preexistingValues });
+    return bpmnFactory.create('bpmn:ExtensionElements', { values: preexistingValues .slice()});
}

[Adrian]

Ah, had not though about that side effect of concat. But adding slice() does not change anything either. The resulting behaviour is still this weird “not-undo” that is put on the stack.

[Adrian]

@barmac Is there maybe some way to log the undo-redo stack? Because I can clearly see that there is something on it, it’s just the wrong thing.

[barmac]

You could place a logpoint in the EventBus#fire method since the event bus is used under the hood for the commands.

[Adrian]

I’ve now solved it - sort of. Meaning it works, but I really hope there is a better way of doing this, because it looks like it could break any minute.

export const getExtensionElements = (bpmnFactory, businessObject) => {
    const extensionElements = businessObject.extensionElements;
    let preexistingValues = extensionElements ? extensionElements.values : [];
    preexistingValues = preexistingValues.map(preValue => bpmnFactory.create(
      preValue.$type,
      {
          "name": preValue.name,
          "string": bpmnFactory.create(
            preValue.string.$type,
            {
                "text": preValue.string.text
            }
          )
      }
    ))
    return bpmnFactory.create('bpmn:ExtensionElements', { values: preexistingValues });
}

I think I understand why this works - I need to create an actual copy of the extension elements if I want to modify and then write them. But I’m afraid that this way of manually recreating them is quite bug-prone.

What is the canonical way to modify existing extensionElements?

[Adrian]

@nikku @barmac Do you think this is a sensible way to go about modifying extension elements? As far as I can tell this works, but because it seems so hacky I’m hesitant to mark this topic as solved.

[nikku]

What is the canonical way to modify existing extensionElements?

Undo and redo must be done through a command, in an editor accessible manner.

modeling.updatePropertes is there to update the direct properties of a diagram shape, such as the label, size, position, …

modeling.updateModdleProperties is there to update a property of a businessObject (moddle element) attached to a diagram shape.

In the properties panel we make extensive use of modeling.updateModdleProperties, this allows you to modify extensionElement.values:

modeling.updateModdleProperties(task, extensionElements, {
  values: [ ...preexistingValues, bpmnFactory.create('activity:SomeElement') ]
});

Note that as indicated by @barmac you need to ensure that you do not leak side-effects. You see in my snippet that I create another [] from the existing values, and add a new value to it, or remove existing values.

[Adrian]

But that last point is exactly what is causing me headaches. As I mentioned earlier in the thread, simply creating a new array from the existing values does not fix the issue.

I had to resort to re-creating every single value before the side-effects stopped leaking. At least, I assume that was the problem.
Specifically, if I delete these lines from my example repository, undo/redo does not properly work.

    preexistingValues = preexistingValues.map(preValue => bpmnFactory.create(
      preValue.$type,
      {
          "name": preValue.name,
          "string": bpmnFactory.create(
            preValue.string.$type,
            {
                "text": preValue.string.text
            }
          )
      }
    ))

But if I understood you right, it should work without this weird hacky copying process.

Any idea why I would need to create copys of the values themselves and not just of the array containing them?

[nikku]

Did you use updateModdleProperties? I don’t see it being used.

Please review the way it is done for Camunda/Zeebe properties updating, i.e. updating Zeebe input fields. Under the hood these employ Modeling#updateModdleProperties.

Updating involves multiple commands:

1. ensure that extension elements are present
2. ensure that values within the extension elements are present
3. …

All of these activities are commonly wrapped in a multi-command handler, so that undo and redo works atomically for all these operations.

I suggest you to create a simple no-UI test case to verify your undo/redo logic works. Then it is easier for you to debug, understand, and fix things.

[Adrian]

I tried using updateModdleProperties, but that breaks pretty much everything. Not only does it lead to undo not working (which by now works with updateProperties), it also leads to the changes not actually being persisted. If I close the panel and open it up again they are gone.

I’ll try to create a test case to explore what exactly is happening here. However, I can’t find an example repo for test cases in the bpmn.io github project. Did I just overlook it or are there none?

[nikku]

I’ll try to create a test case to explore what exactly is happening here. However, I can’t find an example repo for test cases in the bpmn.io github project. Did I just overlook it or are there none?

There indeed exists no example “how to test bpmn-js”, I created an issue as a reminder we’d want to add one. But testing is usually super straight forward, so the valid question is, why do you need one?

A bunch of good practices:

• Test integration test style, in the browser, not using fake browser environments
  • Web Test Runner or Karma
• Use bpmn-js API to drive changes, and assert results

In your case a simple test case looks as following (using mocha and chai, inside a web test runner of your choice) can look like this.