Command stack change event should be more detailed.

[PabloDons]

Is your feature request related to a problem? Please describe.

Using the modeler in an environment with it's own implementation of editor features can be quite frustrating. There is no convenient way to tell what the commandStack did, only that it did something unknown. An example of such an environment is VS Code or atom. Such an environment needs context to function correctly.

Describe the solution you'd like

The commandStack should emit more detailed events, like commandStack.pushed, commandStack.undid & commandStack.redid. Or it could just be added as event data to the commandStack.changed event. Additionally, the executed/reverted commands in that logical unit and their context should be included.

Every commandStack.changed event has an additional property trigger that is either undo, redo, execute or clear. This allows integrators to distinguish the different change events, i.e. to manage dirty state externally.

Describe alternatives you've considered

Overwriting the commandStack module is also an approach I could look into, but plugins shouldn't change core functionality due to compatibility issues. However other than that it would be a very reasonable approach.

I could use the commandStack.execute event and a timer to aggregate all commands with the same id, but it would be great to be able to undo immediately, not after timer.

I could just check the internal data of the commandStack module to determine what exactly changed, but this is unreasonable considering I would be practically implementing my own commandstack in the process, just way more hacky.

[barmac]

Can you please share a use case where such extended context would be required?

[PabloDons]

Going back to my previous statement about environments, it's already very useful. Take for instance a case were I want to perform an undo. Editors have dirty states when compared to the version saved on disk. After one edit, an undo should restore the diagram to the same as the one saved on disk and the diagram shouldn't be considered dirty anymore. Arguably an important feature to implement when integrating with a environment.

Without this context, only knowing something changed, the only solution would be to compare the diagram with the saved one on each change. Afaik updating the dirty state directly is not supported by any APIs, so you'd have to hack it together by simulating changes and undos. It's possible to invoke undo() and redo() manually, so by listening for the environment's own events and disabling them on the editor, you could do the same thing, but these two functions also emit a changed event, so I'd have to ignore one, but that would be unstable without knowing the context of each one.

Adding the stack to the context as well is related to issue #478 as that enables other useful editor features like backups or concurrent editors for the same diagram.

[barmac]

Please have a look at this sandbox: https://codesandbox.io/s/4fh3c?file=/app/index.html

You can actually listen to commandStack.revert|reverted events to register undo actions. Then, via commandStack._stackIdx you can detect dirty state. I'd consider elements.changed to be an event which signalises the need to re-render provided elements.

[PabloDons]

Thank you! Your suggestion really put the idea into a different light. It appears to solve a lot of issues. However the case where I would make a change, save, undo, then redo to restore saved state is problematic though. I would still need to tell the difference between a redo and a regular action to know if the current state is dirty. Either way, trying to indicate to an editor environment that an undo/redo has happened is unintended usage. They have their own "commandStack" and send the difference/changes to the editor which executes it.

In vscode it would work like so:
Whenever I make a change in the editor, the commandStack would send a number of execute events. For each one, I would save it to a list. Then when the changed event appears, I can make an "edit" which consists of what changed (which would be that list of events) an undo function and a redo function and register it as a document change event in vscode. These two functions are called by vscode whenever I hit the shortcuts for undo&redo, so they can be made to invoke the commandStack methods. They would also cause changed events, but with the context of the generated list, they could be ignored. The issue here practically is due to concurrency. Though it would be a rare bug, if I were to redo and make a change simultaneously, this sync could break. Take for example the scenario above, but this time, I redo at the same time I make a change. If vscode sent a redo before receiving anything, it could interpret the new change as a successful redo. It could indicate a saved state when it was in-fact dirty. However I think I will live with this bug if this suggestion is rejected.

I want to add that upon further consideration, including the executed/reverted commands in the context isn't required at all for this use case. It is related to my other issue, but without being serializable is basically pointless. Either way, how it is polled is irrelevant for this issue. I pointed out that I had some difficulty with my PR, but if I remove that part, I can make it ready to merge. Fairly quickly assuming I did it right.

[barmac]

Though it would be a rare bug, if I were to redo and make a change simultaneously, this sync could break.

The commands together with pre-/post-/etc. hooks are considered atomical in that sense that we don't allow user to undo in between.

Given what I referred to in #479 (comment), can you please specify your feature request? Or maybe it's already implemented?

[PabloDons]

I'm talking about the sync between the editor host and the webview (diagram). They can be in disagreement on if the redo or edit happened first without breaking the atomic paradigm. The issue is to the host, the redo event and regular new action event look identical.

Specifically the feature is this:
Add event data to the commandStack.changed event to indicate how the command stack was changed. Specifically if it was due to a new action, a redo or an undo.

This is already implemented in the PR. The detail I want to retract from my request was this:
Add event data to the commandStack.changed event to indicate what changed. Specifically the part of commandStack._stack that was changed.

This is also in the PR, thought not fully functional, but I shall remove it.

[PabloDons]

Is this feature request accepted? The backlog label is added and the PR is fully functional, so I would assume so, but the PR is still closed as you were figuring out what you want to have. I described what the feature is in the above comment and it's exactly what was implemented in the PR:

Add event data to the commandStack.changed event to indicate how the command stack was changed. Specifically if it was due to a new action, a redo or an undo.

[nikku]

I appreciate your persistence on this one. I also have to admit, that I did not fully understand why you need that feature.

I've built a few desktop like apps around diagram-js based editors and did not see the use-case for more detailed events yet. A trick I employed is to checkout the command stacks stackIndex after each elements.changed event. If the index is lower, I know an undo happened, if it is higher a do or redo happened.

Combining that knowledge with the last saved diagram, i.e. remembering stackIndex after every save allows me to reliably dirty check.

To move this issue forward (and increase a likelyhood of the related PR being merged), maybe you can provide us with a simple mock up (i.e. codesandbox) that shows how you plan to accomplish your dirty check with the API update you propose.

---

Correction: CommandStack._stackIdx is the stack index. I'm happy to make that one public API, i.e. expose it via CommandStack#getStackIndex if you find it useful.

[PabloDons]

if it is higher a do or redo happened

This is the specific problem I am trying to solve. I need to distinguish a do from a redo. This is impossible to do 100% reliably with the method you proposed. I made a codesandbox here to show why I want to distinguish these.

Notice the updateDirtyState() function. This is what gets messy without my proposed change. Now there is a possible solution, but it contains a very critical bug. I made the delay between the editor host and webview 1 second to make the bug more obvious. As you can see in my code, I rely on the webview to tell the editor that something changed, but this information isn't enough to differentiate a redo and a new action.

You might be asking why I want this kind of setup. Why not just leave it up to the webview to undo and redo, then just inform the editor of this change? It's a simple answer, the vscode API does not support it. Specifically disabling the undo and redo keybindings in the editor isn't supported (since I need to replace them).

The solution to the lack of information about a new action (whether it was a redo) is that the editor could try to deduce the missing information by predicting the incoming events. Basically the editor goes "I did a redo, so I can expect the next commandStack event to be a redo". The critical bug is a possible desync conflict between the webview and the editor. With this solution implemented, try making a change and then quickly hitting the redo button. Now the editor will expect a redo event, but it was actually a new action. No more events will occur because the redo action will not have anything to redo. Now you will have an incorrect dirty state and a desynced edit stack.

Now notice how simple the solution becomes with my proposed change in the function withMerge(e).

If this is still unclear, please do say so and I can try to implement the proposed "solution" I claim contains a bug and demonstrate it. If you can solve this issue with the setup in the codesandbox without desync issues, then you're definitely smarter than I am and I would appreciate it massively!

[nikku]

Thanks for the sandbox. I'll have a look later this week.

[PabloDons]

Can we get this issue resolved soon? I don't mean to be rude, but I don't want to maintain my own version of diagram-js forever

[nikku]

Sorry for not being able to follow up with this one earlier. We'll look into this issue, eventually.

[nikku]

I looked into your code sandbox and understood, more or less, what is going on. Sure, we could add a trigger field to the commandStack.changed that indicates whether execute, undo, redo or clear were the reason for the change.

What I do not understand yet is this:

You might be asking why I want this kind of setup. Why not just leave it up to the webview to undo and redo, then just inform the editor of this change? It's a simple answer, the vscode API does not support it.

What do you mean by the vscode API does not support it?

We did implement a couple of undo/redo capable editors on top of diagram-js/bpmn-js, but not on top of VSCode I believe.

[PabloDons]

https://code.visualstudio.com/api/references/vscode-api#CustomEditorProvider

Specifically onDidChangeCustomDocument

This event must be fired by your extension whenever an edit happens in a custom editor.

So I need to trigger this whenever commandStack.changed is triggered.

Editors that support undo/redo must fire a CustomDocumentEditEvent whenever an edit happens.

The extension is responsible for instantiating the CustomDocumentEditEvent object. It must contain an undo and a redo method. These map exactly to the commandStack.undo and redo methods in terms of behavior. Except that the commandStack will trigger a changed event and create a new false edit in VSCode.

VSCode host will trigger the undo and redo methods inside the CustomDocumentEditEvent whenever I press the hotkeys, and will update the edit-stack & dirty-state accordingly. There is no way to overwrite or disable this behavior. There is also no way to affect the edit-stack or dirty-state either.

VSCode considers a CustomDocumentEditEvent to be a new edit on top of the edit stack.

What I'm referring to is by hooking into the hotkeys in diagram-js, I could, for example, send a CustomDocumentUndoEvent to VSCode. Since they would conflict with VSCode's own hotkeys and there is no UndoEvent in VSCode, This is not a possible solution.

Will you point me to the editors you say you implemented? I would like to take a look

[nikku]

Hooking into any kind of "keyboard shortcut" on the diagram-js level does not sound like a proper approach to hook into VSCode.

Embedded into VSCode, VSCode should likely be fully in charge of managing keyboard shortcuts, including undo and redo.

We offer an abstraction, EditorAction to be used by you in these cases.

---

If I understand your answer correctly, #479 (comment) is everything you need? Specifically:

[...] add a trigger field to the commandStack.changed that indicates whether execute, undo, redo or clear were the reason for the change.

It allows you to generate these edit events only on DO or CLEAR but never on undo and redo, as these are managed by VSCode.

[PabloDons]

I don't quite understand how this solves my issue. I assume you are referring to this, but commandStack doesn't use it to execute, and calling editorActions.trigger("undo") doesn't prevent the changed event.

Could you please elaborate?

[nikku]

Let's complete one thread after the other. Would #479 (comment) solve your issue?

With #479 (comment), part one I was merely suggesting a good practice, embedding our library into an external editor.

[PabloDons]

Ah sorry, I missunderstood

If I understand your answer correctly, #479 (comment) is everything you need? Specifically:

[...] add a trigger field to the commandStack.changed that indicates whether execute, undo, redo or clear were the reason for the change.

It allows you to generate these edit events only on DO or CLEAR but never on undo and redo, as these are managed by VSCode.

Yes, correct

[nikku]

Great 🎉, 🥳.

Would you like to contribute the described solution, i.e. every commandStack.changed event has an additional property trigger that is either undo, redo, execute or clear? Kind of like #486, just this time we understood what you'd like to accomplish, and why, too?

[nikku]

Updated the issue description, according to the solution we found, both understood, and acknowledged.

Thanks for your patience, and your insights into your use-case.

[PabloDons]

Happy to!

[nikku]

Closed via #529.