On Wed, 16 Oct 2024 23:01:39 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
> Removed "not yet fully implemented" USE_MULTIPLE_NODES and related code. > > I would like to remove the early unfinished idea of using multiple Text nodes > in TextAreaSkin to clean up the code, to make it easier to do fixes for > [JDK-8342233](https://bugs.openjdk.org/browse/JDK-8342233) and > [JDK-8296266](https://bugs.openjdk.org/browse/JDK-8296266). > > Also some minor cleanup. > > ## Summary of Changes > > - removed USE_MULTIPLE_NODES and code paths that correspond to its `true` > value > - permanently adding one Text node to `paragraphNodes` Group, keeping the > latter for compatibility purposes > - removed any code that scans `paragraphNodes` children > - using `getTextNode()` in place of > `((Text)paragraphNodes.getChildren().get(0))` This looks equivalent to the current code, and I couldn't see anything wrong in my testing. I left a couple questions inline. modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextAreaSkin.java line 729: > 727: @Override > 728: protected PathElement[] getUnderlineShape(int start, int end) { > 729: return getTextNode().underlineShape(start, end); The former code would return `null` if `start` > `textNode.textProperty().getValueSafe().length()`. I think this is OK, given that the implementation of `Text::underlineShape` checks -- what do you think? This applies to `getRangeShape`, too. modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextAreaSkin.java line 821: > 819: if (y < contentView.snappedTopInset()) { > 820: // Select the character at x in the first row > 821: return 0; I see that the implementation of the now-removed `getNextInsertionPoint` method also returned `0` unconditionally, so this is equivalent, but... isn't the comment wrong, since `x` isn't used in this case? modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextAreaSkin.java line 824: > 822: } else if (y > contentView.snappedTopInset() + > contentView.getHeight()) { > 823: // Select the character at x in the last row > 824: return (textArea.getLength() - > paragraphNode.getText().length()); Same question about the comment being wrong. ------------- Marked as reviewed by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1601#pullrequestreview-2382994268 PR Review Comment: https://git.openjdk.org/jfx/pull/1601#discussion_r1809276896 PR Review Comment: https://git.openjdk.org/jfx/pull/1601#discussion_r1809316134 PR Review Comment: https://git.openjdk.org/jfx/pull/1601#discussion_r1809324143
