Plugin's appendTransaction should be "appended" by definition

It seems that when appending a transaction via plugin, to the array of current transactions, by definition it should be part of the same history block

The semantics of appendTransaction name is not correlated to the documentation, where it is stated that is to “append” a transaction to be applied after the recently applied transactions. But in this case the name should be more like “addTransaction” or “queueTransaction”.

The name “append” implies that is attached to something, but currently with appended transactions there is no guarantee that when the transactions are “appended” to a existing one, they will be part of the same history group.

In the history module, appendedTransactions are still dependent of the time between changes, so if for example a transaction is appended via plugin and its creation time can’t be guaranteed to be within newGroupDelay value, it will belong to a different undo step, even when what fired the appended transaction creation was the previous transaction (and thus should be considered a unit)

To me, appendedTransactions should be really part of the same history group of the transactions that fired them, or at least there should be a meta to signal that and avoid being dependent of time between transactions.

It might be that the appended transaction takes a long processing time, or it might depend on network conditions, but apart from that, debugging introduces artificial pauses while stepping each statement, and makes the transaction to be added to a different history group, complicating getting real results.

There’s actually code to guarantee just that. Do you have a test case where it fails?

A simple breakpoint in a plugin’s appendTransaction, and a pause of more then 500ms (in the default configuration) just before a step is added to the appended transaction will be enough to make its time stamps surpass newGroupDelay

in history.js::applyTransaction:

let appended = tr.getMeta("appendedTransaction")
...
let newGroup = history.prevTime < (tr.time || 0) - options.newGroupDelay || 
                         !appended && !isAdjacentToLastStep(tr, history.prevMap, history.done)

The appendedTransaction meta is not enough, as it is still dependent of curTransactionTime - prevTransactionTime > newGroupDelay to be included in a new history group

I’m not sure I consider forcing delays through breakpoints a reasonable circumstance to code against, but if you want to submit a patch to address this I’ll take a look.

Ok, I will simply set higher precedence of “appendedTransaction” meta over the newGroupDelay.

something along the lines:

newGroup = !appended && (history.prevTime < (tr.time || 0) - options.newGroupDelay || 
                      !isAdjacentToLastStep(tr, history.prevMap, history.done))

Anyway, the debugger pause is just to easily reproduce this, but what about a plugin that takes a little longer processing time, or what about non-controllable network delays in plugin’s appendTransaction (when plugin depends on network to process its data), as it stands right now, the history module will put the document in an undefined state.

That would be really bad for the user experience—you don’t want slow code in your synchronous state update cycle.

No one does those synchronously in modern JavaScript, so you can’t use them in appendTransaction.

Ok, we can argue all day what really is an “op that takes a little longer processing time” (document size, cpu architecture, processing power, memory limits, vram vs swap, unintended and uncontrollable pauses as per memory relocation, GC kicking in unexpectedly, and what not), so there is no point in that.

The problem as it stands right now, is that appendtedTransactions are not guaranteed to be part of the same history block, and this inconsistency is by design in its current state.

So the question is: are you open to include the change proposed above to guarantee that an appendedTransaction is ALWAYS part of the same history group along with what triggered it, and not dependant on the time passed between the transaction creation and its apply?

The default newGroupDelay is 500 milliseconds. If your script is taking that long you have a lot of other problems. I specifically proposed you submit a pull request earlier. If you’re just going to continue griping here instead I’m not going to engage anymore.

2 Likes