RFR: 8092102: Labeled: truncated property [v9]

[Kevin Rushforth]

On Wed, 10 Apr 2024 21:25:10 GMT, Andy Goryachev <angorya at 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