Add Feature: UndoManager can ignore a change if it is an identity / empty (data) change

[JordanMartinez]

Coming from TomasMikula/RichTextFX#486 (more specifically, Tomas comment on how to solve an issue):

Another option is to give the UndoManager a way to know whether merge resulted in an empty change. This could be done for example by adding an extra parameter Predicate<C> isEmpty to the factory method create above. Undo manager would then test changes for emptiness and automatically discard empty changes without trying to apply them.

[JordanMartinez]

I think "no op" is a better name for the change than "empty" as "empty" tends to be associated with length and we're going for a change that does nothing / no operation takes place when it is applied.

[TomasMikula]

For the UndoManager, a change is both a result of an action, and an action. isEmpty highlights more the result (data) aspect, isNoOp highlights more the action aspect. I don't have a strong preference.

Another possible name, coming from the group analogy, is identity element, or just identity. Note that when we see applying a change to a document as a method

StyledDocument apply(StyledDocument doc, Change c);

then the function

doc -> apply(doc, e)

of type Function<StyledDocument, StyledDocument>, where e is the empty/no-op change, is effectively the same as Function.<StyledDocument>identity(), hence the name.

[JordanMartinez]

For the UndoManager, a change is both a result of an action, and an action. isEmpty highlights more the result (data) aspect, isNoOp highlights more the action aspect. I don't have a strong preference.

Mm... I'll rephrase your words in my own. When an action occurs, the change stream emits an object that stores the necessary data to undo/redo some action. Hence, the stored change is nothing more than data, and the UM uses that data to undo/redo some action. Thus, from a "data" perspective, it should be named isEmpty and from an "action" perspective, it should be named isNoOp.

I think we already both agree that, when some action produces an "empty" change and one wants to silence these changes, that change should be ignored and prevented from being stored in the ChangeQueue.

Given #13, (and in particular, the discovery of the reversal problem and its solution of bumpAmount), a change should be "non-empty" when it is first stored. Otherwise, an "empty" change could become "non-empty" later on. In the PR, a change can become "empty" or "non-empty" during the time that it is stored as a result of some later change that modified it. However, the PR already employs its own isNoOp by checking whether a change isValid (i.e. whether the change, as a result of all the updates through which it has gone, would actually do something if it was passed to the apply function). If it isn't, it finds the next valid change or returns false when isUndoAvailable is called.

So, I guess I would say that, between isEmpty and isNoOp, the isEmpty name makes more sense after all :smile:

identity() only makes sense if one understands functional programming. However, since Java is moving in that direction, I think it'd make more sense to use that name than isEmpty because the latter requires knowing this data idea whereas identity has the connotation of "functional emptiness".

In sum, the ranking goes: identity > isEmpty > isNoOp

[TomasMikula]

Looks like we have a winner, then :)

[TomasMikula]

Resolved.