On Wed, 10 Apr 2024 21:25:10 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> Adds **Labeled.textTruncated** property which indicates when the text is >> visually truncated (and the ellipsis string is inserted) in order to fit the >> available width. >> >> The new property reacts to changes in the following properties: >> - ellipsisString >> - font >> - height >> - text >> - width >> - wrapText >> >> I don't think it's worth creating a headful test (headless won't work) due >> to relative simplicity of the code. >> >> **Alternative** >> >> The desired functionality can be just as easily achieved on an application >> level, by adding a similar property to a subclass. What is the benefit of >> adding this functionality to the core? >> >> UPDATE 2024/03/07: turns out Labeled in a TableView (in a TreeTableView as >> well) lives by different rules (TableCellSkinBase:152, >> TreeTableCellSkin:126). The consequence of this is that the new >> functionality **cannot** be fully implemented with the public APIs alone. >> >> **See Also** >> >> * [JDK-8327483](https://bugs.openjdk.org/browse/JDK-8327483) TreeView: Allow >> for tooltip when cell text is truncated >> * [JDK-8205211](https://bugs.openjdk.org/browse/JDK-8205211) Ability to show >> Tooltip only when text is shown with ellipsis (...) > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 15 commits: > > - missing ) > - review comments > - Merge branch 'master' into 8092102.truncated > - add exports > - added unit tests > - Merge remote-tracking branch 'origin/master' into 8092102.truncated > - test > - Merge remote-tracking branch 'origin/master' into 8092102.truncated > - Merge branch 'master' into 8092102.truncated > - labeled helper > - ... and 5 more: https://git.openjdk.org/jfx/compare/0eb4d719...aa28eb4e The API changes look good. I left a couple comments on the API docs. I also left a couple questions / comments on the fix and test. I haven't tested it yet. modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/LabeledHelper.java line 33: > 31: * Labeled Helper. > 32: */ > 33: public class LabeledHelper { You might consider making `LabeledHelper` a subclass of `ControlHelper`, which is the usual pattern for helper classes of nodes, although it may not matter here (and could be done later if there was a need). modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/LabeledHelper.java line 62: > 60: * @return true to use the actual content width, false to delegate to > the parent > 61: */ > 62: public static boolean isUseActualContentWidth() { If you make the `useActualContentWidth` flag an instance variable of `Labeled` (see my comment in that class), then you would need to add the `Labeled` as an argument to this method (here and in the accessor method). modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line 114: > 112: * by itself looks rather weird. > 113: */ > 114: private static boolean useActualContentWidth; Even given your comment about why it's safe to use a global variable, wouldn't it be cleaner to make it an instance variable? I suppose it might be OK to keep it global, but only if you can show that there are no issues with reentrancy. modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line 848: > 846: /** > 847: * Indicates whether the text has been truncated > 848: * when it cannot fit into the available width. "when" --> "because" modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line 850: > 848: * when it cannot fit into the available width. > 849: * <p> > 850: * When truncated, the {@link #ellipsisStringProperty() ellipsis > string} I recommend either using `@linkplain` or changing the text to `ellipsisString`, matching the name of the property. modules/javafx.controls/src/test/java/test/javafx/scene/control/LabeledTruncatedTest.java line 45: > 43: * in their skins to different code paths. > 44: */ > 45: public class LabeledTruncatedTest { It might be worth adding a test for `Button`. modules/javafx.controls/src/test/java/test/javafx/scene/control/LabeledTruncatedTest.java line 93: > 91: firePulse(); > 92: > 93: assertFalse(control.isTextTruncated()); Can you add a test for the case where we are wrapping and the preferred height is exceeded? ------------- PR Review: https://git.openjdk.org/jfx/pull/1389#pullrequestreview-2036421013 PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588087048 PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588090900 PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588033608 PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588065486 PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588067300 PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588112499 PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588113855
