-
Notifications
You must be signed in to change notification settings - Fork 542
8366201: RichTextArea: remove allowUndo parameter #1941
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
8366201: RichTextArea: remove allowUndo parameter #1941
Conversation
|
👋 Welcome back angorya! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@andy-goryachev-oracle has indicated that a compatibility and specification (CSR) request is needed for this pull request. @andy-goryachev-oracle please create a CSR request for issue JDK-8366201 with the correct fix version. This pull request cannot be integrated until the CSR request is approved. |
|
@andy-goryachev-oracle an approved CSR request is already required for this pull request. |
|
/reviewers 2 |
|
@andy-goryachev-oracle |
Webrevs
|
|
This will also need a csr. /csr |
Ah, and I see you already marked it as such |
|
@kevinrushforth an approved CSR request is already required for this pull request. |
|
I agree that having this as a per-method argument doesn't make sense. I think it's worth considering a future enhancement to add a property to enable or disable the undo feature, with a default of |
|
Thank you @kevinrushforth for suggesting the Consider a scenario of building a (large) document programmatically: in the absence of such a property, the undo entries will be created, consuming the resources, to be immediately discarded by Converting this PR back to Draft to add the property, since the code is interdependent and we'll need a CSR anyway. |
Original user feedback (see https://mail.openjdk.org/pipermail/openjfx-discuss/2025-August/000267.html ) called for adding an
allowUndoparameter toapplyStyle()andsetStyle()methods similarly toreplaceText().Upon further analysis, the
allowUndoparameter 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()andsetUndoRedoEnabled()to theStyledTextModel, as well as its forwarding aliases toRichTextAreato 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 .
/reviewers 2
/csr
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1941/head:pull/1941$ git checkout pull/1941Update a local copy of the PR:
$ git checkout pull/1941$ git pull https://git.openjdk.org/jfx.git pull/1941/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1941View PR using the GUI difftool:
$ git pr show -t 1941Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1941.diff
Using Webrev
Link to Webrev Comment