On Wed, 22 Oct 2025 23:19:40 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 incrementally with one additional > commit since the last revision: > > undo redo enabled logic The API changes look good. I left a couple comments on the API docs. Now that undoRedo is state, you should add tests for the set operations, checking default state and setting it to both false and true on the control both with / without model. I also recommend adding tests of its operation when calling undo / redo with the attr set to both false and true. modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/CodeArea.java line 439: > 437: > 438: /** > 439: * Replaces text in this CodeArea. This method creates an undo/redo > entry. It creates an undo/redo entry if the undoRedo attribute is `true`. modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/RichTextArea.java line 1865: > 1863: > 1864: /** > 1865: * Replaces the specified range with the new input. This method > creates an undo entry. It creates an undo/redo entry if the undoRedo attribute is `true`. modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyledTextModel.java line 624: > 622: * @param end end text position > 623: * @param text text string to insert > 624: * @param allowUndo when true, creates an undo-redo entry Do you need to note that this method creates an undo/redo entry (if the undoRedo attribute is true)? modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyledTextModel.java line 656: > 654: * <p> > 655: * After the model applies the requested changes, an event is sent > to all the registered listeners. > 656: * This method creates an undo/redo entry. It creates an undo/redo entry if the undoRedo attribute is true. ------------- PR Review: https://git.openjdk.org/jfx/pull/1941#pullrequestreview-3391009394 PR Review Comment: https://git.openjdk.org/jfx/pull/1941#discussion_r2471187922 PR Review Comment: https://git.openjdk.org/jfx/pull/1941#discussion_r2471200486 PR Review Comment: https://git.openjdk.org/jfx/pull/1941#discussion_r2471208075 PR Review Comment: https://git.openjdk.org/jfx/pull/1941#discussion_r2471208416
