On Wed, 29 Oct 2025 21:54:17 GMT, Andy Goryachev <[email protected]> wrote:
>> Original user feedback (see >> https://mail.openjdk.org/pipermail/openjfx-discuss/2025-August/000267.html ) >> called for adding an `allowUndo` parameter to `applyStyle()` and >> `setStyle()` methods similarly to `replaceText()`. >> >> Upon further analysis, the `allowUndo` parameter was a mistake: allowing the >> application code to disable creating undo/redo entries messes up the >> internal undo/redo stack. >> There is an internal need (`UndoableChange`), but it should not be exposed >> via public API. >> >> This PR also adds `isUndoRedoEnabled()` and `setUndoRedoEnabled()` to the >> `StyledTextModel`, as well as its forwarding aliases to `RichTextArea` to >> allow for the application to disable undo/redo temporarily, for example, >> when building a document from multiple segments. >> >> WARNING this is an incompatible change, permitted because of the incubator. >> >> There remains a possible issue with currently unlimited size of the >> undo/redo stack - perhaps we should limit its depth to maybe 100-200 >> entries, see https://bugs.openjdk.org/browse/JDK-8370447 . > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 11 additional > commits since the last revision: > > - review comments > - Merge remote-tracking branch 'origin/master' into 8366201.allow.undo > - undo redo enabled logic > - cleanup > - cleanup > - removed allow undo parameter > - nl > - test > - append insert text > - tests > - ... and 1 more: https://git.openjdk.org/jfx/compare/08cea2b1...ffe6894c I left a couple more doc comments. Otherwise the API docs look ready to go. modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/CodeArea.java line 441: > 439: * Replaces text in this CodeArea. > 440: * It creates an undo/redo entry if the model's > 441: * {@link StyledTextModel#isUndoRedoEnabled() isUndoRedoEnabled()} > returns {@code true}. I think it is cleaner to refer to the `isUndoRedoEnable()` method in _this_ class. No need to refer to the model here. Something like: * It creates an undo/redo entry if * {@link #isUndoRedoEnabled() isUndoRedoEnabled()} returns {@code true}. modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/RichTextArea.java line 1077: > 1075: * sequences result in a new paragraph being added. > 1076: * It creates an undo/redo entry if the model's > 1077: * {@link StyledTextModel#isUndoRedoEnabled() isUndoRedoEnabled()} > returns {@code true}. No need to refer to the model. modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyledTextModel.java line 634: > 632: * {@code start} position. > 633: * It creates an undo/redo entry if the model's > 634: * {@link StyledTextModel#isUndoRedoEnabled() isUndoRedoEnabled()} > returns {@code true}. Since this is a method in the model, saying "the model's ..." is not needed. Maybe just say: * It creates an undo/redo entry if * {@link #isUndoRedoEnabled() isUndoRedoEnabled()} returns {@code true}. (like you did in a below method) modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyledTextModel.java line 844: > 842: * Indicates whether undo/redo functionality is enabled. > 843: * @return true if undo/redo functionality is enabled > 844: * @since 26 Please add: * @defaultValue {@code true} modules/jfx.incubator.richtext/src/test/java/test/jfx/incubator/scene/control/richtext/RichTextAreaTest.java line 796: > 794: assertTrue(control.isUndoable()); > 795: control.setUndoRedoEnabled(false); > 796: assertFalse(control.isUndoable()); To check that the stack is actually cleared, you might want to re-enable undoRedo, append text, undo, check for "23", undo again, check that it is still "23". ------------- PR Review: https://git.openjdk.org/jfx/pull/1941#pullrequestreview-3399343298 PR Review Comment: https://git.openjdk.org/jfx/pull/1941#discussion_r2477917825 PR Review Comment: https://git.openjdk.org/jfx/pull/1941#discussion_r2477971753 PR Review Comment: https://git.openjdk.org/jfx/pull/1941#discussion_r2477929496 PR Review Comment: https://git.openjdk.org/jfx/pull/1941#discussion_r2477935045 PR Review Comment: https://git.openjdk.org/jfx/pull/1941#discussion_r2477902651
