On Fri, 27 Jun 2025 15:02:29 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> ## Summary >> This change adds new methods to the `TextFlow` which work correctly in the >> presence of non-empty insets (borders/padding). For backward compatibility, >> the old buggy methods are getting deprecated (not for removal). Also, adds >> new methods in Text which provide missing functionality. >> >> ## Problem >> A number of methods in `TextFlow` fail to return correct values in the >> presence of non-empty insets (i.e. when either padding or border are set): >> - caretShape >> - hitTest >> - rangeShape >> >> Additionally, the current API fail to provide strike-through shape, and >> account for line spacing in the range shape, a problem shared between the >> `TextFlow` and the `Text` classes. >> >> ## Solution >> The solution is two-fold: >> 1) deprecate the buggy methods (not for removal), adding explanations in >> their javadoc comments >> 2) add new methods that behave correctly in the presence of non-empty insets >> and/or implementing the missing functionality. >> >> The proposed solution retains the buggy methods for the purposes of backward >> compatibility in applications which employ the workarounds, while providing >> new APIs with additional parameters similar to those offered by the new >> TextLayout API https://bugs.openjdk.org/browse/JDK-8341670 >> >> ## Testing >> >> Additional visualization of the data returned by the new APIs is available >> in the Monkey Tester using the following branch (in the Text and TextFlow >> pages): >> >> https://github.com/andy-goryachev-oracle/MonkeyTest/tree/text.insets.corrected > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 70 commits: > > - Merge branch 'master' into 8341438.text.shapes.insets > - cleanup > - Merge branch 'master' into 8341438.text.shapes.insets > - layout info > - tests > - Merge remote-tracking branch 'origin/ag.text.layout.api' into > 8341438.text.shapes.insets > - cleanup > - Merge remote-tracking branch 'origin/master' into ag.text.layout.api > - Merge remote-tracking branch 'origin/master' into ag.text.layout.api > - cleanup > - ... and 60 more: https://git.openjdk.org/jfx/compare/b9dd4dec...236c6af1 I left a couple doc comments inline. I'm left wondering if we should do the same thing in Text that you are doing in TextFlow. Namely, deprecate the following four methods, adding getXxxxx methods as replacements: - hitTest - caretShape - rangeShape - underlineShape Unlike TextFlow, there is no functional reason we need to do it. So the question is one of consistency. I note that you are already adding one of the four, getRangeShape (to add the lineSpacing parameter). What do you think? modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1092: > 1090: * Returns the shape for the range of the text in local coordinates. > 1091: * <p> > 1092: * NOTE: this shapes returned do not include line spacing. I would remove the "NOTE:", making it a simple statement (and probably no need for the `<p`> tag), since this is just what the method does. Question: should this method be deprecated to be consistent with TextFlow? modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java line 197: > 195: * Maps local point to {@link HitInfo} in the content. > 196: * <p> > 197: * NOTE: this method may return incorrect value with non-empty > border or padding. You should be more explicit, and say that it will not take border or padding into account. ------------- PR Review: https://git.openjdk.org/jfx/pull/1817#pullrequestreview-2999329020 PR Review Comment: https://git.openjdk.org/jfx/pull/1817#discussion_r2193621878 PR Review Comment: https://git.openjdk.org/jfx/pull/1817#discussion_r2193603871
