On Wed, 14 Feb 2024 22:44:07 GMT, Andy Goryachev <[email protected]> wrote:
> Move caret animation handling due to keyboard input to the Skin, by > registering a listener on the caretPosition property. This will restart > animation only when the caret position changes instead of every key press. > > This also simplifies internal behaviors of TextArea, TextField, and > PasswordField in light of the future InputMap RFE > [JDK-8314968](https://bugs.openjdk.org/browse/JDK-8314968) I think if we're busy here, it would be worthwhile to also remove the other calls to the skin to change the caret animating. I think it would require a focus listener in Skin. Also code like this in `TextInputControlSkin` makes me believe that it may work out of the box correctly, even with selections, and all the code in the behavior is unnecessary (or can at least be fully handled by the `Skin` with perhaps another listener: >From `TextInputControlSkin`: /** * The caret is visible when the text box is focused AND when the selection * is empty. If the selection is non empty or the text box is not focused * then we don't want to show the caret. Also, we show the caret while * performing some operations such as most key strokes. In that case we * simply toggle its opacity. * <p> */ caretVisible = new BooleanBinding() { { bind(control.focusedProperty(), control.anchorProperty(), control.caretPositionProperty(), control.disabledProperty(), control.editableProperty(), displayCaret, blinkProperty());} @Override protected boolean computeValue() { // RT-10682: On Windows, we show the caret during selection, but on others we hide it return !blinkProperty().get() && displayCaret.get() && control.isFocused() && (isWindows() || (control.getCaretPosition() == control.getAnchor())) && !control.isDisabled() && control.isEditable(); } }; At a minimum it seems there is some duplication going on... modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextAreaSkin.java line 183: > 181: setForwardBias(true); > 182: } > 183: setCaretAnimating(true); I find this a bit confusing. By putting the two `setCaretAnimating` separate, you give the impression it must stop blinking for a while before continuing because of the call to `setForwardBias`. However, all we want to is call it with `false` then `true` to restart the animation (for a lack of a `playFromStart` call). Perhaps it may be good to introduce a method that captures this more clearly as I find it confusing (`restartCaretAnimation`) Same comment for `TextFieldSkin`. ------------- Changes requested by jhendrikx (Committer). PR Review: https://git.openjdk.org/jfx/pull/1368#pullrequestreview-1882863451 PR Review Comment: https://git.openjdk.org/jfx/pull/1368#discussion_r1491059806
